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 (#113233): https://edk2.groups.io/g/devel/message/113233
Mute This Topic: https://groups.io/mt/103518742/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
