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

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

On 11/11/21 7:53 PM, Ni, Ray wrote:
> 3 comments:
> 
>> --- 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;
> 
> 1. Can you please group all SEV-ES fields together?

Please see my request about doing the grouping as a follow up patch. I 
have some other cleanup bit for SEV which will simplify code a bit and I 
am thinking to do all of those together.

> Those might include followings:
>    BOOLEAN               SevEsIsEnabled;
>    BOOLEAN               SevSnpIsEnabled;
>    UINTN                 GhcbBase;
>    BOOLEAN               ExtTopoAvail;
> 
> 
>>
>> +  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;
>> +    }
>> +  }
>> +
> 
> 2. With the grouping, can you please create a routine such as FillExchangeInfoDataSevEs (...) in AmdSev.c?
> 

Yep, I will do in v13.

> 
>>     //
>>     // 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
> 
> 3. can you please do same group in NASM struct?
> 

Same as above, I will do the needful in the follow up patch.

> 
>