Re: [PATCH v3 1/4] x86/sev: Avoid WARN()s in early boot code

[Tom Lendacky <thomas.lendacky@amd.com>]

On 6/5/24 05:16, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> Using WARN() before the kernel is even mapped is unlikely to do anything
> useful: the string literals are passed using their kernel virtual
> addresses which are not even mapped yet. But even if they were, calling
> into the printk machinery from the early 1:1 mapped code is not going to
> get very far.
> 
> So drop the WARN()s entirely.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/x86/kernel/sev.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index 3342ed58e168..33a669e85e5b 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -720,7 +720,7 @@ early_set_pages_state(unsigned long vaddr, unsigned long paddr,
>  		if (op == SNP_PAGE_STATE_SHARED) {
>  			/* Page validation must be rescinded before changing to shared */
>  			ret = pvalidate(vaddr, RMP_PG_SIZE_4K, false);
> -			if (WARN(ret, "Failed to validate address 0x%lx ret %d", paddr, ret))
> +			if (ret)
>  				goto e_term;

This area of the code around the pvalidate() calls has changed now. They
are now calls to pvalidate_4k_page() that will issue a WARN() through a
common function available to both the early code and the regular code.

If you want to rework this patch, you can use the added diff below to
remove the calls to the common function and just terminate directly. Or,
I can submit it as a separate patch.

Dropping the other WARN() calls around the GHCB MSR checks is good,
though.

Thanks,
Tom

diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index 71de53194089..ced15210ea50 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -1243,7 +1243,7 @@ static void svsm_pval_terminate(struct svsm_pvalidate_call *pc, int ret, u64 svs
 	__pval_terminate(pfn, action, page_size, ret, svsm_ret);
 }
 
-static void svsm_pval_4k_page(unsigned long paddr, bool validate)
+static void __head svsm_pval_4k_page(unsigned long paddr, bool validate)
 {
 	struct svsm_pvalidate_call *pc;
 	struct svsm_call call = {};
@@ -1275,12 +1275,12 @@ static void svsm_pval_4k_page(unsigned long paddr, bool validate)
 
 	ret = svsm_perform_call_protocol(&call);
 	if (ret)
-		svsm_pval_terminate(pc, ret, call.rax_out);
+		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_PVALIDATE);
 
 	native_local_irq_restore(flags);
 }
 
-static void pvalidate_4k_page(unsigned long vaddr, unsigned long paddr, bool validate)
+static void __head pvalidate_4k_page(unsigned long vaddr, unsigned long paddr, bool validate)
 {
 	int ret;
 
@@ -1293,7 +1293,7 @@ static void pvalidate_4k_page(unsigned long vaddr, unsigned long paddr, bool val
 	} else {
 		ret = pvalidate(vaddr, RMP_PG_SIZE_4K, validate);
 		if (ret)
-			__pval_terminate(PHYS_PFN(paddr), validate, RMP_PG_SIZE_4K, ret, 0);
+			sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_PVALIDATE);
 	}
 }
 

>  		}
>  
> @@ -733,21 +733,16 @@ early_set_pages_state(unsigned long vaddr, unsigned long paddr,
>  
>  		val = sev_es_rd_ghcb_msr();
>  
> -		if (WARN(GHCB_RESP_CODE(val) != GHCB_MSR_PSC_RESP,
> -			 "Wrong PSC response code: 0x%x\n",
> -			 (unsigned int)GHCB_RESP_CODE(val)))
> +		if (GHCB_RESP_CODE(val) != GHCB_MSR_PSC_RESP)
>  			goto e_term;
>  
> -		if (WARN(GHCB_MSR_PSC_RESP_VAL(val),
> -			 "Failed to change page state to '%s' paddr 0x%lx error 0x%llx\n",
> -			 op == SNP_PAGE_STATE_PRIVATE ? "private" : "shared",
> -			 paddr, GHCB_MSR_PSC_RESP_VAL(val)))
> +		if (GHCB_MSR_PSC_RESP_VAL(val))
>  			goto e_term;
>  
>  		if (op == SNP_PAGE_STATE_PRIVATE) {
>  			/* Page validation must be performed after changing to private */
>  			ret = pvalidate(vaddr, RMP_PG_SIZE_4K, true);
> -			if (WARN(ret, "Failed to validate address 0x%lx ret %d", paddr, ret))
> +			if (ret)
>  				goto e_term;
>  		}
>  
> @@ -780,7 +775,7 @@ void __head early_snp_set_memory_private(unsigned long vaddr, unsigned long padd
>  	early_set_pages_state(vaddr, paddr, npages, SNP_PAGE_STATE_PRIVATE);
>  }
>  
> -void __init early_snp_set_memory_shared(unsigned long vaddr, unsigned long paddr,
> +void __head early_snp_set_memory_shared(unsigned long vaddr, unsigned long paddr,
>  					unsigned long npages)
>  {
>  	/*