SEV-SNP support #1269

123abcpp · 2024-05-21T12:45:13Z

No description provided.

It will cause failure when tracking readonly pages and then writing back before unmapping.

This is the start of the SEV-SNP series update, the support will include several parts: 1.SEV-SNP VM Creation 2.KVM VMGEXIT handling 3.qkernel SEV-SNP related initialization 4.qkernel #VC interrupt handling Since this branch is based on cc_quark branch, the private and shared memory isolation is done, do not need to change here.

If cc is enabled then check cpuid for snp support. Switch to the vm creation flow to the snp machine creation

SEV-SNP need to update cpuid page before launching

The vcpu regsiters should be initialized before SNP_LAUNCH_FINISH in InitSevSnp, but not before vcpu.run(). So extract the process to a function x86_init.

Add c bit to inital page table.

Add support for vmgexit

If sev-snp is enabled, parse the page table including c bit. Add a function smash, smash a big page table entry to a smaller page size.

1.Add ghcb protocol related functions. 2.Add ghcb init functions.

1.Add a new crate to disassemble input instruction in #VC Handler 2.Add an atomic bool to check whether log is available, avoiding loop in the interrupt.

If it is SEV-SNP, do the following setup: 1.Accpet unvalidated memory. 2.GHCB and then shared memory initialization. 3.Set registers which not set automatically by kvm.

qkernel/src/interrupt/x86_64/mod.rs

shrik3 · 2024-05-21T13:27:38Z

qlib/pagetable.rs

@@ -127,12 +129,23 @@ impl PageTables {
    }

    pub fn SwitchTo(&self) {
+        #[cfg(feature = "cc")]


I personally prefer having two SwitchTo with different config terms, instead of one SwitchTo with two different behaviors. But this makes no difference, just personal opinion.

shrik3 · 2024-05-21T13:33:04Z

qlib/pagetable.rs

@@ -364,6 +417,13 @@ impl PageTables {
        flags: PageTableFlags,
        pagePool: &Allocator,
    ) -> Result<bool> {
+        #[cfg(feature = "cc")]
+        if ENABLE_CC.load(Ordering::Acquire) {


I was confused earlier because of the ENABLE_CC naming. I think in this context it's a flag variable that means "whether CC is enabled", the ENABLE_CC name sounds like a (rather static) configuration term.

Yes, it is configurable via config file now

shrik3 · 2024-05-21T16:13:26Z

thanks for doing clear commit messages. To be picky, perhaps consider using conventional commit messages, that is, use

SEV-SNP: does xyz

instead of

[SEV-SNP]Does xyz

123abcpp · 2024-05-21T16:17:52Z

thanks for doing clear commit messages. To be picky, perhaps consider using conventional commit messages, that is, use
SEV-SNP: does xyz
instead of
[SEV-SNP]Does xyz

I see, will reword them later.

shrik3 · 2024-05-21T16:24:21Z

qlib/cc/sev_snp/ghcb.rs

+
+            unsafe {
+                if self.vmgexit(SVM_VMGEXIT_PSC, 0, 0).is_err() {
+                    early_panic(4, 0x33);


just a note that you have some hardcoded values (which is fine)

shrik3 · 2024-05-21T16:25:47Z

qlib/cc/sev_snp/ghcb.rs

+
+    for i in desc.cur_entry..=desc.end_entry {
+        let entry = &mut desc.entries[i as usize];
+        let operation = (entry.entry >> 52) & 0xF;


as above, maybe have some defs later. This could be error prone.

qlib/kernel/Kernel_cc.rs

shrik3 · 2024-05-21T16:38:49Z

qlib/pagetable_cc.rs

@@ -0,0 +1,1447 @@
+pub use super::linux_def::*;


would it be better to have a cc directory, instead of having files suffixed "cc"? There should be more such files I suppose? (like Kernel_cc.rs, and maybe there are more to come in the future).

Yes, I can move Kernel_cc.rs and pagetable_cc.rs into the cc folder which already exists. But I am not sure that I should put all the cc related codes into that folder, or it might be easy to add support for cc if you put them together in one folder. You can easily realize there is a related cc file without opening the cc folder.

Also since the *_cc.rs are already their own files, there is no need to suffix the functions within with _cc. The namespace already tells the difference.

Then how to differ from the normal ones? For example, they are both functions of Pagetable struct, and I need to decide which to run in the runtime.

123abcpp · 2024-05-21T16:54:35Z

there are a lot of redundant spaces in the code (red blocks), I think they were added from previous cc commits. Consider clearing them towards the end.

fixed

chl337 · 2024-05-22T08:39:02Z

qkernel/src/interrupt/x86_64/mod.rs

+        sf.ss |= 3;
+        currTask.SaveFp();
+    }
+    /* 


Can you please remove this. And also any leftover commented code that you don't need. Also consider using debug!() or error!() depending on situation...

chl337 · 2024-05-22T08:42:59Z

qkernel/src/lib.rs

@@ -474,6 +482,32 @@ fn InitLoader() {
    LOADER.InitKernel(process).unwrap();
 }

+#[cfg(feature = "cc")]
+//Need to initialize PAGEMGR(pagepool for page allcator) and kernel page table in advance


Typo: *allocator :)

chl337 · 2024-05-22T08:49:04Z

qkernel/src/lib.rs

+#[cfg(feature = "cc")]
+//Need to initialize PAGEMGR(pagepool for page allcator) and kernel page table in advance
+fn InitShareMemory() {
+    *GHCB[0].lock() = Some(GhcbHandle::default());


I see this is a repeated pattern. Maybe have some accessor methods for GHCB elements, that way you can also do sanity check on indexes.

sanity check on indexes.

an assert would do, but that rust runtime will panic on out-of-bound anyways, that's the same effect. I think this depends on whether you consider this operation failiable.

chl337 · 2024-05-22T08:59:29Z

qkernel/src/lib.rs

+
+            //Different from normal vm, mxcsr will not be set by kvm, should set it mannualy
+            if IS_SEV_SNP.load(Ordering::Acquire){
+                let mxcsr_value = 0x1f80;


Could you add a short comment to explain what the mask means... for readability.

actually all these masks and hardcoded values should be constant defs.

chl337 · 2024-05-22T09:07:28Z

qlib/linux_def.rs

@@ -3021,8 +3021,11 @@ impl MemoryDef {
    const GUEST_HOST_SHARED_HEAP_OFFEST: u64 =  MemoryDef::GUEST_PRIVATE_HEAP_END;
    const GUEST_HOST_SHARED_HEAP_END: u64 =  MemoryDef::GUEST_HOST_SHARED_HEAP_OFFEST + MemoryDef::GUEST_HOST_SHARED_HEAP_SIZE;

-    const GHCB_OFFSET: u64 = MemoryDef::GUEST_HOST_SHARED_HEAP_OFFEST + MemoryDef::PAGE_SIZE*2;
-    const HYPERCALL_PARA_PAGE_OFFSET :u64 = MemoryDef::GUEST_HOST_SHARED_HEAP_OFFEST + MemoryDef::PAGE_SIZE*3;
+    // Reuse RDMA area for sev-snp special pages


Doesn't this cause conflict with RDMA?

it was commented elsewhere that RDMA stuffs are not supported under cc context.

chl337 · 2024-05-22T09:10:26Z

qvisor/src/runc/runtime/vm_sev.rs

+        cpuid_page
+            .FillCpuidPage(&kvm_cpuid)
+            .expect("Fail to fill cpuid page");
+        //cpuid_page.dump_cpuid();


Cleanups :)

chl337 · 2024-05-22T09:11:53Z

qlib/cc/mod.rs

@@ -0,0 +1,31 @@
+pub mod sev_snp;


The cc needs an arch directory...

chl337 · 2024-05-22T09:44:39Z

qlib/cc/sev_snp/mod.rs

@@ -0,0 +1,46 @@
+use core::sync::atomic::{AtomicU64, Ordering};
+
+/// The C-Bit mask indicating encrypted physical addresses


Three slashes are meant for documentation. Is that your intention?

shrik3 · 2024-05-22T09:53:02Z

qkernel/src/interrupt/x86_64/mod.rs

@@ -28,6 +28,14 @@ use super::super::threadmgr::task_sched::*;
 use super::super::MainRun;
 use super::super::SignalDef::*;
 use super::super::SHARESPACE;
+cfg_cc! {
+    use super::super::qlib::cc::sev_snp::{VCResult,SVMExitDef,snp_active,C_BIT_MASK};


using relative paths was a mistake and will be fixed later. Perhaps consider ditch this pattern in new code already.

chl337 · 2024-05-22T09:57:35Z

qvisor/Cargo.toml

 #kvm-ioctls = "0.11.0"
-kvm-ioctls = { git = "https://github.com/QuarkContainer/kvm-ioctls.git" }
+#kvm-ioctls = { git = "https://github.com/QuarkContainer/kvm-ioctls.git" }
+kvm-ioctls = { git = "https://github.com/123abcpp/kvm-ioctls.git" }


Can your changes be merged to QC/kvm-ioctls.git?

shrik3 · 2024-05-22T10:02:44Z

qkernel/src/lib.rs

@@ -483,20 +517,47 @@ cfg_if::cfg_if! {
            vdsoParamAddr: u64,
            vcpuCnt: u64,
            autoStart: bool,
+            vm_type: u64


I have a feeling this will cause too much runtime dependency (also the case for IS_SEV_SNP and ENABLE_CC etc). In other words you will have check these flags quite frequently. I don't know what's an alternative though, maybe we can discuss this at some point.

shrik3 · 2024-05-22T10:04:50Z

qkernel/src/lib.rs

-                //check msr to see if sev-snp enabled
-                ENABLE_CC.store(true,Ordering::Release);
-                // ghcb convert shared memory
-
                GLOBAL_ALLOCATOR.InitSharedAllocator(MemoryDef::guest_host_shared_heap_offest_gpa());


not related to this PR, but I'm still not convinced of having getters for static constant values. This only makes code reading harder for me.

shrik3 · 2024-05-22T10:06:47Z

qkernel/src/lib.rs


+                SingletonInit();
+                LOG_AVAILABLE.store(true, Ordering::Release);


I think you may need a comment here, like,

///// LOGGING NOT AVAILABLE BEFORE THIS POINT /////

XD

chl337 · 2024-05-23T09:05:14Z

qkernel/src/lib.rs


                GLOBAL_ALLOCATOR.InitPrivateAllocator(MemoryDef::guest_private_running_heap_offset_gpa());
-
+                unsafe {KERNEL_PAGETABLE.Init(PageTables::Init(CurrentKernelTable()&0xffff_ffff_ffff));}


This is somewhat cryptic to me...

I also don't understand this part. @123abcpp could you explan why you do this? thank you

chl337 · 2024-05-23T14:59:31Z

On Thu May 23, 2024 at 1:03 PM CEST, Tianhao Wang wrote: @shrik3 commented on this pull request. > @@ -3021,8 +3021,11 @@ impl MemoryDef { const GUEST_HOST_SHARED_HEAP_OFFEST: u64 = MemoryDef::GUEST_PRIVATE_HEAP_END; const GUEST_HOST_SHARED_HEAP_END: u64 = MemoryDef::GUEST_HOST_SHARED_HEAP_OFFEST + MemoryDef::GUEST_HOST_SHARED_HEAP_SIZE; - const GHCB_OFFSET: u64 = MemoryDef::GUEST_HOST_SHARED_HEAP_OFFEST + MemoryDef::PAGE_SIZE*2; - const HYPERCALL_PARA_PAGE_OFFSET :u64 = MemoryDef::GUEST_HOST_SHARED_HEAP_OFFEST + MemoryDef::PAGE_SIZE*3; + // Reuse RDMA area for sev-snp special pages it was commented elsewhere that RDMA stuffs are not supported under cc context.

I have missed it then. Sorry for the noise=)

yaoxin1995 · 2024-05-28T09:09:01Z

qkernel/src/interrupt/x86_64/mod.rs

+                    }
+                    let ghcb_option: &mut Option<GhcbHandle<'_>> = &mut *GHCB[vcpuid].lock();
+                    let ghcb = ghcb_option.as_mut().unwrap();
+                    ghcb.invalidate();


Why we need to do ghcb.invalidate twice? I thought you already did it before vm exit

yaoxin1995 · 2024-05-28T09:14:33Z

qkernel/src/lib.rs


                GLOBAL_ALLOCATOR.InitPrivateAllocator(MemoryDef::guest_private_running_heap_offset_gpa());
-
+                unsafe {KERNEL_PAGETABLE.Init(PageTables::Init(CurrentKernelTable()&0xffff_ffff_ffff));}


I also don't understand this part. @123abcpp could you explan why you do this? thank you

yaoxin1995 · 2024-05-28T09:26:09Z

qlib/cc/sev_snp/ghcb.rs

@@ -0,0 +1,593 @@
+use core::ptr;


You may need to add a header here to clarify where you got the code, and who modified the code

yaoxin1995 · 2024-05-28T09:42:30Z

qlib/cc/sev_snp/mod.rs

@@ -0,0 +1,274 @@
+use self::Error::{FailInput, FailSizeMismatch, Unknown};


You need header to clarify where you get the code, for example

// Copyright (c) 2021 The Enarx Authors. // Original code source: link to Enarx // Code modifier: yiliang // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 // // Unless required by applicable law or agreed to in writing, software // distributed under the License is distributed on an "AS IS" BASIS, // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. // See the License for the specific language governing permissions and // limitations under the License.

chl337 · 2024-05-28T15:44:25Z

qvisor/src/kvm_vcpu_x86.rs

-                            let sharedSpace  = unsafe { &mut (*shareSpaceAddr) };
-                            
+                            let sharedSpace = unsafe { &mut (*shareSpaceAddr) };
+
                            // note: in CC mdoe, host can't access PAGE_MGR because it is on private memory


Typo: *mode

chl337 · 2024-05-28T15:47:40Z

qvisor/src/kvm_vcpu_x86.rs

                            // note: in CC mdoe, host can't access PAGE_MGR because it is on private memory
                            // in non-cc case,  host require PAGE_MGR to support hibernate mode
-                            // check pub fn SwapOut(&self, start: u64, len: u64) -> Result<()> 
+                            // check pub fn SwapOut(&self, start: u64, len: u64) -> Result<()>


"check foo() in path" would be enough, no need for the foo()'s signature.

chl337 · 2024-05-30T08:47:07Z

qkernel/src/lib.rs

+        MemoryDef::FILE_MAP_SIZE / MemoryDef::PAGE_SIZE_2M,
+    );
+    ghcb.set_memory_shared_2mb(
+        VirtAddr::new(MemoryDef::guest_host_shared_heap_offest_gpa()),


Typo: *offset

123abcpp added 17 commits May 16, 2024 12:57

Track shared page only when writeable

ae30da1

It will cause failure when tracking readonly pages and then writing back before unmapping.

[SEV-SNP]Add wrapper function for SEV_SNP vm init

72b9469

If cc is enabled then check cpuid for snp support. Switch to the vm creation flow to the snp machine creation

[SEV-SNP]Add cpuid page support

7fdfed6

SEV-SNP need to update cpuid page before launching

[SEV-SNP]Update kvm and snp related crates

750189f

[SEV-SNP]Extract x86 vcpu regs initialization

389fe1f

The vcpu regsiters should be initialized before SNP_LAUNCH_FINISH in InitSevSnp, but not before vcpu.run(). So extract the process to a function x86_init.

[SEV-SNP]Track memory usage of initial private heap

a93372c

[SEV-SNP]Add initial pagetable support

b9b06cd

Add c bit to inital page table.

[SEV-SNP]Map backend memory for special pages

67f20c8

[SEV-SNP]VM creation flow implementation

3d60428

[SEV-SNP]VCPU run flow update

9b9015c

Add support for vmgexit

[SEV-SNP]Add snp page table support

8f33366

If sev-snp is enabled, parse the page table including c bit. Add a function smash, smash a big page table entry to a smaller page size.

[SEV-SNP]Add GHCB support

0f628cb

1.Add ghcb protocol related functions. 2.Add ghcb init functions.

[SEV-SNP]Add #VC Exception Handler

30f3850

1.Add a new crate to disassemble input instruction in #VC Handler 2.Add an atomic bool to check whether log is available, avoiding loop in the interrupt.

[SEV-SNP]Kernel init flow update

dafbdd6

If it is SEV-SNP, do the following setup: 1.Accpet unvalidated memory. 2.GHCB and then shared memory initialization. 3.Set registers which not set automatically by kvm.

[SEV-SNP]Disable ENABLE_EMULATION_CC

802a65b

[SEV-SNP]Add debug hypercall support

db960de

shrik3 reviewed May 21, 2024

View reviewed changes

qkernel/src/interrupt/x86_64/mod.rs Show resolved Hide resolved

shrik3 reviewed May 21, 2024

View reviewed changes

[Fix pagetable] fix switch to logic bug

489c0c6

shrik3 reviewed May 21, 2024

View reviewed changes

qlib/kernel/Kernel_cc.rs Show resolved Hide resolved

shrik3 reviewed May 21, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

[Fixup] Remove redundant spaces in qkernel initialization

41e58c0

chl337 reviewed May 22, 2024

View reviewed changes

qlib/cc/mod.rs

@@ -0,0 +1,31 @@

pub mod sev_snp;

Copy link

Collaborator

chl337 May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cc needs an arch directory...

chl337 reviewed May 22, 2024

View reviewed changes

shrik3 reviewed May 22, 2024

View reviewed changes

chl337 reviewed May 22, 2024

View reviewed changes

shrik3 reviewed May 22, 2024

View reviewed changes

chl337 reviewed May 23, 2024

View reviewed changes

yaoxin1995 reviewed May 28, 2024

View reviewed changes

chl337 reviewed May 28, 2024

View reviewed changes

chl337 reviewed May 30, 2024

View reviewed changes

		@@ -0,0 +1,46 @@
		use core::sync::atomic::{AtomicU64, Ordering};

		/// The C-Bit mask indicating encrypted physical addresses


		SingletonInit();
		LOG_AVAILABLE.store(true, Ordering::Release);


		GLOBAL_ALLOCATOR.InitPrivateAllocator(MemoryDef::guest_private_running_heap_offset_gpa());

		unsafe {KERNEL_PAGETABLE.Init(PageTables::Init(CurrentKernelTable()&0xffff_ffff_ffff));}

		@@ -0,0 +1,274 @@
		use self::Error::{FailInput, FailSizeMismatch, Unknown};

SEV-SNP support #1269

Are you sure you want to change the base?

SEV-SNP support #1269

Conversation

123abcpp commented May 21, 2024

shrik3 May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrik3 commented May 21, 2024

123abcpp commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

123abcpp commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chl337 May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chl337 commented May 23, 2024 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrik3 May 21, 2024 •

edited

Loading

chl337 May 23, 2024 •

edited

Loading