Re: [PATCH v4 07/24] x86/sev: Move MSR save/restore out of early page state change helper

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

On 7/9/25 03:08, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> The function __page_state_change() in the decompressor is very similar
> to the loop in early_set_pages_state(), and they can share this code
> once the MSR save/restore is moved out.
> 
> This also avoids doing the preserve/restore for each page in a longer
> sequence unnecessarily.
> 
> This simplifies subsequent changes, where the APIs used by
> __page_state_change() are modified for better separation between startup
> code and ordinary code.

The reason for the calls being in __page_state_change() is because of
the call to pavalidate_4k_page(). If that code path is ever changed to
cause a #VC, then the GHCB MSR will be messed up when the UEFI #VC
handler gets control and will be an issue.

Thanks,
Tom

> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/x86/boot/compressed/sev.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index 6c0f91d38595..f00f68175f14 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -71,9 +71,6 @@ static void __page_state_change(unsigned long paddr, enum psc_op op)
>  	if (op == SNP_PAGE_STATE_SHARED)
>  		pvalidate_4k_page(paddr, paddr, false);
>  
> -	/* Save the current GHCB MSR value */
> -	msr = sev_es_rd_ghcb_msr();
> -
>  	/* Issue VMGEXIT to change the page state in RMP table. */
>  	sev_es_wr_ghcb_msr(GHCB_MSR_PSC_REQ_GFN(paddr >> PAGE_SHIFT, op));
>  	VMGEXIT();
> @@ -83,9 +80,6 @@ static void __page_state_change(unsigned long paddr, enum psc_op op)
>  	if ((GHCB_RESP_CODE(val) != GHCB_MSR_PSC_RESP) || GHCB_MSR_PSC_RESP_VAL(val))
>  		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_PSC);
>  
> -	/* Restore the GHCB MSR value */
> -	sev_es_wr_ghcb_msr(msr);
> -
>  	/*
>  	 * Now that page state is changed in the RMP table, validate it so that it is
>  	 * consistent with the RMP entry.
> @@ -96,18 +90,26 @@ static void __page_state_change(unsigned long paddr, enum psc_op op)
>  
>  void snp_set_page_private(unsigned long paddr)
>  {
> +	u64 msr;
> +
>  	if (!sev_snp_enabled())
>  		return;
>  
> +	msr = sev_es_rd_ghcb_msr();
>  	__page_state_change(paddr, SNP_PAGE_STATE_PRIVATE);
> +	sev_es_wr_ghcb_msr(msr);
>  }
>  
>  void snp_set_page_shared(unsigned long paddr)
>  {
> +	u64 msr;
> +
>  	if (!sev_snp_enabled())
>  		return;
>  
> +	msr = sev_es_rd_ghcb_msr();
>  	__page_state_change(paddr, SNP_PAGE_STATE_SHARED);
> +	sev_es_wr_ghcb_msr(msr);
>  }
>  
>  bool early_setup_ghcb(void)
> @@ -132,8 +134,11 @@ bool early_setup_ghcb(void)
>  
>  void snp_accept_memory(phys_addr_t start, phys_addr_t end)
>  {
> +	u64 msr = sev_es_rd_ghcb_msr();
> +
>  	for (phys_addr_t pa = start; pa < end; pa += PAGE_SIZE)
>  		__page_state_change(pa, SNP_PAGE_STATE_PRIVATE);
> +	sev_es_wr_ghcb_msr(msr);
>  }
>  
>  void sev_es_shutdown_ghcb(void)