| Message ID | 20240905124107.6954-3-pratikrajesh.sampat@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SEV Kernel Selftests | expand |
On Thu, Sep 05, 2024, Pratik R. Sampat wrote: > Extend sev_smoke_test to also run a minimal SEV-SNP smoke test that > initializes and sets up private memory regions required to run a simple > SEV-SNP guest. > > Similar to its SEV-ES smoke test counterpart, this also does not > support GHCB and ucall yet and uses the GHCB MSR protocol to trigger an > exit of the type KVM_EXIT_SYSTEM_EVENT. > > Also, decouple policy and type and require functions to provide both > such that there is no assumption regarding the type using policy. > > Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com> > Tested-by: Peter Gonda <pgonda@google.com> > Tested-by: Srikanth Aithal <sraithal@amd.com> > --- > .../selftests/kvm/include/x86_64/processor.h | 1 + > .../selftests/kvm/include/x86_64/sev.h | 54 +++++++- > tools/testing/selftests/kvm/lib/kvm_util.c | 8 +- > .../selftests/kvm/lib/x86_64/processor.c | 6 +- > tools/testing/selftests/kvm/lib/x86_64/sev.c | 116 +++++++++++++++++- > .../selftests/kvm/x86_64/sev_smoke_test.c | 67 ++++++++-- > 6 files changed, 230 insertions(+), 22 deletions(-) There is *way* too much going on in this one patch. There's at least 6+ patches worth of stuff here: 1. Add x86 architectural defines 2. SNP ioctl() plumbing 3..N. Other misc plumbing, e.g. is_smt_active() N+1. __vm_create() change to force GUEST_MEMFD for SNP N+2. Whatever the ASSERT_SEV_POLICY() thing is doing, if it's actually necessary N+3..M. Refactorings to existing code to prep for SNP M+1. SNP support In general, if you feel the urge to start a changelog paragraph with "Also," that's a sign you need to break up the patch. > @@ -74,8 +95,18 @@ kvm_static_assert(SEV_RET_SUCCESS == 0); > __TEST_ASSERT_VM_VCPU_IOCTL(!ret, #cmd, ret, vm); \ > }) > > +/* Ensure policy is within bounds for SEV, SEV-ES */ > +#define ASSERT_SEV_POLICY(type, policy) \ > +({ \ > + if (type == KVM_X86_SEV_VM || type == KVM_X86_SEV_ES_VM) { \ > + TEST_ASSERT(policy < ((uint32_t)~0U), \ > + "Policy beyond bounds for SEV"); \ This is asinine. First, there's one user, i.e. I see no reason to have a funky macro to assert on the type. Second, _if_ this is a common macro, "type" can and should be incorporated into the assert. Third, I have no idea why selftests is validating its own inputs to KVM. > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c > index 974bcd2df6d7..981f3c9fd1cf 100644 > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c > @@ -625,7 +625,8 @@ void kvm_arch_vm_post_create(struct kvm_vm *vm) > sync_global_to_guest(vm, host_cpu_is_amd); > sync_global_to_guest(vm, is_forced_emulation_enabled); > > - if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) { > + if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM || > + vm->type == KVM_X86_SNP_VM) { Probably time to add a helper, e.g. is_sev_vm() or something. If we follow KVM's lead, then we'd have is_sev_vm(), is_sev_es_vm(), and is_sev_snp_vm(), where an SNP VM returns true for all of them. Not sure I love that idea, just throwing it out there as one possibility. > struct kvm_sev_init init = { 0 }; > > vm_sev_ioctl(vm, KVM_SEV_INIT2, &init); > @@ -1134,7 +1135,8 @@ void kvm_get_cpu_address_width(unsigned int *pa_bits, unsigned int *va_bits) > > void kvm_init_vm_address_properties(struct kvm_vm *vm) > { > - if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) { > + if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM || > + vm->type == KVM_X86_SNP_VM) { > vm->arch.sev_fd = open_sev_dev_path_or_exit(); > vm->arch.c_bit = BIT_ULL(this_cpu_property(X86_PROPERTY_SEV_C_BIT)); > vm->gpa_tag_mask = vm->arch.c_bit; > diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c b/tools/testing/selftests/kvm/lib/x86_64/sev.c > index 125a72246e09..ff3824564854 100644 > --- a/tools/testing/selftests/kvm/lib/x86_64/sev.c > +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c > @@ -14,7 +14,8 @@ > * and find the first range, but that's correct because the condition > * expression would cause us to quit the loop. > */ > -static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region) > +static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region, > + uint8_t page_type) > { > const struct sparsebit *protected_phy_pages = region->protected_phy_pages; > const vm_paddr_t gpa_base = region->region.guest_phys_addr; > @@ -25,12 +26,23 @@ static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region > if (!sparsebit_any_set(protected_phy_pages)) > return 0; > > - sev_register_encrypted_memory(vm, region); > + if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) And then this would be if (!is_sev_snp_vm()) > + sev_register_encrypted_memory(vm, region); > > sparsebit_for_each_set_range(protected_phy_pages, i, j) { > const uint64_t size = (j - i + 1) * vm->page_size; > const uint64_t offset = (i - lowest_page_in_region) * vm->page_size; > > + if (vm->type == KVM_X86_SNP_VM) { > + vm_mem_set_private(vm, gpa_base + offset, size); Setting memory private seems like something that should be done by common code, not by SNP specific code. > @@ -158,6 +213,45 @@ void sev_vm_launch_finish(struct kvm_vm *vm) > TEST_ASSERT_EQ(status.state, SEV_GUEST_STATE_RUNNING); > } > > +void snp_vm_launch_start(struct kvm_vm *vm, uint64_t policy) > +{ > + int ret = __snp_vm_launch_start(vm, policy, 0); > + > + TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_SNP_LAUNCH_START, ret, vm); Please use vm_ioctl(). TEST_ASSERT_VM_VCPU_IOCTL() should pretty much never be used directly, the only exceptions are cases where '0' is not the only success value, e.g. for ioctls that return a file descriptor. > +static void guest_snp_code(void) > +{ > + GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ENABLED); > + GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ES_ENABLED); > + GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_SNP_ENABLED); Read the MSR once. > + > + wrmsr(MSR_AMD64_SEV_ES_GHCB, GHCB_MSR_TERM_REQ); > + __asm__ __volatile__("rep; vmmcall"); Please add a vmgexit() helper (which I should have done as part of commit 40e09b3ccfac ("KVM: selftests: Add a basic SEV-ES smoke test")). > +} > + > static void guest_sev_es_code(void) > { > /* TODO: Check CPUID after GHCB-based hypercall support is added. */ > @@ -61,7 +82,7 @@ static void compare_xsave(u8 *from_host, u8 *from_guest) > abort(); > } > > -static void test_sync_vmsa(uint32_t policy) > +static void test_sync_vmsa(uint32_t type, uint64_t policy) > { > struct kvm_vcpu *vcpu; > struct kvm_vm *vm; > @@ -77,7 +98,10 @@ static void test_sync_vmsa(uint32_t policy) > .xcrs[0].value = XFEATURE_MASK_X87_AVX, > }; > > - vm = vm_sev_create_with_one_vcpu(KVM_X86_SEV_ES_VM, guest_code_xsave, &vcpu); > + TEST_ASSERT(type != KVM_X86_SEV_VM, > + "sync_vmsa only supported for SEV-ES and SNP VM types"); > + > + vm = vm_sev_create_with_one_vcpu(type, guest_code_xsave, &vcpu); > gva = vm_vaddr_alloc_shared(vm, PAGE_SIZE, KVM_UTIL_MIN_VADDR, > MEM_REGION_TEST_DATA); > hva = addr_gva2hva(vm, gva); > @@ -99,7 +123,7 @@ static void test_sync_vmsa(uint32_t policy) > : "ymm4", "st", "st(1)", "st(2)", "st(3)", "st(4)", "st(5)", "st(6)", "st(7)"); > vcpu_xsave_set(vcpu, &xsave); > > - vm_sev_launch(vm, SEV_POLICY_ES | policy, NULL); > + vm_sev_launch(vm, policy, NULL); > > /* This page is shared, so make it decrypted. */ > memset(hva, 0, 4096); > @@ -118,14 +142,12 @@ static void test_sync_vmsa(uint32_t policy) > kvm_vm_free(vm); > } > > -static void test_sev(void *guest_code, uint64_t policy) > +static void test_sev(void *guest_code, uint32_t type, uint64_t policy) > { > struct kvm_vcpu *vcpu; > struct kvm_vm *vm; > struct ucall uc; > > - uint32_t type = policy & SEV_POLICY_ES ? KVM_X86_SEV_ES_VM : KVM_X86_SEV_VM; > - > vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu); > > /* TODO: Validate the measurement is as expected. */ > @@ -134,7 +156,7 @@ static void test_sev(void *guest_code, uint64_t policy) > for (;;) { > vcpu_run(vcpu); > > - if (policy & SEV_POLICY_ES) { > + if (vm->type == KVM_X86_SEV_ES_VM || vm->type == KVM_X86_SNP_VM) { > TEST_ASSERT(vcpu->run->exit_reason == KVM_EXIT_SYSTEM_EVENT, > "Wanted SYSTEM_EVENT, got %s", > exit_reason_str(vcpu->run->exit_reason)); > @@ -194,19 +216,38 @@ int main(int argc, char *argv[]) > { > TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SEV)); > > - test_sev(guest_sev_code, SEV_POLICY_NO_DBG); > - test_sev(guest_sev_code, 0); > + test_sev(guest_sev_code, KVM_X86_SEV_VM, SEV_POLICY_NO_DBG); > + test_sev(guest_sev_code, KVM_X86_SEV_VM, 0); To cut down on the amount of copy+paste, and line lengths, I vote to add separate wrappers, e.g. test_sev(<policy>) test_sev_es(<policy>) test_sev_snp(<policy>); > > if (kvm_cpu_has(X86_FEATURE_SEV_ES)) { > - test_sev(guest_sev_es_code, SEV_POLICY_ES | SEV_POLICY_NO_DBG); > - test_sev(guest_sev_es_code, SEV_POLICY_ES); > + test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES | SEV_POLICY_NO_DBG); Please wrap at ~80 chars. > + test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES); > > test_sev_es_shutdown(); > > if (kvm_has_cap(KVM_CAP_XCRS) && > (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) { > - test_sync_vmsa(0); > - test_sync_vmsa(SEV_POLICY_NO_DBG); > + test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES); > + test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES | SEV_POLICY_NO_DBG); > + } > + } > + > + if (kvm_cpu_has(X86_FEATURE_SNP) && is_kvm_snp_supported()) { Why do we need both? KVM shouldn't advertise SNP if it's not supported. > + unsigned long snp_policy = SNP_POLICY; u64, no? > + > + if (unlikely(!is_smt_active())) > + snp_policy &= ~SNP_POLICY_SMT; Why does SNP_POLICY assume SMT? And what is RSVD_MBO? E.g. why not this? u64 policy = is_smt_active() ? SNP_POLICY_SMT : SNP_POLICY; > + > + test_sev(guest_snp_code, KVM_X86_SNP_VM, snp_policy); > + /* Test minimum firmware level */ > + test_sev(guest_snp_code, KVM_X86_SNP_VM, > + snp_policy | > + SNP_FW_VER_MAJOR(SNP_FW_REQ_VER_MAJOR) | > + SNP_FW_VER_MINOR(SNP_FW_REQ_VER_MINOR)); > + > + if (kvm_has_cap(KVM_CAP_XCRS) && > + (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) { Curly braces are unnecessary. > + test_sync_vmsa(KVM_X86_SNP_VM, snp_policy); > } > } > > -- > 2.34.1 >
Hi Sean, Thank you for your comments. ... >> .../selftests/kvm/include/x86_64/processor.h | 1 + >> .../selftests/kvm/include/x86_64/sev.h | 54 +++++++- >> tools/testing/selftests/kvm/lib/kvm_util.c | 8 +- >> .../selftests/kvm/lib/x86_64/processor.c | 6 +- >> tools/testing/selftests/kvm/lib/x86_64/sev.c | 116 +++++++++++++++++- >> .../selftests/kvm/x86_64/sev_smoke_test.c | 67 ++++++++-- >> 6 files changed, 230 insertions(+), 22 deletions(-) > > There is *way* too much going on in this one patch. There's at least 6+ patches > worth of stuff here: > > 1. Add x86 architectural defines > 2. SNP ioctl() plumbing > 3..N. Other misc plumbing, e.g. is_smt_active() > N+1. __vm_create() change to force GUEST_MEMFD for SNP > N+2. Whatever the ASSERT_SEV_POLICY() thing is doing, if it's actually necessary > N+3..M. Refactorings to existing code to prep for SNP > M+1. SNP support > > In general, if you feel the urge to start a changelog paragraph with "Also," > that's a sign you need to break up the patch. Sure. I will split this into multiple patches which should form the basis of the #1 patch series. > >> @@ -74,8 +95,18 @@ kvm_static_assert(SEV_RET_SUCCESS == 0); >> __TEST_ASSERT_VM_VCPU_IOCTL(!ret, #cmd, ret, vm); \ >> }) >> >> +/* Ensure policy is within bounds for SEV, SEV-ES */ >> +#define ASSERT_SEV_POLICY(type, policy) \ >> +({ \ >> + if (type == KVM_X86_SEV_VM || type == KVM_X86_SEV_ES_VM) { \ >> + TEST_ASSERT(policy < ((uint32_t)~0U), \ >> + "Policy beyond bounds for SEV"); \ > > This is asinine. First, there's one user, i.e. I see no reason to have a funky > macro to assert on the type. Second, _if_ this is a common macro, "type" can and > should be incorporated into the assert. Third, I have no idea why selftests is > validating its own inputs to KVM. It wasn't strictly necessary to validate this. Since the function vm_sev_launch() now took a u64 in policy (for SNP), I thought it maybe useful to validate u32 for the rest, as library function can be called by other selftests as well. However, I do see your point that it doesn't make too much sense for selftests to try and validate it's own inputs. I'm open to both - reducing the macro to a just a check within the function or removing the macro altogether. > >> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c >> index 974bcd2df6d7..981f3c9fd1cf 100644 >> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c >> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c >> @@ -625,7 +625,8 @@ void kvm_arch_vm_post_create(struct kvm_vm *vm) >> sync_global_to_guest(vm, host_cpu_is_amd); >> sync_global_to_guest(vm, is_forced_emulation_enabled); >> >> - if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) { >> + if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM || >> + vm->type == KVM_X86_SNP_VM) { > > Probably time to add a helper, e.g. is_sev_vm() or something. If we follow KVM's > lead, then we'd have is_sev_vm(), is_sev_es_vm(), and is_sev_snp_vm(), where an > SNP VM returns true for all of them. Not sure I love that idea, just throwing it > out there as one possibility. Agreed. It will be cleaner to have helpers since similar checks are being made in multiple places. > >> struct kvm_sev_init init = { 0 }; >> >> vm_sev_ioctl(vm, KVM_SEV_INIT2, &init); >> @@ -1134,7 +1135,8 @@ void kvm_get_cpu_address_width(unsigned int *pa_bits, unsigned int *va_bits) >> >> void kvm_init_vm_address_properties(struct kvm_vm *vm) >> { >> - if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) { >> + if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM || >> + vm->type == KVM_X86_SNP_VM) { >> vm->arch.sev_fd = open_sev_dev_path_or_exit(); >> vm->arch.c_bit = BIT_ULL(this_cpu_property(X86_PROPERTY_SEV_C_BIT)); >> vm->gpa_tag_mask = vm->arch.c_bit; >> diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c b/tools/testing/selftests/kvm/lib/x86_64/sev.c >> index 125a72246e09..ff3824564854 100644 >> --- a/tools/testing/selftests/kvm/lib/x86_64/sev.c >> +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c >> @@ -14,7 +14,8 @@ >> * and find the first range, but that's correct because the condition >> * expression would cause us to quit the loop. >> */ >> -static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region) >> +static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region, >> + uint8_t page_type) >> { >> const struct sparsebit *protected_phy_pages = region->protected_phy_pages; >> const vm_paddr_t gpa_base = region->region.guest_phys_addr; >> @@ -25,12 +26,23 @@ static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region >> if (!sparsebit_any_set(protected_phy_pages)) >> return 0; >> >> - sev_register_encrypted_memory(vm, region); >> + if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) > > And then this would be > > if (!is_sev_snp_vm()) Ack. > >> + sev_register_encrypted_memory(vm, region); >> >> sparsebit_for_each_set_range(protected_phy_pages, i, j) { >> const uint64_t size = (j - i + 1) * vm->page_size; >> const uint64_t offset = (i - lowest_page_in_region) * vm->page_size; >> >> + if (vm->type == KVM_X86_SNP_VM) { >> + vm_mem_set_private(vm, gpa_base + offset, size); > > Setting memory private seems like something that should be done by common code, > not by SNP specific code. That's fair. I will make a helper for this and call this common code separate from encrypt_region() > >> @@ -158,6 +213,45 @@ void sev_vm_launch_finish(struct kvm_vm *vm) >> TEST_ASSERT_EQ(status.state, SEV_GUEST_STATE_RUNNING); >> } >> >> +void snp_vm_launch_start(struct kvm_vm *vm, uint64_t policy) >> +{ >> + int ret = __snp_vm_launch_start(vm, policy, 0); >> + >> + TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_SNP_LAUNCH_START, ret, vm); > > Please use vm_ioctl(). TEST_ASSERT_VM_VCPU_IOCTL() should pretty much never be > used directly, the only exceptions are cases where '0' is not the only success > value, e.g. for ioctls that return a file descriptor. Sure. This was done for trying to be inclusive for the negative cases by decoupling ioctl calls from their asserts. Since, code for negative tests is are going to architected separately altogether, I will make sure to clean this up with vm_ioctl() everywhere. >> +static void guest_snp_code(void) >> +{ >> + GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ENABLED); >> + GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ES_ENABLED); >> + GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_SNP_ENABLED); > > Read the MSR once. > >> + >> + wrmsr(MSR_AMD64_SEV_ES_GHCB, GHCB_MSR_TERM_REQ); >> + __asm__ __volatile__("rep; vmmcall"); > > Please add a vmgexit() helper (which I should have done as part of commit > 40e09b3ccfac ("KVM: selftests: Add a basic SEV-ES smoke test")). > Sure, will do so for this and apply to the other guest_code as well. >> +} >> + >> static void guest_sev_es_code(void) >> { >> /* TODO: Check CPUID after GHCB-based hypercall support is added. */ >> @@ -61,7 +82,7 @@ static void compare_xsave(u8 *from_host, u8 *from_guest) >> abort(); >> } >> >> -static void test_sync_vmsa(uint32_t policy) >> +static void test_sync_vmsa(uint32_t type, uint64_t policy) >> { >> struct kvm_vcpu *vcpu; >> struct kvm_vm *vm; >> @@ -77,7 +98,10 @@ static void test_sync_vmsa(uint32_t policy) >> .xcrs[0].value = XFEATURE_MASK_X87_AVX, >> }; >> >> - vm = vm_sev_create_with_one_vcpu(KVM_X86_SEV_ES_VM, guest_code_xsave, &vcpu); >> + TEST_ASSERT(type != KVM_X86_SEV_VM, >> + "sync_vmsa only supported for SEV-ES and SNP VM types"); >> + >> + vm = vm_sev_create_with_one_vcpu(type, guest_code_xsave, &vcpu); >> gva = vm_vaddr_alloc_shared(vm, PAGE_SIZE, KVM_UTIL_MIN_VADDR, >> MEM_REGION_TEST_DATA); >> hva = addr_gva2hva(vm, gva); >> @@ -99,7 +123,7 @@ static void test_sync_vmsa(uint32_t policy) >> : "ymm4", "st", "st(1)", "st(2)", "st(3)", "st(4)", "st(5)", "st(6)", "st(7)"); >> vcpu_xsave_set(vcpu, &xsave); >> >> - vm_sev_launch(vm, SEV_POLICY_ES | policy, NULL); >> + vm_sev_launch(vm, policy, NULL); >> >> /* This page is shared, so make it decrypted. */ >> memset(hva, 0, 4096); >> @@ -118,14 +142,12 @@ static void test_sync_vmsa(uint32_t policy) >> kvm_vm_free(vm); >> } >> >> -static void test_sev(void *guest_code, uint64_t policy) >> +static void test_sev(void *guest_code, uint32_t type, uint64_t policy) >> { >> struct kvm_vcpu *vcpu; >> struct kvm_vm *vm; >> struct ucall uc; >> >> - uint32_t type = policy & SEV_POLICY_ES ? KVM_X86_SEV_ES_VM : KVM_X86_SEV_VM; >> - >> vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu); >> >> /* TODO: Validate the measurement is as expected. */ >> @@ -134,7 +156,7 @@ static void test_sev(void *guest_code, uint64_t policy) >> for (;;) { >> vcpu_run(vcpu); >> >> - if (policy & SEV_POLICY_ES) { >> + if (vm->type == KVM_X86_SEV_ES_VM || vm->type == KVM_X86_SNP_VM) { >> TEST_ASSERT(vcpu->run->exit_reason == KVM_EXIT_SYSTEM_EVENT, >> "Wanted SYSTEM_EVENT, got %s", >> exit_reason_str(vcpu->run->exit_reason)); >> @@ -194,19 +216,38 @@ int main(int argc, char *argv[]) >> { >> TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SEV)); >> >> - test_sev(guest_sev_code, SEV_POLICY_NO_DBG); >> - test_sev(guest_sev_code, 0); >> + test_sev(guest_sev_code, KVM_X86_SEV_VM, SEV_POLICY_NO_DBG); >> + test_sev(guest_sev_code, KVM_X86_SEV_VM, 0); > > To cut down on the amount of copy+paste, and line lengths, I vote to add separate > wrappers, e.g. > > test_sev(<policy>) > test_sev_es(<policy>) > test_sev_snp(<policy>); Sure. >> >> if (kvm_cpu_has(X86_FEATURE_SEV_ES)) { >> - test_sev(guest_sev_es_code, SEV_POLICY_ES | SEV_POLICY_NO_DBG); >> - test_sev(guest_sev_es_code, SEV_POLICY_ES); >> + test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES | SEV_POLICY_NO_DBG); > > Please wrap at ~80 chars. Ack. Will do. > > >> + test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES); >> >> test_sev_es_shutdown(); >> >> if (kvm_has_cap(KVM_CAP_XCRS) && >> (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) { >> - test_sync_vmsa(0); >> - test_sync_vmsa(SEV_POLICY_NO_DBG); >> + test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES); >> + test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES | SEV_POLICY_NO_DBG); >> + } >> + } >> + >> + if (kvm_cpu_has(X86_FEATURE_SNP) && is_kvm_snp_supported()) { > > Why do we need both? KVM shouldn't advertise SNP if it's not supported. My rationale behind needing this was that the feature can be advertised but it may not have the right API major or minor release which could be updated post boot and could determine it's support during runtime. > >> + unsigned long snp_policy = SNP_POLICY; > > u64, no? Yes, sorry for the oversight. Will change it to u64. > >> + >> + if (unlikely(!is_smt_active())) >> + snp_policy &= ~SNP_POLICY_SMT; > > Why does SNP_POLICY assume SMT? And what is RSVD_MBO? E.g. why not this? > > u64 policy = is_smt_active() ? SNP_POLICY_SMT : SNP_POLICY; > I think most systems support SMT so I enabled the bit in by default and only unset it when there isn't any support. RSVD_MBO is a reserved bit that must be set to one for SNP guest policy to be enabled. The FW spec specifies this - Sec 4.3 Pg 27 - Guest Policy https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf >> + >> + test_sev(guest_snp_code, KVM_X86_SNP_VM, snp_policy); >> + /* Test minimum firmware level */ >> + test_sev(guest_snp_code, KVM_X86_SNP_VM, >> + snp_policy | >> + SNP_FW_VER_MAJOR(SNP_FW_REQ_VER_MAJOR) | >> + SNP_FW_VER_MINOR(SNP_FW_REQ_VER_MINOR)); >> + >> + if (kvm_has_cap(KVM_CAP_XCRS) && >> + (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) { > > Curly braces are unnecessary. Ack. Thanks again for the detailed review!
On Mon, Oct 21, 2024, Pratik R. Sampat wrote: > >> + test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES); > >> > >> test_sev_es_shutdown(); > >> > >> if (kvm_has_cap(KVM_CAP_XCRS) && > >> (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) { > >> - test_sync_vmsa(0); > >> - test_sync_vmsa(SEV_POLICY_NO_DBG); > >> + test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES); > >> + test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES | SEV_POLICY_NO_DBG); > >> + } > >> + } > >> + > >> + if (kvm_cpu_has(X86_FEATURE_SNP) && is_kvm_snp_supported()) { > > > > Why do we need both? KVM shouldn't advertise SNP if it's not supported. > > My rationale behind needing this was that the feature can be advertised > but it may not have the right API major or minor release which could be > updated post boot and could determine it's support during runtime. KVM will never determine support after KVM has been loaded. If *KVM* has a dependency on the API major.minor, then X86_FEATURE_SNP must be set if and only if the supported API version is available. If the API major.minor is purely a userspace thing, then is_kvm_snp_supported() is misnamed, because the check has nothing to do with KVM. E.g. something like is_snp_api_version_supported() would be more appropriate. > >> + unsigned long snp_policy = SNP_POLICY; > > > > u64, no? > > Yes, sorry for the oversight. Will change it to u64. > > > > >> + > >> + if (unlikely(!is_smt_active())) > >> + snp_policy &= ~SNP_POLICY_SMT; > > > > Why does SNP_POLICY assume SMT? And what is RSVD_MBO? E.g. why not this? > > > > u64 policy = is_smt_active() ? SNP_POLICY_SMT : SNP_POLICY; > > > > I think most systems support SMT so I enabled the bit in by default and > only unset it when there isn't any support. That's confusing though, because you're mixing architectural defines with semi- arbitrary selftests behavior. RSVD_MBO on the other is apparently tightly coupled with SNP, i.e. SNP can't exist without that bit, so it makes sense that RSVD_MBO needs to be part of SNP_POLICY If you want to have a *software*-defined default policy, then make it obvious that it's software defined. E.g. name the #define SNP_DEFAULT_POLICY, not simply SNP_POLICY, because the latter is too easily misconstrued as the base SNP policy, which it is not. That said, IIUC, SMT *must* match the host configuration, i.e. whether or not SMT is set is non-negotiable. In that case, there's zero value in defining SNP_DEFAULT_POLICY, because it can't be a sane default for all systems. Side topic, I assume one of SEV_POLICY_NO_DBG or SNP_POLICY_DBG *must* be specified, and that they are mutualy exclusive? E.g. what happens if the full policy is simply SNP_POLICY_RSVD_MBO?
Hi Sean, On 10/28/2024 12:55 PM, Sean Christopherson wrote: > On Mon, Oct 21, 2024, Pratik R. Sampat wrote: >>>> + test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES); >>>> >>>> test_sev_es_shutdown(); >>>> >>>> if (kvm_has_cap(KVM_CAP_XCRS) && >>>> (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) { >>>> - test_sync_vmsa(0); >>>> - test_sync_vmsa(SEV_POLICY_NO_DBG); >>>> + test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES); >>>> + test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES | SEV_POLICY_NO_DBG); >>>> + } >>>> + } >>>> + >>>> + if (kvm_cpu_has(X86_FEATURE_SNP) && is_kvm_snp_supported()) { >>> >>> Why do we need both? KVM shouldn't advertise SNP if it's not supported. >> >> My rationale behind needing this was that the feature can be advertised >> but it may not have the right API major or minor release which could be >> updated post boot and could determine it's support during runtime. > > KVM will never determine support after KVM has been loaded. If *KVM* has a > dependency on the API major.minor, then X86_FEATURE_SNP must be set if and only > if the supported API version is available. > > If the API major.minor is purely a userspace thing, then is_kvm_snp_supported() > is misnamed, because the check has nothing to do with KVM. E.g. something like > is_snp_api_version_supported() would be more appropriate. That's fair. It is related to the FW supplied to it from userspace and naming it with kvm prefix is misleading. I'll change that. > >>>> + unsigned long snp_policy = SNP_POLICY; >>> >>> u64, no? >> >> Yes, sorry for the oversight. Will change it to u64. >> >>> >>>> + >>>> + if (unlikely(!is_smt_active())) >>>> + snp_policy &= ~SNP_POLICY_SMT; >>> >>> Why does SNP_POLICY assume SMT? And what is RSVD_MBO? E.g. why not this? >>> >>> u64 policy = is_smt_active() ? SNP_POLICY_SMT : SNP_POLICY; >>> >> >> I think most systems support SMT so I enabled the bit in by default and >> only unset it when there isn't any support. > > That's confusing though, because you're mixing architectural defines with semi- > arbitrary selftests behavior. RSVD_MBO on the other is apparently tightly coupled > with SNP, i.e. SNP can't exist without that bit, so it makes sense that RSVD_MBO > needs to be part of SNP_POLICY > > If you want to have a *software*-defined default policy, then make it obvious that > it's software defined. E.g. name the #define SNP_DEFAULT_POLICY, not simply > SNP_POLICY, because the latter is too easily misconstrued as the base SNP policy, > which it is not. That said, IIUC, SMT *must* match the host configuration, i.e. > whether or not SMT is set is non-negotiable. In that case, there's zero value in > defining SNP_DEFAULT_POLICY, because it can't be a sane default for all systems. > Right, SMT should match the host configuration. Would a SNP_DEFAULT_POLICY work if we made it check for SMT too in the macro? Instead of, #define SNP_POLICY (SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO) Have something like this instead to make it generic and less ambiguous? #define SNP_DEFAULT_POLICY() \ ({ \ SNP_POLICY_RSVD_MBO | (is_smt_active() ? SNP_POLICY_SMT : 0); \ }) > Side topic, I assume one of SEV_POLICY_NO_DBG or SNP_POLICY_DBG *must* be specified, > and that they are mutualy exclusive? E.g. what happens if the full policy is simply > SNP_POLICY_RSVD_MBO? SEV_POLICY_NO_DBG is mainly for the guest policy structure of SEV and SEV-ES - pg 31, Table 2 https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/programmer-references/55766_SEV-KM_API_Specification.pdf and, SNP_POLICY_DBG is a bit in the guest policy structure of SNP - pg 27, Table 9 https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf In the former, a SEV guest disables debugging if SEV_POLICY_NO_DBG is set. Similarly, a SNP guest enables debugging if SNP_POLICY_DBG is set. An SNP guest can certainly just have the policy SNP_POLICY_RSVD_MBO, barring the case on a SMT system where that bit must be set too for a successful launch.
On Mon, Oct 28, 2024, Pratik R. Sampat wro4te: > On 10/28/2024 12:55 PM, Sean Christopherson wrote: > > On Mon, Oct 21, 2024, Pratik R. Sampat wrote: > >>>> + if (unlikely(!is_smt_active())) > >>>> + snp_policy &= ~SNP_POLICY_SMT; > >>> > >>> Why does SNP_POLICY assume SMT? And what is RSVD_MBO? E.g. why not this? > >>> > >>> u64 policy = is_smt_active() ? SNP_POLICY_SMT : SNP_POLICY; > >>> > >> > >> I think most systems support SMT so I enabled the bit in by default and > >> only unset it when there isn't any support. > > > > That's confusing though, because you're mixing architectural defines with semi- > > arbitrary selftests behavior. RSVD_MBO on the other is apparently tightly coupled > > with SNP, i.e. SNP can't exist without that bit, so it makes sense that RSVD_MBO > > needs to be part of SNP_POLICY > > > > If you want to have a *software*-defined default policy, then make it obvious that > > it's software defined. E.g. name the #define SNP_DEFAULT_POLICY, not simply > > SNP_POLICY, because the latter is too easily misconstrued as the base SNP policy, > > which it is not. That said, IIUC, SMT *must* match the host configuration, i.e. > > whether or not SMT is set is non-negotiable. In that case, there's zero value in > > defining SNP_DEFAULT_POLICY, because it can't be a sane default for all systems. > > > > Right, SMT should match the host configuration. Would a > SNP_DEFAULT_POLICY work if we made it check for SMT too in the macro? > > Instead of, > #define SNP_POLICY (SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO) > > Have something like this instead to make it generic and less ambiguous? > #define SNP_DEFAULT_POLICY() \ > ({ \ > SNP_POLICY_RSVD_MBO | (is_smt_active() ? SNP_POLICY_SMT : 0); \ > }) No, unless it's the least awful option, don't hide dynamic functionality in a macro that looks like it holds static data. The idea is totally fine, but put it in an actual helper, not a macro, _if_ there's actually a need for a default policy. If there's only ever one main path that creates SNP VMs, then I don't see the point in specifying a default policy. > > Side topic, I assume one of SEV_POLICY_NO_DBG or SNP_POLICY_DBG *must* be specified, > > and that they are mutualy exclusive? E.g. what happens if the full policy is simply > > SNP_POLICY_RSVD_MBO? > > SEV_POLICY_NO_DBG is mainly for the guest policy structure of SEV and > SEV-ES - pg 31, Table 2 > https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/programmer-references/55766_SEV-KM_API_Specification.pdf > > and, SNP_POLICY_DBG is a bit in the guest policy structure of SNP - pg > 27, Table 9 > https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf > > In the former, a SEV guest disables debugging if SEV_POLICY_NO_DBG is > set. Similarly, a SNP guest enables debugging if SNP_POLICY_DBG is set. Ugh, one is SEV_xxx, the other is SNP_xxx. Argh! And IIUC, they are mutually exclusive (totally separate thigns?), because SNP guests use an 8-byte structure, whereas SEV/SEV-ES use a 4-byte structure, and with different layouts. That means this is _extremely_ confusing. Separate the SEV_xxx defines from the SNP_xxx defines, because other than a name, they have nothing in common. +/* Minimum firmware version required for the SEV-SNP support */ +#define SNP_FW_REQ_VER_MAJOR 1 +#define SNP_FW_REQ_VER_MINOR 51 Side topic, why are these hardcoded? And where did they come from? If they're arbitrary KVM selftests values, make that super duper clear. +#define SNP_POLICY_MINOR_BIT 0 +#define SNP_POLICY_MAJOR_BIT 8 s/BIT/SHIFT. "BIT" implies they are a single bit, which is obviously not the case. But I vote to omit the extra #define entirely and just open code the shift in the SNP_FW_VER_{MAJOR,MINOR} macros. #define SEV_POLICY_NO_DBG (1UL << 0) #define SEV_POLICY_ES (1UL << 2) +#define SNP_POLICY_SMT (1ULL << 16) +#define SNP_POLICY_RSVD_MBO (1ULL << 17) +#define SNP_POLICY_DBG (1ULL << 19) +#define SNP_POLICY (SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO) + +#define SNP_FW_VER_MAJOR(maj) ((uint8_t)(maj) << SNP_POLICY_MAJOR_BIT) +#define SNP_FW_VER_MINOR(min) ((uint8_t)(min) << SNP_POLICY_MINOR_BIT)
Hi Sean, On 10/30/2024 8:46 AM, Sean Christopherson wrote: > On Mon, Oct 28, 2024, Pratik R. Sampat wro4te: >> On 10/28/2024 12:55 PM, Sean Christopherson wrote: >>> On Mon, Oct 21, 2024, Pratik R. Sampat wrote: >>>>>> + if (unlikely(!is_smt_active())) >>>>>> + snp_policy &= ~SNP_POLICY_SMT; >>>>> >>>>> Why does SNP_POLICY assume SMT? And what is RSVD_MBO? E.g. why not this? >>>>> >>>>> u64 policy = is_smt_active() ? SNP_POLICY_SMT : SNP_POLICY; >>>>> >>>> >>>> I think most systems support SMT so I enabled the bit in by default and >>>> only unset it when there isn't any support. >>> >>> That's confusing though, because you're mixing architectural defines with semi- >>> arbitrary selftests behavior. RSVD_MBO on the other is apparently tightly coupled >>> with SNP, i.e. SNP can't exist without that bit, so it makes sense that RSVD_MBO >>> needs to be part of SNP_POLICY >>> >>> If you want to have a *software*-defined default policy, then make it obvious that >>> it's software defined. E.g. name the #define SNP_DEFAULT_POLICY, not simply >>> SNP_POLICY, because the latter is too easily misconstrued as the base SNP policy, >>> which it is not. That said, IIUC, SMT *must* match the host configuration, i.e. >>> whether or not SMT is set is non-negotiable. In that case, there's zero value in >>> defining SNP_DEFAULT_POLICY, because it can't be a sane default for all systems. >>> >> >> Right, SMT should match the host configuration. Would a >> SNP_DEFAULT_POLICY work if we made it check for SMT too in the macro? >> >> Instead of, >> #define SNP_POLICY (SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO) >> >> Have something like this instead to make it generic and less ambiguous? >> #define SNP_DEFAULT_POLICY() \ >> ({ \ >> SNP_POLICY_RSVD_MBO | (is_smt_active() ? SNP_POLICY_SMT : 0); \ >> }) > > No, unless it's the least awful option, don't hide dynamic functionality in a macro > that looks like it holds static data. The idea is totally fine, but put it in an > actual helper, not a macro, _if_ there's actually a need for a default policy. > If there's only ever one main path that creates SNP VMs, then I don't see the point > in specifying a default policy. > Currently, there just seems to be one path of doing (later the prefault tests exercise it) so I'm not too averse to just dropping it and having what bits needs to be set during the main path. I had only introduced it so that it would be easy to specify a minimal working SNP policy as it was for SEV and SEV-ES without too much ambiguity. But if it's causing more issues than resolving, I can definitely get rid of it. >>> Side topic, I assume one of SEV_POLICY_NO_DBG or SNP_POLICY_DBG *must* be specified, >>> and that they are mutualy exclusive? E.g. what happens if the full policy is simply >>> SNP_POLICY_RSVD_MBO? >> >> SEV_POLICY_NO_DBG is mainly for the guest policy structure of SEV and >> SEV-ES - pg 31, Table 2 >> https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/programmer-references/55766_SEV-KM_API_Specification.pdf >> >> and, SNP_POLICY_DBG is a bit in the guest policy structure of SNP - pg >> 27, Table 9 >> https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf >> >> In the former, a SEV guest disables debugging if SEV_POLICY_NO_DBG is >> set. Similarly, a SNP guest enables debugging if SNP_POLICY_DBG is set. > > Ugh, one is SEV_xxx, the other is SNP_xxx. Argh! And IIUC, they are mutually > exclusive (totally separate thigns?), because SNP guests use an 8-byte structure, > whereas SEV/SEV-ES use a 4-byte structure, and with different layouts. > > That means this is _extremely_ confusing. Separate the SEV_xxx defines from the > SNP_xxx defines, because other than a name, they have nothing in common. > Right. I see how that can be confusing. Sure I can make sure not to bundle up these defines together. > +/* Minimum firmware version required for the SEV-SNP support */ > +#define SNP_FW_REQ_VER_MAJOR 1 > +#define SNP_FW_REQ_VER_MINOR 51 > > Side topic, why are these hardcoded? And where did they come from? If they're > arbitrary KVM selftests values, make that super duper clear. Well, it's not entirely arbitrary. This was the version that SNP GA'd with first so that kind of became the minimum required version needed. I think the only place we've documented this is here - https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware. Maybe, I can modify the comment above to say something like - Minimum general availability release firmware required for SEV-SNP support. > > +#define SNP_POLICY_MINOR_BIT 0 > +#define SNP_POLICY_MAJOR_BIT 8 > > s/BIT/SHIFT. "BIT" implies they are a single bit, which is obviously not the > case. But I vote to omit the extra #define entirely and just open code the shift > in the SNP_FW_VER_{MAJOR,MINOR} macros. Sure, I'll get rid of those couple of #defines and use them directly in the macros. Thanks! Pratik > > #define SEV_POLICY_NO_DBG (1UL << 0) > #define SEV_POLICY_ES (1UL << 2) > +#define SNP_POLICY_SMT (1ULL << 16) > +#define SNP_POLICY_RSVD_MBO (1ULL << 17) > +#define SNP_POLICY_DBG (1ULL << 19) > +#define SNP_POLICY (SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO) > + > +#define SNP_FW_VER_MAJOR(maj) ((uint8_t)(maj) << SNP_POLICY_MAJOR_BIT) > +#define SNP_FW_VER_MINOR(min) ((uint8_t)(min) << SNP_POLICY_MINOR_BIT)
On Wed, Oct 30, 2024, Pratik R. Sampat wrote: > On 10/30/2024 8:46 AM, Sean Christopherson wrote: > > +/* Minimum firmware version required for the SEV-SNP support */ > > +#define SNP_FW_REQ_VER_MAJOR 1 > > +#define SNP_FW_REQ_VER_MINOR 51 > > > > Side topic, why are these hardcoded? And where did they come from? If they're > > arbitrary KVM selftests values, make that super duper clear. > > Well, it's not entirely arbitrary. This was the version that SNP GA'd > with first so that kind of became the minimum required version needed. > > I think the only place we've documented this is here - > https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware. > > Maybe, I can modify the comment above to say something like - > Minimum general availability release firmware required for SEV-SNP support. Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, why on earth is that not checked and enforced by the kernel? Relying on userspace to not crash the host (or worse) because of unsupported firmware is not a winning strategy.
Hi Sean, On 10/30/2024 12:57 PM, Sean Christopherson wrote: > On Wed, Oct 30, 2024, Pratik R. Sampat wrote: >> On 10/30/2024 8:46 AM, Sean Christopherson wrote: >>> +/* Minimum firmware version required for the SEV-SNP support */ >>> +#define SNP_FW_REQ_VER_MAJOR 1 >>> +#define SNP_FW_REQ_VER_MINOR 51 >>> >>> Side topic, why are these hardcoded? And where did they come from? If they're >>> arbitrary KVM selftests values, make that super duper clear. >> >> Well, it's not entirely arbitrary. This was the version that SNP GA'd >> with first so that kind of became the minimum required version needed. >> >> I think the only place we've documented this is here - >> https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware. >> >> Maybe, I can modify the comment above to say something like - >> Minimum general availability release firmware required for SEV-SNP support. > > Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, why on > earth is that not checked and enforced by the kernel? Relying on userspace to > not crash the host (or worse) because of unsupported firmware is not a winning > strategy. We do check against the firmware level 1.51 while setting things up first (drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked()) and we bail out if it's otherwise. From the userspace, calls to KVM_SEV_INIT2 or any other corresponding SNP calls should fail cleanly without any adverse effects to the host. From the positive selftest perspective though, we want to make sure it's both supported and enabled, and skip the test if not. I believe we can tell if it's supported by the platform using the MSR - MSR_AMD64_SEV_SNP_ENABLED or the X86_FEATURE_SEV_SNP from the KVM capabilities. However, to determine if it's enabled from the kernel, I made this check here. Having said that, I do agree that there should probably be a better way to expose this support to the userspace. Thanks Pratik
On Thu, Oct 31, 2024, Pratik R. Sampat wrote: > Hi Sean, > > On 10/30/2024 12:57 PM, Sean Christopherson wrote: > > On Wed, Oct 30, 2024, Pratik R. Sampat wrote: > >> On 10/30/2024 8:46 AM, Sean Christopherson wrote: > >>> +/* Minimum firmware version required for the SEV-SNP support */ > >>> +#define SNP_FW_REQ_VER_MAJOR 1 > >>> +#define SNP_FW_REQ_VER_MINOR 51 > >>> > >>> Side topic, why are these hardcoded? And where did they come from? If they're > >>> arbitrary KVM selftests values, make that super duper clear. > >> > >> Well, it's not entirely arbitrary. This was the version that SNP GA'd > >> with first so that kind of became the minimum required version needed. > >> > >> I think the only place we've documented this is here - > >> https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware. > >> > >> Maybe, I can modify the comment above to say something like - > >> Minimum general availability release firmware required for SEV-SNP support. > > > > Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, why on > > earth is that not checked and enforced by the kernel? Relying on userspace to > > not crash the host (or worse) because of unsupported firmware is not a winning > > strategy. > > We do check against the firmware level 1.51 while setting things up > first (drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked()) and we bail > out if it's otherwise. From the userspace, calls to KVM_SEV_INIT2 or any > other corresponding SNP calls should fail cleanly without any adverse > effects to the host. And I'm saying, that's not good enough. If the platform doesn't support SNP, the KVM *must not* advertise support for SNP. > From the positive selftest perspective though, we want to make sure it's > both supported and enabled, and skip the test if not. No, we want the test to assert that KVM reports SNP support if and only if SNP is 100% supported. > I believe we can tell if it's supported by the platform using the MSR - > MSR_AMD64_SEV_SNP_ENABLED or the X86_FEATURE_SEV_SNP from the KVM > capabilities. However, to determine if it's enabled from the kernel, I > made this check here. Having said that, I do agree that there should > probably be a better way to expose this support to the userspace. > > Thanks > Pratik
On 10/31/2024 11:27 AM, Sean Christopherson wrote: > On Thu, Oct 31, 2024, Pratik R. Sampat wrote: >> Hi Sean, >> >> On 10/30/2024 12:57 PM, Sean Christopherson wrote: >>> On Wed, Oct 30, 2024, Pratik R. Sampat wrote: >>>> On 10/30/2024 8:46 AM, Sean Christopherson wrote: >>>>> +/* Minimum firmware version required for the SEV-SNP support */ >>>>> +#define SNP_FW_REQ_VER_MAJOR 1 >>>>> +#define SNP_FW_REQ_VER_MINOR 51 >>>>> >>>>> Side topic, why are these hardcoded? And where did they come from? If they're >>>>> arbitrary KVM selftests values, make that super duper clear. >>>> >>>> Well, it's not entirely arbitrary. This was the version that SNP GA'd >>>> with first so that kind of became the minimum required version needed. >>>> >>>> I think the only place we've documented this is here - >>>> https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware. >>>> >>>> Maybe, I can modify the comment above to say something like - >>>> Minimum general availability release firmware required for SEV-SNP support. >>> >>> Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, why on >>> earth is that not checked and enforced by the kernel? Relying on userspace to >>> not crash the host (or worse) because of unsupported firmware is not a winning >>> strategy. >> >> We do check against the firmware level 1.51 while setting things up >> first (drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked()) and we bail >> out if it's otherwise. From the userspace, calls to KVM_SEV_INIT2 or any >> other corresponding SNP calls should fail cleanly without any adverse >> effects to the host. > > And I'm saying, that's not good enough. If the platform doesn't support SNP, > the KVM *must not* advertise support for SNP. > Sure, fair to expect this. Currently, if the FW check fails, SNP is not setup and there is nothing that indicates in the KVM capabilities (apart from one dmesg error) that the support does not exist. One thing I could do (as an independent patch) is to introduce a CC API that abstracts the FW version check made by the CCP module. Since sev platform status can be gotten before INIT to extract the major and minor version numbers, KVM can also call into this API and use that to decide if the KVM capabilities for SNP must be set or not. Thanks! Pratik >> From the positive selftest perspective though, we want to make sure it's >> both supported and enabled, and skip the test if not. > > No, we want the test to assert that KVM reports SNP support if and only if SNP > is 100% supported. > >> I believe we can tell if it's supported by the platform using the MSR - >> MSR_AMD64_SEV_SNP_ENABLED or the X86_FEATURE_SEV_SNP from the KVM >> capabilities. However, to determine if it's enabled from the kernel, I >> made this check here. Having said that, I do agree that there should >> probably be a better way to expose this support to the userspace. >> >> Thanks >> Pratik
On Mon, Nov 04, 2024, Pratik R. Sampat wrote: > > > On 10/31/2024 11:27 AM, Sean Christopherson wrote: > > On Thu, Oct 31, 2024, Pratik R. Sampat wrote: > >> Hi Sean, > >> > >> On 10/30/2024 12:57 PM, Sean Christopherson wrote: > >>> On Wed, Oct 30, 2024, Pratik R. Sampat wrote: > >>>> On 10/30/2024 8:46 AM, Sean Christopherson wrote: > >>>>> +/* Minimum firmware version required for the SEV-SNP support */ > >>>>> +#define SNP_FW_REQ_VER_MAJOR 1 > >>>>> +#define SNP_FW_REQ_VER_MINOR 51 > >>>>> > >>>>> Side topic, why are these hardcoded? And where did they come from? If they're > >>>>> arbitrary KVM selftests values, make that super duper clear. > >>>> > >>>> Well, it's not entirely arbitrary. This was the version that SNP GA'd > >>>> with first so that kind of became the minimum required version needed. > >>>> > >>>> I think the only place we've documented this is here - > >>>> https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware. > >>>> > >>>> Maybe, I can modify the comment above to say something like - > >>>> Minimum general availability release firmware required for SEV-SNP support. > >>> > >>> Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, why on > >>> earth is that not checked and enforced by the kernel? Relying on userspace to > >>> not crash the host (or worse) because of unsupported firmware is not a winning > >>> strategy. > >> > >> We do check against the firmware level 1.51 while setting things up > >> first (drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked()) and we bail > >> out if it's otherwise. From the userspace, calls to KVM_SEV_INIT2 or any > >> other corresponding SNP calls should fail cleanly without any adverse > >> effects to the host. > > > > And I'm saying, that's not good enough. If the platform doesn't support SNP, > > the KVM *must not* advertise support for SNP. > > > > Sure, fair to expect this. Currently, if the FW check fails, SNP is not > setup and there is nothing that indicates in the KVM capabilities (apart > from one dmesg error) that the support does not exist. > > One thing I could do (as an independent patch) is to introduce a CC API > that abstracts the FW version check made by the CCP module. Since sev > platform status can be gotten before INIT to extract the major and minor > version numbers, KVM can also call into this API and use that to decide > if the KVM capabilities for SNP must be set or not. Why is CC_ATTR_HOST_SEV_SNP set if hardware/firmware can't actually support SNP? KVM shouldn't have to care about some arbitrary firmware API version, the whole point of a driver is so that KVM doesn't have to deal with such details. I'm a-ok with a KVM selftest *asserting* that the kernel isn't broken, but KVM itself shouldn't need to manually check the firmware version.
On 11/4/2024 5:47 PM, Sean Christopherson wrote: > On Mon, Nov 04, 2024, Pratik R. Sampat wrote: >> >> >> On 10/31/2024 11:27 AM, Sean Christopherson wrote: >>> On Thu, Oct 31, 2024, Pratik R. Sampat wrote: >>>> Hi Sean, >>>> >>>> On 10/30/2024 12:57 PM, Sean Christopherson wrote: >>>>> On Wed, Oct 30, 2024, Pratik R. Sampat wrote: >>>>>> On 10/30/2024 8:46 AM, Sean Christopherson wrote: >>>>>>> +/* Minimum firmware version required for the SEV-SNP support */ >>>>>>> +#define SNP_FW_REQ_VER_MAJOR 1 >>>>>>> +#define SNP_FW_REQ_VER_MINOR 51 >>>>>>> >>>>>>> Side topic, why are these hardcoded? And where did they come from? If they're >>>>>>> arbitrary KVM selftests values, make that super duper clear. >>>>>> >>>>>> Well, it's not entirely arbitrary. This was the version that SNP GA'd >>>>>> with first so that kind of became the minimum required version needed. >>>>>> >>>>>> I think the only place we've documented this is here - >>>>>> https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware. >>>>>> >>>>>> Maybe, I can modify the comment above to say something like - >>>>>> Minimum general availability release firmware required for SEV-SNP support. >>>>> >>>>> Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, why on >>>>> earth is that not checked and enforced by the kernel? Relying on userspace to >>>>> not crash the host (or worse) because of unsupported firmware is not a winning >>>>> strategy. >>>> >>>> We do check against the firmware level 1.51 while setting things up >>>> first (drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked()) and we bail >>>> out if it's otherwise. From the userspace, calls to KVM_SEV_INIT2 or any >>>> other corresponding SNP calls should fail cleanly without any adverse >>>> effects to the host. >>> >>> And I'm saying, that's not good enough. If the platform doesn't support SNP, >>> the KVM *must not* advertise support for SNP. >>> >> >> Sure, fair to expect this. Currently, if the FW check fails, SNP is not >> setup and there is nothing that indicates in the KVM capabilities (apart >> from one dmesg error) that the support does not exist. >> >> One thing I could do (as an independent patch) is to introduce a CC API >> that abstracts the FW version check made by the CCP module. Since sev >> platform status can be gotten before INIT to extract the major and minor >> version numbers, KVM can also call into this API and use that to decide >> if the KVM capabilities for SNP must be set or not. > > Why is CC_ATTR_HOST_SEV_SNP set if hardware/firmware can't actually support SNP? > KVM shouldn't have to care about some arbitrary firmware API version, the whole > point of a driver is so that KVM doesn't have to deal with such details. > > I'm a-ok with a KVM selftest *asserting* that the kernel isn't broken, but KVM > itself shouldn't need to manually check the firmware version. Clearing CC_ATTR_HOST_SEV_SNP when the init fails is one approach to go about it. Here we could clear it from here and eventually that would prevent the the SNP feature being set in KVM capability. +++ b/drivers/crypto/ccp/sev-dev.c @@ -1099,6 +1099,7 @@ static int __sev_snp_init_locked(int *error) return 0; if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) { + cc_platform_clear(CC_ATTR_HOST_SEV_SNP); A suggestion where we could more directly approach this could be by exporting an explicit check from ccp instead? --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -122,6 +122,12 @@ static inline bool sev_version_greater_or_equal(u8 maj, u8 min) return false; } +bool sev_snp_fw_available(void) +{ + return sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR); +} +EXPORT_SYMBOL_GPL(sev_snp_fw_available); which could be then called on will as follows: --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -3050,7 +3050,9 @@ void __init sev_hardware_setup(void) sev_es_asid_count = min_sev_asid - 1; WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count)); sev_es_supported = true; - sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP); + sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP) && sev_snp_fw_available(); out: if (boot_cpu_has(X86_FEATURE_SEV)) This would ensure that we could enable and disable the SNP capability even in the case where maybe the firmware can get hotloaded using the proposed download_firmware_ex[1] or in cases where the INIT could be deferred; all while KVM wouldn't need to be bothered with the API details. [1]: https://lore.kernel.org/lkml/20241029183907.3536683-1-dionnaglaze@google.com/
diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h index e247f99e0473..1dfa2c03b40f 100644 --- a/tools/testing/selftests/kvm/include/x86_64/processor.h +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h @@ -199,6 +199,7 @@ struct kvm_x86_cpu_feature { #define X86_FEATURE_VGIF KVM_X86_CPU_FEATURE(0x8000000A, 0, EDX, 16) #define X86_FEATURE_SEV KVM_X86_CPU_FEATURE(0x8000001F, 0, EAX, 1) #define X86_FEATURE_SEV_ES KVM_X86_CPU_FEATURE(0x8000001F, 0, EAX, 3) +#define X86_FEATURE_SNP KVM_X86_CPU_FEATURE(0x8000001F, 0, EAX, 4) /* * KVM defined paravirt features. diff --git a/tools/testing/selftests/kvm/include/x86_64/sev.h b/tools/testing/selftests/kvm/include/x86_64/sev.h index 3998152cc081..658c3cca208d 100644 --- a/tools/testing/selftests/kvm/include/x86_64/sev.h +++ b/tools/testing/selftests/kvm/include/x86_64/sev.h @@ -22,8 +22,21 @@ enum sev_guest_state { SEV_GUEST_STATE_RUNNING, }; +/* Minimum firmware version required for the SEV-SNP support */ +#define SNP_FW_REQ_VER_MAJOR 1 +#define SNP_FW_REQ_VER_MINOR 51 +#define SNP_POLICY_MINOR_BIT 0 +#define SNP_POLICY_MAJOR_BIT 8 + #define SEV_POLICY_NO_DBG (1UL << 0) #define SEV_POLICY_ES (1UL << 2) +#define SNP_POLICY_SMT (1ULL << 16) +#define SNP_POLICY_RSVD_MBO (1ULL << 17) +#define SNP_POLICY_DBG (1ULL << 19) +#define SNP_POLICY (SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO) + +#define SNP_FW_VER_MAJOR(maj) ((uint8_t)(maj) << SNP_POLICY_MAJOR_BIT) +#define SNP_FW_VER_MINOR(min) ((uint8_t)(min) << SNP_POLICY_MINOR_BIT) #define GHCB_MSR_TERM_REQ 0x100 @@ -32,14 +45,22 @@ int __sev_vm_launch_start(struct kvm_vm *vm, uint32_t policy); int __sev_vm_launch_update(struct kvm_vm *vm, uint32_t policy); int __sev_vm_launch_measure(struct kvm_vm *vm, uint8_t *measurement); int __sev_vm_launch_finish(struct kvm_vm *vm); +int __snp_vm_launch_start(struct kvm_vm *vm, uint64_t policy, uint8_t flags); +int __snp_vm_launch_update(struct kvm_vm *vm, uint8_t page_type); +int __snp_vm_launch_finish(struct kvm_vm *vm, uint16_t flags); void sev_vm_launch(struct kvm_vm *vm, uint32_t policy); void sev_vm_launch_measure(struct kvm_vm *vm, uint8_t *measurement); void sev_vm_launch_finish(struct kvm_vm *vm); +void snp_vm_launch_start(struct kvm_vm *vm, uint64_t policy); +void snp_vm_launch_update(struct kvm_vm *vm); +void snp_vm_launch_finish(struct kvm_vm *vm); + +bool is_kvm_snp_supported(void); struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t type, void *guest_code, struct kvm_vcpu **cpu); -void vm_sev_launch(struct kvm_vm *vm, uint32_t policy, uint8_t *measurement); +void vm_sev_launch(struct kvm_vm *vm, uint64_t policy, uint8_t *measurement); kvm_static_assert(SEV_RET_SUCCESS == 0); @@ -74,8 +95,18 @@ kvm_static_assert(SEV_RET_SUCCESS == 0); __TEST_ASSERT_VM_VCPU_IOCTL(!ret, #cmd, ret, vm); \ }) +/* Ensure policy is within bounds for SEV, SEV-ES */ +#define ASSERT_SEV_POLICY(type, policy) \ +({ \ + if (type == KVM_X86_SEV_VM || type == KVM_X86_SEV_ES_VM) { \ + TEST_ASSERT(policy < ((uint32_t)~0U), \ + "Policy beyond bounds for SEV"); \ + } \ +}) \ + void sev_vm_init(struct kvm_vm *vm); void sev_es_vm_init(struct kvm_vm *vm); +void snp_vm_init(struct kvm_vm *vm); static inline void sev_register_encrypted_memory(struct kvm_vm *vm, struct userspace_mem_region *region) @@ -99,6 +130,19 @@ static inline int __sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa, return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data); } +static inline int __snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa, + uint64_t hva, uint64_t size, uint8_t type) +{ + struct kvm_sev_snp_launch_update update_data = { + .uaddr = hva, + .gfn_start = gpa >> PAGE_SHIFT, + .len = size, + .type = type, + }; + + return __vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data); +} + static inline void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa, uint64_t hva, uint64_t size) { @@ -107,4 +151,12 @@ static inline void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa, TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_LAUNCH_UPDATE_DATA, ret, vm); } +static inline int snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa, + uint64_t hva, uint64_t size, uint8_t type) +{ + int ret = __snp_launch_update_data(vm, gpa, hva, size, type); + + TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_SNP_LAUNCH_UPDATE, ret, vm); +} + #endif /* SELFTEST_KVM_SEV_H */ diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index a2b7df5f1d39..bbf90ad224da 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -413,14 +413,18 @@ struct kvm_vm *__vm_create(struct vm_shape shape, uint32_t nr_runnable_vcpus, nr_extra_pages); struct userspace_mem_region *slot0; struct kvm_vm *vm; - int i; + int i, flags = 0; pr_debug("%s: mode='%s' type='%d', pages='%ld'\n", __func__, vm_guest_mode_string(shape.mode), shape.type, nr_pages); vm = ____vm_create(shape); - vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, 0, 0, nr_pages, 0); + if (shape.type == KVM_X86_SNP_VM) + flags |= KVM_MEM_GUEST_MEMFD; + + vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, 0, 0, nr_pages, flags); + for (i = 0; i < NR_MEM_REGIONS; i++) vm->memslots[i] = 0; diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c index 974bcd2df6d7..981f3c9fd1cf 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c @@ -625,7 +625,8 @@ void kvm_arch_vm_post_create(struct kvm_vm *vm) sync_global_to_guest(vm, host_cpu_is_amd); sync_global_to_guest(vm, is_forced_emulation_enabled); - if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) { + if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM || + vm->type == KVM_X86_SNP_VM) { struct kvm_sev_init init = { 0 }; vm_sev_ioctl(vm, KVM_SEV_INIT2, &init); @@ -1134,7 +1135,8 @@ void kvm_get_cpu_address_width(unsigned int *pa_bits, unsigned int *va_bits) void kvm_init_vm_address_properties(struct kvm_vm *vm) { - if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) { + if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM || + vm->type == KVM_X86_SNP_VM) { vm->arch.sev_fd = open_sev_dev_path_or_exit(); vm->arch.c_bit = BIT_ULL(this_cpu_property(X86_PROPERTY_SEV_C_BIT)); vm->gpa_tag_mask = vm->arch.c_bit; diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c b/tools/testing/selftests/kvm/lib/x86_64/sev.c index 125a72246e09..ff3824564854 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/sev.c +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c @@ -14,7 +14,8 @@ * and find the first range, but that's correct because the condition * expression would cause us to quit the loop. */ -static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region) +static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region, + uint8_t page_type) { const struct sparsebit *protected_phy_pages = region->protected_phy_pages; const vm_paddr_t gpa_base = region->region.guest_phys_addr; @@ -25,12 +26,23 @@ static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region if (!sparsebit_any_set(protected_phy_pages)) return 0; - sev_register_encrypted_memory(vm, region); + if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) + sev_register_encrypted_memory(vm, region); sparsebit_for_each_set_range(protected_phy_pages, i, j) { const uint64_t size = (j - i + 1) * vm->page_size; const uint64_t offset = (i - lowest_page_in_region) * vm->page_size; + if (vm->type == KVM_X86_SNP_VM) { + vm_mem_set_private(vm, gpa_base + offset, size); + ret = __snp_launch_update_data(vm, gpa_base + offset, + (uint64_t)addr_gpa2hva(vm, gpa_base + offset), + size, page_type); + if (ret) + return ret; + continue; + } + ret = __sev_launch_update_data(vm, gpa_base + offset, (uint64_t)addr_gpa2hva(vm, gpa_base + offset), size); @@ -68,6 +80,14 @@ void sev_es_vm_init(struct kvm_vm *vm) } } +void snp_vm_init(struct kvm_vm *vm) +{ + struct kvm_sev_init init = { 0 }; + + assert(vm->type == KVM_X86_SNP_VM); + vm_sev_ioctl(vm, KVM_SEV_INIT2, &init); +} + int __sev_vm_launch_start(struct kvm_vm *vm, uint32_t policy) { struct kvm_sev_launch_start launch_start = { @@ -83,7 +103,7 @@ int __sev_vm_launch_update(struct kvm_vm *vm, uint32_t policy) int ctr; hash_for_each(vm->regions.slot_hash, ctr, region, slot_node) { - int ret = encrypt_region(vm, region); + int ret = encrypt_region(vm, region, 0); if (ret) return ret; @@ -112,6 +132,41 @@ int __sev_vm_launch_finish(struct kvm_vm *vm) return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_FINISH, NULL); } +int __snp_vm_launch_start(struct kvm_vm *vm, uint64_t policy, uint8_t flags) +{ + struct kvm_sev_snp_launch_start launch_start = { + .policy = policy, + .flags = flags, + }; + + return __vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_START, &launch_start); +} + +int __snp_vm_launch_update(struct kvm_vm *vm, uint8_t page_type) +{ + struct userspace_mem_region *region; + int ctr, ret; + + hash_for_each(vm->regions.slot_hash, ctr, region, slot_node) { + ret = encrypt_region(vm, region, page_type); + if (ret) + return ret; + } + + vm->arch.is_pt_protected = true; + + return 0; +} + +int __snp_vm_launch_finish(struct kvm_vm *vm, uint16_t flags) +{ + struct kvm_sev_snp_launch_finish launch_finish = { + .flags = flags, + }; + + return __vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_FINISH, &launch_finish); +} + void sev_vm_launch(struct kvm_vm *vm, uint32_t policy) { struct kvm_sev_guest_status status; @@ -158,6 +213,45 @@ void sev_vm_launch_finish(struct kvm_vm *vm) TEST_ASSERT_EQ(status.state, SEV_GUEST_STATE_RUNNING); } +void snp_vm_launch_start(struct kvm_vm *vm, uint64_t policy) +{ + int ret = __snp_vm_launch_start(vm, policy, 0); + + TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_SNP_LAUNCH_START, ret, vm); +} + +void snp_vm_launch_update(struct kvm_vm *vm) +{ + int ret = __snp_vm_launch_update(vm, KVM_SEV_SNP_PAGE_TYPE_NORMAL); + + TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_SNP_LAUNCH_UPDATE, ret, vm); +} + +void snp_vm_launch_finish(struct kvm_vm *vm) +{ + int ret = __snp_vm_launch_finish(vm, 0); + + TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_SNP_LAUNCH_FINISH, ret, vm); +} + +bool is_kvm_snp_supported(void) +{ + int sev_fd = open_sev_dev_path_or_exit(); + struct sev_user_data_status sev_status; + + struct sev_issue_cmd arg = { + .cmd = SEV_PLATFORM_STATUS, + .data = (unsigned long)&sev_status, + }; + + kvm_ioctl(sev_fd, SEV_ISSUE_CMD, &arg); + close(sev_fd); + + return sev_status.api_major > SNP_FW_REQ_VER_MAJOR || + (sev_status.api_major == SNP_FW_REQ_VER_MAJOR && + sev_status.api_minor >= SNP_FW_REQ_VER_MINOR); +} + struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t type, void *guest_code, struct kvm_vcpu **cpu) { @@ -174,8 +268,22 @@ struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t type, void *guest_code, return vm; } -void vm_sev_launch(struct kvm_vm *vm, uint32_t policy, uint8_t *measurement) +void vm_sev_launch(struct kvm_vm *vm, uint64_t policy, uint8_t *measurement) { + if (vm->type == KVM_X86_SNP_VM) { + vm_enable_cap(vm, KVM_CAP_EXIT_HYPERCALL, (1 << KVM_HC_MAP_GPA_RANGE)); + + snp_vm_launch_start(vm, policy); + + snp_vm_launch_update(vm); + + snp_vm_launch_finish(vm); + + return; + } + + ASSERT_SEV_POLICY(vm->type, policy); + sev_vm_launch(vm, policy); if (!measurement) diff --git a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c index 2e9197eb1652..12d466915074 100644 --- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c +++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c @@ -16,6 +16,27 @@ #define XFEATURE_MASK_X87_AVX (XFEATURE_MASK_FP | XFEATURE_MASK_SSE | XFEATURE_MASK_YMM) +static bool is_smt_active(void) +{ + FILE *f; + + f = fopen("/sys/devices/system/cpu/smt/active", "r"); + if (!f) + return false; + + return fgetc(f) - '0'; +} + +static void guest_snp_code(void) +{ + GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ENABLED); + GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ES_ENABLED); + GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_SNP_ENABLED); + + wrmsr(MSR_AMD64_SEV_ES_GHCB, GHCB_MSR_TERM_REQ); + __asm__ __volatile__("rep; vmmcall"); +} + static void guest_sev_es_code(void) { /* TODO: Check CPUID after GHCB-based hypercall support is added. */ @@ -61,7 +82,7 @@ static void compare_xsave(u8 *from_host, u8 *from_guest) abort(); } -static void test_sync_vmsa(uint32_t policy) +static void test_sync_vmsa(uint32_t type, uint64_t policy) { struct kvm_vcpu *vcpu; struct kvm_vm *vm; @@ -77,7 +98,10 @@ static void test_sync_vmsa(uint32_t policy) .xcrs[0].value = XFEATURE_MASK_X87_AVX, }; - vm = vm_sev_create_with_one_vcpu(KVM_X86_SEV_ES_VM, guest_code_xsave, &vcpu); + TEST_ASSERT(type != KVM_X86_SEV_VM, + "sync_vmsa only supported for SEV-ES and SNP VM types"); + + vm = vm_sev_create_with_one_vcpu(type, guest_code_xsave, &vcpu); gva = vm_vaddr_alloc_shared(vm, PAGE_SIZE, KVM_UTIL_MIN_VADDR, MEM_REGION_TEST_DATA); hva = addr_gva2hva(vm, gva); @@ -99,7 +123,7 @@ static void test_sync_vmsa(uint32_t policy) : "ymm4", "st", "st(1)", "st(2)", "st(3)", "st(4)", "st(5)", "st(6)", "st(7)"); vcpu_xsave_set(vcpu, &xsave); - vm_sev_launch(vm, SEV_POLICY_ES | policy, NULL); + vm_sev_launch(vm, policy, NULL); /* This page is shared, so make it decrypted. */ memset(hva, 0, 4096); @@ -118,14 +142,12 @@ static void test_sync_vmsa(uint32_t policy) kvm_vm_free(vm); } -static void test_sev(void *guest_code, uint64_t policy) +static void test_sev(void *guest_code, uint32_t type, uint64_t policy) { struct kvm_vcpu *vcpu; struct kvm_vm *vm; struct ucall uc; - uint32_t type = policy & SEV_POLICY_ES ? KVM_X86_SEV_ES_VM : KVM_X86_SEV_VM; - vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu); /* TODO: Validate the measurement is as expected. */ @@ -134,7 +156,7 @@ static void test_sev(void *guest_code, uint64_t policy) for (;;) { vcpu_run(vcpu); - if (policy & SEV_POLICY_ES) { + if (vm->type == KVM_X86_SEV_ES_VM || vm->type == KVM_X86_SNP_VM) { TEST_ASSERT(vcpu->run->exit_reason == KVM_EXIT_SYSTEM_EVENT, "Wanted SYSTEM_EVENT, got %s", exit_reason_str(vcpu->run->exit_reason)); @@ -194,19 +216,38 @@ int main(int argc, char *argv[]) { TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SEV)); - test_sev(guest_sev_code, SEV_POLICY_NO_DBG); - test_sev(guest_sev_code, 0); + test_sev(guest_sev_code, KVM_X86_SEV_VM, SEV_POLICY_NO_DBG); + test_sev(guest_sev_code, KVM_X86_SEV_VM, 0); if (kvm_cpu_has(X86_FEATURE_SEV_ES)) { - test_sev(guest_sev_es_code, SEV_POLICY_ES | SEV_POLICY_NO_DBG); - test_sev(guest_sev_es_code, SEV_POLICY_ES); + test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES | SEV_POLICY_NO_DBG); + test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES); test_sev_es_shutdown(); if (kvm_has_cap(KVM_CAP_XCRS) && (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) { - test_sync_vmsa(0); - test_sync_vmsa(SEV_POLICY_NO_DBG); + test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES); + test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES | SEV_POLICY_NO_DBG); + } + } + + if (kvm_cpu_has(X86_FEATURE_SNP) && is_kvm_snp_supported()) { + unsigned long snp_policy = SNP_POLICY; + + if (unlikely(!is_smt_active())) + snp_policy &= ~SNP_POLICY_SMT; + + test_sev(guest_snp_code, KVM_X86_SNP_VM, snp_policy); + /* Test minimum firmware level */ + test_sev(guest_snp_code, KVM_X86_SNP_VM, + snp_policy | + SNP_FW_VER_MAJOR(SNP_FW_REQ_VER_MAJOR) | + SNP_FW_VER_MINOR(SNP_FW_REQ_VER_MINOR)); + + if (kvm_has_cap(KVM_CAP_XCRS) && + (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) { + test_sync_vmsa(KVM_X86_SNP_VM, snp_policy); } }