Re: [PATCH v3 4/4] UefiCpuPkg: Store SEV-SNP AP jump table in the secrets page

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 5/20/22 10:27, Michael Roth wrote:
> A full-featured SEV-SNP guest will not rely on the AP jump table, and
> will instead use the AP Creation interface defined by the GHCB. However,
> a guest is still allowed to use the AP jump table if desired.
> 
> However, unlike with SEV-ES guests, SEV-SNP guests should not
> store/retrieve the jump table address via GHCB requests to the
> hypervisor, they should instead store/retrieve it via the SEV-SNP
> secrets page. Implement the store side of this for OVMF.
> 
> Suggested-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
>   UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  1 +
>   UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       | 10 ++++++++++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index e1cd0b3500..d8cfddcd82 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -80,3 +80,4 @@
>     gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                      ## CONSUMES
>     gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase                           ## CONSUMES
>     gEfiMdePkgTokenSpaceGuid.PcdConfidentialComputingGuestAttr           ## CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdSevSnpSecretsAddress                     ## CONSUMES
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 60d14a5a0e..4d6f7643db 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -15,6 +15,7 @@
>   #include <Library/VmgExitLib.h>
>   #include <Register/Amd/Fam17Msr.h>
>   #include <Register/Amd/Ghcb.h>
> +#include <Register/Amd/SnpSecretsPage.h>
>   
>   #include <Protocol/Timer.h>
>   
> @@ -216,6 +217,15 @@ GetSevEsAPMemory (
>   
>     DEBUG ((DEBUG_INFO, "Dxe: SevEsAPMemory = %lx\n", (UINTN)StartAddress));
>   
> +  if (ConfidentialComputingGuestHas (CCAttrAmdSevSnp)) {
> +    SNP_SECRETS_PAGE  *Secrets;
> +
> +    Secrets                       = (SNP_SECRETS_PAGE *)(INTN)PcdGet64 (PcdSevSnpSecretsAddress);
> +    Secrets->OsArea.ApJumpTablePa = (UINT64)(UINTN)StartAddress;
> +
> +    return (UINTN)StartAddress;
> +  }
> +

Just a nit, but I probably would have still put this under the comment 
below, because you are still saving the SevEsAPMemory as the AP jump table.

It might look cleaner with the non-SNP path as the else and have the 
single return.

Probably not worth another version, but up to you.

Thanks,
Tom

>     //
>     // Save the SevEsAPMemory as the AP jump table.
>     //