[edk2-devel] [PATCH 0/2] Change the usage of input parameter ProcessorNumber in MpInitLibGetProcessorInfo() of MpInitLibUp

[edk2-devel] [PATCH 0/2] Change the usage of input parameter ProcessorNumber in MpInitLibGetProcessorInfo() of MpInitLibUp

[duntan]

In this patch set, the usage of input parameter ProcessorNumber in MpInitLibGetProcessorInfo() of MpInitLibUp instance is changed.
1.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.

2.Also, check lower 24 bits of ProcessorNumber instead of the value of ProcessorNumber in the API MpInitLibGetProcessorInfo() of MpInitLibUp instance.
The BIT24 of input ProcessorNumber might be set to indicate if the EXTENDED_PROCESSOR_INFORMATION will be retrived.

Dun Tan (2):
  UefiCpuPkg: Retrive EXTENDED_PROCESSOR_INFORMATION
  UefiCpuPkg: Check lower 24 bits of ProcessorNumber

 UefiCpuPkg/Include/Library/MpInitLib.h       |  2 ++
 UefiCpuPkg/Library/MpInitLib/MpLib.c         |  2 ++
 UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c | 17 ++++++++++++++++-
 3 files changed, 20 insertions(+), 1 deletion(-)

-- 
2.31.1.windows.1



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

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

[duntan]

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) {
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113138): https://edk2.groups.io/g/devel/message/113138
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]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 2/2] UefiCpuPkg: Check lower 24 bits of ProcessorNumber

[duntan]

Check lower 24 bits of ProcessorNumber instead of
the value of ProcessorNumber in the API
MpInitLibGetProcessorInfo() of MpInitLibUp instance.
Lower 24 bits of ProcessorNumber contains the actual
processor number.
The BIT24 of input ProcessorNumber might be set to
indicate if the EXTENDED_PROCESSOR_INFORMATION will
be retrived.

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/Library/MpInitLibUp/MpInitLibUp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c b/UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c
index 3af4911d4b..b804e400e6 100644
--- a/UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c
+++ b/UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c
@@ -106,7 +106,10 @@ MpInitLibGetProcessorInfo (
     return EFI_INVALID_PARAMETER;
   }
 
-  if (ProcessorNumber != 0) {
+  //
+  // Lower 24 bits contains the actual processor number.
+  //
+  if ((ProcessorNumber & (CPU_V2_EXTENDED_TOPOLOGY - 1)) != 0) {
     return EFI_NOT_FOUND;
   }
 
-- 
2.31.1.windows.1



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

Re: [edk2-devel] [PATCH 2/2] UefiCpuPkg: Check lower 24 bits of ProcessorNumber

[Laszlo Ersek]

On 1/4/24 08:32, duntan wrote:
> Check lower 24 bits of ProcessorNumber instead of
> the value of ProcessorNumber in the API
> MpInitLibGetProcessorInfo() of MpInitLibUp instance.
> Lower 24 bits of ProcessorNumber contains the actual
> processor number.
> The BIT24 of input ProcessorNumber might be set to
> indicate if the EXTENDED_PROCESSOR_INFORMATION will
> be retrived.
> 
> 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/Library/MpInitLibUp/MpInitLibUp.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c b/UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c
> index 3af4911d4b..b804e400e6 100644
> --- a/UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c
> +++ b/UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c
> @@ -106,7 +106,10 @@ MpInitLibGetProcessorInfo (
>      return EFI_INVALID_PARAMETER;
>    }
>  
> -  if (ProcessorNumber != 0) {
> +  //
> +  // Lower 24 bits contains the actual processor number.
> +  //
> +  if ((ProcessorNumber & (CPU_V2_EXTENDED_TOPOLOGY - 1)) != 0) {
>      return EFI_NOT_FOUND;
>    }
>  

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



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

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

[Laszlo Ersek]

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

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

[duntan]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] UefiCpuPkg: Check lower 24 bits of ProcessorNumber

[Ni, Ray]

> > -  if (ProcessorNumber != 0) {
> > +  //
> > +  // Lower 24 bits contains the actual processor number.
> > +  //
> > +  if ((ProcessorNumber & (CPU_V2_EXTENDED_TOPOLOGY - 1)) != 0) {
I suggest we explicitly use BIT24 instead of CPU_V2_EXTENDED_TOPOLOGY.
Using BIT24 clearly tells that processor number only occupies the lower 24 bits.


> >      return EFI_NOT_FOUND;
> >    }
> >
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>



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

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

[Ni, Ray]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] UefiCpuPkg: Check lower 24 bits of ProcessorNumber

[Laszlo Ersek]

On 1/5/24 13:55, Ni, Ray wrote:
>>> -  if (ProcessorNumber != 0) {
>>> +  //
>>> +  // Lower 24 bits contains the actual processor number.
>>> +  //
>>> +  if ((ProcessorNumber & (CPU_V2_EXTENDED_TOPOLOGY - 1)) != 0) {
> I suggest we explicitly use BIT24 instead of CPU_V2_EXTENDED_TOPOLOGY.
> Using BIT24 clearly tells that processor number only occupies the lower 24 bits.

Yes, I've noticed this discrepancy too; I agree BIT24 is clearer here!

> 
> 
>>>      return EFI_NOT_FOUND;
>>>    }
>>>
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 



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

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

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] UefiCpuPkg: Check lower 24 bits of ProcessorNumber

[duntan]

Thanks for the comments. The code has been updated in the V2 patch set.

Thanks,
Dun

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: Friday, January 5, 2024 9:56 PM
To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
Cc: 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 2/2] UefiCpuPkg: Check lower 24 bits of ProcessorNumber

On 1/5/24 13:55, Ni, Ray wrote:
>>> -  if (ProcessorNumber != 0) {
>>> +  //
>>> +  // Lower 24 bits contains the actual processor number.
>>> +  //
>>> +  if ((ProcessorNumber & (CPU_V2_EXTENDED_TOPOLOGY - 1)) != 0) {
> I suggest we explicitly use BIT24 instead of CPU_V2_EXTENDED_TOPOLOGY.
> Using BIT24 clearly tells that processor number only occupies the lower 24 bits.

Yes, I've noticed this discrepancy too; I agree BIT24 is clearer here!

> 
> 
>>>      return EFI_NOT_FOUND;
>>>    }
>>>
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 



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