Re: [PATCH] x86/sev: Share the sev_secrets_pa value again

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

On 4/24/25 10:34, Ingo Molnar wrote:
> 
> * Tom Lendacky <thomas.lendacky@amd.com> wrote:
> 
>> On 4/18/25 09:12, Ard Biesheuvel wrote:
>>> From: Ard Biesheuvel <ardb@kernel.org>
>>>
>>> Disentangle the SEV core code and the SEV code that is called during
>>> early boot. The latter piece will be moved into startup/ in a subsequent
>>> patch.
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>
>> This patch breaks SNP guests. The SNP guest boots, but no longer has
>> access to the VMPCK keys needed to communicate with the ASP, which is
>> used, for example, to obtain an attestation report.
>>
>> It looks like the secrets_pa is defined as static in both startup.c and
>> core.c. It is set by a function in startup.c and so when used in core.c
>> its value will be 0.
>>
>> The following fixed the issue for me. Let me know if it can be squashed
>> in or a full patch is needed. Although, it likely should be named
>> sev_secrets_pa since it is no longer static.
> 
> Thanks for the fix!
> 
> I wrote a changelog for it and also included the suggested rename to 
> sev_secrets_pa as it's now a global symbol. I've added your SOB as 
> well. Does this patch look good to you?

Yes, looks good.

Thanks,
Tom

> 
> Thanks,
> 
> 	Ingo
> 
> ==========================================>
> From: Tom Lendacky <thomas.lendacky@amd.com>
> Date: Wed, 23 Apr 2025 10:22:31 -0500
> Subject: [PATCH] x86/sev: Share the sev_secrets_pa value again
> 
> This commits breaks SNP guests:
> 
>   234cf67fc3bd ("x86/sev: Split off startup code from core code")
> 
> The SNP guest boots, but no longer has access to the VMPCK keys needed
> to communicate with the ASP, which is used, for example, to obtain an
> attestation report.
> 
> The secrets_pa value is defined as static in both startup.c and
> core.c. It is set by a function in startup.c and so when used in
> core.c its value will be 0.
> 
> Share it again and add the sev_ prefix to put it into the global
> SEV symbols namespace.
> 
> [ mingo: Renamed to sev_secrets_pa ]
> 
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
> Cc: Dionna Amalie Glaze <dionnaglaze@google.com>
> Cc: Kevin Loughlin <kevinloughlin@google.com>
> Link: https://lore.kernel.org/r/cf878810-81ed-3017-52c6-ce6aa41b5f01@amd.com
> ---
>  arch/x86/boot/startup/sev-startup.c | 4 ++--
>  arch/x86/coco/sev/core.c            | 7 ++-----
>  arch/x86/include/asm/sev-internal.h | 1 +
>  3 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/boot/startup/sev-startup.c b/arch/x86/boot/startup/sev-startup.c
> index 36a75c5096b0..f901ce9680e6 100644
> --- a/arch/x86/boot/startup/sev-startup.c
> +++ b/arch/x86/boot/startup/sev-startup.c
> @@ -55,7 +55,7 @@ struct ghcb *boot_ghcb __section(".data");
>  u64 sev_hv_features __ro_after_init;
>  
>  /* Secrets page physical address from the CC blob */
> -static u64 secrets_pa __ro_after_init;
> +u64 sev_secrets_pa __ro_after_init;
>  
>  /* For early boot SVSM communication */
>  struct svsm_ca boot_svsm_ca_page __aligned(PAGE_SIZE);
> @@ -1367,7 +1367,7 @@ bool __head snp_init(struct boot_params *bp)
>  		return false;
>  
>  	if (cc_info->secrets_phys && cc_info->secrets_len == PAGE_SIZE)
> -		secrets_pa = cc_info->secrets_phys;
> +		sev_secrets_pa = cc_info->secrets_phys;
>  	else
>  		return false;
>  
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index 617988a5f3d7..ac400525de73 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -80,9 +80,6 @@ static const char * const sev_status_feat_names[] = {
>  	[MSR_AMD64_SNP_SMT_PROT_BIT]		= "SMTProt",
>  };
>  
> -/* Secrets page physical address from the CC blob */
> -static u64 secrets_pa __ro_after_init;
> -
>  /*
>   * For Secure TSC guests, the BSP fetches TSC_INFO using SNP guest messaging and
>   * initializes snp_tsc_scale and snp_tsc_offset. These values are replicated
> @@ -109,7 +106,7 @@ static u64 __init get_snp_jump_table_addr(void)
>  	void __iomem *mem;
>  	u64 addr;
>  
> -	mem = ioremap_encrypted(secrets_pa, PAGE_SIZE);
> +	mem = ioremap_encrypted(sev_secrets_pa, PAGE_SIZE);
>  	if (!mem) {
>  		pr_err("Unable to locate AP jump table address: failed to map the SNP secrets page.\n");
>  		return 0;
> @@ -1599,7 +1596,7 @@ struct snp_msg_desc *snp_msg_alloc(void)
>  	if (!mdesc)
>  		return ERR_PTR(-ENOMEM);
>  
> -	mem = ioremap_encrypted(secrets_pa, PAGE_SIZE);
> +	mem = ioremap_encrypted(sev_secrets_pa, PAGE_SIZE);
>  	if (!mem)
>  		goto e_free_mdesc;
>  
> diff --git a/arch/x86/include/asm/sev-internal.h b/arch/x86/include/asm/sev-internal.h
> index e54847a69107..a78f97208a39 100644
> --- a/arch/x86/include/asm/sev-internal.h
> +++ b/arch/x86/include/asm/sev-internal.h
> @@ -5,6 +5,7 @@
>  extern struct ghcb boot_ghcb_page;
>  extern struct ghcb *boot_ghcb;
>  extern u64 sev_hv_features;
> +extern u64 sev_secrets_pa;
>  
>  /* #VC handler runtime per-CPU data */
>  struct sev_es_runtime_data {