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

["Ni, Ray" <ray.ni@intel.com>]

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.

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 (#113282): https://edk2.groups.io/g/devel/message/113282
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]
-=-=-=-=-=-=-=-=-=-=-=-