Received-SPF: pass (google.com: domain of linux-kernel+bounces-155993-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 23 Apr 2024 16:12:56 -0700
In-Reply-To: <20240409133959.2888018-6-pgonda@google.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240409133959.2888018-1-pgonda@google.com> <20240409133959.2888018-6-pgonda@google.com>
Message-ID: <ZihAeH4PXSUtT4hM@google.com>
Subject: Re: [PATCH 5/6] Add is_sev_enabled() helpers
From: Sean Christopherson <seanjc@google.com>
To: Peter Gonda <pgonda@google.com>
Cc: linux-kernel@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>, 
	Claudio Imbrenda <imbrenda@linux.ibm.com>, Carlos Bilbao <carlos.bilbao@amd.com>, 
	Tom Lendacky <thomas.lendacky@amd.com>, Michael Roth <michael.roth@amd.com>
Content-Type: text/plain; charset="us-ascii"

On Tue, Apr 09, 2024, Peter Gonda wrote:
> Add helper functions for guest code to check the status of SEV and
> SEV-ES.

Why?  The names are super ambiguous, e.g. they could just as easily mean "is SEV
enabled in KVM" or "is SEV enabled in CPUID".  And if an assert fires because
is_sev_es_enabled() returns false, the user will get a _worse_ error message because
all they'll know is _something_ in is_sev_es_enabled() failed, not which MSR bit
came back 'bad.

> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Carlos Bilbao <carlos.bilbao@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Michael Roth <michael.roth@amd.com>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> ---
>  tools/testing/selftests/kvm/include/x86_64/sev.h    |  3 +++
>  tools/testing/selftests/kvm/lib/x86_64/sev.c        | 11 +++++++++++
>  tools/testing/selftests/kvm/x86_64/sev_smoke_test.c |  5 ++---
>  3 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/x86_64/sev.h b/tools/testing/selftests/kvm/include/x86_64/sev.h
> index bfd481707f67..691dc005e2a1 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/sev.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/sev.h
> @@ -106,4 +106,7 @@ 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);
>  }
>  
> +bool is_sev_enabled(void);
> +bool is_sev_es_enabled(void);
> +
>  #endif /* SELFTEST_KVM_SEV_H */
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c b/tools/testing/selftests/kvm/lib/x86_64/sev.c
> index 27ae1d3b1355..5b3f0a8a931a 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/sev.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c
> @@ -189,3 +189,14 @@ struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t policy, void *guest_code,
>  
>  	return vm;
>  }
> +
> +bool is_sev_enabled(void)
> +{
> +	return rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ENABLED;
> +}
> +
> +bool is_sev_es_enabled(void)
> +{
> +	return is_sev_enabled() &&
> +	       rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ES_ENABLED;
> +}
> diff --git a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
> index 026779f3ed06..1d84e78e7ae2 100644
> --- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
> @@ -16,8 +16,7 @@
>  static void guest_sev_es_code(void)
>  {
>  	/* TODO: Check CPUID after GHCB-based hypercall support is added. */
> -	GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ENABLED);
> -	GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ES_ENABLED);
> +	GUEST_ASSERT(is_sev_es_enabled());
>  
>  	/*
>  	 * TODO: Add GHCB and ucall support for SEV-ES guests.  For now, simply
> @@ -30,7 +29,7 @@ static void guest_sev_es_code(void)
>  static void guest_sev_code(void)
>  {
>  	GUEST_ASSERT(this_cpu_has(X86_FEATURE_SEV));
> -	GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ENABLED);
> +	GUEST_ASSERT(is_sev_enabled());
>  
>  	GUEST_DONE();
>  }
> -- 
> 2.44.0.478.gd926399ef9-goog
>