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

["duntan" <dun.tan@intel.com>]

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 (#113233): https://edk2.groups.io/g/devel/message/113233
Mute This Topic: https://groups.io/mt/103518742/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-