Re: [PATCH v6 6/9] KVM: selftests: Add library support for interacting with SNP

[Sean Christopherson <seanjc@google.com>]

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