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....@intel.com>
> Sent: Friday, January 5, 2024 5:25 PM
> To: Laszlo Ersek <ler...@redhat.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray...@intel.com>; Kumar, Rahul R <rahul.r.ku...@intel.com>;
> Gerd Hoffmann <kra...@redhat.com>; Xu, Min M <min.m...@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 <ler...@redhat.com>
> Sent: Thursday, January 4, 2024 10:53 PM
> To: devel@edk2.groups.io; Tan, Dun <dun....@intel.com>
> Cc: Ni, Ray <ray...@intel.com>; Kumar, Rahul R <rahul.r.ku...@intel.com>;
> Gerd Hoffmann <kra...@redhat.com>; Xu, Min M <min.m...@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....@intel.com>
> > Cc: Ray Ni <ray...@intel.com>
> > Cc: Laszlo Ersek <ler...@redhat.com>
> > Cc: Rahul Kumar <rahul1.ku...@intel.com>
> > Cc: Gerd Hoffmann <kra...@redhat.com>
> > Cc: Min Xu <min.m...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
