This series primarily introduces SEV-SNP test for the kernel selftest framework. It tests boot, ioctl, pre fault, and fallocate in various combinations to exercise both positive and negative launch flow paths.
Patch 1 - Adds a wrapper for the ioctl calls that decouple ioctl and asserts, which enables the use of negative test cases. No functional change intended. Patch 2 - Extend the sev smoke tests to use the SNP specific ioctl calls and sets up memory to boot a SNP guest VM Patch 3 - Adds SNP to shutdown testing Patch 4, 5 - Tests the ioctl path for SEV, SEV-ES and SNP Patch 6 - Adds support for SNP in KVM_SEV_INIT2 tests Patch 7,8,9 - Enable Prefault tests for SEV, SEV-ES and SNP
The patchset is rebased on top of kvm-x86/next branch.
v3: 1. Remove the assignments for the prefault and fallocate test type enums. 2. Fix error message for sev launch measure and finish. 3. Collect tested-by tags [Peter, Srikanth]
v2: https://lore.kernel.org/kvm/20240816192310.117456-1-pratikrajesh.sampat@amd.... 1. Add SMT parsing check to populate SNP policy flags 2. Extend Peter Gonda's shutdown test to include SNP 3. Introduce new tests for prefault which include exercising prefault, fallocate, hole-punch in various combinations. 4. Decouple ioctl patch reworked to introduce private variants of the the functions that call into the ioctl. Also reordered the patch for it to arrive first so that new APIs are not written right after their introduction. 5. General cleanups - adding comments, avoiding local booleans, better error message. Suggestions incorporated from Peter, Tom, and Sean.
RFC: https://lore.kernel.org/kvm/20240710220540.188239-1-pratikrajesh.sampat@amd....
Any feedback/review is highly appreciated!
Michael Roth (2): KVM: selftests: Add interface to manually flag protected/encrypted ranges KVM: selftests: Add a CoCo-specific test for KVM_PRE_FAULT_MEMORY
Pratik R. Sampat (7): KVM: selftests: Decouple SEV ioctls from asserts KVM: selftests: Add a basic SNP smoke test KVM: selftests: Add SNP to shutdown testing KVM: selftests: SEV IOCTL test KVM: selftests: SNP IOCTL test KVM: selftests: SEV-SNP test for KVM_SEV_INIT2 KVM: selftests: Interleave fallocate for KVM_PRE_FAULT_MEMORY
tools/testing/selftests/kvm/Makefile | 1 + .../testing/selftests/kvm/include/kvm_util.h | 13 + .../selftests/kvm/include/x86_64/processor.h | 1 + .../selftests/kvm/include/x86_64/sev.h | 76 +++- tools/testing/selftests/kvm/lib/kvm_util.c | 53 ++- .../selftests/kvm/lib/x86_64/processor.c | 6 +- tools/testing/selftests/kvm/lib/x86_64/sev.c | 190 +++++++- .../kvm/x86_64/coco_pre_fault_memory_test.c | 421 ++++++++++++++++++ .../selftests/kvm/x86_64/sev_init2_tests.c | 13 + .../selftests/kvm/x86_64/sev_smoke_test.c | 297 +++++++++++- 10 files changed, 1023 insertions(+), 48 deletions(-) create mode 100644 tools/testing/selftests/kvm/x86_64/coco_pre_fault_memory_test.c
Add variants of sev, sev-es launch path that return the status of the ioctl call instead of asserting for success. This enables both positive and negative testing of the path.
No functional impact intended.
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/sev.h | 22 +++++- tools/testing/selftests/kvm/lib/x86_64/sev.c | 78 +++++++++++++++---- 2 files changed, 80 insertions(+), 20 deletions(-)
diff --git a/tools/testing/selftests/kvm/include/x86_64/sev.h b/tools/testing/selftests/kvm/include/x86_64/sev.h index 82c11c81a956..3998152cc081 100644 --- a/tools/testing/selftests/kvm/include/x86_64/sev.h +++ b/tools/testing/selftests/kvm/include/x86_64/sev.h @@ -27,6 +27,12 @@ enum sev_guest_state {
#define GHCB_MSR_TERM_REQ 0x100
+/* Variants of the SEV launch path that do not assert the ioctl status */ +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); + 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); @@ -82,15 +88,23 @@ static inline void sev_register_encrypted_memory(struct kvm_vm *vm, vm_ioctl(vm, KVM_MEMORY_ENCRYPT_REG_REGION, &range); }
-static inline void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa, - uint64_t size) +static inline int __sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa, + uint64_t hva, uint64_t size) { struct kvm_sev_launch_update_data update_data = { - .uaddr = (unsigned long)addr_gpa2hva(vm, gpa), + .uaddr = hva, .len = size, };
- vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data); + return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data); +} + +static inline void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa, + uint64_t hva, uint64_t size) +{ + int ret = __sev_launch_update_data(vm, gpa, hva, size); + + TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_LAUNCH_UPDATE_DATA, ret, vm); }
#endif /* SELFTEST_KVM_SEV_H */ diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c b/tools/testing/selftests/kvm/lib/x86_64/sev.c index e9535ee20b7f..125a72246e09 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/sev.c +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c @@ -14,15 +14,16 @@ * and find the first range, but that's correct because the condition * expression would cause us to quit the loop. */ -static void encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region) +static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region) { const struct sparsebit *protected_phy_pages = region->protected_phy_pages; const vm_paddr_t gpa_base = region->region.guest_phys_addr; const sparsebit_idx_t lowest_page_in_region = gpa_base >> vm->page_shift; sparsebit_idx_t i, j; + int ret;
if (!sparsebit_any_set(protected_phy_pages)) - return; + return 0;
sev_register_encrypted_memory(vm, region);
@@ -30,8 +31,15 @@ static void encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *regio const uint64_t size = (j - i + 1) * vm->page_size; const uint64_t offset = (i - lowest_page_in_region) * vm->page_size;
- sev_launch_update_data(vm, gpa_base + offset, size); + ret = __sev_launch_update_data(vm, gpa_base + offset, + (uint64_t)addr_gpa2hva(vm, gpa_base + offset), + size); + if (ret) + return ret; + } + + return 0; }
void sev_vm_init(struct kvm_vm *vm) @@ -60,38 +68,74 @@ void sev_es_vm_init(struct kvm_vm *vm) } }
-void sev_vm_launch(struct kvm_vm *vm, uint32_t policy) +int __sev_vm_launch_start(struct kvm_vm *vm, uint32_t policy) { struct kvm_sev_launch_start launch_start = { .policy = policy, }; + + return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_START, &launch_start); +} + +int __sev_vm_launch_update(struct kvm_vm *vm, uint32_t policy) +{ struct userspace_mem_region *region; - struct kvm_sev_guest_status status; int ctr;
- vm_sev_ioctl(vm, KVM_SEV_LAUNCH_START, &launch_start); - vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &status); + hash_for_each(vm->regions.slot_hash, ctr, region, slot_node) { + int ret = encrypt_region(vm, region);
- TEST_ASSERT_EQ(status.policy, policy); - TEST_ASSERT_EQ(status.state, SEV_GUEST_STATE_LAUNCH_UPDATE); - - hash_for_each(vm->regions.slot_hash, ctr, region, slot_node) - encrypt_region(vm, region); + if (ret) + return ret; + }
if (policy & SEV_POLICY_ES) vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL);
vm->arch.is_pt_protected = true; + + return 0; }
-void sev_vm_launch_measure(struct kvm_vm *vm, uint8_t *measurement) +int __sev_vm_launch_measure(struct kvm_vm *vm, uint8_t *measurement) { struct kvm_sev_launch_measure launch_measure; - struct kvm_sev_guest_status guest_status;
launch_measure.len = 256; launch_measure.uaddr = (__u64)measurement; - vm_sev_ioctl(vm, KVM_SEV_LAUNCH_MEASURE, &launch_measure); + + return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_MEASURE, &launch_measure); +} + +int __sev_vm_launch_finish(struct kvm_vm *vm) +{ + return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_FINISH, NULL); +} + +void sev_vm_launch(struct kvm_vm *vm, uint32_t policy) +{ + struct kvm_sev_guest_status status; + int ret; + + ret = __sev_vm_launch_start(vm, policy); + TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_LAUNCH_START, ret, vm); + + vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &status); + + TEST_ASSERT_EQ(status.policy, policy); + TEST_ASSERT_EQ(status.state, SEV_GUEST_STATE_LAUNCH_UPDATE); + + ret = __sev_vm_launch_update(vm, policy); + TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_LAUNCH_UPDATE_DATA, ret, vm); +} + +void sev_vm_launch_measure(struct kvm_vm *vm, uint8_t *measurement) +{ + struct kvm_sev_guest_status guest_status; + int ret; + + ret = __sev_vm_launch_measure(vm, measurement); + TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_LAUNCH_MEASURE, ret, vm);
vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &guest_status); TEST_ASSERT_EQ(guest_status.state, SEV_GUEST_STATE_LAUNCH_SECRET); @@ -100,13 +144,15 @@ void sev_vm_launch_measure(struct kvm_vm *vm, uint8_t *measurement) void sev_vm_launch_finish(struct kvm_vm *vm) { struct kvm_sev_guest_status status; + int ret;
vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &status); TEST_ASSERT(status.state == SEV_GUEST_STATE_LAUNCH_UPDATE || status.state == SEV_GUEST_STATE_LAUNCH_SECRET, "Unexpected guest state: %d", status.state);
- vm_sev_ioctl(vm, KVM_SEV_LAUNCH_FINISH, NULL); + ret = __sev_vm_launch_finish(vm); + TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_LAUNCH_FINISH, ret, vm);
vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &status); TEST_ASSERT_EQ(status.state, SEV_GUEST_STATE_RUNNING);
On Thu, Sep 05, 2024, Pratik R. Sampat wrote:
+static inline int __sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
uint64_t hva, uint64_t size){ struct kvm_sev_launch_update_data update_data = {
.uaddr = (unsigned long)addr_gpa2hva(vm, gpa),
.uaddr = hva,
- vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data);
- return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data);
+}
+static inline void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
uint64_t hva, uint64_t size)+{
- int ret = __sev_launch_update_data(vm, gpa, hva, size);
- TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_LAUNCH_UPDATE_DATA, ret, vm);
} #endif /* SELFTEST_KVM_SEV_H */ diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c b/tools/testing/selftests/kvm/lib/x86_64/sev.c index e9535ee20b7f..125a72246e09 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/sev.c +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c @@ -14,15 +14,16 @@
- and find the first range, but that's correct because the condition
- expression would cause us to quit the loop.
*/ -static void encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region) +static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region)
This is all kinds of wrong. encrypt_region() should never fail. And by allowing it to fail, any unexpected failure becomes harder to debug. It's also a lie, because sev_register_encrypted_memory() isn't allowed to fail, and I would bet that most readers would expect _that_ call to fail given the name.
The granularity is also poor, and the complete lack of idempotency is going to be problematic. E.g. only the first region is actually tested, and if someone tries to do negative testing on multiple regions, sev_register_encrypted_memory() will fail due to trying to re-encrypt a region.
__sev_vm_launch_update() has similar issues. encrypt_region() is allowed to fail, but its call to KVM_SEV_LAUNCH_UPDATE_VMSA is not.
And peeking ahead, passing an @assert parameter to __test_snp_launch_start() (or any helper) is a non-starter. Readers should not have to dive into a helper's implementation to understand that this
__test_snp_launch_start(type, policy, 0, true);
is a happy path and this
ret = __test_snp_launch_start(type, policy, BIT(i), false);
is a sad path.
And re-creating the VM every time is absurdly wasteful. While performance isn't a priority for selftests, there's no reason to make everything as slow as possible.
Even just passing the page type to encrypt_region() is confusing. When the test is actually going to run the guest, applying ZERO and CPUID types to _all_ pages is completely nonsensical.
In general, I think trying to reuse the happy path's infrastructure is going to do more harm than good. This is what I was trying to get at in my feedback for the previous version.
For negative tests, I would honestly say development them "from scratch", i.e. deliberately don't reuse the existing SEV-MEM/ES infrastructure. It'll require more copy+paste to get rolling, but I suspect that the end result will be less churn and far easier to read.
Hi Sean,
On 10/14/2024 5:18 PM, Sean Christopherson wrote:
On Thu, Sep 05, 2024, Pratik R. Sampat wrote:
+static inline int __sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
uint64_t hva, uint64_t size){ struct kvm_sev_launch_update_data update_data = {
.uaddr = (unsigned long)addr_gpa2hva(vm, gpa),
.uaddr = hva,
- vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data);
- return __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data);
+}
+static inline void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
uint64_t hva, uint64_t size)+{
- int ret = __sev_launch_update_data(vm, gpa, hva, size);
- TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_LAUNCH_UPDATE_DATA, ret, vm);
} #endif /* SELFTEST_KVM_SEV_H */ diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c b/tools/testing/selftests/kvm/lib/x86_64/sev.c index e9535ee20b7f..125a72246e09 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/sev.c +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c @@ -14,15 +14,16 @@
- and find the first range, but that's correct because the condition
- expression would cause us to quit the loop.
*/ -static void encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region) +static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region)
This is all kinds of wrong. encrypt_region() should never fail. And by allowing it to fail, any unexpected failure becomes harder to debug. It's also a lie, because sev_register_encrypted_memory() isn't allowed to fail, and I would bet that most readers would expect _that_ call to fail given the name.
The granularity is also poor, and the complete lack of idempotency is going to be problematic. E.g. only the first region is actually tested, and if someone tries to do negative testing on multiple regions, sev_register_encrypted_memory() will fail due to trying to re-encrypt a region.
__sev_vm_launch_update() has similar issues. encrypt_region() is allowed to fail, but its call to KVM_SEV_LAUNCH_UPDATE_VMSA is not.
And peeking ahead, passing an @assert parameter to __test_snp_launch_start() (or any helper) is a non-starter. Readers should not have to dive into a helper's implementation to understand that this
__test_snp_launch_start(type, policy, 0, true);
is a happy path and this
ret = __test_snp_launch_start(type, policy, BIT(i), false);
is a sad path.
And re-creating the VM every time is absurdly wasteful. While performance isn't a priority for selftests, there's no reason to make everything as slow as possible.
Even just passing the page type to encrypt_region() is confusing. When the test is actually going to run the guest, applying ZERO and CPUID types to _all_ pages is completely nonsensical.
In general, I think trying to reuse the happy path's infrastructure is going to do more harm than good. This is what I was trying to get at in my feedback for the previous version.
For negative tests, I would honestly say development them "from scratch", i.e. deliberately don't reuse the existing SEV-MEM/ES infrastructure. It'll require more copy+paste to get rolling, but I suspect that the end result will be less churn and far easier to read.
This makes sense. I was trying to be as minimal as possible without a lot of replication while trying to introduce the negative tests. I see that this has created several issues of granularity, even general correctness and overall has created more problems than it solves.
I will try to develop the negative interface separately tailored for this specific use-case rather than piggybacking on the happy path when I send out the patchset #2.
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(-)
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); } }
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) {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:
- Add x86 architectural defines
- 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) {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/specifi...
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/program...
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/specifi...
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/program...
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/specifi...
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/program...
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/specifi...
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-....
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-....
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-....
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-....
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-....
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-....
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-....
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...
Parameterize the shutdown test to include the SEV-SNP VM type
Signed-off-by: Pratik R. Sampat pratikrajesh.sampat@amd.com Tested-by: Peter Gonda pgonda@google.com Tested-by: Srikanth Aithal sraithal@amd.com --- tools/testing/selftests/kvm/x86_64/sev_smoke_test.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
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 12d466915074..8e798f5a2a53 100644 --- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c +++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c @@ -193,16 +193,14 @@ static void guest_shutdown_code(void) __asm__ __volatile__("ud2"); }
-static void test_sev_es_shutdown(void) +static void test_sev_shutdown(uint32_t type, uint64_t policy) { struct kvm_vcpu *vcpu; struct kvm_vm *vm;
- uint32_t type = KVM_X86_SEV_ES_VM; - vm = vm_sev_create_with_one_vcpu(type, guest_shutdown_code, &vcpu);
- vm_sev_launch(vm, SEV_POLICY_ES, NULL); + vm_sev_launch(vm, policy, NULL);
vcpu_run(vcpu); TEST_ASSERT(vcpu->run->exit_reason == KVM_EXIT_SHUTDOWN, @@ -223,7 +221,7 @@ int main(int argc, char *argv[]) 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(); + test_sev_shutdown(KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
if (kvm_has_cap(KVM_CAP_XCRS) && (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) { @@ -245,6 +243,8 @@ int main(int argc, char *argv[]) SNP_FW_VER_MAJOR(SNP_FW_REQ_VER_MAJOR) | SNP_FW_VER_MINOR(SNP_FW_REQ_VER_MINOR));
+ test_sev_shutdown(KVM_X86_SNP_VM, snp_policy); + 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);
Introduce tests for sev and sev-es ioctl that exercises the boot path of launch, update and finish on an invalid 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/x86_64/sev_smoke_test.c | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+)
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 8e798f5a2a53..5fa4ee27609b 100644 --- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c +++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c @@ -142,12 +142,96 @@ static void test_sync_vmsa(uint32_t type, uint64_t policy) kvm_vm_free(vm); }
+static void sev_guest_neg_status_assert(struct kvm_vm *vm, uint32_t type) +{ + struct kvm_sev_guest_status status; + int ret; + + ret = __vm_sev_ioctl(vm, KVM_SEV_GUEST_STATUS, &status); + TEST_ASSERT(ret, "KVM_SEV_GUEST_STATUS should fail, invalid VM Type."); +} + +static void vm_sev_es_launch_neg(struct kvm_vm *vm, uint32_t type, uint64_t policy) +{ + int ret; + + /* Launch start with policy SEV_POLICY_NO_DBG (0x0) */ + ret = __sev_vm_launch_start(vm, 0); + TEST_ASSERT(ret, + "KVM_SEV_LAUNCH_START should fail due to type (%d) - policy(0x0) mismatch", + type); + + ret = __sev_vm_launch_update(vm, policy); + TEST_ASSERT(ret, + "KVM_SEV_LAUNCH_UPDATE should fail due to LAUNCH_START. type: %d policy: 0x%lx", + type, policy); + sev_guest_neg_status_assert(vm, type); + + ret = __sev_vm_launch_measure(vm, alloca(256)); + TEST_ASSERT(ret, + "KVM_SEV_LAUNCH_UPDATE should fail due to LAUNCH_START. type: %d policy: 0x%lx", + type, policy); + sev_guest_neg_status_assert(vm, type); + + ret = __sev_vm_launch_finish(vm); + TEST_ASSERT(ret, + "KVM_SEV_LAUNCH_UPDATE should fail due to LAUNCH_START. type: %d policy: 0x%lx", + type, policy); + sev_guest_neg_status_assert(vm, type); +} + +/* + * Test for SEV ioctl launch path + * VMs of the type SEV and SEV-ES are created, however they are launched with + * an empty policy to observe the effect on the control flow of launching a VM. + * + * SEV - Expected to pass through the path of launch start, update, measure, + * and finish. vcpu_run expected to fail with error KVM_EXIT_IO. + * + * SEV-ES - Expected to fail the launch start as vm created with type + * KVM_X86_DEFAULT_VM but policy passed to launch start is KVM_X86_SEV_ES_VM. + * Post this, calls that pass the correct policy to update, measure, and finish + * are also expected to fail cascading. + */ +static void test_sev_launch(void *guest_code, uint32_t type, uint64_t policy) +{ + struct kvm_vcpu *vcpu; + int exp_exit_reason; + struct kvm_vm *vm; + struct ucall uc; + + vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu); + + if (type == KVM_X86_SEV_VM) { + sev_vm_launch(vm, 0); + sev_vm_launch_measure(vm, alloca(256)); + sev_vm_launch_finish(vm); + } else { + vm_sev_es_launch_neg(vm, type, policy); + } + + vcpu_run(vcpu); + get_ucall(vcpu, &uc); + if (type == KVM_X86_SEV_VM) + exp_exit_reason = KVM_EXIT_IO; + else + exp_exit_reason = KVM_EXIT_FAIL_ENTRY; + + TEST_ASSERT(vcpu->run->exit_reason == exp_exit_reason, + "vcpu_run failed exit expected: %d, got: %d", + exp_exit_reason, vcpu->run->exit_reason); + + kvm_vm_free(vm); +} + static void test_sev(void *guest_code, uint32_t type, uint64_t policy) { struct kvm_vcpu *vcpu; struct kvm_vm *vm; struct ucall uc;
+ test_sev_launch(guest_code, type, policy); + vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu);
/* TODO: Validate the measurement is as expected. */
Introduce testing of SNP ioctl calls. Tests attributes such as flags, page types, and policies in various combinations along the SNP launch path.
Signed-off-by: Pratik R. Sampat pratikrajesh.sampat@amd.com Tested-by: Peter Gonda pgonda@google.com Tested-by: Srikanth Aithal sraithal@amd.com --- .../testing/selftests/kvm/include/kvm_util.h | 11 ++ .../selftests/kvm/x86_64/sev_smoke_test.c | 140 +++++++++++++++++- 2 files changed, 150 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h index bc7c242480d6..ab213708b551 100644 --- a/tools/testing/selftests/kvm/include/kvm_util.h +++ b/tools/testing/selftests/kvm/include/kvm_util.h @@ -912,6 +912,17 @@ static inline struct kvm_vm *vm_create(uint32_t nr_runnable_vcpus) return __vm_create(VM_SHAPE_DEFAULT, nr_runnable_vcpus, 0); }
+static inline struct kvm_vm *vm_create_type(unsigned long type, + uint32_t nr_runnable_vcpus) +{ + const struct vm_shape shape = { + .mode = VM_MODE_DEFAULT, + .type = type, + }; + + return __vm_create(shape, nr_runnable_vcpus, 0); +} + struct kvm_vm *__vm_create_with_vcpus(struct vm_shape shape, uint32_t nr_vcpus, uint64_t extra_mem_pages, void *guest_code, struct kvm_vcpu *vcpus[]); 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 5fa4ee27609b..9a7efbe214ce 100644 --- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c +++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c @@ -224,13 +224,151 @@ static void test_sev_launch(void *guest_code, uint32_t type, uint64_t policy) kvm_vm_free(vm); }
+static int __test_snp_launch_start(uint32_t type, uint64_t policy, + uint8_t flags, bool assert) +{ + struct kvm_vm *vm; + int ret = 0; + + vm = vm_create_type(type, 1); + ret = __snp_vm_launch_start(vm, policy, flags); + if (assert) + TEST_ASSERT_VM_VCPU_IOCTL(!ret, KVM_SEV_SNP_LAUNCH_START, ret, vm); + kvm_vm_free(vm); + + return ret; +} + +static void test_snp_launch_start(uint32_t type, uint64_t policy) +{ + uint8_t i; + int ret; + + /* Flags must be zero for success */ + __test_snp_launch_start(type, policy, 0, true); + + for (i = 1; i < 8; i++) { + ret = __test_snp_launch_start(type, policy, BIT(i), false); + TEST_ASSERT(ret && errno == EINVAL, + "KVM_SEV_SNP_LAUNCH_START should fail, invalid flag\n" + "(type: %d policy: 0x%lx, flag: 0x%lx)", + type, policy, BIT(i)); + } + + ret = __test_snp_launch_start(type, SNP_POLICY_SMT, 0, false); + TEST_ASSERT(ret && errno == EINVAL, + "KVM_SEV_SNP_LAUNCH_START should fail, SNP_POLICY_RSVD_MBO policy bit not set\n" + "(type: %d policy: 0x%llx, flags: 0x0)", + type, SNP_POLICY_SMT); + + ret = __test_snp_launch_start(type, SNP_POLICY_RSVD_MBO, 0, false); + if (unlikely(!is_smt_active())) { + TEST_ASSERT(!ret, + "KVM_SEV_SNP_LAUNCH_START should succeed, SNP_POLICY_SMT not required on non-SMT systems\n" + "(type: %d policy: 0x%llx, flags: 0x0)", + type, SNP_POLICY_RSVD_MBO); + } else { + TEST_ASSERT(ret && errno == EINVAL, + "KVM_SEV_SNP_LAUNCH_START should fail, SNP_POLICY_SMT is not set on a SMT system\n" + "(type: %d policy: 0x%llx, flags: 0x0)", + type, SNP_POLICY_RSVD_MBO); + } + + ret = __test_snp_launch_start(type, SNP_POLICY | + SNP_FW_VER_MAJOR(UINT8_MAX) | + SNP_FW_VER_MINOR(UINT8_MAX), 0, false); + TEST_ASSERT(ret && errno == EIO, + "KVM_SEV_SNP_LAUNCH_START should fail, invalid version\n" + "expected: %d.%d got: %d.%d (type: %d policy: 0x%llx, flags: 0x0)", + SNP_FW_REQ_VER_MAJOR, SNP_FW_REQ_VER_MINOR, + UINT8_MAX, UINT8_MAX, type, + SNP_POLICY | SNP_FW_VER_MAJOR(UINT8_MAX) | SNP_FW_VER_MINOR(UINT8_MAX)); +} + +static void test_snp_launch_update(uint32_t type, uint64_t policy) +{ + struct kvm_vm *vm; + int ret; + + for (int pgtype = 0; pgtype <= KVM_SEV_SNP_PAGE_TYPE_CPUID + 1; pgtype++) { + vm = vm_create_type(type, 1); + snp_vm_launch_start(vm, policy); + ret = __snp_vm_launch_update(vm, pgtype); + + switch (pgtype) { + case KVM_SEV_SNP_PAGE_TYPE_NORMAL: + case KVM_SEV_SNP_PAGE_TYPE_ZERO: + case KVM_SEV_SNP_PAGE_TYPE_UNMEASURED: + case KVM_SEV_SNP_PAGE_TYPE_SECRETS: + TEST_ASSERT(!ret, + "KVM_SEV_SNP_LAUNCH_UPDATE should succeed, invalid Page type %d", + pgtype); + break; + case KVM_SEV_SNP_PAGE_TYPE_CPUID: + /* + * Expect failure if performed on random pages of + * guest memory rather than properly formatted CPUID Page + */ + TEST_ASSERT(ret && errno == EIO, + "KVM_SEV_SNP_LAUNCH_UPDATE should fail,\n" + "CPUID page type only valid for CPUID pages"); + break; + default: + TEST_ASSERT(ret && errno == EINVAL, + "KVM_SEV_SNP_LAUNCH_UPDATE should fail, invalid Page type"); + } + + kvm_vm_free(vm); + } +} + +void test_snp_launch_finish(uint32_t type, uint64_t policy) +{ + struct kvm_vm *vm; + int ret; + + vm = vm_create_type(type, 1); + snp_vm_launch_start(vm, policy); + snp_vm_launch_update(vm); + /* Flags must be zero for success */ + snp_vm_launch_finish(vm); + kvm_vm_free(vm); + + for (int i = 1; i < 16; i++) { + vm = vm_create_type(type, 1); + snp_vm_launch_start(vm, policy); + snp_vm_launch_update(vm); + ret = __snp_vm_launch_finish(vm, BIT(i)); + TEST_ASSERT(ret && errno == EINVAL, + "KVM_SEV_SNP_LAUNCH_FINISH should fail, invalid flag\n" + "(type: %d policy: 0x%lx, flag: 0x%lx)", + type, policy, BIT(i)); + kvm_vm_free(vm); + } +} + +static void test_snp_ioctl(void *guest_code, uint32_t type, uint64_t policy) +{ + test_snp_launch_start(type, policy); + test_snp_launch_update(type, policy); + test_snp_launch_finish(type, policy); +} + +static void test_sev_ioctl(void *guest_code, uint32_t type, uint64_t policy) +{ + test_sev_launch(guest_code, type, 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;
- test_sev_launch(guest_code, type, policy); + if (type == KVM_X86_SNP_VM) + test_snp_ioctl(guest_code, type, policy); + else + test_sev_ioctl(guest_code, type, policy);
vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu);
Add SEV-SNP VM type to exercise the KVM_SEV_INIT2 call.
Also ensure that SNP case is skipped for scenarios where CPUID supports it but KVM does not so that a failure is not reported for such cases.
Signed-off-by: Pratik R. Sampat pratikrajesh.sampat@amd.com Tested-by: Peter Gonda pgonda@google.com Tested-by: Srikanth Aithal sraithal@amd.com --- .../testing/selftests/kvm/x86_64/sev_init2_tests.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c b/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c index 3fb967f40c6a..3f8fb2cc3431 100644 --- a/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c +++ b/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c @@ -28,6 +28,7 @@ int kvm_fd; u64 supported_vmsa_features; bool have_sev_es; +bool have_snp;
static int __sev_ioctl(int vm_fd, int cmd_id, void *data) { @@ -83,6 +84,9 @@ void test_vm_types(void) if (have_sev_es) test_init2(KVM_X86_SEV_ES_VM, &(struct kvm_sev_init){});
+ if (have_snp) + test_init2(KVM_X86_SNP_VM, &(struct kvm_sev_init){}); + test_init2_invalid(0, &(struct kvm_sev_init){}, "VM type is KVM_X86_DEFAULT_VM"); if (kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SW_PROTECTED_VM)) @@ -138,15 +142,24 @@ int main(int argc, char *argv[]) "sev-es: KVM_CAP_VM_TYPES (%x) does not match cpuid (checking %x)", kvm_check_cap(KVM_CAP_VM_TYPES), 1 << KVM_X86_SEV_ES_VM);
+ have_snp = kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SNP_VM); + TEST_ASSERT(!have_snp || kvm_cpu_has(X86_FEATURE_SNP), + "sev-snp: KVM_CAP_VM_TYPES (%x) indicates SNP support (bit %d), but CPUID does not", + kvm_check_cap(KVM_CAP_VM_TYPES), KVM_X86_SNP_VM); + test_vm_types();
test_flags(KVM_X86_SEV_VM); if (have_sev_es) test_flags(KVM_X86_SEV_ES_VM); + if (have_snp) + test_flags(KVM_X86_SNP_VM);
test_features(KVM_X86_SEV_VM, 0); if (have_sev_es) test_features(KVM_X86_SEV_ES_VM, supported_vmsa_features); + if (have_snp) + test_features(KVM_X86_SNP_VM, supported_vmsa_features);
return 0; }
From: Michael Roth michael.roth@amd.com
For SEV and SNP, currently __vm_phy_pages_alloc() handles setting the region->protected_phy_pages bitmap to mark that the region needs to be encrypted/measured into the initial guest state prior to finalizing/starting the guest. It also marks what GPAs need to be mapped as encrypted in the initial guest page table.
This works when using virtual/physical allocators to manage memory, but if the test manages allocations/mapping directly then an alternative is needed to set region->protected_phy_pages directly. Add an interface to handle that.
Signed-off-by: Michael Roth michael.roth@amd.com Signed-off-by: Pratik R. Sampat pratikrajesh.sampat@amd.com Tested-by: Peter Gonda pgonda@google.com Tested-by: Srikanth Aithal sraithal@amd.com --- .../testing/selftests/kvm/include/kvm_util.h | 2 + tools/testing/selftests/kvm/lib/kvm_util.c | 45 +++++++++++++++++-- 2 files changed, 43 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h index ab213708b551..642740fe1c59 100644 --- a/tools/testing/selftests/kvm/include/kvm_util.h +++ b/tools/testing/selftests/kvm/include/kvm_util.h @@ -394,6 +394,8 @@ static inline void vm_set_memory_attributes(struct kvm_vm *vm, uint64_t gpa, vm_ioctl(vm, KVM_SET_MEMORY_ATTRIBUTES, &attr); }
+void vm_mem_set_protected(struct kvm_vm *vm, uint32_t memslot, + vm_paddr_t paddr, size_t num);
static inline void vm_mem_set_private(struct kvm_vm *vm, uint64_t gpa, uint64_t size) diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index bbf90ad224da..d44a37aebcec 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -1991,6 +1991,43 @@ const char *exit_reason_str(unsigned int exit_reason) return "Unknown"; }
+/* + * Set what guest GFNs need to be encrypted prior to finalizing a CoCo VM. + * + * Input Args: + * vm - Virtual Machine + * memslot - Memory region to allocate page from + * paddr - Start of physical address to mark as encrypted + * num - number of pages + * + * Output Args: None + * + * Return: None + * + * Generally __vm_phy_pages_alloc() will handle this automatically, but + * for cases where the test handles managing the physical allocation and + * mapping directly this interface should be used to mark physical pages + * that are intended to be encrypted as part of the initial guest state. + * This will also affect whether virt_map()/virt_pg_map() will map the + * page as encrypted or not in the initial guest page table. + * + * If the initial guest state has already been finalized, then setting + * it as encrypted will essentially be a noop since nothing more can be + * encrypted into the initial guest state at that point. + */ +void vm_mem_set_protected(struct kvm_vm *vm, uint32_t memslot, + vm_paddr_t paddr, size_t num) +{ + struct userspace_mem_region *region; + sparsebit_idx_t pg, base; + + base = paddr >> vm->page_shift; + region = memslot2region(vm, memslot); + + for (pg = base; pg < base + num; ++pg) + sparsebit_set(region->protected_phy_pages, pg); +} + /* * Physical Contiguous Page Allocator * @@ -2048,11 +2085,11 @@ vm_paddr_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, abort(); }
- for (pg = base; pg < base + num; ++pg) { + for (pg = base; pg < base + num; ++pg) sparsebit_clear(region->unused_phy_pages, pg); - if (protected) - sparsebit_set(region->protected_phy_pages, pg); - } + + if (protected) + vm_mem_set_protected(vm, memslot, base << vm->page_shift, num);
return base * vm->page_size; }
On Thu, Sep 05, 2024, Pratik R. Sampat wrote:
From: Michael Roth michael.roth@amd.com
For SEV and SNP, currently __vm_phy_pages_alloc() handles setting the region->protected_phy_pages bitmap to mark that the region needs to be encrypted/measured into the initial guest state prior to
Nothing needs to be measured, no? (because there's no attestation)
finalizing/starting the guest. It also marks what GPAs need to be mapped as encrypted in the initial guest page table.
...
static inline void vm_mem_set_private(struct kvm_vm *vm, uint64_t gpa, uint64_t size) diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index bbf90ad224da..d44a37aebcec 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -1991,6 +1991,43 @@ const char *exit_reason_str(unsigned int exit_reason) return "Unknown"; } +/*
- Set what guest GFNs need to be encrypted prior to finalizing a CoCo VM.
- Input Args:
- vm - Virtual Machine
- memslot - Memory region to allocate page from
- paddr - Start of physical address to mark as encrypted
- num - number of pages
- Output Args: None
- Return: None
- Generally __vm_phy_pages_alloc() will handle this automatically, but
- for cases where the test handles managing the physical allocation and
- mapping directly this interface should be used to mark physical pages
- that are intended to be encrypted as part of the initial guest state.
- This will also affect whether virt_map()/virt_pg_map() will map the
- page as encrypted or not in the initial guest page table.
- If the initial guest state has already been finalized, then setting
- it as encrypted will essentially be a noop since nothing more can be
- encrypted into the initial guest state at that point.
- */
+void vm_mem_set_protected(struct kvm_vm *vm, uint32_t memslot,
vm_paddr_t paddr, size_t num)+{
- struct userspace_mem_region *region;
- sparsebit_idx_t pg, base;
- base = paddr >> vm->page_shift;
- region = memslot2region(vm, memslot);
Please no, doing a memslot lookup in a helper like this is only going to encourage proliferation of bad code. vm_mem_add() really should be able to mark the region as protected.
E.g. practically speaking, the only code that will be able to use this helper is code that is marking the entire memslot as protection. And ability to _clear_ the protected_phy_pages bit is conspicuously missing.
- for (pg = base; pg < base + num; ++pg)
sparsebit_set(region->protected_phy_pages, pg);+}
Hi Sean,
On 10/14/2024 5:58 PM, Sean Christopherson wrote:
On Thu, Sep 05, 2024, Pratik R. Sampat wrote:
From: Michael Roth michael.roth@amd.com
For SEV and SNP, currently __vm_phy_pages_alloc() handles setting the region->protected_phy_pages bitmap to mark that the region needs to be encrypted/measured into the initial guest state prior to
Nothing needs to be measured, no? (because there's no attestation)
Right.
finalizing/starting the guest. It also marks what GPAs need to be mapped as encrypted in the initial guest page table.
...
static inline void vm_mem_set_private(struct kvm_vm *vm, uint64_t gpa, uint64_t size) diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index bbf90ad224da..d44a37aebcec 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -1991,6 +1991,43 @@ const char *exit_reason_str(unsigned int exit_reason) return "Unknown"; } +/*
- Set what guest GFNs need to be encrypted prior to finalizing a CoCo VM.
- Input Args:
- vm - Virtual Machine
- memslot - Memory region to allocate page from
- paddr - Start of physical address to mark as encrypted
- num - number of pages
- Output Args: None
- Return: None
- Generally __vm_phy_pages_alloc() will handle this automatically, but
- for cases where the test handles managing the physical allocation and
- mapping directly this interface should be used to mark physical pages
- that are intended to be encrypted as part of the initial guest state.
- This will also affect whether virt_map()/virt_pg_map() will map the
- page as encrypted or not in the initial guest page table.
- If the initial guest state has already been finalized, then setting
- it as encrypted will essentially be a noop since nothing more can be
- encrypted into the initial guest state at that point.
- */
+void vm_mem_set_protected(struct kvm_vm *vm, uint32_t memslot,
vm_paddr_t paddr, size_t num)+{
- struct userspace_mem_region *region;
- sparsebit_idx_t pg, base;
- base = paddr >> vm->page_shift;
- region = memslot2region(vm, memslot);
Please no, doing a memslot lookup in a helper like this is only going to encourage proliferation of bad code. vm_mem_add() really should be able to mark the region as protected.
E.g. practically speaking, the only code that will be able to use this helper is code that is marking the entire memslot as protection. And ability to _clear_ the protected_phy_pages bit is conspicuously missing.
From my understanding, vm_mem_add() only allocates the protected sparsebits but does not populate them. For that maybe a better path would be to go through, vm_phy_pages_alloc() - something similar to how the set_memory_region_test.c does? I think we avoided doing that because vm_phys_pages_alloc() takes a paddr_min rather than guaranteeing the specific paddr and even then would need the similar virt_map() logic to stay. If this is a cleaner approach though, I'm happy to redo this code that way.
Thanks! Pratik
- for (pg = base; pg < base + num; ++pg)
sparsebit_set(region->protected_phy_pages, pg);+}
From: Michael Roth michael.roth@amd.com
SEV, SEV-ES, and SNP have a few corner cases where there is potential for KVM_PRE_FAULT_MEMORY to behave differently depending on when it is issued during initial guest setup. Exercising these various paths requires a bit more fine-grained control over when the KVM_PRE_FAULT_MEMORY requests are issued while setting up the guests.
Since these CoCo-specific events are likely to be architecture-specific KST helpers, take the existing generic test in pre_fault_memory_test.c as a starting template, and then introduce an x86-specific version of it with expanded coverage for SEV, SEV-ES, and SNP.
Since there's a reasonable chance that TDX could extend this for similar testing of TDX, give it a "coco-" prefix rather than an SEV-specific one.
Signed-off-by: Michael Roth michael.roth@amd.com Co-developed-by: Pratik R. Sampat pratikrajesh.sampat@amd.com Signed-off-by: Pratik R. Sampat pratikrajesh.sampat@amd.com Tested-by: Peter Gonda pgonda@google.com Tested-by: Srikanth Aithal sraithal@amd.com --- tools/testing/selftests/kvm/Makefile | 1 + .../kvm/x86_64/coco_pre_fault_memory_test.c | 314 ++++++++++++++++++ 2 files changed, 315 insertions(+) create mode 100644 tools/testing/selftests/kvm/x86_64/coco_pre_fault_memory_test.c
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index 45cb70c048bb..7b97750a7d71 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -129,6 +129,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/amx_test TEST_GEN_PROGS_x86_64 += x86_64/max_vcpuid_cap_test TEST_GEN_PROGS_x86_64 += x86_64/triple_fault_event_test TEST_GEN_PROGS_x86_64 += x86_64/recalc_apic_map_test +TEST_GEN_PROGS_x86_64 += x86_64/coco_pre_fault_memory_test TEST_GEN_PROGS_x86_64 += access_tracking_perf_test TEST_GEN_PROGS_x86_64 += coalesced_io_test TEST_GEN_PROGS_x86_64 += demand_paging_test diff --git a/tools/testing/selftests/kvm/x86_64/coco_pre_fault_memory_test.c b/tools/testing/selftests/kvm/x86_64/coco_pre_fault_memory_test.c new file mode 100644 index 000000000000..c31a5f9e18f4 --- /dev/null +++ b/tools/testing/selftests/kvm/x86_64/coco_pre_fault_memory_test.c @@ -0,0 +1,314 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/sizes.h> + +#include <test_util.h> +#include <kvm_util.h> +#include <processor.h> +#include "sev.h" + +/* Arbitrarily chosen values */ +#define TEST_SIZE (SZ_2M + PAGE_SIZE) +#define TEST_NPAGES (TEST_SIZE / PAGE_SIZE) +#define TEST_SLOT 10 +#define TEST_GPA 0x100000000ul +#define TEST_GVA 0x100000000ul + +enum prefault_snp_test_type { + /* Skip pre-faulting tests. */ + NO_PREFAULT_TYPE, + /* + * Issue KVM_PRE_FAULT_MEMORY for GFNs mapping non-private memory + * before finalizing the initial guest contents (e.g. via + * KVM_SEV_SNP_LAUNCH_FINISH for SNP guests). + * + * This should result in failure since KVM explicitly disallows + * KVM_PRE_FAULT_MEMORY from being issued prior to finalizing the + * initial guest contents. + */ + PREFAULT_SHARED_BEFORE_FINALIZING, + /* + * Issue KVM_PRE_FAULT_MEMORY for GFNs mapping private memory + * before finalizing the initial guest contents (e.g. via + * KVM_SEV_SNP_LAUNCH_FINISH for SNP guests). + * + * This should result in failure since KVM explicitly disallows + * KVM_PRE_FAULT_MEMORY from being issued prior to finalizing the + * initial guest contents. + */ + PREFAULT_PRIVATE_BEFORE_FINALIZING, + /* + * Issue KVM_PRE_FAULT_MEMORY for GFNs mapping shared/private + * memory after finalizing the initial guest contents + * (e.g. via * KVM_SEV_SNP_LAUNCH_FINISH for SNP guests). + * + * This should succeed since pre-faulting is supported for both + * non-private/private memory once the guest contents are finalized. + */ + PREFAULT_PRIVATE_SHARED_AFTER_FINALIZING +}; + +static void guest_code_sev(void) +{ + int i; + + GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ENABLED); + + for (i = 0; i < TEST_NPAGES; i++) { + uint64_t *src = (uint64_t *)(TEST_GVA + i * PAGE_SIZE); + uint64_t val = *src; + + /* Validate the data stored in the pages */ + if ((i < TEST_NPAGES / 2 && val != i + 1) || + (i >= TEST_NPAGES / 2 && val != 0)) { + GUEST_FAIL("Inconsistent view of memory values in guest"); + } + } + + if (rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ES_ENABLED) { + wrmsr(MSR_AMD64_SEV_ES_GHCB, GHCB_MSR_TERM_REQ); + __asm__ __volatile__("rep; vmmcall"); + GUEST_FAIL("This should be unreachable."); + } + + GUEST_DONE(); +} + +static void __pre_fault_memory(struct kvm_vcpu *vcpu, u64 gpa, u64 size, + u64 left, bool expect_fail) +{ + struct kvm_pre_fault_memory range = { + .gpa = gpa, + .size = size, + .flags = 0, + }; + int ret, save_errno; + u64 prev; + + do { + prev = range.size; + ret = __vcpu_ioctl(vcpu, KVM_PRE_FAULT_MEMORY, &range); + save_errno = errno; + TEST_ASSERT((range.size < prev) ^ (ret < 0), + "%sexpecting range.size to change on %s", + ret < 0 ? "not " : "", + ret < 0 ? "failure" : "success"); + } while (ret >= 0 ? range.size : save_errno == EINTR); + + TEST_ASSERT(expect_fail ? !(range.size == left) : (range.size == left), + "[EXPECT %s] completed with %lld bytes left, expected %" PRId64, + expect_fail ? "FAIL" : "PASS", + range.size, left); + + if (left == 0) { + TEST_ASSERT(expect_fail ? ret : !ret, + "[EXPECT %s] KVM_PRE_FAULT_MEMORY", + expect_fail ? "FAIL" : "PASS"); + } else { + /* + * For shared memory, no memory slot causes RET_PF_EMULATE. It + * results in -ENOENT. + * + * For private memory, no memory slot is an error case returning + * -EFAULT. However, it is also possible that only the GPA + * ranges backed by a slot are marked as private, in which case + * the noslot range will also result in -ENOENT. + * + * So allow both errors for now, but in the future it would be + * good to distinguish between these cases to tighten up the + * error-checking. + */ + TEST_ASSERT(expect_fail ? !ret : + (ret && (save_errno == EFAULT || save_errno == ENOENT)), + "[EXPECT %s] KVM_PRE_FAULT_MEMORY", + expect_fail ? "FAIL" : "PASS"); + } +} + +static void pre_fault_memory(struct kvm_vcpu *vcpu, u64 gpa, + u64 size, u64 left) +{ + __pre_fault_memory(vcpu, gpa, size, left, false); +} + +static void pre_fault_memory_negative(struct kvm_vcpu *vcpu, u64 gpa, + u64 size, u64 left) +{ + __pre_fault_memory(vcpu, gpa, size, left, true); +} + +static void pre_fault_memory_snp(struct kvm_vcpu *vcpu, struct kvm_vm *vm, + bool private, enum prefault_snp_test_type p_type) +{ + if (p_type == PREFAULT_SHARED_BEFORE_FINALIZING) + pre_fault_memory_negative(vcpu, TEST_GPA, SZ_2M, 0); + + snp_vm_launch_start(vm, SNP_POLICY); + + if (p_type == PREFAULT_SHARED_BEFORE_FINALIZING) + pre_fault_memory_negative(vcpu, TEST_GPA, SZ_2M, 0); + + if (private) { + /* + * Make sure when pages are pre-faulted later after + * finalization they are treated the same as a private + * access by the guest so that the expected gmem + * backing pages are used. + */ + vm_mem_set_private(vm, TEST_GPA, TEST_SIZE); + if (p_type == PREFAULT_PRIVATE_BEFORE_FINALIZING) + pre_fault_memory_negative(vcpu, TEST_GPA, SZ_2M, 0); + } else { + if (p_type == PREFAULT_SHARED_BEFORE_FINALIZING) + pre_fault_memory_negative(vcpu, TEST_GPA, SZ_2M, 0); + } + + snp_vm_launch_update(vm); + + if (p_type == PREFAULT_SHARED_BEFORE_FINALIZING) + pre_fault_memory_negative(vcpu, TEST_GPA, SZ_2M, 0); + + snp_vm_launch_finish(vm); + + /* + * After finalization, pre-faulting either private or shared + * ranges should work regardless of whether the pages were + * encrypted as part of setting up initial guest state. + */ + if (p_type == PREFAULT_PRIVATE_SHARED_AFTER_FINALIZING) { + pre_fault_memory(vcpu, TEST_GPA, SZ_2M, 0); + pre_fault_memory(vcpu, TEST_GPA + SZ_2M, PAGE_SIZE * 2, PAGE_SIZE); + pre_fault_memory(vcpu, TEST_GPA + TEST_SIZE, PAGE_SIZE, PAGE_SIZE); + } +} + +static void pre_fault_memory_sev(unsigned long vm_type, struct kvm_vcpu *vcpu, + struct kvm_vm *vm) +{ + uint32_t policy = (vm_type == KVM_X86_SEV_ES_VM) ? SEV_POLICY_ES : 0; + + pre_fault_memory(vcpu, TEST_GPA, SZ_2M, 0); + pre_fault_memory(vcpu, TEST_GPA + SZ_2M, PAGE_SIZE * 2, PAGE_SIZE); + pre_fault_memory(vcpu, TEST_GPA + TEST_SIZE, PAGE_SIZE, PAGE_SIZE); + + sev_vm_launch(vm, policy); + + pre_fault_memory(vcpu, TEST_GPA, SZ_2M, 0); + pre_fault_memory(vcpu, TEST_GPA + SZ_2M, PAGE_SIZE * 2, PAGE_SIZE); + pre_fault_memory(vcpu, TEST_GPA + TEST_SIZE, PAGE_SIZE, PAGE_SIZE); + + sev_vm_launch_measure(vm, alloca(256)); + + pre_fault_memory(vcpu, TEST_GPA, SZ_2M, 0); + pre_fault_memory(vcpu, TEST_GPA + SZ_2M, PAGE_SIZE * 2, PAGE_SIZE); + pre_fault_memory(vcpu, TEST_GPA + TEST_SIZE, PAGE_SIZE, PAGE_SIZE); + + sev_vm_launch_finish(vm); + + pre_fault_memory(vcpu, TEST_GPA, SZ_2M, 0); + pre_fault_memory(vcpu, TEST_GPA + SZ_2M, PAGE_SIZE * 2, PAGE_SIZE); + pre_fault_memory(vcpu, TEST_GPA + TEST_SIZE, PAGE_SIZE, PAGE_SIZE); +} + +static void test_pre_fault_memory_sev(unsigned long vm_type, bool private, + enum prefault_snp_test_type p_type) +{ + struct kvm_vcpu *vcpu; + struct kvm_vm *vm; + struct ucall uc; + int i; + + vm = vm_sev_create_with_one_vcpu(vm_type, guest_code_sev, &vcpu); + + vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, + TEST_GPA, TEST_SLOT, TEST_NPAGES, + (vm_type == KVM_X86_SNP_VM) ? KVM_MEM_GUEST_MEMFD : 0); + + /* + * Make sure guest page table is in agreement with what pages will be + * initially encrypted by the ASP. + */ + if (private) + vm_mem_set_protected(vm, TEST_SLOT, TEST_GPA, TEST_NPAGES); + + virt_map(vm, TEST_GVA, TEST_GPA, TEST_NPAGES); + + /* + * Populate the pages to compare data consistency in the guest + * Fill the first half with data and second half with zeros + */ + for (i = 0; i < TEST_NPAGES; i++) { + uint64_t *hva = addr_gva2hva(vm, TEST_GVA + i * PAGE_SIZE); + + if (i < TEST_NPAGES / 2) + *hva = i + 1; + else + *hva = 0; + } + + if (vm_type == KVM_X86_SNP_VM) + pre_fault_memory_snp(vcpu, vm, private, p_type); + else + pre_fault_memory_sev(vm_type, vcpu, vm); + + vcpu_run(vcpu); + + 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)); + TEST_ASSERT_EQ(vcpu->run->system_event.type, KVM_SYSTEM_EVENT_SEV_TERM); + TEST_ASSERT_EQ(vcpu->run->system_event.ndata, 1); + TEST_ASSERT_EQ(vcpu->run->system_event.data[0], GHCB_MSR_TERM_REQ); + goto out; + } + + switch (get_ucall(vcpu, &uc)) { + case UCALL_DONE: + break; + case UCALL_ABORT: + REPORT_GUEST_ASSERT(uc); + default: + TEST_FAIL("Unexpected exit: %s", + exit_reason_str(vcpu->run->exit_reason)); + } + +out: + kvm_vm_free(vm); +} + +static void test_pre_fault_memory(unsigned long vm_type, bool private) +{ + int pt; + + if (vm_type && !(kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(vm_type))) { + pr_info("Skipping tests for vm_type 0x%lx\n", vm_type); + return; + } + + switch (vm_type) { + case KVM_X86_SEV_VM: + case KVM_X86_SEV_ES_VM: + test_pre_fault_memory_sev(vm_type, private, NO_PREFAULT_TYPE); + break; + case KVM_X86_SNP_VM: + for (pt = 0; pt <= PREFAULT_PRIVATE_SHARED_AFTER_FINALIZING; pt++) + test_pre_fault_memory_sev(vm_type, private, pt); + break; + default: + abort(); + } +} + +int main(int argc, char *argv[]) +{ + TEST_REQUIRE(kvm_check_cap(KVM_CAP_PRE_FAULT_MEMORY)); + + test_pre_fault_memory(KVM_X86_SEV_VM, false); + test_pre_fault_memory(KVM_X86_SEV_VM, true); + test_pre_fault_memory(KVM_X86_SEV_ES_VM, false); + test_pre_fault_memory(KVM_X86_SEV_ES_VM, true); + test_pre_fault_memory(KVM_X86_SNP_VM, false); + test_pre_fault_memory(KVM_X86_SNP_VM, true); + + return 0; +}
fallocate triggers gmem_prepare(), and KVM_PRE_FAULT_MEMORY can cause guest page faults at unexpected points. Therefore, introduce several test cases to interleave fallocate, hole punching through various parts of the SNP launch lifecycle, and observe both positive and negative vcpcu_run exit statuses.
Signed-off-by: Pratik R. Sampat pratikrajesh.sampat@amd.com Tested-by: Peter Gonda pgonda@google.com Tested-by: Srikanth Aithal sraithal@amd.com --- .../kvm/x86_64/coco_pre_fault_memory_test.c | 121 +++++++++++++++++- 1 file changed, 114 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/kvm/x86_64/coco_pre_fault_memory_test.c b/tools/testing/selftests/kvm/x86_64/coco_pre_fault_memory_test.c index c31a5f9e18f4..e9757ba3234c 100644 --- a/tools/testing/selftests/kvm/x86_64/coco_pre_fault_memory_test.c +++ b/tools/testing/selftests/kvm/x86_64/coco_pre_fault_memory_test.c @@ -47,6 +47,31 @@ enum prefault_snp_test_type { PREFAULT_PRIVATE_SHARED_AFTER_FINALIZING };
+enum falloc_snp_test_type { + /* Skip alloc tests. */ + NO_ALLOC_TYPE, + /* + * Allocate and/or deallocate a region of guest memfd before + * memory regions are updated to be protected and encrypted + * + * This should succeed since allocation and deallocation is + * supported before the memory is finalized. + */ + ALLOC_BEFORE_UPDATE, + ALLOC_AFTER_UPDATE, + DEALLOC_BEFORE_UPDATE, + ALLOC_DEALLOC_BEFORE_UPDATE, + /* + * Allocate and/or deallocate a region of guest memfd after + * memory regions are updated to be protected and encrypted + * + * This should fail since dealloc will nuke the pages that + * contain the initial code that the guest will run. + */ + DEALLOC_AFTER_UPDATE, + ALLOC_DEALLOC_AFTER_UPDATE +}; + static void guest_code_sev(void) { int i; @@ -73,6 +98,29 @@ static void guest_code_sev(void) GUEST_DONE(); }
+static void __falloc_region(struct kvm_vm *vm, bool punch_hole) +{ + int ctr, ret, flags = FALLOC_FL_KEEP_SIZE; + struct userspace_mem_region *region; + + hash_for_each(vm->regions.slot_hash, ctr, region, slot_node) { + if (punch_hole) + flags |= FALLOC_FL_PUNCH_HOLE; + ret = fallocate(region->region.guest_memfd, flags, 0, PAGE_SIZE * TEST_NPAGES); + TEST_ASSERT(!ret, "fallocate should succeed."); + } +} + +static void gmemfd_alloc(struct kvm_vm *vm) +{ + __falloc_region(vm, false); +} + +static void gmemfd_dealloc(struct kvm_vm *vm) +{ + __falloc_region(vm, true); +} + static void __pre_fault_memory(struct kvm_vcpu *vcpu, u64 gpa, u64 size, u64 left, bool expect_fail) { @@ -137,13 +185,34 @@ static void pre_fault_memory_negative(struct kvm_vcpu *vcpu, u64 gpa, }
static void pre_fault_memory_snp(struct kvm_vcpu *vcpu, struct kvm_vm *vm, - bool private, enum prefault_snp_test_type p_type) + bool private, enum prefault_snp_test_type p_type, + enum falloc_snp_test_type f_type) { + if (f_type == ALLOC_BEFORE_UPDATE || + f_type == ALLOC_DEALLOC_BEFORE_UPDATE) { + gmemfd_alloc(vm); + } + + if (f_type == DEALLOC_BEFORE_UPDATE || + f_type == ALLOC_DEALLOC_BEFORE_UPDATE) { + gmemfd_dealloc(vm); + } + if (p_type == PREFAULT_SHARED_BEFORE_FINALIZING) pre_fault_memory_negative(vcpu, TEST_GPA, SZ_2M, 0);
snp_vm_launch_start(vm, SNP_POLICY);
+ if (f_type == ALLOC_BEFORE_UPDATE || + f_type == ALLOC_DEALLOC_BEFORE_UPDATE) { + gmemfd_alloc(vm); + } + + if (f_type == DEALLOC_BEFORE_UPDATE || + f_type == ALLOC_DEALLOC_BEFORE_UPDATE) { + gmemfd_dealloc(vm); + } + if (p_type == PREFAULT_SHARED_BEFORE_FINALIZING) pre_fault_memory_negative(vcpu, TEST_GPA, SZ_2M, 0);
@@ -164,11 +233,36 @@ static void pre_fault_memory_snp(struct kvm_vcpu *vcpu, struct kvm_vm *vm,
snp_vm_launch_update(vm);
+ if (f_type == ALLOC_AFTER_UPDATE || + f_type == ALLOC_DEALLOC_AFTER_UPDATE) { + gmemfd_alloc(vm); + } + + /* + * Hole-punch after SNP LAUNCH UPDATE is not expected to fail + * immediately, rather its affects are observed on vcpu_run() + * as the pages that contain the initial code is nuked. + */ + if (f_type == DEALLOC_AFTER_UPDATE || + f_type == ALLOC_DEALLOC_AFTER_UPDATE) { + gmemfd_dealloc(vm); + } + if (p_type == PREFAULT_SHARED_BEFORE_FINALIZING) pre_fault_memory_negative(vcpu, TEST_GPA, SZ_2M, 0);
snp_vm_launch_finish(vm);
+ if (f_type == ALLOC_AFTER_UPDATE || + f_type == ALLOC_DEALLOC_AFTER_UPDATE) { + gmemfd_alloc(vm); + } + + if (f_type == DEALLOC_AFTER_UPDATE || + f_type == ALLOC_DEALLOC_AFTER_UPDATE) { + gmemfd_dealloc(vm); + } + /* * After finalization, pre-faulting either private or shared * ranges should work regardless of whether the pages were @@ -210,7 +304,8 @@ static void pre_fault_memory_sev(unsigned long vm_type, struct kvm_vcpu *vcpu, }
static void test_pre_fault_memory_sev(unsigned long vm_type, bool private, - enum prefault_snp_test_type p_type) + enum prefault_snp_test_type p_type, + enum falloc_snp_test_type f_type) { struct kvm_vcpu *vcpu; struct kvm_vm *vm; @@ -246,12 +341,22 @@ static void test_pre_fault_memory_sev(unsigned long vm_type, bool private, }
if (vm_type == KVM_X86_SNP_VM) - pre_fault_memory_snp(vcpu, vm, private, p_type); + pre_fault_memory_snp(vcpu, vm, private, p_type, f_type); else pre_fault_memory_sev(vm_type, vcpu, vm);
vcpu_run(vcpu);
+ /* Expect SHUTDOWN when we falloc using PUNCH_HOLE after SNP_UPDATE */ + if (vm->type == KVM_X86_SNP_VM && + (f_type == DEALLOC_AFTER_UPDATE || + f_type == ALLOC_DEALLOC_AFTER_UPDATE)) { + TEST_ASSERT(vcpu->run->exit_reason == KVM_EXIT_SHUTDOWN, + "Wanted SYSTEM_EVENT, got %s", + exit_reason_str(vcpu->run->exit_reason)); + goto out; + } + 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", @@ -278,7 +383,7 @@ static void test_pre_fault_memory_sev(unsigned long vm_type, bool private,
static void test_pre_fault_memory(unsigned long vm_type, bool private) { - int pt; + int pt, ft;
if (vm_type && !(kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(vm_type))) { pr_info("Skipping tests for vm_type 0x%lx\n", vm_type); @@ -288,11 +393,13 @@ static void test_pre_fault_memory(unsigned long vm_type, bool private) switch (vm_type) { case KVM_X86_SEV_VM: case KVM_X86_SEV_ES_VM: - test_pre_fault_memory_sev(vm_type, private, NO_PREFAULT_TYPE); + test_pre_fault_memory_sev(vm_type, private, NO_PREFAULT_TYPE, NO_ALLOC_TYPE); break; case KVM_X86_SNP_VM: - for (pt = 0; pt <= PREFAULT_PRIVATE_SHARED_AFTER_FINALIZING; pt++) - test_pre_fault_memory_sev(vm_type, private, pt); + for (pt = 0; pt <= PREFAULT_PRIVATE_SHARED_AFTER_FINALIZING; pt++) { + for (ft = 0; ft <= ALLOC_DEALLOC_AFTER_UPDATE; ft++) + test_pre_fault_memory_sev(vm_type, private, pt, ft); + } break; default: abort();
On Thu, Sep 05, 2024, Pratik R. Sampat wrote:
This series primarily introduces SEV-SNP test for the kernel selftest framework. It tests boot, ioctl, pre fault, and fallocate in various combinations to exercise both positive and negative launch flow paths.
Patch 1 - Adds a wrapper for the ioctl calls that decouple ioctl and asserts, which enables the use of negative test cases. No functional change intended. Patch 2 - Extend the sev smoke tests to use the SNP specific ioctl calls and sets up memory to boot a SNP guest VM Patch 3 - Adds SNP to shutdown testing Patch 4, 5 - Tests the ioctl path for SEV, SEV-ES and SNP Patch 6 - Adds support for SNP in KVM_SEV_INIT2 tests Patch 7,8,9 - Enable Prefault tests for SEV, SEV-ES and SNP
There are three separate series here:
1. Smoke test support for SNP 2. Negative tests for SEV+ 3. Prefault tests for SEV+
#3 likely has a dependency on #1, and probably on #2 as well (for style if nothing else). But that's really just an argument for focuing on #1 first, and the moving onto the others once that's ready to go.
Hi Sean,
On 10/14/2024 5:23 PM, Sean Christopherson wrote:
On Thu, Sep 05, 2024, Pratik R. Sampat wrote:
This series primarily introduces SEV-SNP test for the kernel selftest framework. It tests boot, ioctl, pre fault, and fallocate in various combinations to exercise both positive and negative launch flow paths.
Patch 1 - Adds a wrapper for the ioctl calls that decouple ioctl and asserts, which enables the use of negative test cases. No functional change intended. Patch 2 - Extend the sev smoke tests to use the SNP specific ioctl calls and sets up memory to boot a SNP guest VM Patch 3 - Adds SNP to shutdown testing Patch 4, 5 - Tests the ioctl path for SEV, SEV-ES and SNP Patch 6 - Adds support for SNP in KVM_SEV_INIT2 tests Patch 7,8,9 - Enable Prefault tests for SEV, SEV-ES and SNP
There are three separate series here:
- Smoke test support for SNP
- Negative tests for SEV+
- Prefault tests for SEV+
#3 likely has a dependency on #1, and probably on #2 as well (for style if nothing else). But that's really just an argument for focuing on #1 first, and the moving onto the others once that's ready to go.
Based on your feedback on the rest of this patchset, this makes sense to me. I will first prep for the changes for patchset #1 and once we lock that down I can introduce patchset #2 and #3 based on that design.
Thank you again for your feedback! Pratik
linux-kselftest-mirror@lists.linaro.org
-
Pratik R. Sampat -
Sean Christopherson