Re: [PATCH Part1 RFC v2 09/20] x86/sev: check SEV-SNP features support

From: Borislav Petkov <bp@alien8.de>
To: Brijesh Singh <brijesh.singh@amd.com>
Cc: x86@kernel.org, linux-kernel@vger.kernel.org,
	kvm@vger.kernel.org, tglx@linutronix.de, jroedel@suse.de,
	thomas.lendacky@amd.com, pbonzini@redhat.com, mingo@redhat.com,
	dave.hansen@intel.com, rientjes@google.com, seanjc@google.com,
	peterz@infradead.org, hpa@zytor.com, tony.luck@intel.com
Subject: Re: [PATCH Part1 RFC v2 09/20] x86/sev: check SEV-SNP features support
Date: Thu, 20 May 2021 18:02:20 +0200	[thread overview]
Message-ID: <YKaIDAHOz4+soLxi@zn.tnic> (raw)
In-Reply-To: <20210430121616.2295-10-brijesh.singh@amd.com>

On Fri, Apr 30, 2021 at 07:16:05AM -0500, Brijesh Singh wrote:
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index 6d9055427f37..7badbeb6cb95 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -25,6 +25,8 @@
>  
>  struct ghcb boot_ghcb_page __aligned(PAGE_SIZE);
>  struct ghcb *boot_ghcb;
> +static u64 sev_status_val;

msr_sev_status should be more descriptive.

> +static bool sev_status_checked;

You don't need this one - you can simply do

	if (!msr_sev_status)
		read the MSR.

>  /*
>   * Copy a version of this function here - insn-eval.c can't be used in
> @@ -119,11 +121,30 @@ static enum es_result vc_read_mem(struct es_em_ctxt *ctxt,
>  /* Include code for early handlers */
>  #include "../../kernel/sev-shared.c"
>  
> +static inline bool sev_snp_enabled(void)
> +{
> +	unsigned long low, high;
> +
> +	if (!sev_status_checked) {
> +		asm volatile("rdmsr\n"
> +			     : "=a" (low), "=d" (high)
> +			     : "c" (MSR_AMD64_SEV));
> +		sev_status_val = (high << 32) | low;
> +		sev_status_checked = true;
> +	}
> +
> +	return sev_status_val & MSR_AMD64_SEV_SNP_ENABLED ? true : false;

	return msr_sev_status & MSR_AMD64_SEV_SNP_ENABLED;

is enough.

> +}
> +
>  static bool early_setup_sev_es(void)
>  {
>  	if (!sev_es_negotiate_protocol())
>  		sev_es_terminate(0, GHCB_SEV_ES_REASON_PROTOCOL_UNSUPPORTED);
>  
> +	/* If SEV-SNP is enabled then check if the hypervisor supports the SEV-SNP features. */

80 cols like the rest of this file pls.

> +	if (sev_snp_enabled() && !sev_snp_check_hypervisor_features())
> +		sev_es_terminate(0, GHCB_SEV_ES_REASON_SNP_UNSUPPORTED);
> +
>  	if (set_page_decrypted((unsigned long)&boot_ghcb_page))
>  		return false;

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette