Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg: Retrive EXTENDED_PROCESSOR_INFORMATION

["Laszlo Ersek" <lersek@redhat.com>]

On 1/5/24 13:56, Ni, Ray wrote:
> Laszlo,
> Good suggestion.
> 
> Your solution will not work if in future some extra fields might require to be set to non-zero.
> But future is not coming yet. I agree with your approach.

Well, if we need to set some fields to nonzero, manual assignments will
become necessary either way, with or without the ZeroMem(). With the
ZeroMem(), we just overwrite the zero values later.

I certainly agree that there is a tipping point. Like, if we have 5
fields, and we need to set 4 of them to nonzero values, then an initial
ZeroMem is wasteful. Right now the ZeroMem() looks much more frugal, and
a bit more future-proof too.

Thanks!
Laszlo

> 
> Thanks,
> Ray
>> -----Original Message-----
>> From: Tan, Dun <dun.tan@intel.com>
>> Sent: Friday, January 5, 2024 5:25 PM
>> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
>> Cc: Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
>> Gerd Hoffmann <kraxel@redhat.com>; Xu, Min M <min.m.xu@intel.com>
>> Subject: RE: [edk2-devel] [PATCH 1/2] UefiCpuPkg: Retrive
>> EXTENDED_PROCESSOR_INFORMATION
>>
>> Hi Laszlo,
>>
>> Thanks for your comments. I agree with your solution. It seems simpler and
>> clearer. Will change the code and keep the additional function comments in
>> next version patch set.
>>
>> Thanks,
>> Dun
>>
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Thursday, January 4, 2024 10:53 PM
>> To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
>> Cc: Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
>> Gerd Hoffmann <kraxel@redhat.com>; Xu, Min M <min.m.xu@intel.com>
>> Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg: Retrive
>> EXTENDED_PROCESSOR_INFORMATION
>>
>> On 1/4/24 08:32, duntan wrote:
>>> Retrive EXTENDED_PROCESSOR_INFORMATION in the API
>>> MpInitLibGetProcessorInfo() of MpInitLibUp instance when the BIT24 of
>>> input ProcessorNumber is set.
>>> It's to align with the behavior in PEI/DXE MpInitLib
>>>
>>> Signed-off-by: Dun Tan <dun.tan@intel.com>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Cc: Min Xu <min.m.xu@intel.com>
>>> ---
>>>  UefiCpuPkg/Include/Library/MpInitLib.h       |  2 ++
>>>  UefiCpuPkg/Library/MpInitLib/MpLib.c         |  2 ++
>>>  UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c | 12 ++++++++++++
>>>  3 files changed, 16 insertions(+)
>>>
>>> diff --git a/UefiCpuPkg/Include/Library/MpInitLib.h
>>> b/UefiCpuPkg/Include/Library/MpInitLib.h
>>> index 1853c46415..842c6f7ff9 100644
>>> --- a/UefiCpuPkg/Include/Library/MpInitLib.h
>>> +++ b/UefiCpuPkg/Include/Library/MpInitLib.h
>>> @@ -63,6 +63,8 @@ MpInitLibGetNumberOfProcessors (
>>>    instant this call is made. This service may only be called from the BSP.
>>>
>>>    @param[in]  ProcessorNumber       The handle number of processor.
>>> +                                    Lower 24 bits contains the actual processor number.
>>> +                                    BIT24 indicates if the
>> EXTENDED_PROCESSOR_INFORMATION will be retrived.
>>>    @param[out] ProcessorInfoBuffer   A pointer to the buffer where
>> information for
>>>                                      the requested processor is deposited.
>>>    @param[out] HealthData            Return processor health data.
>>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> index a359906923..cdfb570e61 100644
>>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> @@ -2333,6 +2333,8 @@ MpInitLibInitialize (
>>>    instant this call is made. This service may only be called from the BSP.
>>>
>>>    @param[in]  ProcessorNumber       The handle number of processor.
>>> +                                    Lower 24 bits contains the actual processor number.
>>> +                                    BIT24 indicates if the
>> EXTENDED_PROCESSOR_INFORMATION will be retrived.
>>>    @param[out] ProcessorInfoBuffer   A pointer to the buffer where
>> information for
>>>                                      the requested processor is deposited.
>>>    @param[out]  HealthData            Return processor health data.
>>> diff --git a/UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c
>>> b/UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c
>>> index 86f9fbf903..3af4911d4b 100644
>>> --- a/UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c
>>> +++ b/UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c
>>> @@ -77,6 +77,8 @@ MpInitLibGetNumberOfProcessors (
>>>    instant this call is made. This service may only be called from the BSP.
>>>
>>>    @param[in]  ProcessorNumber       The handle number of processor.
>>> +                                    Lower 24 bits contains the actual processor number.
>>> +                                    BIT24 indicates if the
>> EXTENDED_PROCESSOR_INFORMATION will be retrived.
>>>    @param[out] ProcessorInfoBuffer   A pointer to the buffer where
>> information for
>>>                                      the requested processor is deposited.
>>>    @param[out] HealthData            Return processor health data.
>>> @@ -115,6 +117,16 @@ MpInitLibGetProcessorInfo (
>>>    ProcessorInfoBuffer->Location.Package = 0;
>>>    ProcessorInfoBuffer->Location.Core    = 0;
>>>    ProcessorInfoBuffer->Location.Thread  = 0;
>>> +
>>> +  if ((ProcessorNumber & CPU_V2_EXTENDED_TOPOLOGY) != 0) {
>>> +    ProcessorInfoBuffer->ExtendedInformation.Location2.Package = 0;
>>> +    ProcessorInfoBuffer->ExtendedInformation.Location2.Die     = 0;
>>> +    ProcessorInfoBuffer->ExtendedInformation.Location2.Tile    = 0;
>>> +    ProcessorInfoBuffer->ExtendedInformation.Location2.Module  = 0;
>>> +    ProcessorInfoBuffer->ExtendedInformation.Location2.Core    = 0;
>>> +    ProcessorInfoBuffer->ExtendedInformation.Location2.Thread  = 0;
>>> + }
>>> +
>>>    if (HealthData != NULL) {
>>>      GuidHob = GetFirstGuidHob (&gEfiSecPlatformInformationPpiGuid);
>>>      if (GuidHob != NULL) {
>>
>> (1) For the UP implementation of MpInitLibGetProcessorInfo():
>>
>> How about, for a *complete* solution (covering both pre-patch and post-
>> patch functionality):
>>
>>   ZeroMem (ProcessorInfoBuffer, sizeof *ProcessorInfoBuffer);
>>   ProcessorInfoBuffer->StatusFlag  = PROCESSOR_AS_BSP_BIT  |
>>                                      PROCESSOR_ENABLED_BIT |
>>                                      PROCESSOR_HEALTH_STATUS_BIT;
>>
>> This approach is not slow (most of the time I expect the platform will have an
>> optimized ZeroMem() implementation), it is frugal with code (replaces a
>> bunch of manual zeroing of fields), and it is relatively future-proof (the next
>> time EFI_PROCESSOR_INFORMATION is extended, you likely won't have to
>> touch up the code again, because the ZeroMem() will cover the new fields
>> automatically).
>>
>> Also, this approach will zero out
>> ProcessorInfoBuffer->ExtendedInformation *regardless* of BIT24 in the
>> input, which I kind of consider an advantage! (No garbage in the output
>> structure.) Again, I don't think the zeroing is wasteful, regarding CPU cycles.
>>
>> I do agree that the leading function comments should mention BIT24
>>
>> Thanks
>> Laszlo
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113288): https://edk2.groups.io/g/devel/message/113288
Mute This Topic: https://groups.io/mt/103518742/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-