linux-kernel - Re: [RFC 2/5] selftests: KVM: Decouple SEV ioctls from asserts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZrY4e39Q2_WxhrkI@google.com>
Date: Fri, 9 Aug 2024 08:40:43 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: "Pratik R. Sampat" <pratikrajesh.sampat@....com>
Cc: kvm@...r.kernel.org, shuah@...nel.org, thomas.lendacky@....com, 
	michael.roth@....com, pbonzini@...hat.com, pgonda@...gle.com, 
	linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC 2/5] selftests: KVM: Decouple SEV ioctls from asserts

On Wed, Jul 10, 2024, Pratik R. Sampat wrote:
> This commit separates the SEV, SEV-ES, SEV-SNP ioctl calls from its

Don't start with "This commit".  Please read Documentation/process/maintainer-kvm-x86.rst,
and by extension, Documentation/process/maintainer-tip.rst.

> positive test asserts. This is done so that negative tests can be
> introduced and both kinds of testing can be performed independently
> using the same base helpers of the ioctl.
> 
> This commit also adds additional parameters such as flags to improve
> testing coverage for the ioctls.
> 
> Cleanups performed with no functional change intended.
> 
> Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@....com>
> ---
>  .../selftests/kvm/include/x86_64/sev.h        |  20 +--
>  tools/testing/selftests/kvm/lib/x86_64/sev.c  | 145 ++++++++++++------
>  2 files changed, 108 insertions(+), 57 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/x86_64/sev.h b/tools/testing/selftests/kvm/include/x86_64/sev.h
> index 43b6c52831b2..ef99151e13a7 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/sev.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/sev.h
> @@ -37,14 +37,16 @@ enum sev_guest_state {
>  #define GHCB_MSR_TERM_REQ	0x100
>  
>  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);
> +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);
>  
>  bool is_kvm_snp_supported(void);
>  
> -void snp_vm_launch(struct kvm_vm *vm, uint32_t policy);
> -void snp_vm_launch_update(struct kvm_vm *vm);
> -void snp_vm_launch_finish(struct kvm_vm *vm);
> +int snp_vm_launch(struct kvm_vm *vm, uint32_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);
>  
>  struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t type, void *guest_code,
>  					   struct kvm_vcpu **cpu);
> @@ -98,7 +100,7 @@ static inline void sev_register_encrypted_memory(struct kvm_vm *vm,
>  	vm_ioctl(vm, KVM_MEMORY_ENCRYPT_REG_REGION, &range);
>  }
>  
> -static inline void snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
> +static inline int snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
>  					   uint64_t size, uint8_t type)
>  {
>  	struct kvm_sev_snp_launch_update update_data = {
> @@ -108,10 +110,10 @@ static inline void snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
>  		.type = type,
>  	};
>  
> -	vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data);
> +	return __vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data);

Don't introduce APIs and then immediately rewrite all of the users.  If you want
to rework similar APIs, do the rework, then add the new APIs.  Doing things in
this order adds a pile of pointless churn.

But that's a moot point, because it's far easier to just add __snp_launch_update_data().
And if you look through other APIs in kvm_util.h, you'll see that the strong
preference is to let vm_ioctl(), or in this case vm_sev_ioctl(), do the heavy
lifting.  Yeah, it requires copy+pasting marshalling parameters into the struct,
but that's relatively uninteresting code, _and_ piggybacking the "good" version
means you can't do things like pass in a garbage virtual address (because the
"good" version always guarantees a good virtual address).