public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.
@ 2020-01-15  6:06 Dong, Eric
  2020-01-15  7:43 ` [edk2-devel] " Ni, Ray
  2020-01-15 10:04 ` Laszlo Ersek
  0 siblings, 2 replies; 7+ messages in thread
From: Dong, Eric @ 2020-01-15  6:06 UTC (permalink / raw)
  To: devel; +Cc: Ray Ni, Laszlo Ersek

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2392

Current code implementation assumes BSP index is 0 at the begin.
This code change removes this assumption. It get BSP index from
the saved data structure if it existed.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 6ec9b172b8..922c87b766 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -636,7 +636,7 @@ ApWakeupFunction (
       //   to initialize AP in InitConfig path.
       // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
       //
-      RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
+      RestoreVolatileRegisters (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, FALSE);
       InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
       ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
 
@@ -1615,6 +1615,7 @@ MpInitLibInitialize (
   UINTN                    ApResetVectorSize;
   UINTN                    BackupBufferAddr;
   UINTN                    ApIdtBase;
+  UINT64                   BspTopOfStack;
 
   OldCpuMpData = GetCpuMpDataFromGuidedHob ();
   if (OldCpuMpData == NULL) {
@@ -1677,7 +1678,7 @@ MpInitLibInitialize (
   CpuMpData->BackupBufferSize = ApResetVectorSize;
   CpuMpData->WakeupBuffer     = (UINTN) -1;
   CpuMpData->CpuCount         = 1;
-  CpuMpData->BspNumber        = 0;
+  CpuMpData->BspNumber        = OldCpuMpData != NULL ? OldCpuMpData->BspNumber : 0;
   CpuMpData->WaitEvent        = NULL;
   CpuMpData->SwitchBspFlag    = FALSE;
   CpuMpData->CpuData          = (CPU_AP_DATA *) (CpuMpData + 1);
@@ -1704,11 +1705,12 @@ MpInitLibInitialize (
   // Don't pass BSP's TR to APs to avoid AP init failure.
   //
   VolatileRegisters.Tr = 0;
-  CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters));
+  CopyMem (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters));
   //
   // Set BSP basic information
   //
-  InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + ApStackSize);
+  BspTopOfStack = CpuMpData->Buffer + (CpuMpData->BspNumber + 1) * CpuMpData->CpuApStackSize;
+  InitializeApData (CpuMpData, CpuMpData->BspNumber, 0, BspTopOfStack);
   //
   // Save assembly code information
   //
-- 
2.23.0.windows.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.
  2020-01-15  6:06 [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption Dong, Eric
@ 2020-01-15  7:43 ` Ni, Ray
  2020-01-15  7:52   ` Dong, Eric
  2020-01-15 10:04 ` Laszlo Ersek
  1 sibling, 1 reply; 7+ messages in thread
From: Ni, Ray @ 2020-01-15  7:43 UTC (permalink / raw)
  To: devel@edk2.groups.io, Dong, Eric; +Cc: Laszlo Ersek

Eric,
What unit test was done for this patch?

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dong, Eric
> Sent: Wednesday, January 15, 2020 2:07 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2392
> 
> 
> Current code implementation assumes BSP index is 0 at the begin.
> This code change removes this assumption. It get BSP index from
> the saved data structure if it existed.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 6ec9b172b8..922c87b766 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -636,7 +636,7 @@ ApWakeupFunction (
>        //   to initialize AP in InitConfig path.
> 
>        // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
> 
>        //
> 
> -      RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
> 
> +      RestoreVolatileRegisters (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, FALSE);
> 
>        InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
> 
>        ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
> 
> 
> 
> @@ -1615,6 +1615,7 @@ MpInitLibInitialize (
>    UINTN                    ApResetVectorSize;
> 
>    UINTN                    BackupBufferAddr;
> 
>    UINTN                    ApIdtBase;
> 
> +  UINT64                   BspTopOfStack;
> 
> 
> 
>    OldCpuMpData = GetCpuMpDataFromGuidedHob ();
> 
>    if (OldCpuMpData == NULL) {
> 
> @@ -1677,7 +1678,7 @@ MpInitLibInitialize (
>    CpuMpData->BackupBufferSize = ApResetVectorSize;
> 
>    CpuMpData->WakeupBuffer     = (UINTN) -1;
> 
>    CpuMpData->CpuCount         = 1;
> 
> -  CpuMpData->BspNumber        = 0;
> 
> +  CpuMpData->BspNumber        = OldCpuMpData != NULL ? OldCpuMpData->BspNumber : 0;
> 
>    CpuMpData->WaitEvent        = NULL;
> 
>    CpuMpData->SwitchBspFlag    = FALSE;
> 
>    CpuMpData->CpuData          = (CPU_AP_DATA *) (CpuMpData + 1);
> 
> @@ -1704,11 +1705,12 @@ MpInitLibInitialize (
>    // Don't pass BSP's TR to APs to avoid AP init failure.
> 
>    //
> 
>    VolatileRegisters.Tr = 0;
> 
> -  CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters));
> 
> +  CopyMem (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, &VolatileRegisters, sizeof
> (VolatileRegisters));
> 
>    //
> 
>    // Set BSP basic information
> 
>    //
> 
> -  InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + ApStackSize);
> 
> +  BspTopOfStack = CpuMpData->Buffer + (CpuMpData->BspNumber + 1) * CpuMpData->CpuApStackSize;
> 
> +  InitializeApData (CpuMpData, CpuMpData->BspNumber, 0, BspTopOfStack);
> 
>    //
> 
>    // Save assembly code information
> 
>    //
> 
> --
> 2.23.0.windows.1
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#53263): https://edk2.groups.io/g/devel/message/53263
> Mute This Topic: https://groups.io/mt/69712223/1712937
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [ray.ni@intel.com]
> -=-=-=-=-=-=


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.
  2020-01-15  7:43 ` [edk2-devel] " Ni, Ray
@ 2020-01-15  7:52   ` Dong, Eric
  2020-01-16 12:23     ` Ni, Ray
  0 siblings, 1 reply; 7+ messages in thread
From: Dong, Eric @ 2020-01-15  7:52 UTC (permalink / raw)
  To: Ni, Ray, devel@edk2.groups.io; +Cc: Laszlo Ersek

[-- Attachment #1: Type: text/plain, Size: 4412 bytes --]

Ray,

I applied this change to an internal desktop machine and did below tests:

1.       boot it to shell

2.       reboot the system to shell.

Thanks,
Eric
From: Ni, Ray
Sent: Wednesday, January 15, 2020 3:43 PM
To: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.

Eric,
What unit test was done for this patch?

> -----Original Message-----
> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Dong, Eric
> Sent: Wednesday, January 15, 2020 2:07 PM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Subject: [edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2392
>
>
> Current code implementation assumes BSP index is 0 at the begin.
> This code change removes this assumption. It get BSP index from
> the saved data structure if it existed.
>
> Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Signed-off-by: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 6ec9b172b8..922c87b766 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -636,7 +636,7 @@ ApWakeupFunction (
>        //   to initialize AP in InitConfig path.
>
>        // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
>
>        //
>
> -      RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
>
> +      RestoreVolatileRegisters (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, FALSE);
>
>        InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
>
>        ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
>
>
>
> @@ -1615,6 +1615,7 @@ MpInitLibInitialize (
>    UINTN                    ApResetVectorSize;
>
>    UINTN                    BackupBufferAddr;
>
>    UINTN                    ApIdtBase;
>
> +  UINT64                   BspTopOfStack;
>
>
>
>    OldCpuMpData = GetCpuMpDataFromGuidedHob ();
>
>    if (OldCpuMpData == NULL) {
>
> @@ -1677,7 +1678,7 @@ MpInitLibInitialize (
>    CpuMpData->BackupBufferSize = ApResetVectorSize;
>
>    CpuMpData->WakeupBuffer     = (UINTN) -1;
>
>    CpuMpData->CpuCount         = 1;
>
> -  CpuMpData->BspNumber        = 0;
>
> +  CpuMpData->BspNumber        = OldCpuMpData != NULL ? OldCpuMpData->BspNumber : 0;
>
>    CpuMpData->WaitEvent        = NULL;
>
>    CpuMpData->SwitchBspFlag    = FALSE;
>
>    CpuMpData->CpuData          = (CPU_AP_DATA *) (CpuMpData + 1);
>
> @@ -1704,11 +1705,12 @@ MpInitLibInitialize (
>    // Don't pass BSP's TR to APs to avoid AP init failure.
>
>    //
>
>    VolatileRegisters.Tr = 0;
>
> -  CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters));
>
> +  CopyMem (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, &VolatileRegisters, sizeof
> (VolatileRegisters));
>
>    //
>
>    // Set BSP basic information
>
>    //
>
> -  InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + ApStackSize);
>
> +  BspTopOfStack = CpuMpData->Buffer + (CpuMpData->BspNumber + 1) * CpuMpData->CpuApStackSize;
>
> +  InitializeApData (CpuMpData, CpuMpData->BspNumber, 0, BspTopOfStack);
>
>    //
>
>    // Save assembly code information
>
>    //
>
> --
> 2.23.0.windows.1
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
>
> View/Reply Online (#53263): https://edk2.groups.io/g/devel/message/53263
> Mute This Topic: https://groups.io/mt/69712223/1712937
> Group Owner: devel+owner@edk2.groups.io<mailto:devel+owner@edk2.groups.io>
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [ray.ni@intel.com]
> -=-=-=-=-=-=

[-- Attachment #2: Type: text/html, Size: 18649 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.
  2020-01-15  6:06 [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption Dong, Eric
  2020-01-15  7:43 ` [edk2-devel] " Ni, Ray
@ 2020-01-15 10:04 ` Laszlo Ersek
  2020-01-16  3:15   ` Dong, Eric
  1 sibling, 1 reply; 7+ messages in thread
From: Laszlo Ersek @ 2020-01-15 10:04 UTC (permalink / raw)
  To: Eric Dong, devel; +Cc: Ray Ni

On 01/15/20 07:06, Eric Dong wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2392
> 
> Current code implementation assumes BSP index is 0 at the begin.
> This code change removes this assumption. It get BSP index from
> the saved data structure if it existed.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 6ec9b172b8..922c87b766 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -636,7 +636,7 @@ ApWakeupFunction (
>        //   to initialize AP in InitConfig path.
>        // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
>        //
> -      RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
> +      RestoreVolatileRegisters (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, FALSE);
>        InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
>        ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
>  
> @@ -1615,6 +1615,7 @@ MpInitLibInitialize (
>    UINTN                    ApResetVectorSize;
>    UINTN                    BackupBufferAddr;
>    UINTN                    ApIdtBase;
> +  UINT64                   BspTopOfStack;
>  
>    OldCpuMpData = GetCpuMpDataFromGuidedHob ();
>    if (OldCpuMpData == NULL) {
> @@ -1677,7 +1678,7 @@ MpInitLibInitialize (
>    CpuMpData->BackupBufferSize = ApResetVectorSize;
>    CpuMpData->WakeupBuffer     = (UINTN) -1;
>    CpuMpData->CpuCount         = 1;
> -  CpuMpData->BspNumber        = 0;
> +  CpuMpData->BspNumber        = OldCpuMpData != NULL ? OldCpuMpData->BspNumber : 0;
>    CpuMpData->WaitEvent        = NULL;
>    CpuMpData->SwitchBspFlag    = FALSE;
>    CpuMpData->CpuData          = (CPU_AP_DATA *) (CpuMpData + 1);
> @@ -1704,11 +1705,12 @@ MpInitLibInitialize (
>    // Don't pass BSP's TR to APs to avoid AP init failure.
>    //
>    VolatileRegisters.Tr = 0;
> -  CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters));
> +  CopyMem (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters));
>    //
>    // Set BSP basic information
>    //
> -  InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + ApStackSize);
> +  BspTopOfStack = CpuMpData->Buffer + (CpuMpData->BspNumber + 1) * CpuMpData->CpuApStackSize;
> +  InitializeApData (CpuMpData, CpuMpData->BspNumber, 0, BspTopOfStack);
>    //
>    // Save assembly code information
>    //
> 

The patch seems reasonable to me (although I have not tried verifying
that all necessary spots are updated).

However, there is one thing I certainly don't understand, and the commit
message doesn't explain it. In the "BspTopOfStack" calculation, why do
we change the *second* factor, when we change the multiplication from:

  (0                    + 1) * ApStackSize

(where the (0 + 1) is implied in the old code), to:

  (CpuMpData->BspNumber + 1) * CpuMpData->CpuApStackSize

?

I understand why the *first* factor is changed -- we basically replace
"0" with "CpuMpData->BspNumber" --; what I don't understand is why we
replace "ApStackSize" with "CpuMpData->CpuApStackSize", in the second
factor.

... Higher up in the code, we have:

  CpuMpData->CpuApStackSize   = ApStackSize;

so this part of the patch might actually have no effect. But, even then,
I think it makes the patch harder to understand. So in that case, I'd
suggest sticking with "ApStackSize", just for keeping the patch simpler.

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.
  2020-01-15 10:04 ` Laszlo Ersek
@ 2020-01-16  3:15   ` Dong, Eric
  2020-01-16  8:35     ` Laszlo Ersek
  0 siblings, 1 reply; 7+ messages in thread
From: Dong, Eric @ 2020-01-16  3:15 UTC (permalink / raw)
  To: Laszlo Ersek, devel@edk2.groups.io; +Cc: Ni, Ray

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, January 15, 2020 6:05 PM
> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: Re: [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0
> Assumption.
> 
> On 01/15/20 07:06, Eric Dong wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2392
> >
> > Current code implementation assumes BSP index is 0 at the begin.
> > This code change removes this assumption. It get BSP index from the
> > saved data structure if it existed.
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> >  UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index 6ec9b172b8..922c87b766 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > @@ -636,7 +636,7 @@ ApWakeupFunction (
> >        //   to initialize AP in InitConfig path.
> >        // NOTE: IDTR.BASE stored in CpuMpData-
> >CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
> >        //
> > -      RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters,
> FALSE);
> > +      RestoreVolatileRegisters
> > + (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters,
> > + FALSE);
> >        InitializeApData (CpuMpData, ProcessorNumber, BistData,
> ApTopOfStack);
> >        ApStartupSignalBuffer =
> > CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
> >
> > @@ -1615,6 +1615,7 @@ MpInitLibInitialize (
> >    UINTN                    ApResetVectorSize;
> >    UINTN                    BackupBufferAddr;
> >    UINTN                    ApIdtBase;
> > +  UINT64                   BspTopOfStack;
> >
> >    OldCpuMpData = GetCpuMpDataFromGuidedHob ();
> >    if (OldCpuMpData == NULL) {
> > @@ -1677,7 +1678,7 @@ MpInitLibInitialize (
> >    CpuMpData->BackupBufferSize = ApResetVectorSize;
> >    CpuMpData->WakeupBuffer     = (UINTN) -1;
> >    CpuMpData->CpuCount         = 1;
> > -  CpuMpData->BspNumber        = 0;
> > +  CpuMpData->BspNumber        = OldCpuMpData != NULL ?
> OldCpuMpData->BspNumber : 0;
> >    CpuMpData->WaitEvent        = NULL;
> >    CpuMpData->SwitchBspFlag    = FALSE;
> >    CpuMpData->CpuData          = (CPU_AP_DATA *) (CpuMpData + 1);
> > @@ -1704,11 +1705,12 @@ MpInitLibInitialize (
> >    // Don't pass BSP's TR to APs to avoid AP init failure.
> >    //
> >    VolatileRegisters.Tr = 0;
> > -  CopyMem (&CpuMpData->CpuData[0].VolatileRegisters,
> > &VolatileRegisters, sizeof (VolatileRegisters));
> > +  CopyMem
> > + (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters,
> > + &VolatileRegisters, sizeof (VolatileRegisters));
> >    //
> >    // Set BSP basic information
> >    //
> > -  InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer +
> > ApStackSize);
> > +  BspTopOfStack = CpuMpData->Buffer + (CpuMpData->BspNumber + 1)
> *
> > + CpuMpData->CpuApStackSize;  InitializeApData (CpuMpData,
> > + CpuMpData->BspNumber, 0, BspTopOfStack);
> >    //
> >    // Save assembly code information
> >    //
> >
> 
> The patch seems reasonable to me (although I have not tried verifying that
> all necessary spots are updated).
> 
> However, there is one thing I certainly don't understand, and the commit
> message doesn't explain it. In the "BspTopOfStack" calculation, why do we
> change the *second* factor, when we change the multiplication from:
> 
>   (0                    + 1) * ApStackSize
> 
> (where the (0 + 1) is implied in the old code), to:
> 
>   (CpuMpData->BspNumber + 1) * CpuMpData->CpuApStackSize
> 
> ?
> 
> I understand why the *first* factor is changed -- we basically replace "0" with
> "CpuMpData->BspNumber" --; what I don't understand is why we replace
> "ApStackSize" with "CpuMpData->CpuApStackSize", in the second factor.
> 
> ... Higher up in the code, we have:
> 
>   CpuMpData->CpuApStackSize   = ApStackSize;
> 
> so this part of the patch might actually have no effect. But, even then, I think
> it makes the patch harder to understand. So in that case, I'd suggest sticking
> with "ApStackSize", just for keeping the patch simpler.
> 
[[Eric]] driver has two places to call InitializeApData (). Here is one and the other in ApWakeupFunction().
      InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
At that function, it calculates the ApTopOfStack like below:
      ApTopOfStack  = CpuMpData->Buffer + (ProcessorNumber + 1) * CpuMpData->CpuApStackSize;

I update new code to follow this coding style. I think after this change, the exit two code pieces are follow
the same coding style.  So I think we can keep my original change.

Thanks,
Eric

> Thanks
> Laszlo

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.
  2020-01-16  3:15   ` Dong, Eric
@ 2020-01-16  8:35     ` Laszlo Ersek
  0 siblings, 0 replies; 7+ messages in thread
From: Laszlo Ersek @ 2020-01-16  8:35 UTC (permalink / raw)
  To: Dong, Eric, devel@edk2.groups.io; +Cc: Ni, Ray

On 01/16/20 04:15, Dong, Eric wrote:
> Hi Laszlo,
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, January 15, 2020 6:05 PM
>> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
>> Cc: Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0
>> Assumption.
>>
>> On 01/15/20 07:06, Eric Dong wrote:
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2392
>>>
>>> Current code implementation assumes BSP index is 0 at the begin.
>>> This code change removes this assumption. It get BSP index from the
>>> saved data structure if it existed.
>>>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Signed-off-by: Eric Dong <eric.dong@intel.com>
>>> ---
>>>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> index 6ec9b172b8..922c87b766 100644
>>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> @@ -636,7 +636,7 @@ ApWakeupFunction (
>>>        //   to initialize AP in InitConfig path.
>>>        // NOTE: IDTR.BASE stored in CpuMpData-
>>> CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
>>>        //
>>> -      RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters,
>> FALSE);
>>> +      RestoreVolatileRegisters
>>> + (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters,
>>> + FALSE);
>>>        InitializeApData (CpuMpData, ProcessorNumber, BistData,
>> ApTopOfStack);
>>>        ApStartupSignalBuffer =
>>> CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
>>>
>>> @@ -1615,6 +1615,7 @@ MpInitLibInitialize (
>>>    UINTN                    ApResetVectorSize;
>>>    UINTN                    BackupBufferAddr;
>>>    UINTN                    ApIdtBase;
>>> +  UINT64                   BspTopOfStack;
>>>
>>>    OldCpuMpData = GetCpuMpDataFromGuidedHob ();
>>>    if (OldCpuMpData == NULL) {
>>> @@ -1677,7 +1678,7 @@ MpInitLibInitialize (
>>>    CpuMpData->BackupBufferSize = ApResetVectorSize;
>>>    CpuMpData->WakeupBuffer     = (UINTN) -1;
>>>    CpuMpData->CpuCount         = 1;
>>> -  CpuMpData->BspNumber        = 0;
>>> +  CpuMpData->BspNumber        = OldCpuMpData != NULL ?
>> OldCpuMpData->BspNumber : 0;
>>>    CpuMpData->WaitEvent        = NULL;
>>>    CpuMpData->SwitchBspFlag    = FALSE;
>>>    CpuMpData->CpuData          = (CPU_AP_DATA *) (CpuMpData + 1);
>>> @@ -1704,11 +1705,12 @@ MpInitLibInitialize (
>>>    // Don't pass BSP's TR to APs to avoid AP init failure.
>>>    //
>>>    VolatileRegisters.Tr = 0;
>>> -  CopyMem (&CpuMpData->CpuData[0].VolatileRegisters,
>>> &VolatileRegisters, sizeof (VolatileRegisters));
>>> +  CopyMem
>>> + (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters,
>>> + &VolatileRegisters, sizeof (VolatileRegisters));
>>>    //
>>>    // Set BSP basic information
>>>    //
>>> -  InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer +
>>> ApStackSize);
>>> +  BspTopOfStack = CpuMpData->Buffer + (CpuMpData->BspNumber + 1)
>> *
>>> + CpuMpData->CpuApStackSize;  InitializeApData (CpuMpData,
>>> + CpuMpData->BspNumber, 0, BspTopOfStack);
>>>    //
>>>    // Save assembly code information
>>>    //
>>>
>>
>> The patch seems reasonable to me (although I have not tried verifying that
>> all necessary spots are updated).
>>
>> However, there is one thing I certainly don't understand, and the commit
>> message doesn't explain it. In the "BspTopOfStack" calculation, why do we
>> change the *second* factor, when we change the multiplication from:
>>
>>   (0                    + 1) * ApStackSize
>>
>> (where the (0 + 1) is implied in the old code), to:
>>
>>   (CpuMpData->BspNumber + 1) * CpuMpData->CpuApStackSize
>>
>> ?
>>
>> I understand why the *first* factor is changed -- we basically replace "0" with
>> "CpuMpData->BspNumber" --; what I don't understand is why we replace
>> "ApStackSize" with "CpuMpData->CpuApStackSize", in the second factor.
>>
>> ... Higher up in the code, we have:
>>
>>   CpuMpData->CpuApStackSize   = ApStackSize;
>>
>> so this part of the patch might actually have no effect. But, even then, I think
>> it makes the patch harder to understand. So in that case, I'd suggest sticking
>> with "ApStackSize", just for keeping the patch simpler.
>>
> [[Eric]] driver has two places to call InitializeApData (). Here is one and the other in ApWakeupFunction().
>       InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
> At that function, it calculates the ApTopOfStack like below:
>       ApTopOfStack  = CpuMpData->Buffer + (ProcessorNumber + 1) * CpuMpData->CpuApStackSize;
> 
> I update new code to follow this coding style. I think after this change, the exit two code pieces are follow
> the same coding style.  So I think we can keep my original change.

That's fine, but then please include this specific argument in the
commit message.

Thanks,
Laszlo


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.
  2020-01-15  7:52   ` Dong, Eric
@ 2020-01-16 12:23     ` Ni, Ray
  0 siblings, 0 replies; 7+ messages in thread
From: Ni, Ray @ 2020-01-16 12:23 UTC (permalink / raw)
  To: Dong, Eric, devel@edk2.groups.io; +Cc: Laszlo Ersek

[-- Attachment #1: Type: text/plain, Size: 4838 bytes --]

I recommend to switch BSP in PEI and see whether system is good in DXE.

From: Dong, Eric <eric.dong@intel.com>
Sent: Wednesday, January 15, 2020 3:53 PM
To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
Cc: Laszlo Ersek <lersek@redhat.com>
Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.

Ray,

I applied this change to an internal desktop machine and did below tests:

  1.  boot it to shell
  2.  reboot the system to shell.

Thanks,
Eric
From: Ni, Ray
Sent: Wednesday, January 15, 2020 3:43 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.

Eric,
What unit test was done for this patch?

> -----Original Message-----
> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Dong, Eric
> Sent: Wednesday, January 15, 2020 2:07 PM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Subject: [edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2392
>
>
> Current code implementation assumes BSP index is 0 at the begin.
> This code change removes this assumption. It get BSP index from
> the saved data structure if it existed.
>
> Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Signed-off-by: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 6ec9b172b8..922c87b766 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -636,7 +636,7 @@ ApWakeupFunction (
>        //   to initialize AP in InitConfig path.
>
>        // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
>
>        //
>
> -      RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
>
> +      RestoreVolatileRegisters (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, FALSE);
>
>        InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
>
>        ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
>
>
>
> @@ -1615,6 +1615,7 @@ MpInitLibInitialize (
>    UINTN                    ApResetVectorSize;
>
>    UINTN                    BackupBufferAddr;
>
>    UINTN                    ApIdtBase;
>
> +  UINT64                   BspTopOfStack;
>
>
>
>    OldCpuMpData = GetCpuMpDataFromGuidedHob ();
>
>    if (OldCpuMpData == NULL) {
>
> @@ -1677,7 +1678,7 @@ MpInitLibInitialize (
>    CpuMpData->BackupBufferSize = ApResetVectorSize;
>
>    CpuMpData->WakeupBuffer     = (UINTN) -1;
>
>    CpuMpData->CpuCount         = 1;
>
> -  CpuMpData->BspNumber        = 0;
>
> +  CpuMpData->BspNumber        = OldCpuMpData != NULL ? OldCpuMpData->BspNumber : 0;
>
>    CpuMpData->WaitEvent        = NULL;
>
>    CpuMpData->SwitchBspFlag    = FALSE;
>
>    CpuMpData->CpuData          = (CPU_AP_DATA *) (CpuMpData + 1);
>
> @@ -1704,11 +1705,12 @@ MpInitLibInitialize (
>    // Don't pass BSP's TR to APs to avoid AP init failure.
>
>    //
>
>    VolatileRegisters.Tr = 0;
>
> -  CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters));
>
> +  CopyMem (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, &VolatileRegisters, sizeof
> (VolatileRegisters));
>
>    //
>
>    // Set BSP basic information
>
>    //
>
> -  InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + ApStackSize);
>
> +  BspTopOfStack = CpuMpData->Buffer + (CpuMpData->BspNumber + 1) * CpuMpData->CpuApStackSize;
>
> +  InitializeApData (CpuMpData, CpuMpData->BspNumber, 0, BspTopOfStack);
>
>    //
>
>    // Save assembly code information
>
>    //
>
> --
> 2.23.0.windows.1
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
>
> View/Reply Online (#53263): https://edk2.groups.io/g/devel/message/53263
> Mute This Topic: https://groups.io/mt/69712223/1712937
> Group Owner: devel+owner@edk2.groups.io<mailto:devel+owner@edk2.groups.io>
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [ray.ni@intel.com]
> -=-=-=-=-=-=

[-- Attachment #2: Type: text/html, Size: 19733 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-01-16 12:23 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-01-15  6:06 [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption Dong, Eric
2020-01-15  7:43 ` [edk2-devel] " Ni, Ray
2020-01-15  7:52   ` Dong, Eric
2020-01-16 12:23     ` Ni, Ray
2020-01-15 10:04 ` Laszlo Ersek
2020-01-16  3:15   ` Dong, Eric
2020-01-16  8:35     ` Laszlo Ersek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox