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);