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@xxxxxxx>
> Signed-off-by: Pratik R. Sampat <prsampat@xxxxxxx>
> ---
> 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);
