Re: [PATCH v13 27/32] UefiCpuPkg/MpInitLib: use BSP to do extended topology check

["Ni, Ray" <ray.ni@intel.com>]

I saw you only addressed one of three comments.
Since you said you will propose more patches for the cleanup, Acked-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Brijesh Singh <brijesh.singh@amd.com>
> Sent: Saturday, November 13, 2021 1:40 AM
> To: devel@edk2.groups.io
> Cc: James Bottomley <jejb@linux.ibm.com>; Xu, Min M <min.m.xu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom
> Lendacky <thomas.lendacky@amd.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Erdem Aktas <erdemaktas@google.com>; Michael Roth <Michael.Roth@amd.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Michael Roth <michael.roth@amd.com>; Brijesh Singh <brijesh.singh@amd.com>
> Subject: [PATCH v13 27/32] UefiCpuPkg/MpInitLib: use BSP to do extended topology check
> 
> From: Michael Roth <michael.roth@amd.com>
> 
> During AP bringup, just after switching to long mode, APs will do some
> cpuid calls to verify that the extended topology leaf (0xB) is available
> so they can fetch their x2 APIC IDs from it. In the case of SEV-ES,
> these cpuid instructions must be handled by direct use of the GHCB MSR
> protocol to fetch the values from the hypervisor, since a #VC handler
> is not yet available due to the AP's stack not being set up yet.
> 
> For SEV-SNP, rather than relying on the GHCB MSR protocol, it is
> expected that these values would be obtained from the SEV-SNP CPUID
> table instead. The actual x2 APIC ID (and 8-bit APIC IDs) would still
> be fetched from hypervisor using the GHCB MSR protocol however, so
> introducing support for the SEV-SNP CPUID table in that part of the AP
> bring-up code would only be to handle the checks/validation of the
> extended topology leaf.
> 
> Rather than introducing all the added complexity needed to handle these
> checks via the CPUID table, instead let the BSP do the check in advance,
> since it can make use of the #VC handler to avoid the need to scan the
> SNP CPUID table directly, and add a flag in ExchangeInfo to communicate
> the result of this check to APs.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> Suggested-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.h         | 11 ++++++++
>  UefiCpuPkg/Library/MpInitLib/AmdSev.c        | 21 +++++++++++++++
>  UefiCpuPkg/Library/MpInitLib/MpLib.c         |  7 +++++
>  UefiCpuPkg/Library/MpInitLib/MpEqu.inc       |  1 +
>  UefiCpuPkg/Library/MpInitLib/X64/AmdSev.nasm | 27 ++++++++++++++++++++
>  5 files changed, 67 insertions(+)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 45bc1de23e3c..c5887ff6f647 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -224,6 +224,7 @@ typedef struct {
>    BOOLEAN               SevEsIsEnabled;
>    BOOLEAN               SevSnpIsEnabled;
>    UINTN                 GhcbBase;
> +  BOOLEAN               ExtTopoAvail;
>  } MP_CPU_EXCHANGE_INFO;
> 
>  #pragma pack()
> @@ -789,5 +790,15 @@ ConfidentialComputingGuestHas (
>    CONFIDENTIAL_COMPUTING_GUEST_ATTR     Attr
>    );
> 
> +/**
> +  The function fills the exchange data for the AP.
> +
> +  @param[in]   ExchangeInfo  The pointer to CPU Exchange Data structure
> +**/
> +VOID
> +FillExchangeInfoDataSevEs (
> +  IN volatile MP_CPU_EXCHANGE_INFO    *ExchangeInfo
> +  );
> +
>  #endif
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/AmdSev.c b/UefiCpuPkg/Library/MpInitLib/AmdSev.c
> index 7dbf117c2b71..db02d059512c 100644
> --- a/UefiCpuPkg/Library/MpInitLib/AmdSev.c
> +++ b/UefiCpuPkg/Library/MpInitLib/AmdSev.c
> @@ -237,3 +237,24 @@ SevEsPlaceApHlt (
> 
>    MpInitLibSevEsAPReset (Ghcb, CpuMpData);
>  }
> +
> +/**
> +  The function fills the exchange data for the AP.
> +
> +  @param[in]   ExchangeInfo  The pointer to CPU Exchange Data structure
> +**/
> +VOID
> +FillExchangeInfoDataSevEs (
> +  IN volatile MP_CPU_EXCHANGE_INFO    *ExchangeInfo
> +  )
> +{
> +  UINT32          StdRangeMax;
> +
> +  AsmCpuid (CPUID_SIGNATURE, &StdRangeMax, NULL, NULL, NULL);
> +  if (StdRangeMax >= CPUID_EXTENDED_TOPOLOGY) {
> +    CPUID_EXTENDED_TOPOLOGY_EBX ExtTopoEbx;
> +
> +    AsmCpuid (CPUID_EXTENDED_TOPOLOGY, NULL, &ExtTopoEbx.Uint32, NULL, NULL);
> +    ExchangeInfo->ExtTopoAvail = !!ExtTopoEbx.Bits.LogicalProcessors;
> +  }
> +}
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 315172fb937a..b13ba3075273 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -892,6 +892,13 @@ FillExchangeInfoData (
>    ExchangeInfo->SevSnpIsEnabled = CpuMpData->SevSnpIsEnabled;
>    ExchangeInfo->GhcbBase        = (UINTN) CpuMpData->GhcbBase;
> 
> +  //
> +  // Populate SEV-ES specific exchange data.
> +  //
> +  if (ExchangeInfo->SevSnpIsEnabled) {
> +    FillExchangeInfoDataSevEs (ExchangeInfo);
> +  }
> +
>    //
>    // Get the BSP's data of GDT and IDT
>    //
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpEqu.inc b/UefiCpuPkg/Library/MpInitLib/MpEqu.inc
> index 01668638f245..aba53f57201c 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpEqu.inc
> +++ b/UefiCpuPkg/Library/MpInitLib/MpEqu.inc
> @@ -94,6 +94,7 @@ struc MP_CPU_EXCHANGE_INFO
>    .SevEsIsEnabled:               CTYPE_BOOLEAN 1
>    .SevSnpIsEnabled               CTYPE_BOOLEAN 1
>    .GhcbBase:                     CTYPE_UINTN 1
> +  .ExtTopoAvail:                 CTYPE_BOOLEAN 1
>  endstruc
> 
>  MP_CPU_EXCHANGE_INFO_OFFSET equ (SwitchToRealProcEnd - RendezvousFunnelProcStart)
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.nasm b/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.nasm
> index 0034920b2f6b..8bb1161fa0f7 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.nasm
> @@ -118,6 +118,32 @@ SevEsGetApicId:
>      or         rax, rdx
>      mov        rdi, rax             ; RDI now holds the original GHCB GPA
> 
> +    ;
> +    ; For SEV-SNP, the recommended handling for getting the x2APIC ID
> +    ; would be to use the SNP CPUID table to fetch CPUID.00H:EAX and
> +    ; CPUID:0BH:EBX[15:0] instead of the GHCB MSR protocol vmgexits
> +    ; below.
> +    ;
> +    ; To avoid the unecessary ugliness to accomplish that here, the BSP
> +    ; has performed these checks in advance (where #VC handler handles
> +    ; the CPUID table lookups automatically) and cached them in a flag
> +    ; so those checks can be skipped here.
> +    ;
> +    mov        eax, [esi + MP_CPU_EXCHANGE_INFO_FIELD (SevSnpIsEnabled)]
> +    cmp        al, 1
> +    jne        CheckExtTopoAvail
> +
> +    ;
> +    ; Even with SEV-SNP, the actual x2APIC ID in CPUID.0BH:EDX
> +    ; fetched from the hypervisor the same way SEV-ES does it.
> +    ;
> +    mov        eax, [esi + MP_CPU_EXCHANGE_INFO_FIELD (ExtTopoAvail)]
> +    cmp        al, 1
> +    je         GetApicIdSevEs
> +    ; The 8-bit APIC ID fallback is also the same as with SEV-ES
> +    jmp        NoX2ApicSevEs
> +
> +CheckExtTopoAvail:
>      mov        rdx, 0               ; CPUID function 0
>      mov        rax, 0               ; RAX register requested
>      or         rax, 4
> @@ -136,6 +162,7 @@ SevEsGetApicId:
>      test       edx, 0ffffh
>      jz         NoX2ApicSevEs        ; CPUID.0BH:EBX[15:0] is zero
> 
> +GetApicIdSevEs:
>      mov        rdx, 0bh             ; CPUID function 0x0b
>      mov        rax, 0c0000000h      ; RDX register requested
>      or         rax, 4
> --
> 2.25.1