This patch series extends the sev_init2 and the sev_smoke test to exercise the SEV-SNP VM launch workflow.
Primarily, it introduces the architectural defines, its support in the SEV library and extends the tests to interact with the SEV-SNP ioctl() wrappers.
Patch 1 - Do not advertize SNP on incompatible firmware Patch 2 - Remove SEV support on platform init failure Patch 3 - SNP test for KVM_SEV_INIT2 Patch 4 - Add VMGEXIT helper Patch 5 - Introduce SEV+ VM type check Patch 6 - SNP iotcl() plumbing for the SEV library Patch 7 - Force set GUEST_MEMFD for SNP Patch 8 - Cleanups of smoke test - Decouple policy from type Patch 9 - SNP smoke test
The series is based on git.kernel.org/pub/scm/virt/kvm/kvm.git next
v5..v6 * Rename is_sev_platform_init to sev_fw_initialized (Nikunj) * Rename KVM CPU feature X86_FEATURE_SNP to X86_FEATURE_SEV_SNP (Nikunj) * Collected Tags from Nikunj, Pankaj, Srikanth.
v4..v5: https://lore.kernel.org/kvm/8e7d8172-879e-4a28-8438-343b1c386ec9@amd.com/ * Introduced a check to disable advertising support for SEV, SEV-ES and SNP when platform initialization fails (Nikunj) * Remove the redundant SNP check within is_sev_vm() (Nikunj) * Cleanup of the encrypt_region flow for better readability (Nikunj) * Refactor paths to use the canonical $(ARCH) to rebase for kvm/next
v3..v4: https://lore.kernel.org/kvm/20241114234104.128532-1-pratikrajesh.sampat@amd.... * Remove SNP FW API version check in the test and ensure the KVM capability advertizes the presence of the feature. Retain the minimum version definitions to exercise these API versions in the smoke test * Retained only the SNP smoke test and SNP_INIT2 test * The SNP architectural defined merged with SNP_INIT2 test patch * SNP shutdown merged with SNP smoke test patch * Add SEV VM type check to abstract comparisons and reduce clutter * Define a SNP default policy which sets bits based on the presence of SMT * Decouple privatization and encryption for it to be SNP agnostic * Assert for only positive tests using vm_ioctl() * Dropped tested-by tags
In summary - based on comments from Sean, I have primarily reduced the scope of this patch series to focus on breaking down the SNP smoke test patch (v3 - patch2) to first introduce SEV-SNP support and use this interface to extend the sev_init2 and the sev_smoke test.
The rest of the v3 patchset that introduces ioctl, pre fault, fallocate and negative tests, will be re-worked and re-introduced subsequently in future patch series post addressing the issues discussed.
v2..v3: https://lore.kernel.org/kvm/20240905124107.6954-1-pratikrajesh.sampat@amd.co... * Remove the assignments for the prefault and fallocate test type enums. * Fix error message for sev launch measure and finish. * Collect tested-by tags [Peter, Srikanth]
Pratik R. Sampat (9): KVM: SEV: Disable SEV-SNP on FW validation failure KVM: SEV: Disable SEV on platform init failure KVM: selftests: SEV-SNP test for KVM_SEV_INIT2 KVM: selftests: Add VMGEXIT helper KVM: selftests: Introduce SEV VM type check KVM: selftests: Add library support for interacting with SNP KVM: selftests: Force GUEST_MEMFD flag for SNP VM type KVM: selftests: Abstractions for SEV to decouple policy from type KVM: selftests: Add a basic SEV-SNP smoke test
arch/x86/kvm/svm/sev.c | 6 +- drivers/crypto/ccp/sev-dev.c | 16 +++ include/linux/psp-sev.h | 6 ++ .../selftests/kvm/include/x86/processor.h | 1 + tools/testing/selftests/kvm/include/x86/sev.h | 55 ++++++++++- tools/testing/selftests/kvm/lib/kvm_util.c | 7 +- .../testing/selftests/kvm/lib/x86/processor.c | 4 +- tools/testing/selftests/kvm/lib/x86/sev.c | 99 ++++++++++++++++++- .../selftests/kvm/x86/sev_init2_tests.c | 13 +++ .../selftests/kvm/x86/sev_smoke_test.c | 96 ++++++++++++++---- 10 files changed, 272 insertions(+), 31 deletions(-)
On incompatible firmware versions, SEV-SNP support is pulled and the setup is not performed. However, the platform and subsequently the KVM capability may continue to advertise support for it. Disable support for SEV-SNP if the FW version validation fails.
Fixes: 1dfe571c12cf ("KVM: SEV: Add initial SEV-SNP support") Suggested-by: Sean Christopherson seanjc@google.com Reviewed-by: Nikunj A Dadhania nikunj@amd.com Tested-by: Srikanth Aithal sraithal@amd.com Signed-off-by: Pratik R. Sampat prsampat@amd.com --- v5..v6:
* Collected tags from Nikunj and Srikanth. --- arch/x86/kvm/svm/sev.c | 4 +++- drivers/crypto/ccp/sev-dev.c | 6 ++++++ include/linux/psp-sev.h | 3 +++ 3 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 0f04f365885c..b709c2f0945c 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -3040,7 +3040,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) && + snp_fw_valid());
out: if (boot_cpu_has(X86_FEATURE_SEV)) diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index af018afd9cd7..b45cd60c19b0 100644 --- 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 snp_fw_valid(void) +{ + return sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR); +} +EXPORT_SYMBOL_GPL(snp_fw_valid); + static void sev_irq_handler(int irq, void *data, unsigned int status) { struct sev_device *sev = data; diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h index 903ddfea8585..e841a8fbbb15 100644 --- a/include/linux/psp-sev.h +++ b/include/linux/psp-sev.h @@ -945,6 +945,7 @@ int sev_do_cmd(int cmd, void *data, int *psp_ret); void *psp_copy_user_blob(u64 uaddr, u32 len); void *snp_alloc_firmware_page(gfp_t mask); void snp_free_firmware_page(void *addr); +bool snp_fw_valid(void);
#else /* !CONFIG_CRYPTO_DEV_SP_PSP */
@@ -979,6 +980,8 @@ static inline void *snp_alloc_firmware_page(gfp_t mask)
static inline void snp_free_firmware_page(void *addr) { }
+static inline bool snp_fw_valid(void) { return false; } + #endif /* CONFIG_CRYPTO_DEV_SP_PSP */
#endif /* __PSP_SEV_H__ */
On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 0f04f365885c..b709c2f0945c 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -3040,7 +3040,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) &&snp_fw_valid());out: if (boot_cpu_has(X86_FEATURE_SEV)) diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index af018afd9cd7..b45cd60c19b0 100644 --- 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 snp_fw_valid(void) +{
- return sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR);
This lacks any sense of abstraction. KVM doesn't care if the firmware is valid, KVM only cares if SNP is support. KVM _must_ run after ASP initialization[*], so I'm pretty sure KVM just needs a way to query sev->snp_initialized.
[*] https://lore.kernel.org/all/f78ddb64087df27e7bcb1ae0ab53f55aa0804fab.1739226...
Hello Sean,
On 2/11/25 7:54 PM, Sean Christopherson wrote:
On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 0f04f365885c..b709c2f0945c 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -3040,7 +3040,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) &&snp_fw_valid());out: if (boot_cpu_has(X86_FEATURE_SEV)) diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index af018afd9cd7..b45cd60c19b0 100644 --- 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 snp_fw_valid(void) +{
- return sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR);
This lacks any sense of abstraction. KVM doesn't care if the firmware is valid, KVM only cares if SNP is support. KVM _must_ run after ASP initialization[*], so I'm pretty sure KVM just needs a way to query sev->snp_initialized.
[*] https://lore.kernel.org/all/f78ddb64087df27e7bcb1ae0ab53f55aa0804fab.1739226...
Thank you for your reviews!
Sure, I can instead just export this function (with a better name) to return sev->snp_initialized within sev_hardware_setup().
If the platform initialization sev_platform_init() fails, SEV cannot be set up and a secure VM cannot be spawned. Therefore, in this case, ensure that KVM does not set up, nor advertise support for SEV, SEV-ES, and SEV-SNP.
Suggested-by: Nikunj A Dadhania nikunj@amd.com Tested-by: Srikanth Aithal sraithal@amd.com Signed-off-by: Pratik R. Sampat prsampat@amd.com --- v5..v6:
* Rename is_sev_platform_init to sev_fw_initialized (Nikunj) * Collected tags from Srikanth. --- arch/x86/kvm/svm/sev.c | 2 +- drivers/crypto/ccp/sev-dev.c | 10 ++++++++++ include/linux/psp-sev.h | 3 +++ 3 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index b709c2f0945c..42d1309f8a54 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -2957,7 +2957,7 @@ void __init sev_hardware_setup(void) bool sev_es_supported = false; bool sev_supported = false;
- if (!sev_enabled || !npt_enabled || !nrips) + if (!sev_fw_initialized() || !sev_enabled || !npt_enabled || !nrips) goto out;
/* diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index b45cd60c19b0..bd166b997ac9 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -81,6 +81,8 @@ MODULE_FIRMWARE("amd/amd_sev_fam19h_model1xh.sbin"); /* 4th gen EPYC */ static bool psp_dead; static int psp_timeout;
+static bool platform_init; + /* Trusted Memory Region (TMR): * The TMR is a 1MB area that must be 1MB aligned. Use the page allocator * to allocate the memory, which will return aligned memory for the specified @@ -1358,6 +1360,12 @@ int sev_platform_init(struct sev_platform_init_args *args) } EXPORT_SYMBOL_GPL(sev_platform_init);
+bool sev_fw_initialized(void) +{ + return platform_init; +} +EXPORT_SYMBOL_GPL(sev_fw_initialized); + static int __sev_platform_shutdown_locked(int *error) { struct psp_device *psp = psp_master; @@ -2427,6 +2435,8 @@ void sev_pci_init(void) if (rc) dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n", args.error, rc); + else + platform_init = true;
dev_info(sev->dev, "SEV%s API:%d.%d build:%d\n", sev->snp_initialized ? "-SNP" : "", sev->api_major, sev->api_minor, sev->build); diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h index e841a8fbbb15..55ef473953e3 100644 --- a/include/linux/psp-sev.h +++ b/include/linux/psp-sev.h @@ -946,6 +946,7 @@ void *psp_copy_user_blob(u64 uaddr, u32 len); void *snp_alloc_firmware_page(gfp_t mask); void snp_free_firmware_page(void *addr); bool snp_fw_valid(void); +bool sev_fw_initialized(void);
#else /* !CONFIG_CRYPTO_DEV_SP_PSP */
@@ -982,6 +983,8 @@ static inline void snp_free_firmware_page(void *addr) { }
static inline bool snp_fw_valid(void) { return false; }
+static inline bool sev_fw_initialized(void) { return false; } + #endif /* CONFIG_CRYPTO_DEV_SP_PSP */
#endif /* __PSP_SEV_H__ */
On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
If the platform initialization sev_platform_init() fails, SEV cannot be set up and a secure VM cannot be spawned. Therefore, in this case, ensure that KVM does not set up, nor advertise support for SEV, SEV-ES, and SEV-SNP.
Suggested-by: Nikunj A Dadhania nikunj@amd.com Tested-by: Srikanth Aithal sraithal@amd.com Signed-off-by: Pratik R. Sampat prsampat@amd.com
v5..v6:
- Rename is_sev_platform_init to sev_fw_initialized (Nikunj)
- Collected tags from Srikanth.
arch/x86/kvm/svm/sev.c | 2 +- drivers/crypto/ccp/sev-dev.c | 10 ++++++++++ include/linux/psp-sev.h | 3 +++ 3 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index b709c2f0945c..42d1309f8a54 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -2957,7 +2957,7 @@ void __init sev_hardware_setup(void) bool sev_es_supported = false; bool sev_supported = false;
- if (!sev_enabled || !npt_enabled || !nrips)
- if (!sev_fw_initialized() || !sev_enabled || !npt_enabled || !nrips) goto out;
Me thinks this wasn't tested with KVM_AMD built-in[1]. I'm pretty sure Ashish's fix[2] solves all of this?
[1] https://lore.kernel.org/all/d6d08c6b-9602-4f3d-92c2-8db6d50a1b92@amd.com [2] https://lore.kernel.org/all/f78ddb64087df27e7bcb1ae0ab53f55aa0804fab.1739226...
On 2/11/25 7:58 PM, Sean Christopherson wrote:
On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
If the platform initialization sev_platform_init() fails, SEV cannot be set up and a secure VM cannot be spawned. Therefore, in this case, ensure that KVM does not set up, nor advertise support for SEV, SEV-ES, and SEV-SNP.
Suggested-by: Nikunj A Dadhania nikunj@amd.com Tested-by: Srikanth Aithal sraithal@amd.com Signed-off-by: Pratik R. Sampat prsampat@amd.com
v5..v6:
- Rename is_sev_platform_init to sev_fw_initialized (Nikunj)
- Collected tags from Srikanth.
arch/x86/kvm/svm/sev.c | 2 +- drivers/crypto/ccp/sev-dev.c | 10 ++++++++++ include/linux/psp-sev.h | 3 +++ 3 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index b709c2f0945c..42d1309f8a54 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -2957,7 +2957,7 @@ void __init sev_hardware_setup(void) bool sev_es_supported = false; bool sev_supported = false;
- if (!sev_enabled || !npt_enabled || !nrips)
- if (!sev_fw_initialized() || !sev_enabled || !npt_enabled || !nrips) goto out;
Me thinks this wasn't tested with KVM_AMD built-in[1]. I'm pretty sure Ashish's fix[2] solves all of this?
[1] https://lore.kernel.org/all/d6d08c6b-9602-4f3d-92c2-8db6d50a1b92@amd.com [2] https://lore.kernel.org/all/f78ddb64087df27e7bcb1ae0ab53f55aa0804fab.1739226...
Thanks for pointing this out. I hadn't considered this earlier.
The combination of both of these should solve all of the issues caused by a failing SEV INIT. I will remove this patch for the next iteration then.
Thanks!
Add the X86_FEATURE_SEV_SNP CPU feature to the architectural definition for the SEV-SNP VM type to exercise the KVM_SEV_INIT2 call. Ensure that the SNP test is skipped in scenarios where CPUID supports it but KVM does not, preventing reporting of failure in such cases.
Reviewed-by: Nikunj A Dadhania nikunj@amd.com Tested-by: Srikanth Aithal sraithal@amd.com Signed-off-by: Pratik R. Sampat prsampat@amd.com --- v5..v6:
* Rename KVM CPU feature X86_FEATURE_SNP to X86_FEATURE_SEV_SNP (Nikunj) * Collected tags from Nikunj and Srikanth. --- tools/testing/selftests/kvm/include/x86/processor.h | 1 + tools/testing/selftests/kvm/x86/sev_init2_tests.c | 13 +++++++++++++ 2 files changed, 14 insertions(+)
diff --git a/tools/testing/selftests/kvm/include/x86/processor.h b/tools/testing/selftests/kvm/include/x86/processor.h index d60da8966772..6f63fd10bbc6 100644 --- a/tools/testing/selftests/kvm/include/x86/processor.h +++ b/tools/testing/selftests/kvm/include/x86/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_SEV_SNP KVM_X86_CPU_FEATURE(0x8000001F, 0, EAX, 4)
/* * KVM defined paravirt features. diff --git a/tools/testing/selftests/kvm/x86/sev_init2_tests.c b/tools/testing/selftests/kvm/x86/sev_init2_tests.c index 3fb967f40c6a..ab3dd11ac163 100644 --- a/tools/testing/selftests/kvm/x86/sev_init2_tests.c +++ b/tools/testing/selftests/kvm/x86/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_SEV_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; }
Abstract rep vmmcall coded into the VMGEXIT helper for the sev library.
No functional change intended.
Reviewed-by: Pankaj Gupta pankaj.gupta@amd.com Tested-by: Srikanth Aithal sraithal@amd.com Signed-off-by: Pratik R. Sampat prsampat@amd.com --- v5..v6:
* Collected tags from Pankaj and Srikanth. --- tools/testing/selftests/kvm/include/x86/sev.h | 2 ++ tools/testing/selftests/kvm/x86/sev_smoke_test.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/kvm/include/x86/sev.h b/tools/testing/selftests/kvm/include/x86/sev.h index 82c11c81a956..e7df5d0987f6 100644 --- a/tools/testing/selftests/kvm/include/x86/sev.h +++ b/tools/testing/selftests/kvm/include/x86/sev.h @@ -27,6 +27,8 @@ enum sev_guest_state {
#define GHCB_MSR_TERM_REQ 0x100
+#define VMGEXIT() { __asm__ __volatile__("rep; vmmcall"); } + 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); diff --git a/tools/testing/selftests/kvm/x86/sev_smoke_test.c b/tools/testing/selftests/kvm/x86/sev_smoke_test.c index a1a688e75266..38f647fe55d2 100644 --- a/tools/testing/selftests/kvm/x86/sev_smoke_test.c +++ b/tools/testing/selftests/kvm/x86/sev_smoke_test.c @@ -27,7 +27,7 @@ static void guest_sev_es_code(void) * force "termination" to signal "done" via the GHCB MSR protocol. */ wrmsr(MSR_AMD64_SEV_ES_GHCB, GHCB_MSR_TERM_REQ); - __asm__ __volatile__("rep; vmmcall"); + VMGEXIT(); }
static void guest_sev_code(void)
On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
Abstract rep vmmcall coded into the VMGEXIT helper for the sev library.
No functional change intended.
Reviewed-by: Pankaj Gupta pankaj.gupta@amd.com Tested-by: Srikanth Aithal sraithal@amd.com Signed-off-by: Pratik R. Sampat prsampat@amd.com
v5..v6:
- Collected tags from Pankaj and Srikanth.
tools/testing/selftests/kvm/include/x86/sev.h | 2 ++ tools/testing/selftests/kvm/x86/sev_smoke_test.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/kvm/include/x86/sev.h b/tools/testing/selftests/kvm/include/x86/sev.h index 82c11c81a956..e7df5d0987f6 100644 --- a/tools/testing/selftests/kvm/include/x86/sev.h +++ b/tools/testing/selftests/kvm/include/x86/sev.h @@ -27,6 +27,8 @@ enum sev_guest_state { #define GHCB_MSR_TERM_REQ 0x100 +#define VMGEXIT() { __asm__ __volatile__("rep; vmmcall"); }
Please make this a proper inline function, there's no reason to use a macro.
On 2/11/25 7:59 PM, Sean Christopherson wrote:
On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
Abstract rep vmmcall coded into the VMGEXIT helper for the sev library.
No functional change intended.
Reviewed-by: Pankaj Gupta pankaj.gupta@amd.com Tested-by: Srikanth Aithal sraithal@amd.com Signed-off-by: Pratik R. Sampat prsampat@amd.com
v5..v6:
- Collected tags from Pankaj and Srikanth.
tools/testing/selftests/kvm/include/x86/sev.h | 2 ++ tools/testing/selftests/kvm/x86/sev_smoke_test.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/kvm/include/x86/sev.h b/tools/testing/selftests/kvm/include/x86/sev.h index 82c11c81a956..e7df5d0987f6 100644 --- a/tools/testing/selftests/kvm/include/x86/sev.h +++ b/tools/testing/selftests/kvm/include/x86/sev.h @@ -27,6 +27,8 @@ enum sev_guest_state { #define GHCB_MSR_TERM_REQ 0x100 +#define VMGEXIT() { __asm__ __volatile__("rep; vmmcall"); }
Please make this a proper inline function, there's no reason to use a macro.
Sure, I can make this an inline function.
In preparation for SNP, declutter the vm type check by introducing a SEV-SNP VM type check as well as a transitive set of helper functions.
The SNP VM type is the subset of SEV-ES. Similarly, the SEV-ES and SNP types are subset of the SEV VM type check.
Tested-by: Srikanth Aithal sraithal@amd.com Signed-off-by: Pratik R. Sampat prsampat@amd.com --- v5..v6:
* Commit message grammar * Collected tags from Srikanth. --- tools/testing/selftests/kvm/include/x86/sev.h | 4 ++++ tools/testing/selftests/kvm/lib/x86/processor.c | 4 ++-- tools/testing/selftests/kvm/lib/x86/sev.c | 17 +++++++++++++++++ .../testing/selftests/kvm/x86/sev_smoke_test.c | 2 +- 4 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/kvm/include/x86/sev.h b/tools/testing/selftests/kvm/include/x86/sev.h index e7df5d0987f6..faed91435963 100644 --- a/tools/testing/selftests/kvm/include/x86/sev.h +++ b/tools/testing/selftests/kvm/include/x86/sev.h @@ -29,6 +29,10 @@ enum sev_guest_state {
#define VMGEXIT() { __asm__ __volatile__("rep; vmmcall"); }
+bool is_sev_vm(struct kvm_vm *vm); +bool is_sev_es_vm(struct kvm_vm *vm); +bool is_sev_snp_vm(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); diff --git a/tools/testing/selftests/kvm/lib/x86/processor.c b/tools/testing/selftests/kvm/lib/x86/processor.c index bd5a802fa7a5..a92dc1dad085 100644 --- a/tools/testing/selftests/kvm/lib/x86/processor.c +++ b/tools/testing/selftests/kvm/lib/x86/processor.c @@ -639,7 +639,7 @@ 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 (is_sev_vm(vm)) { struct kvm_sev_init init = { 0 };
vm_sev_ioctl(vm, KVM_SEV_INIT2, &init); @@ -1156,7 +1156,7 @@ 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 (is_sev_vm(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/sev.c b/tools/testing/selftests/kvm/lib/x86/sev.c index e9535ee20b7f..280ec42e281b 100644 --- a/tools/testing/selftests/kvm/lib/x86/sev.c +++ b/tools/testing/selftests/kvm/lib/x86/sev.c @@ -4,6 +4,23 @@
#include "sev.h"
+bool is_sev_snp_vm(struct kvm_vm *vm) +{ + return vm->type == KVM_X86_SNP_VM; +} + +/* A SNP VM is also a SEV-ES VM */ +bool is_sev_es_vm(struct kvm_vm *vm) +{ + return is_sev_snp_vm(vm) || vm->type == KVM_X86_SEV_ES_VM; +} + +/* A SEV-ES and SNP VM is also a SEV VM */ +bool is_sev_vm(struct kvm_vm *vm) +{ + return is_sev_es_vm(vm) || vm->type == KVM_X86_SEV_VM; +} + /* * sparsebit_next_clear() can return 0 if [x, 2**64-1] are all set, and the * -1 would then cause an underflow back to 2**64 - 1. This is expected and diff --git a/tools/testing/selftests/kvm/x86/sev_smoke_test.c b/tools/testing/selftests/kvm/x86/sev_smoke_test.c index 38f647fe55d2..b18c78314d5b 100644 --- a/tools/testing/selftests/kvm/x86/sev_smoke_test.c +++ b/tools/testing/selftests/kvm/x86/sev_smoke_test.c @@ -123,7 +123,7 @@ static void test_sev(void *guest_code, uint64_t policy) for (;;) { vcpu_run(vcpu);
- if (policy & SEV_POLICY_ES) { + if (is_sev_es_vm(vm)) { TEST_ASSERT(vcpu->run->exit_reason == KVM_EXIT_SYSTEM_EVENT, "Wanted SYSTEM_EVENT, got %s", exit_reason_str(vcpu->run->exit_reason));
Extend the SEV library to include support for SNP ioctl() wrappers, which aid in launching and interacting with a SEV-SNP guest.
Tested-by: Srikanth Aithal sraithal@amd.com Signed-off-by: Pratik R. Sampat prsampat@amd.com --- v5..v6:
* Collected tags from Srikanth. --- tools/testing/selftests/kvm/include/x86/sev.h | 49 ++++++++++- tools/testing/selftests/kvm/lib/x86/sev.c | 82 +++++++++++++++++-- 2 files changed, 125 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/kvm/include/x86/sev.h b/tools/testing/selftests/kvm/include/x86/sev.h index faed91435963..fd5d5261e10e 100644 --- a/tools/testing/selftests/kvm/include/x86/sev.h +++ b/tools/testing/selftests/kvm/include/x86/sev.h @@ -22,9 +22,20 @@ enum sev_guest_state { SEV_GUEST_STATE_RUNNING, };
+/* Minimum firmware version required for the SEV-SNP support */ +#define SNP_MIN_API_MAJOR 1 +#define SNP_MIN_API_MINOR 51 + #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_FW_VER_MINOR(min) ((uint8_t)(min) << 0) +#define SNP_FW_VER_MAJOR(maj) ((uint8_t)(maj) << 8) + #define GHCB_MSR_TERM_REQ 0x100
#define VMGEXIT() { __asm__ __volatile__("rep; vmmcall"); } @@ -36,13 +47,35 @@ bool is_sev_snp_vm(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); +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);
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);
+/* + * A SEV-SNP VM requires the policy reserved bit to always be set. + * The SMT policy bit is also required to be set based on SMT being + * available and active on the system. + */ +static inline u64 snp_default_policy(void) +{ + bool smt_active = false; + FILE *f; + + f = fopen("/sys/devices/system/cpu/smt/active", "r"); + if (f) { + smt_active = fgetc(f) - '0'; + fclose(f); + } + + return SNP_POLICY_RSVD_MBO | (smt_active ? SNP_POLICY_SMT : 0); +} + /* * The KVM_MEMORY_ENCRYPT_OP uAPI is utter garbage and takes an "unsigned long" * instead of a proper struct. The size of the parameter is embedded in the @@ -76,6 +109,7 @@ kvm_static_assert(SEV_RET_SUCCESS == 0);
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,4 +133,17 @@ static inline void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa, vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data); }
+static inline void 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, + }; + + vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data); +} + #endif /* SELFTEST_KVM_SEV_H */ diff --git a/tools/testing/selftests/kvm/lib/x86/sev.c b/tools/testing/selftests/kvm/lib/x86/sev.c index 280ec42e281b..17d493e9907a 100644 --- a/tools/testing/selftests/kvm/lib/x86/sev.c +++ b/tools/testing/selftests/kvm/lib/x86/sev.c @@ -31,7 +31,8 @@ bool is_sev_vm(struct kvm_vm *vm) * 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 void 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; @@ -41,13 +42,35 @@ static void encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *regio if (!sparsebit_any_set(protected_phy_pages)) return;
- sev_register_encrypted_memory(vm, region); + if (!is_sev_snp_vm(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;
- sev_launch_update_data(vm, gpa_base + offset, size); + if (is_sev_snp_vm(vm)) { + snp_launch_update_data(vm, gpa_base + offset, + (uint64_t)addr_gpa2hva(vm, gpa_base + offset), + size, page_type); + } else { + sev_launch_update_data(vm, gpa_base + offset, size); + } + } +} + +static void privatize_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; + + 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; + + vm_mem_set_private(vm, gpa_base + offset, size); } }
@@ -77,6 +100,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); +} + void sev_vm_launch(struct kvm_vm *vm, uint32_t policy) { struct kvm_sev_launch_start launch_start = { @@ -93,7 +124,7 @@ void sev_vm_launch(struct kvm_vm *vm, uint32_t 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); + encrypt_region(vm, region, 0);
if (policy & SEV_POLICY_ES) vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL); @@ -129,6 +160,35 @@ 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) +{ + struct kvm_sev_snp_launch_start launch_start = { + .policy = policy, + }; + + vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_START, &launch_start); +} + +void snp_vm_launch_update(struct kvm_vm *vm) +{ + struct userspace_mem_region *region; + int ctr; + + hash_for_each(vm->regions.slot_hash, ctr, region, slot_node) { + privatize_region(vm, region); + encrypt_region(vm, region, KVM_SEV_SNP_PAGE_TYPE_NORMAL); + } + + vm->arch.is_pt_protected = true; +} + +void snp_vm_launch_finish(struct kvm_vm *vm) +{ + struct kvm_sev_snp_launch_finish launch_finish = { 0 }; + + vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_FINISH, &launch_finish); +} + struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t type, void *guest_code, struct kvm_vcpu **cpu) { @@ -145,8 +205,20 @@ 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 (is_sev_snp_vm(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; + } + sev_vm_launch(vm, policy);
if (!measurement)
On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
Extend the SEV library to include support for SNP ioctl() wrappers, which aid in launching and interacting with a SEV-SNP guest.
Tested-by: Srikanth Aithal sraithal@amd.com Signed-off-by: Pratik R. Sampat prsampat@amd.com
v5..v6:
- Collected tags from Srikanth.
tools/testing/selftests/kvm/include/x86/sev.h | 49 ++++++++++- tools/testing/selftests/kvm/lib/x86/sev.c | 82 +++++++++++++++++-- 2 files changed, 125 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/kvm/include/x86/sev.h b/tools/testing/selftests/kvm/include/x86/sev.h index faed91435963..fd5d5261e10e 100644 --- a/tools/testing/selftests/kvm/include/x86/sev.h +++ b/tools/testing/selftests/kvm/include/x86/sev.h @@ -22,9 +22,20 @@ enum sev_guest_state { SEV_GUEST_STATE_RUNNING, }; +/* Minimum firmware version required for the SEV-SNP support */ +#define SNP_MIN_API_MAJOR 1 +#define SNP_MIN_API_MINOR 51
Dead code. Selftests don't care about this.
#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_FW_VER_MINOR(min) ((uint8_t)(min) << 0) +#define SNP_FW_VER_MAJOR(maj) ((uint8_t)(maj) << 8)
Also dead code.
#define GHCB_MSR_TERM_REQ 0x100 #define VMGEXIT() { __asm__ __volatile__("rep; vmmcall"); } @@ -36,13 +47,35 @@ bool is_sev_snp_vm(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); +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); 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); +/*
- A SEV-SNP VM requires the policy reserved bit to always be set.
- The SMT policy bit is also required to be set based on SMT being
- available and active on the system.
- */
+static inline u64 snp_default_policy(void) +{
- bool smt_active = false;
- FILE *f;
- f = fopen("/sys/devices/system/cpu/smt/active", "r");
Please add a helper to query if SMT is enabled. I doubt there will ever be many users of this, but it doesn't seem like something that should buried in SNP code.
Ha! smt_possible() in tools/testing/selftests/kvm/x86/hyperv_cpuid.c is already guilty of burying a related helper, and it looks like it's a more robust version.
- if (f) {
smt_active = fgetc(f) - '0';fclose(f);- }
- return SNP_POLICY_RSVD_MBO | (smt_active ? SNP_POLICY_SMT : 0);
+}
/*
- The KVM_MEMORY_ENCRYPT_OP uAPI is utter garbage and takes an "unsigned long"
- instead of a proper struct. The size of the parameter is embedded in the
@@ -76,6 +109,7 @@ kvm_static_assert(SEV_RET_SUCCESS == 0); 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,4 +133,17 @@ static inline void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa, vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data); } +static inline void 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,- };
- vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data);
+}
#endif /* SELFTEST_KVM_SEV_H */ diff --git a/tools/testing/selftests/kvm/lib/x86/sev.c b/tools/testing/selftests/kvm/lib/x86/sev.c index 280ec42e281b..17d493e9907a 100644 --- a/tools/testing/selftests/kvm/lib/x86/sev.c +++ b/tools/testing/selftests/kvm/lib/x86/sev.c @@ -31,7 +31,8 @@ bool is_sev_vm(struct kvm_vm *vm)
- 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 void 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; @@ -41,13 +42,35 @@ static void encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *regio if (!sparsebit_any_set(protected_phy_pages)) return;
- sev_register_encrypted_memory(vm, region);
- if (!is_sev_snp_vm(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;
sev_launch_update_data(vm, gpa_base + offset, size);
if (is_sev_snp_vm(vm)) {
Curly braces are unnecessary.
snp_launch_update_data(vm, gpa_base + offset,(uint64_t)addr_gpa2hva(vm, gpa_base + offset),size, page_type);} else {sev_launch_update_data(vm, gpa_base + offset, size);}- }
+}
+static void privatize_region(struct kvm_vm *vm, struct userspace_mem_region *region)
Can't this just be a param to encrypt_region() that also says "make it private"?
+{
- 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;
- 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;vm_mem_set_private(vm, gpa_base + offset, size);} @@ -77,6 +100,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);
Use TEST_ASSERT(), or do nothing, don't use assert().
- vm_sev_ioctl(vm, KVM_SEV_INIT2, &init);
+}
void sev_vm_launch(struct kvm_vm *vm, uint32_t policy) { struct kvm_sev_launch_start launch_start = { @@ -93,7 +124,7 @@ void sev_vm_launch(struct kvm_vm *vm, uint32_t 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);
encrypt_region(vm, region, 0);
Please add an enum/macro instead of open coding a literal '0'. I gotta assume there's an appropriate name for page type '0'.
if (policy & SEV_POLICY_ES) vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL);
On 2/11/25 8:12 PM, Sean Christopherson wrote:
On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
Extend the SEV library to include support for SNP ioctl() wrappers, which aid in launching and interacting with a SEV-SNP guest.
Tested-by: Srikanth Aithal sraithal@amd.com Signed-off-by: Pratik R. Sampat prsampat@amd.com
[..snip..]
+/*
- A SEV-SNP VM requires the policy reserved bit to always be set.
- The SMT policy bit is also required to be set based on SMT being
- available and active on the system.
- */
+static inline u64 snp_default_policy(void) +{
- bool smt_active = false;
- FILE *f;
- f = fopen("/sys/devices/system/cpu/smt/active", "r");
Please add a helper to query if SMT is enabled. I doubt there will ever be many users of this, but it doesn't seem like something that should buried in SNP code.
Ha! smt_possible() in tools/testing/selftests/kvm/x86/hyperv_cpuid.c is already guilty of burying a related helper, and it looks like it's a more robust version.
You're right, a more general helper is in order here.
Since the hyperv_cpuid selftest only seems to care about whether SMT is possible (i.e., it may or may not be enabled) and we care about it being enabled as well, for the flag to be set. I should make a more generic variant(s) that can be accessible to both. Maybe I can implement it within testing/selftests/kvm/include/x86_processor.h?
- if (f) {
smt_active = fgetc(f) - '0';fclose(f);- }
- return SNP_POLICY_RSVD_MBO | (smt_active ? SNP_POLICY_SMT : 0);
+}
/*
- The KVM_MEMORY_ENCRYPT_OP uAPI is utter garbage and takes an "unsigned long"
- instead of a proper struct. The size of the parameter is embedded in the
@@ -76,6 +109,7 @@ kvm_static_assert(SEV_RET_SUCCESS == 0); 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,4 +133,17 @@ static inline void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa, vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data); } +static inline void 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,- };
- vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data);
+}
#endif /* SELFTEST_KVM_SEV_H */ diff --git a/tools/testing/selftests/kvm/lib/x86/sev.c b/tools/testing/selftests/kvm/lib/x86/sev.c index 280ec42e281b..17d493e9907a 100644 --- a/tools/testing/selftests/kvm/lib/x86/sev.c +++ b/tools/testing/selftests/kvm/lib/x86/sev.c @@ -31,7 +31,8 @@ bool is_sev_vm(struct kvm_vm *vm)
- 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 void 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; @@ -41,13 +42,35 @@ static void encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *regio if (!sparsebit_any_set(protected_phy_pages)) return;
- sev_register_encrypted_memory(vm, region);
- if (!is_sev_snp_vm(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;
sev_launch_update_data(vm, gpa_base + offset, size);
if (is_sev_snp_vm(vm)) {Curly braces are unnecessary.
Ack. Will be remove them.
snp_launch_update_data(vm, gpa_base + offset,(uint64_t)addr_gpa2hva(vm, gpa_base + offset),size, page_type);} else {sev_launch_update_data(vm, gpa_base + offset, size);}- }
+}
+static void privatize_region(struct kvm_vm *vm, struct userspace_mem_region *region)
Can't this just be a param to encrypt_region() that also says "make it private"?
Sure, I can parameterize encrypt_region() to have a private boolean. The idea was essentially to ensure that privatizing and encryption can be called separately if they wanted to. However, since this is currently only used in tandem (by SNP) I can remove this to reduce duplication.
+{
- 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;
- 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;vm_mem_set_private(vm, gpa_base + offset, size);} @@ -77,6 +100,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);
Use TEST_ASSERT(), or do nothing, don't use assert().
Alright, I can do that. I will also send another cleanups patch to remove asserts() in the SEV, SEV-ES counterparts too in that case!
- vm_sev_ioctl(vm, KVM_SEV_INIT2, &init);
+}
void sev_vm_launch(struct kvm_vm *vm, uint32_t policy) { struct kvm_sev_launch_start launch_start = { @@ -93,7 +124,7 @@ void sev_vm_launch(struct kvm_vm *vm, uint32_t 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);
encrypt_region(vm, region, 0);Please add an enum/macro instead of open coding a literal '0'. I gotta assume there's an appropriate name for page type '0'.
For SNP, we supply this parameter to determine the page type for SNP launch update defined as KVM_SEV_SNP_PAGE_TYPE_*. For SEV/SEV-ES, however, the page type doesn't really get factored in and falls through unaccounted in that case, so I had passed a zero to it.
Having said that, having a literal here is quite unclean. Maybe I can pass one of the existing page types to it or, better yet, define a new KVM_SEV_PAGE_TYPE_[RESERVED|UNUSED] type instead for our selftest header?
if (policy & SEV_POLICY_ES) vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL);
On Fri, Feb 14, 2025, Pratik Rajesh Sampat wrote:
On 2/11/25 8:12 PM, Sean Christopherson wrote:
On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
Extend the SEV library to include support for SNP ioctl() wrappers, which aid in launching and interacting with a SEV-SNP guest.
Tested-by: Srikanth Aithal sraithal@amd.com Signed-off-by: Pratik R. Sampat prsampat@amd.com
[..snip..]
+/*
- A SEV-SNP VM requires the policy reserved bit to always be set.
- The SMT policy bit is also required to be set based on SMT being
- available and active on the system.
- */
+static inline u64 snp_default_policy(void) +{
- bool smt_active = false;
- FILE *f;
- f = fopen("/sys/devices/system/cpu/smt/active", "r");
Please add a helper to query if SMT is enabled. I doubt there will ever be many users of this, but it doesn't seem like something that should buried in SNP code.
Ha! smt_possible() in tools/testing/selftests/kvm/x86/hyperv_cpuid.c is already guilty of burying a related helper, and it looks like it's a more robust version.
You're right, a more general helper is in order here.
Since the hyperv_cpuid selftest only seems to care about whether SMT is possible (i.e., it may or may not be enabled) and we care about it being enabled as well, for the flag to be set. I should make a more generic variant(s) that can be accessible to both. Maybe I can implement it within testing/selftests/kvm/include/x86_processor.h?
It should go in kvm_util.h, /sys/devices/system/cpu/smt/active is a generic interface provided by kernel/cpu.c.
@@ -93,7 +124,7 @@ void sev_vm_launch(struct kvm_vm *vm, uint32_t 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);
encrypt_region(vm, region, 0);Please add an enum/macro instead of open coding a literal '0'. I gotta assume there's an appropriate name for page type '0'.
For SNP, we supply this parameter to determine the page type for SNP launch update defined as KVM_SEV_SNP_PAGE_TYPE_*. For SEV/SEV-ES, however, the page type doesn't really get factored in and falls through unaccounted in that case, so I had passed a zero to it.
Having said that, having a literal here is quite unclean. Maybe I can pass one of the existing page types to it or, better yet, define a new KVM_SEV_PAGE_TYPE_[RESERVED|UNUSED] type instead for our selftest header?
Ya, define something new and arbitrary. I vote for either KVM_SEV_PAGE_TYPE_NONE or KVM_SEV_PAGE_TYPE_INVALID. RESERVED suggests the page is "valid" but reserved for some entity. Ditto for UNUSED; valid, but not yet claimed.
On 2/19/25 10:55 AM, Sean Christopherson wrote:
On Fri, Feb 14, 2025, Pratik Rajesh Sampat wrote:
On 2/11/25 8:12 PM, Sean Christopherson wrote:
On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
Extend the SEV library to include support for SNP ioctl() wrappers, which aid in launching and interacting with a SEV-SNP guest.
Tested-by: Srikanth Aithal sraithal@amd.com Signed-off-by: Pratik R. Sampat prsampat@amd.com
[..snip..]
+/*
- A SEV-SNP VM requires the policy reserved bit to always be set.
- The SMT policy bit is also required to be set based on SMT being
- available and active on the system.
- */
+static inline u64 snp_default_policy(void) +{
- bool smt_active = false;
- FILE *f;
- f = fopen("/sys/devices/system/cpu/smt/active", "r");
Please add a helper to query if SMT is enabled. I doubt there will ever be many users of this, but it doesn't seem like something that should buried in SNP code.
Ha! smt_possible() in tools/testing/selftests/kvm/x86/hyperv_cpuid.c is already guilty of burying a related helper, and it looks like it's a more robust version.
You're right, a more general helper is in order here.
Since the hyperv_cpuid selftest only seems to care about whether SMT is possible (i.e., it may or may not be enabled) and we care about it being enabled as well, for the flag to be set. I should make a more generic variant(s) that can be accessible to both. Maybe I can implement it within testing/selftests/kvm/include/x86_processor.h?
It should go in kvm_util.h, /sys/devices/system/cpu/smt/active is a generic interface provided by kernel/cpu.c.
Sure, I will move it to the kvm_util header.
@@ -93,7 +124,7 @@ void sev_vm_launch(struct kvm_vm *vm, uint32_t 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);
encrypt_region(vm, region, 0);Please add an enum/macro instead of open coding a literal '0'. I gotta assume there's an appropriate name for page type '0'.
For SNP, we supply this parameter to determine the page type for SNP launch update defined as KVM_SEV_SNP_PAGE_TYPE_*. For SEV/SEV-ES, however, the page type doesn't really get factored in and falls through unaccounted in that case, so I had passed a zero to it.
Having said that, having a literal here is quite unclean. Maybe I can pass one of the existing page types to it or, better yet, define a new KVM_SEV_PAGE_TYPE_[RESERVED|UNUSED] type instead for our selftest header?
Ya, define something new and arbitrary. I vote for either KVM_SEV_PAGE_TYPE_NONE or KVM_SEV_PAGE_TYPE_INVALID. RESERVED suggests the page is "valid" but reserved for some entity. Ditto for UNUSED; valid, but not yet claimed.
Got it. Thank you!
Force the SEV-SNP VM type to set the KVM_MEM_GUEST_MEMFD flag for the creation of private memslots.
Tested-by: Srikanth Aithal sraithal@amd.com Signed-off-by: Pratik R. Sampat prsampat@amd.com --- v5..v6:
* Collected tags from Srikanth. --- tools/testing/selftests/kvm/lib/kvm_util.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index 33fefeb3ca44..089488e2eaf6 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -413,14 +413,17 @@ 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;
In preparation for SNP, cleanup the smoke test to decouple deriving type from policy. Introduce, wrappers for SEV and SEV-ES types to abstract the parametrized launch tests calls and reduce verbosity.
No functional change intended.
Tested-by: Srikanth Aithal sraithal@amd.com Signed-off-by: Pratik R. Sampat prsampat@amd.com --- v5..v6:
* Collected tags from Srikanth. --- .../selftests/kvm/x86/sev_smoke_test.c | 50 ++++++++++++------- 1 file changed, 33 insertions(+), 17 deletions(-)
diff --git a/tools/testing/selftests/kvm/x86/sev_smoke_test.c b/tools/testing/selftests/kvm/x86/sev_smoke_test.c index b18c78314d5b..3a36cd3ca151 100644 --- a/tools/testing/selftests/kvm/x86/sev_smoke_test.c +++ b/tools/testing/selftests/kvm/x86/sev_smoke_test.c @@ -61,7 +61,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; @@ -71,7 +71,7 @@ static void test_sync_vmsa(uint32_t policy) double x87val = M_PI; struct kvm_xsave __attribute__((aligned(64))) xsave = { 0 };
- vm = vm_sev_create_with_one_vcpu(KVM_X86_SEV_ES_VM, guest_code_xsave, &vcpu); + 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); @@ -88,7 +88,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); @@ -107,14 +107,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. */ @@ -149,6 +147,21 @@ static void test_sev(void *guest_code, uint64_t policy) kvm_vm_free(vm); }
+static void test_sev(uint64_t policy) +{ + __test_sev(guest_sev_code, KVM_X86_SEV_VM, policy); +} + +static void test_sev_es(uint64_t policy) +{ + __test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, policy); +} + +static void test_sync_vmsa_sev_es(uint64_t policy) +{ + __test_sync_vmsa(KVM_X86_SEV_ES_VM, policy); +} + static void guest_shutdown_code(void) { struct desc_ptr idt; @@ -160,16 +173,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, @@ -179,25 +190,30 @@ static void test_sev_es_shutdown(void) kvm_vm_free(vm); }
+static void test_sev_es_shutdown(uint64_t policy) +{ + __test_sev_shutdown(KVM_X86_SEV_ES_VM, SEV_POLICY_ES); +} + int main(int argc, char *argv[]) { const u64 xf_mask = XFEATURE_MASK_X87_AVX;
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(SEV_POLICY_NO_DBG); + test_sev(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_es(SEV_POLICY_ES | SEV_POLICY_NO_DBG); + test_sev_es(SEV_POLICY_ES);
- test_sev_es_shutdown(); + test_sev_es_shutdown(SEV_POLICY_ES);
if (kvm_has_cap(KVM_CAP_XCRS) && (xgetbv(0) & kvm_cpu_supported_xcr0() & xf_mask) == xf_mask) { - test_sync_vmsa(0); - test_sync_vmsa(SEV_POLICY_NO_DBG); + test_sync_vmsa_sev_es(SEV_POLICY_ES); + test_sync_vmsa_sev_es(SEV_POLICY_NO_DBG | SEV_POLICY_ES); } }
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.
Tested-by: Srikanth Aithal sraithal@amd.com Signed-off-by: Pratik R. Sampat prsampat@amd.com --- v5..v6:
* Rename KVM CPU feature X86_FEATURE_SNP to X86_FEATURE_SEV_SNP (Nikunj) * Collected tags from Srikanth. --- .../selftests/kvm/x86/sev_smoke_test.c | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+)
diff --git a/tools/testing/selftests/kvm/x86/sev_smoke_test.c b/tools/testing/selftests/kvm/x86/sev_smoke_test.c index 3a36cd3ca151..3336550152c0 100644 --- a/tools/testing/selftests/kvm/x86/sev_smoke_test.c +++ b/tools/testing/selftests/kvm/x86/sev_smoke_test.c @@ -16,6 +16,18 @@
#define XFEATURE_MASK_X87_AVX (XFEATURE_MASK_FP | XFEATURE_MASK_SSE | XFEATURE_MASK_YMM)
+static void guest_snp_code(void) +{ + uint64_t sev_msr = rdmsr(MSR_AMD64_SEV); + + GUEST_ASSERT(sev_msr & MSR_AMD64_SEV_ENABLED); + GUEST_ASSERT(sev_msr & MSR_AMD64_SEV_ES_ENABLED); + GUEST_ASSERT(sev_msr & MSR_AMD64_SEV_SNP_ENABLED); + + wrmsr(MSR_AMD64_SEV_ES_GHCB, GHCB_MSR_TERM_REQ); + VMGEXIT(); +} + static void guest_sev_es_code(void) { /* TODO: Check CPUID after GHCB-based hypercall support is added. */ @@ -157,11 +169,21 @@ static void test_sev_es(uint64_t policy) __test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, policy); }
+static void test_snp(uint64_t policy) +{ + __test_sev(guest_snp_code, KVM_X86_SNP_VM, policy); +} + static void test_sync_vmsa_sev_es(uint64_t policy) { __test_sync_vmsa(KVM_X86_SEV_ES_VM, policy); }
+static void test_sync_vmsa_snp(uint64_t policy) +{ + __test_sync_vmsa(KVM_X86_SNP_VM, policy); +} + static void guest_shutdown_code(void) { struct desc_ptr idt; @@ -195,6 +217,11 @@ static void test_sev_es_shutdown(uint64_t policy) __test_sev_shutdown(KVM_X86_SEV_ES_VM, SEV_POLICY_ES); }
+static void test_snp_shutdown(uint64_t policy) +{ + __test_sev_shutdown(KVM_X86_SNP_VM, policy); +} + int main(int argc, char *argv[]) { const u64 xf_mask = XFEATURE_MASK_X87_AVX; @@ -217,5 +244,20 @@ int main(int argc, char *argv[]) } }
+ if (kvm_cpu_has(X86_FEATURE_SEV_SNP)) { + uint64_t snp_policy = snp_default_policy(); + + test_snp(snp_policy); + /* Test minimum firmware level */ + test_snp(snp_policy | SNP_FW_VER_MAJOR(SNP_MIN_API_MAJOR) | + SNP_FW_VER_MINOR(SNP_MIN_API_MINOR)); + + test_snp_shutdown(snp_policy); + + if (kvm_has_cap(KVM_CAP_XCRS) && + (xgetbv(0) & kvm_cpu_supported_xcr0() & xf_mask) == xf_mask) + test_sync_vmsa_snp(snp_policy); + } + return 0; }
On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
@@ -217,5 +244,20 @@ int main(int argc, char *argv[]) } }
- if (kvm_cpu_has(X86_FEATURE_SEV_SNP)) {
uint64_t snp_policy = snp_default_policy();test_snp(snp_policy);/* Test minimum firmware level */test_snp(snp_policy | SNP_FW_VER_MAJOR(SNP_MIN_API_MAJOR) |SNP_FW_VER_MINOR(SNP_MIN_API_MINOR));
Ah, this is where the firmware policy stuff is used. Refresh me, can userspace request _any_ major/minor as the min, and expect failure if the version isn't supported? If so, the test should iterate over the major/minor combinations that are guaranteed to fail. And if userspace can query the supported minor/major, the test should iterate over all the happy versions too.
Unless there's nothing interesting to test, I would move the major/minor stuff to a separate patch.
test_snp_shutdown(snp_policy);if (kvm_has_cap(KVM_CAP_XCRS) &&(xgetbv(0) & kvm_cpu_supported_xcr0() & xf_mask) == xf_mask)test_sync_vmsa_snp(snp_policy);
This is all copy+paste from SEV-ES tests, minus SEV_POLICY_NO_DBG. There's gotta be a way to dedup this code.
Something like this?
static void needs_a_better_name(uint32_t type, uint64_t policy) { const u64 xf_mask = XFEATURE_MASK_X87_AVX;
test_sev(guest_sev_code, policy | SEV_POLICY_NO_DBG); test_sev(guest_sev_code, policy);
if (type == KVM_X86_SEV_VM) return;
test_sev_shutdown(policy);
if (kvm_has_cap(KVM_CAP_XCRS) && (xgetbv(0) & kvm_cpu_supported_xcr0() & xf_mask) == xf_mask) { test_sync_vmsa(policy); test_sync_vmsa(policy | SEV_POLICY_NO_DBG); } }
int main(int argc, char *argv[]) { TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SEV));
needs_a_better_name(KVM_X86_SEV_VM, 0);
if (kvm_cpu_has(X86_FEATURE_SEV_ES)) needs_a_better_name(KVM_X86_SEV_ES_VM, 0);
if (kvm_cpu_has(X86_FEATURE_SEV_SNP)) needs_a_better_name(KVM_X86_SEV_SNP_VM, 0);
return 0; }
On 2/11/25 8:31 PM, Sean Christopherson wrote:
On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
@@ -217,5 +244,20 @@ int main(int argc, char *argv[]) } }
- if (kvm_cpu_has(X86_FEATURE_SEV_SNP)) {
uint64_t snp_policy = snp_default_policy();test_snp(snp_policy);/* Test minimum firmware level */test_snp(snp_policy | SNP_FW_VER_MAJOR(SNP_MIN_API_MAJOR) |SNP_FW_VER_MINOR(SNP_MIN_API_MINOR));Ah, this is where the firmware policy stuff is used. Refresh me, can userspace request _any_ major/minor as the min, and expect failure if the version isn't supported? If so, the test should iterate over the major/minor combinations that are guaranteed to fail. And if userspace can query the supported minor/major, the test should iterate over all the happy versions too.
Yes, any policy greater than the min policy (defined in sev-dev.c) should be supported. The sad path tests were intended to be added in the upcoming negative test patch series so that we could have the proper infrastructure to handle and report failures.
Unless there's nothing interesting to test, I would move the major/minor stuff to a separate patch.
Would you rather prefer I do the happy tests here (something like - min_policy and min_policy + 1?) and defer the failure tests for the next patchset? Or, I can remove policy testing from here entirely and introduce it only when the sad path testing infrastructure is ready, so that we can test this completely at once?
test_snp_shutdown(snp_policy);if (kvm_has_cap(KVM_CAP_XCRS) &&(xgetbv(0) & kvm_cpu_supported_xcr0() & xf_mask) == xf_mask)test_sync_vmsa_snp(snp_policy);This is all copy+paste from SEV-ES tests, minus SEV_POLICY_NO_DBG. There's gotta be a way to dedup this code.
Something like this?
static void needs_a_better_name(uint32_t type, uint64_t policy) { const u64 xf_mask = XFEATURE_MASK_X87_AVX;
test_sev(guest_sev_code, policy | SEV_POLICY_NO_DBG); test_sev(guest_sev_code, policy);
if (type == KVM_X86_SEV_VM) return;
test_sev_shutdown(policy);
if (kvm_has_cap(KVM_CAP_XCRS) && (xgetbv(0) & kvm_cpu_supported_xcr0() & xf_mask) == xf_mask) { test_sync_vmsa(policy); test_sync_vmsa(policy | SEV_POLICY_NO_DBG); } }
int main(int argc, char *argv[]) { TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SEV));
needs_a_better_name(KVM_X86_SEV_VM, 0);
if (kvm_cpu_has(X86_FEATURE_SEV_ES)) needs_a_better_name(KVM_X86_SEV_ES_VM, 0);
if (kvm_cpu_has(X86_FEATURE_SEV_SNP)) needs_a_better_name(KVM_X86_SEV_SNP_VM, 0);
return 0; }
Sure, I can definitely clean this up so that we have less duplication of code all around for this test.
Thanks again! Pratik
On Fri, Feb 14, 2025, Pratik Rajesh Sampat wrote:
On 2/11/25 8:31 PM, Sean Christopherson wrote:
On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
@@ -217,5 +244,20 @@ int main(int argc, char *argv[]) } }
- if (kvm_cpu_has(X86_FEATURE_SEV_SNP)) {
uint64_t snp_policy = snp_default_policy();test_snp(snp_policy);/* Test minimum firmware level */test_snp(snp_policy | SNP_FW_VER_MAJOR(SNP_MIN_API_MAJOR) |SNP_FW_VER_MINOR(SNP_MIN_API_MINOR));Ah, this is where the firmware policy stuff is used. Refresh me, can userspace request _any_ major/minor as the min, and expect failure if the version isn't supported? If so, the test should iterate over the major/minor combinations that are guaranteed to fail. And if userspace can query the supported minor/major, the test should iterate over all the happy versions too.
Yes, any policy greater than the min policy (defined in sev-dev.c) should be supported. The sad path tests were intended to be added in the upcoming negative test patch series so that we could have the proper infrastructure to handle and report failures.
Unless there's nothing interesting to test, I would move the major/minor stuff to a separate patch.
Would you rather prefer I do the happy tests here (something like - min_policy and min_policy + 1?) and defer the failure tests for the next patchset? Or, I can remove policy testing from here entirely and introduce it only when the sad path testing infrastructure is ready, so that we can test this completely at once?
Let's do the latter. For the initial series, do the bare minimum so that we can get that merged, and then focus on the min API version stuff in a separate series. The version testing shouldn't be terribly complex, but it doesn't seem like it's entirely trivial either, and I don't want it to block the base SNP support.
On 2/18/25 6:54 PM, Sean Christopherson wrote:
On Fri, Feb 14, 2025, Pratik Rajesh Sampat wrote:
On 2/11/25 8:31 PM, Sean Christopherson wrote:
On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
@@ -217,5 +244,20 @@ int main(int argc, char *argv[]) } }
- if (kvm_cpu_has(X86_FEATURE_SEV_SNP)) {
uint64_t snp_policy = snp_default_policy();test_snp(snp_policy);/* Test minimum firmware level */test_snp(snp_policy | SNP_FW_VER_MAJOR(SNP_MIN_API_MAJOR) |SNP_FW_VER_MINOR(SNP_MIN_API_MINOR));Ah, this is where the firmware policy stuff is used. Refresh me, can userspace request _any_ major/minor as the min, and expect failure if the version isn't supported? If so, the test should iterate over the major/minor combinations that are guaranteed to fail. And if userspace can query the supported minor/major, the test should iterate over all the happy versions too.
Yes, any policy greater than the min policy (defined in sev-dev.c) should be supported. The sad path tests were intended to be added in the upcoming negative test patch series so that we could have the proper infrastructure to handle and report failures.
Unless there's nothing interesting to test, I would move the major/minor stuff to a separate patch.
Would you rather prefer I do the happy tests here (something like - min_policy and min_policy + 1?) and defer the failure tests for the next patchset? Or, I can remove policy testing from here entirely and introduce it only when the sad path testing infrastructure is ready, so that we can test this completely at once?
Let's do the latter. For the initial series, do the bare minimum so that we can get that merged, and then focus on the min API version stuff in a separate series. The version testing shouldn't be terribly complex, but it doesn't seem like it's entirely trivial either, and I don't want it to block the base SNP support.
Sure thing, will do. Thanks!
linux-kselftest-mirror@lists.linaro.org
-
Pratik R. Sampat -
Pratik Rajesh Sampat
-
Sean Christopherson