Re: [edk2-devel] [PATCH v11 27/32] UefiCpuPkg/MpInitLib: use BSP to do extended topology check

["Brijesh Singh" <brijesh.singh@amd.com>]

Hi Ray and Rahul,

Any comment on this patch ? If you are okay with the approach then can I
get Ack or R-b ?

-Brijesh

On 10/22/21 11:13 PM, Brijesh Singh via groups.io wrote:
> 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         |  1 +
>  UefiCpuPkg/Library/MpInitLib/MpLib.c         | 11 ++++++++
>  UefiCpuPkg/Library/MpInitLib/MpEqu.inc       |  1 +
>  UefiCpuPkg/Library/MpInitLib/X64/AmdSev.nasm | 27 ++++++++++++++++++++
>  4 files changed, 40 insertions(+)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 45bc1de23e3c..c52b6157429b 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()
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index ab838cbc0ff6..d8372ffa9d5a 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -853,6 +853,7 @@ FillExchangeInfoData (
>    UINTN                            Size;
>    IA32_SEGMENT_DESCRIPTOR          *Selector;
>    IA32_CR4                         Cr4;
> +  UINT32                           StdRangeMax;
>  
>    ExchangeInfo                  = CpuMpData->MpCpuExchangeInfo;
>    ExchangeInfo->StackStart      = CpuMpData->Buffer;
> @@ -892,6 +893,16 @@ FillExchangeInfoData (
>    ExchangeInfo->SevSnpIsEnabled = CpuMpData->SevSnpIsEnabled;
>    ExchangeInfo->GhcbBase        = (UINTN) CpuMpData->GhcbBase;
>  
> +  if (ExchangeInfo->SevSnpIsEnabled) {
> +    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;
> +    }
> +  }
> +
>    //
>    // 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