[PATCH v2 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.

[PATCH v2 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.

[Dong, Eric]

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

This patch fixes an assertion because AP can't find the CpuMpData.
When AP is waken up through Init-Sipi-Sipi, AP's IDT should
be restored to pre-allocated buffer so AP can get the CpuMpData
through the IDT base address.
Current code already has logic to handle this when CpuMpData->
InitFlag is ApInitConfig but misses the logic
when CpuMpData->InitFlag is ApInitReconfig.
This patch fixes this gap.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Chandana Kumar <chandana.c.kumar@intel.com>
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
V2:
1. Enhance code comments.

 UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 64a4c3546e..2e87aa1f06 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -692,6 +692,16 @@ ApWakeupFunction (
         //
         RestoreVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, TRUE);
       } else {
+        if (CpuMpData->InitFlag == ApInitReconfig) {
+          //
+          // Initialize AP volatile registers in ApInitReconfig path.
+          // ApInitReconfig happens when:
+          // 1. AP is re-enabled after it's disabled, in either PEI or DXE phase.
+          // 2. AP is initialized in DXE phase.
+          //
+          RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
+        }
+
         //
         // The CPU driver might not flush TLB for APs on spot after updating
         // page attributes. AP in mwait loop mode needs to take care of it when
-- 
2.23.0.windows.1

[PATCH v2 2/2] UefiCpuPkg/MpInitLib: Avoid ApInitReconfig in PEI.

[Dong, Eric]

From: "Dong, Eric" <eric.dong@intel.com>

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

In PEI phase, AP already been waked up through ApInitConfig,
so it can directly wake up it through change wakup buffer
instead of use ApInitReconfig flag. It can save some time.

Change code to only use ApInitReconfig flag in DXE phase
which must need to update the wake up buffer.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Chandana Kumar <chandana.c.kumar@intel.com>
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
V2:
1. Enhance code to remove CpuMpData->ApLoopMode == ApInHltLoop check.

 UefiCpuPkg/Library/MpInitLib/MpLib.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 2e87aa1f06..6d3a0ccc72 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1818,7 +1818,14 @@ MpInitLibInitialize (
   // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
   //
   if (CpuMpData->CpuCount > 1) {
-    CpuMpData->InitFlag = ApInitReconfig;
+    if (OldCpuMpData != NULL) {
+      //
+      // Only needs to use this flag for DXE phase to update the wake up
+      // buffer. Wakeup buffer allocated in PEI phase is no longer valid
+      // in DXE.
+      //
+      CpuMpData->InitFlag = ApInitReconfig;
+    }
     WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
     //
     // Wait for all APs finished initialization
@@ -1826,7 +1833,9 @@ MpInitLibInitialize (
     while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) {
       CpuPause ();
     }
-    CpuMpData->InitFlag = ApInitDone;
+    if (OldCpuMpData != NULL) {
+      CpuMpData->InitFlag = ApInitDone;
+    }
     for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
       SetApState (&CpuMpData->CpuData[Index], CpuStateIdle);
     }
-- 
2.23.0.windows.1

Re: [PATCH v2 2/2] UefiCpuPkg/MpInitLib: Avoid ApInitReconfig in PEI.

[Ni, Ray]

> -----Original Message-----
> From: Dong, Eric <eric.dong@intel.com>
> Sent: Wednesday, April 22, 2020 5:01 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
> Ersek <lersek@redhat.com>; Kumar, Chandana C
> <chandana.c.kumar@intel.com>
> Subject: [PATCH v2 2/2] UefiCpuPkg/MpInitLib: Avoid ApInitReconfig in PEI.
> 
> From: "Dong, Eric" <eric.dong@intel.com>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683
> 
> In PEI phase, AP already been waked up through ApInitConfig,
> so it can directly wake up it through change wakup buffer
> instead of use ApInitReconfig flag. It can save some time.
> 
> Change code to only use ApInitReconfig flag in DXE phase
> which must need to update the wake up buffer.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> 
> V2:
> 
> 1. Enhance code to remove CpuMpData->ApLoopMode == ApInHltLoop
> check.

What does this mean for this patch?

Re: [PATCH v2 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.

[Ni, Ray]

Eric,
It's natural to use the volatile registers value derived from BSP in ApInitReconfig path.
So I still prefer to use the code I suggested in the review comment to the v1 patch.

We can remove the below line that specially for ApInitReconfig path in MpInitLbInitialize().
https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpInitLib/MpLib.c#L1783:
      CopyMem (&CpuMpData->CpuData[Index].VolatileRegisters, &VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS));

Thanks,
Ray
> -----Original Message-----
> From: Dong, Eric <eric.dong@intel.com>
> Sent: Wednesday, April 22, 2020 5:01 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar,
> Chandana C <chandana.c.kumar@intel.com>
> Subject: [PATCH v2 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683
> 
> This patch fixes an assertion because AP can't find the CpuMpData.
> When AP is waken up through Init-Sipi-Sipi, AP's IDT should
> be restored to pre-allocated buffer so AP can get the CpuMpData
> through the IDT base address.
> Current code already has logic to handle this when CpuMpData->
> InitFlag is ApInitConfig but misses the logic
> when CpuMpData->InitFlag is ApInitReconfig.
> This patch fixes this gap.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> 
> V2:
> 
> 1. Enhance code comments.
> 
> 
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 64a4c3546e..2e87aa1f06 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -692,6 +692,16 @@ ApWakeupFunction (
>          //
> 
>          RestoreVolatileRegisters (&CpuMpData-
> >CpuData[ProcessorNumber].VolatileRegisters, TRUE);
> 
>        } else {
> 
> +        if (CpuMpData->InitFlag == ApInitReconfig) {
> 
> +          //
> 
> +          // Initialize AP volatile registers in ApInitReconfig path.
> 
> +          // ApInitReconfig happens when:
> 
> +          // 1. AP is re-enabled after it's disabled, in either PEI or DXE phase.
> 
> +          // 2. AP is initialized in DXE phase.
> 
> +          //
> 
> +          RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters,
> FALSE);
> 
> +        }
> 
> +
> 
>          //
> 
>          // The CPU driver might not flush TLB for APs on spot after updating
> 
>          // page attributes. AP in mwait loop mode needs to take care of it
> when
> 
> --
> 2.23.0.windows.1

Re: [edk2-devel] [PATCH v2 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.

[Laszlo Ersek]

Eric,

On 04/22/20 11:01, Dong, Eric wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683

I don't know what patches I'm supposed to review now. You seem to have
sent 4 (four) versions of this patch set to the list, but you didn't
call them {v1, v2, v3, v4}; you called them {v1, v2, v2, v2}. I can only
work off of the dates/times at which you sent the paches.

To make the confusion worse:

- your third (v3?) posting says "0/2" in the cover letter, but I can
only see one patch below it.

- your last (v4?) posting (that I can see) doesn't even have a cover
letter.

(a) [edk2-devel] [PATCH 0/2] UefiCpuPkg/MpInitLib: Fix ASSERT in AP procedure.
    https://edk2.groups.io/g/devel/message/57772

    Date: Wed, 22 Apr 2020 09:34:54 +0800

(b) [edk2-devel] [PATCH v2 0/2] UefiCpuPkg/MpInitLib: Fix ASSERT in AP procedure
    https://edk2.groups.io/g/devel/message/57786

    Date: Wed, 22 Apr 2020 16:45:56 +0800
    (~7 hours after (a))

(c) [edk2-devel] [PATCH v2 0/2] UefiCpuPkg/MpInitLib: Fix ASSERT in AP procedure
    https://edk2.groups.io/g/devel/message/57791

    Date: Wed, 22 Apr 2020 16:52:51 +0800
    (~7 minutes after (b))

(d) [edk2-devel] [PATCH v2 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.
    https://edk2.groups.io/g/devel/message/57794

    Date: Wed, 22 Apr 2020 17:01:10 +0800
    (~8 minutes after (c))

And your latest comment on the bugzilla ticket, at
<https://bugzilla.tianocore.org/show_bug.cgi?id=2683#c2>, seems to
indicate that (b) is what you consider the most up-to-date version. But
you send two versions ((c) and (d)) after that.

I think I won't review any of these patches now.

(I don't even know under which I should send this response.)

Thanks
Laszlo

Re: [edk2-devel] [PATCH v2 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.

[Dong, Eric]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, April 23, 2020 5:41 AM
> To: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Kumar, Chandana C
> <chandana.c.kumar@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2 1/2] UefiCpuPkg/MpInitLib: Restore IDT
> context for APs.
> 
> Eric,
> 
> On 04/22/20 11:01, Dong, Eric wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683
> 
> I don't know what patches I'm supposed to review now. You seem to have
> sent 4 (four) versions of this patch set to the list, but you didn't call them {v1,
> v2, v3, v4}; you called them {v1, v2, v2, v2}. I can only work off of the
> dates/times at which you sent the paches.

Laszlo,

Sorry, It's my mistake to send out error patches. I have send out patch three times:
1) Send v1 patch set.
2) Try to send V2 patch set, but I missed the "V2" tag for the 1/2 and 2/2 patches, just add it to 0/2.
3) only send 1/2 and 2/2 of V2 patch and add "V2" tag on these two patches. 

So please just review the patches which has V2 tag and ignore the other error patches. 

Thanks,
Eric
> 
> To make the confusion worse:
> 
> - your third (v3?) posting says "0/2" in the cover letter, but I can only see one
> patch below it.
> 
> - your last (v4?) posting (that I can see) doesn't even have a cover letter.
> 
> (a) [edk2-devel] [PATCH 0/2] UefiCpuPkg/MpInitLib: Fix ASSERT in AP
> procedure.
>     https://edk2.groups.io/g/devel/message/57772
> 
>     Date: Wed, 22 Apr 2020 09:34:54 +0800
> 
> (b) [edk2-devel] [PATCH v2 0/2] UefiCpuPkg/MpInitLib: Fix ASSERT in AP
> procedure
>     https://edk2.groups.io/g/devel/message/57786
> 
>     Date: Wed, 22 Apr 2020 16:45:56 +0800
>     (~7 hours after (a))
> 
> (c) [edk2-devel] [PATCH v2 0/2] UefiCpuPkg/MpInitLib: Fix ASSERT in AP
> procedure
>     https://edk2.groups.io/g/devel/message/57791
> 
>     Date: Wed, 22 Apr 2020 16:52:51 +0800
>     (~7 minutes after (b))
> 
> (d) [edk2-devel] [PATCH v2 1/2] UefiCpuPkg/MpInitLib: Restore IDT context
> for APs.
>     https://edk2.groups.io/g/devel/message/57794
> 
>     Date: Wed, 22 Apr 2020 17:01:10 +0800
>     (~8 minutes after (c))
> 
> And your latest comment on the bugzilla ticket, at
> <https://bugzilla.tianocore.org/show_bug.cgi?id=2683#c2>, seems to
> indicate that (b) is what you consider the most up-to-date version. But you
> send two versions ((c) and (d)) after that.
> 
> I think I won't review any of these patches now.
> 
> (I don't even know under which I should send this response.)
> 
> Thanks
> Laszlo
> 
> 
> 

Re: [edk2-devel] [PATCH v2 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.

[Laszlo Ersek]

On 04/22/20 11:01, Dong, Eric wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683
> 
> This patch fixes an assertion because AP can't find the CpuMpData.
> When AP is waken up through Init-Sipi-Sipi, AP's IDT should
> be restored to pre-allocated buffer so AP can get the CpuMpData
> through the IDT base address.
> Current code already has logic to handle this when CpuMpData->
> InitFlag is ApInitConfig but misses the logic
> when CpuMpData->InitFlag is ApInitReconfig.
> This patch fixes this gap.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> V2:
> 1. Enhance code comments.
> 
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 64a4c3546e..2e87aa1f06 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -692,6 +692,16 @@ ApWakeupFunction (
>          //
>          RestoreVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, TRUE);
>        } else {
> +        if (CpuMpData->InitFlag == ApInitReconfig) {
> +          //
> +          // Initialize AP volatile registers in ApInitReconfig path.
> +          // ApInitReconfig happens when:
> +          // 1. AP is re-enabled after it's disabled, in either PEI or DXE phase.
> +          // 2. AP is initialized in DXE phase.
> +          //
> +          RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
> +        }
> +
>          //
>          // The CPU driver might not flush TLB for APs on spot after updating
>          // page attributes. AP in mwait loop mode needs to take care of it when
> 

Thanks for spelling out the scenarios when this code path could be taken.

None of both cases seem to apply to OVMF (CPUs are initialized by
CpuMpPei, and I don't think I've ever tested CPI disable/enable). So I
don't expect any regression here.

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

Thanks
Laszlo

Re: [edk2-devel] [PATCH v2 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.

[Laszlo Ersek]

On 04/22/20 13:47, Ni, Ray wrote:
> Eric,
> It's natural to use the volatile registers value derived from BSP in ApInitReconfig path.
> So I still prefer to use the code I suggested in the review comment to the v1 patch.
> 
> We can remove the below line that specially for ApInitReconfig path in MpInitLbInitialize().
> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpInitLib/MpLib.c#L1783:
>       CopyMem (&CpuMpData->CpuData[Index].VolatileRegisters, &VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS));

Sorry I didn't mean to disagree or disregard this feedback.

Laszlo

Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/MpInitLib: Avoid ApInitReconfig in PEI.

[Laszlo Ersek]

On 04/22/20 11:01, Dong, Eric wrote:
> From: "Dong, Eric" <eric.dong@intel.com>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683
> 
> In PEI phase, AP already been waked up through ApInitConfig,
> so it can directly wake up it through change wakup buffer
> instead of use ApInitReconfig flag. It can save some time.
> 
> Change code to only use ApInitReconfig flag in DXE phase
> which must need to update the wake up buffer.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> V2:
> 1. Enhance code to remove CpuMpData->ApLoopMode == ApInHltLoop check.
> 
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 2e87aa1f06..6d3a0ccc72 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1818,7 +1818,14 @@ MpInitLibInitialize (
>    // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
>    //
>    if (CpuMpData->CpuCount > 1) {
> -    CpuMpData->InitFlag = ApInitReconfig;
> +    if (OldCpuMpData != NULL) {
> +      //
> +      // Only needs to use this flag for DXE phase to update the wake up
> +      // buffer. Wakeup buffer allocated in PEI phase is no longer valid
> +      // in DXE.
> +      //
> +      CpuMpData->InitFlag = ApInitReconfig;
> +    }
>      WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
>      //
>      // Wait for all APs finished initialization
> @@ -1826,7 +1833,9 @@ MpInitLibInitialize (
>      while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) {
>        CpuPause ();
>      }
> -    CpuMpData->InitFlag = ApInitDone;
> +    if (OldCpuMpData != NULL) {
> +      CpuMpData->InitFlag = ApInitDone;
> +    }
>      for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
>        SetApState (&CpuMpData->CpuData[Index], CpuStateIdle);
>      }
> 

Hm. The only feedback I could provide for this patch is a regression
test result. However, seeing how Ray has outstanding questions for this
patch, I think I'll delay my testing to the next version (or do it later
for this version, if Ray is ultimately OK with it).

Thanks
Laszlo