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...@intel.com>
> -----Original Message-----
> From: Brijesh Singh <brijesh.si...@amd.com>
> Sent: Saturday, November 13, 2021 1:40 AM
> To: devel@edk2.groups.io
> Cc: James Bottomley <j...@linux.ibm.com>; Xu, Min M <min.m...@intel.com>;
> Yao, Jiewen <jiewen....@intel.com>; Tom
> Lendacky <thomas.lenda...@amd.com>; Justen, Jordan L
> <jordan.l.jus...@intel.com>; Ard Biesheuvel
> <ardb+tianoc...@kernel.org>; Erdem Aktas <erdemak...@google.com>; Michael
> Roth <michael.r...@amd.com>; Gerd
> Hoffmann <kra...@redhat.com>; Kinney, Michael D <michael.d.kin...@intel.com>;
> Liming Gao <gaolim...@byosoft.com.cn>;
> Liu, Zhiguang <zhiguang....@intel.com>; Ni, Ray <ray...@intel.com>; Kumar,
> Rahul1 <rahul1.ku...@intel.com>; Dong, Eric
> <eric.d...@intel.com>; Michael Roth <michael.r...@amd.com>; Brijesh Singh
> <brijesh.si...@amd.com>
> Subject: [PATCH v13 27/32] UefiCpuPkg/MpInitLib: use BSP to do extended
> topology check
>
> From: Michael Roth <michael.r...@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.d...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Rahul Kumar <rahul1.ku...@intel.com>
> Cc: James Bottomley <j...@linux.ibm.com>
> Cc: Min Xu <min.m...@intel.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
> Cc: Erdem Aktas <erdemak...@google.com>
> Cc: Gerd Hoffmann <kra...@redhat.com>
> Acked-by: Gerd Hoffmann <kra...@redhat.com>
> Suggested-by: Brijesh Singh <brijesh.si...@amd.com>
> Signed-off-by: Michael Roth <michael.r...@amd.com>
> Signed-off-by: Brijesh Singh <brijesh.si...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#84159): https://edk2.groups.io/g/devel/message/84159
Mute This Topic: https://groups.io/mt/87011899/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
