1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH v2 00/16] OvmfPkg: support VCPU hotplug with -D SMM_REQUIRE
  4. View patch

[edk2-devel] [PATCH v2 02/16] UefiCpuPkg/PiSmmCpuDxeSmm: fix S3 Resume for CPU hotplug

Laszlo Ersek posted 16 patches 5 years, 11 months ago
[edk2-devel] [PATCH v2 02/16] UefiCpuPkg/PiSmmCpuDxeSmm: fix S3 Resume for CPU hotplug
Posted by Laszlo Ersek 5 years, 11 months ago 
The "ACPI_CPU_DATA.NumberOfCpus" field is specified as follows, in
"UefiCpuPkg/Include/AcpiCpuData.h" (rewrapped for this commit message):

  //
  // The number of CPUs.  If a platform does not support hot plug CPUs,
  // then this is the number of CPUs detected when the platform is booted,
  // regardless of being enabled or disabled.  If a platform does support
  // hot plug CPUs, then this is the maximum number of CPUs that the
  // platform supports.
  //

The InitializeCpuBeforeRebase() and InitializeCpuAfterRebase() functions
in "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c" try to restore CPU configuration on
the S3 Resume path for *all* CPUs accounted for in
"ACPI_CPU_DATA.NumberOfCpus". This is wrong, as with CPU hotplug, not all
of the possible CPUs may be present at the time of S3 Suspend / Resume.
The symptom is an infinite wait.

Instead, the "mNumberOfCpus" variable should be used, which is properly
maintained through the EFI_SMM_CPU_SERVICE_PROTOCOL implementation (see
SmmAddProcessor(), SmmRemoveProcessor(), SmmCpuUpdate() in
"UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c").

When CPU hotplug is disabled, "mNumberOfCpus" is constant, and equals
"ACPI_CPU_DATA.NumberOfCpus" at all times.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Ray Ni <ray.ni@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---

Notes:
    v2:
    
    - Pick up Ard's Acked-by, which is conditional on approval from Intel
      reviewers on Cc. (I'd like to save Ard the churn of re-acking
      unmodified patches.)

 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index ba5cc0194c2d..1e0840119724 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -597,75 +597,85 @@ PrepareApStartupVector (
 }
 
 /**
   The function is invoked before SMBASE relocation in S3 path to restores CPU status.
 
   The function is invoked before SMBASE relocation in S3 path. It does first time microcode load
   and restores MTRRs for both BSP and APs.
 
 **/
 VOID
 InitializeCpuBeforeRebase (
   VOID
   )
 {
   LoadMtrrData (mAcpiCpuData.MtrrTable);
 
   SetRegister (TRUE);
 
   ProgramVirtualWireMode ();
 
   PrepareApStartupVector (mAcpiCpuData.StartupVector);
 
-  mNumberToFinish = mAcpiCpuData.NumberOfCpus - 1;
+  if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
+    ASSERT (mNumberOfCpus <= mAcpiCpuData.NumberOfCpus);
+  } else {
+    ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);
+  }
+  mNumberToFinish = mNumberOfCpus - 1;
   mExchangeInfo->ApFunction  = (VOID *) (UINTN) InitializeAp;
 
   //
   // Execute code for before SmmBaseReloc. Note: This flag is maintained across S3 boots.
   //
   mInitApsAfterSmmBaseReloc = FALSE;
 
   //
   // Send INIT IPI - SIPI to all APs
   //
   SendInitSipiSipiAllExcludingSelf ((UINT32)mAcpiCpuData.StartupVector);
 
   while (mNumberToFinish > 0) {
     CpuPause ();
   }
 }
 
 /**
   The function is invoked after SMBASE relocation in S3 path to restores CPU status.
 
   The function is invoked after SMBASE relocation in S3 path. It restores configuration according to
   data saved by normal boot path for both BSP and APs.
 
 **/
 VOID
 InitializeCpuAfterRebase (
   VOID
   )
 {
-  mNumberToFinish = mAcpiCpuData.NumberOfCpus - 1;
+  if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
+    ASSERT (mNumberOfCpus <= mAcpiCpuData.NumberOfCpus);
+  } else {
+    ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);
+  }
+  mNumberToFinish = mNumberOfCpus - 1;
 
   //
   // Signal that SMM base relocation is complete and to continue initialization for all APs.
   //
   mInitApsAfterSmmBaseReloc = TRUE;
 
   //
   // Must begin set register after all APs have continue their initialization.
   // This is a requirement to support semaphore mechanism in register table.
   // Because if semaphore's dependence type is package type, semaphore will wait
   // for all Aps in one package finishing their tasks before set next register
   // for all APs. If the Aps not begin its task during BSP doing its task, the
   // BSP thread will hang because it is waiting for other Aps in the same
   // package finishing their task.
   //
   SetRegister (FALSE);
 
   while (mNumberToFinish > 0) {
     CpuPause ();
   }
 }
 
-- 
2.19.1.3.g30247aa5d201



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#54946): https://edk2.groups.io/g/devel/message/54946
Mute This Topic: https://groups.io/mt/71575170/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 02/16] UefiCpuPkg/PiSmmCpuDxeSmm: fix S3 Resume for CPU hotplug
Posted by Dong, Eric 5 years, 11 months ago 
Hi Laszlo,

Thanks for your patch. The change make sense base on the comments in the data structure header file.

I also checked all the code related to this data structure. The inputs for this data structure are CpuS3DataDxe and RegisterCpuFeaturesLib. Both these two drivers not support CPU hot plug feature, so the real inputs for mAcpiCpuData.NumberOfCpus is the enabled CPU number in this system. So before and after your code change, the CPU values are same. But the data structure comments said it can support CPU hot plug, so I agree your code change.

Reviewed-by: Eric Dong <eric.dong@intel.com>

Thanks,
Eric

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
Sent: Thursday, February 27, 2020 6:12 AM
To: edk2-devel-groups-io <devel@edk2.groups.io>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Igor Mammedov <imammedo@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; Ni, Ray <ray.ni@intel.com>
Subject: [edk2-devel] [PATCH v2 02/16] UefiCpuPkg/PiSmmCpuDxeSmm: fix S3 Resume for CPU hotplug

The "ACPI_CPU_DATA.NumberOfCpus" field is specified as follows, in "UefiCpuPkg/Include/AcpiCpuData.h" (rewrapped for this commit message):

  //
  // The number of CPUs.  If a platform does not support hot plug CPUs,
  // then this is the number of CPUs detected when the platform is booted,
  // regardless of being enabled or disabled.  If a platform does support
  // hot plug CPUs, then this is the maximum number of CPUs that the
  // platform supports.
  //

The InitializeCpuBeforeRebase() and InitializeCpuAfterRebase() functions in "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c" try to restore CPU configuration on the S3 Resume path for *all* CPUs accounted for in "ACPI_CPU_DATA.NumberOfCpus". This is wrong, as with CPU hotplug, not all of the possible CPUs may be present at the time of S3 Suspend / Resume.
The symptom is an infinite wait.

Instead, the "mNumberOfCpus" variable should be used, which is properly maintained through the EFI_SMM_CPU_SERVICE_PROTOCOL implementation (see SmmAddProcessor(), SmmRemoveProcessor(), SmmCpuUpdate() in "UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c").

When CPU hotplug is disabled, "mNumberOfCpus" is constant, and equals "ACPI_CPU_DATA.NumberOfCpus" at all times.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Ray Ni <ray.ni@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---

Notes:
    v2:
    
    - Pick up Ard's Acked-by, which is conditional on approval from Intel
      reviewers on Cc. (I'd like to save Ard the churn of re-acking
      unmodified patches.)

 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index ba5cc0194c2d..1e0840119724 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -597,75 +597,85 @@ PrepareApStartupVector (  }
 
 /**
   The function is invoked before SMBASE relocation in S3 path to restores CPU status.
 
   The function is invoked before SMBASE relocation in S3 path. It does first time microcode load
   and restores MTRRs for both BSP and APs.
 
 **/
 VOID
 InitializeCpuBeforeRebase (
   VOID
   )
 {
   LoadMtrrData (mAcpiCpuData.MtrrTable);
 
   SetRegister (TRUE);
 
   ProgramVirtualWireMode ();
 
   PrepareApStartupVector (mAcpiCpuData.StartupVector);
 
-  mNumberToFinish = mAcpiCpuData.NumberOfCpus - 1;
+  if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
+    ASSERT (mNumberOfCpus <= mAcpiCpuData.NumberOfCpus);  } else {
+    ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);  }  
+ mNumberToFinish = mNumberOfCpus - 1;
   mExchangeInfo->ApFunction  = (VOID *) (UINTN) InitializeAp;
 
   //
   // Execute code for before SmmBaseReloc. Note: This flag is maintained across S3 boots.
   //
   mInitApsAfterSmmBaseReloc = FALSE;
 
   //
   // Send INIT IPI - SIPI to all APs
   //
   SendInitSipiSipiAllExcludingSelf ((UINT32)mAcpiCpuData.StartupVector);
 
   while (mNumberToFinish > 0) {
     CpuPause ();
   }
 }
 
 /**
   The function is invoked after SMBASE relocation in S3 path to restores CPU status.
 
   The function is invoked after SMBASE relocation in S3 path. It restores configuration according to
   data saved by normal boot path for both BSP and APs.
 
 **/
 VOID
 InitializeCpuAfterRebase (
   VOID
   )
 {
-  mNumberToFinish = mAcpiCpuData.NumberOfCpus - 1;
+  if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
+    ASSERT (mNumberOfCpus <= mAcpiCpuData.NumberOfCpus);  } else {
+    ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);  }  
+ mNumberToFinish = mNumberOfCpus - 1;
 
   //
   // Signal that SMM base relocation is complete and to continue initialization for all APs.
   //
   mInitApsAfterSmmBaseReloc = TRUE;
 
   //
   // Must begin set register after all APs have continue their initialization.
   // This is a requirement to support semaphore mechanism in register table.
   // Because if semaphore's dependence type is package type, semaphore will wait
   // for all Aps in one package finishing their tasks before set next register
   // for all APs. If the Aps not begin its task during BSP doing its task, the
   // BSP thread will hang because it is waiting for other Aps in the same
   // package finishing their task.
   //
   SetRegister (FALSE);
 
   while (mNumberToFinish > 0) {
     CpuPause ();
   }
 }
 
--
2.19.1.3.g30247aa5d201






-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#55045): https://edk2.groups.io/g/devel/message/55045
Mute This Topic: https://groups.io/mt/71575170/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 02/16] UefiCpuPkg/PiSmmCpuDxeSmm: fix S3 Resume for CPU hotplug
Posted by Laszlo Ersek 5 years, 11 months ago 
On 02/28/20 04:05, Dong, Eric wrote:
> Hi Laszlo,
> 
> Thanks for your patch. The change make sense base on the comments in the data structure header file.
> 
> I also checked all the code related to this data structure. The inputs for this data structure are CpuS3DataDxe and RegisterCpuFeaturesLib. Both these two drivers not support CPU hot plug feature, so the real inputs for mAcpiCpuData.NumberOfCpus is the enabled CPU number in this system. So before and after your code change, the CPU values are same. But the data structure comments said it can support CPU hot plug, so I agree your code change.
> 
> Reviewed-by: Eric Dong <eric.dong@intel.com>

Thank you!

Please allow me one additional comment on CpuS3DataDxe however:

According to the comments in UefiCpuPkg/CpuS3DataDxe:

    [...] this module does not support hot plug CPUs.  This module can
    be copied into a CPU specific package and customized if these
    additional features are required [...]

in the last three patches of the series, I clone CpuS3DataDxe for
OvmfPkg, and extend it for CPU hotplug:

  [edk2-devel] [PATCH v2 14/16]
  OvmfPkg: clone CpuS3DataDxe from UefiCpuPkg

  [edk2-devel] [PATCH v2 15/16]
  OvmfPkg/CpuS3DataDxe: superficial cleanups

  [edk2-devel] [PATCH v2 16/16]
  OvmfPkg/CpuS3DataDxe: enable S3 resume after CPU hotplug

(This extension occurs in a QEMU-specific way -- in other words,
OvmfPkg/CpuS3DataDxe is really a platform driver.)

What I'm trying to say is: the PiSmmCpuDxeSmm changes from the present
patch *are* utilized in a CPU hotplug situation too.

In other words, PiSmmCpuDxeSmm is really exposed to a situation where
the following expression is TRUE:

  (FeaturePcdGet (PcdCpuHotPlugSupport) &&
   mNumberOfCpus < mAcpiCpuData.NumberOfCpus)

It is testable with OVMF, after this series is applied.

Thanks
Laszlo

> 
> Thanks,
> Eric
> 
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Thursday, February 27, 2020 6:12 AM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Igor Mammedov <imammedo@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [edk2-devel] [PATCH v2 02/16] UefiCpuPkg/PiSmmCpuDxeSmm: fix S3 Resume for CPU hotplug
> 
> The "ACPI_CPU_DATA.NumberOfCpus" field is specified as follows, in "UefiCpuPkg/Include/AcpiCpuData.h" (rewrapped for this commit message):
> 
>   //
>   // The number of CPUs.  If a platform does not support hot plug CPUs,
>   // then this is the number of CPUs detected when the platform is booted,
>   // regardless of being enabled or disabled.  If a platform does support
>   // hot plug CPUs, then this is the maximum number of CPUs that the
>   // platform supports.
>   //
> 
> The InitializeCpuBeforeRebase() and InitializeCpuAfterRebase() functions in "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c" try to restore CPU configuration on the S3 Resume path for *all* CPUs accounted for in "ACPI_CPU_DATA.NumberOfCpus". This is wrong, as with CPU hotplug, not all of the possible CPUs may be present at the time of S3 Suspend / Resume.
> The symptom is an infinite wait.
> 
> Instead, the "mNumberOfCpus" variable should be used, which is properly maintained through the EFI_SMM_CPU_SERVICE_PROTOCOL implementation (see SmmAddProcessor(), SmmRemoveProcessor(), SmmCpuUpdate() in "UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c").
> 
> When CPU hotplug is disabled, "mNumberOfCpus" is constant, and equals "ACPI_CPU_DATA.NumberOfCpus" at all times.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> 
> Notes:
>     v2:
>     
>     - Pick up Ard's Acked-by, which is conditional on approval from Intel
>       reviewers on Cc. (I'd like to save Ard the churn of re-acking
>       unmodified patches.)
> 
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index ba5cc0194c2d..1e0840119724 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -597,75 +597,85 @@ PrepareApStartupVector (  }
>
>  /**
>    The function is invoked before SMBASE relocation in S3 path to restores CPU status.
>
>    The function is invoked before SMBASE relocation in S3 path. It does first time microcode load
>    and restores MTRRs for both BSP and APs.
>
>  **/
>  VOID
>  InitializeCpuBeforeRebase (
>    VOID
>    )
>  {
>    LoadMtrrData (mAcpiCpuData.MtrrTable);
>
>    SetRegister (TRUE);
>
>    ProgramVirtualWireMode ();
>
>    PrepareApStartupVector (mAcpiCpuData.StartupVector);
>
> -  mNumberToFinish = mAcpiCpuData.NumberOfCpus - 1;
> +  if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
> +    ASSERT (mNumberOfCpus <= mAcpiCpuData.NumberOfCpus);  } else {
> +    ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);  }
> + mNumberToFinish = mNumberOfCpus - 1;
>    mExchangeInfo->ApFunction  = (VOID *) (UINTN) InitializeAp;
>
>    //
>    // Execute code for before SmmBaseReloc. Note: This flag is maintained across S3 boots.
>    //
>    mInitApsAfterSmmBaseReloc = FALSE;
>
>    //
>    // Send INIT IPI - SIPI to all APs
>    //
>    SendInitSipiSipiAllExcludingSelf ((UINT32)mAcpiCpuData.StartupVector);
>
>    while (mNumberToFinish > 0) {
>      CpuPause ();
>    }
>  }
>
>  /**
>    The function is invoked after SMBASE relocation in S3 path to restores CPU status.
>
>    The function is invoked after SMBASE relocation in S3 path. It restores configuration according to
>    data saved by normal boot path for both BSP and APs.
>
>  **/
>  VOID
>  InitializeCpuAfterRebase (
>    VOID
>    )
>  {
> -  mNumberToFinish = mAcpiCpuData.NumberOfCpus - 1;
> +  if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
> +    ASSERT (mNumberOfCpus <= mAcpiCpuData.NumberOfCpus);  } else {
> +    ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);  }
> + mNumberToFinish = mNumberOfCpus - 1;
>
>    //
>    // Signal that SMM base relocation is complete and to continue initialization for all APs.
>    //
>    mInitApsAfterSmmBaseReloc = TRUE;
>
>    //
>    // Must begin set register after all APs have continue their initialization.
>    // This is a requirement to support semaphore mechanism in register table.
>    // Because if semaphore's dependence type is package type, semaphore will wait
>    // for all Aps in one package finishing their tasks before set next register
>    // for all APs. If the Aps not begin its task during BSP doing its task, the
>    // BSP thread will hang because it is waiting for other Aps in the same
>    // package finishing their task.
>    //
>    SetRegister (FALSE);
>
>    while (mNumberToFinish > 0) {
>      CpuPause ();
>    }
>  }
>
> --
> 2.19.1.3.g30247aa5d201
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#55080): https://edk2.groups.io/g/devel/message/55080
Mute This Topic: https://groups.io/mt/71575170/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 02/16] UefiCpuPkg/PiSmmCpuDxeSmm: fix S3 Resume for CPU hotplug
Posted by Laszlo Ersek 5 years, 11 months ago 
Hi Eric,

On 02/28/20 04:05, Dong, Eric wrote:
> Hi Laszlo,
>
> Thanks for your patch. The change make sense base on the comments in
> the data structure header file.
>
> I also checked all the code related to this data structure. The inputs
> for this data structure are CpuS3DataDxe and RegisterCpuFeaturesLib.
> Both these two drivers not support CPU hot plug feature, so the real
> inputs for mAcpiCpuData.NumberOfCpus is the enabled CPU number in this
> system. So before and after your code change, the CPU values are same.
> But the data structure comments said it can support CPU hot plug, so I
> agree your code change.
>
> Reviewed-by: Eric Dong <eric.dong@intel.com>

while merging this patch, the edk2 CI system reported the following
build warning (in the "Windows VS2019 PR" check):

https://github.com/tianocore/edk2/pull/416/checks?check_run_id=484688972
https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=4568&view=logs&j=898a5c7a-7a49-5be1-c417-92a6761a8039&t=7c50f5c2-8a2c-50f9-5007-e26f12377af8&l=64

> 2020-03-04T11:06:29.7838532Z ERROR - Compiler #2220 from d:\a\1\s\UefiCpuPkg\PiSmmCpuDxeSmm\CpuS3.c(624):   the following warning is treated as an error
> 2020-03-04T11:06:29.7839570Z WARNING - Compiler #4244 from d:\a\1\s\UefiCpuPkg\PiSmmCpuDxeSmm\CpuS3.c(624):   '=': conversion from 'UINTN' to 'volatile UINT32', possible loss of data
> 2020-03-04T11:06:29.7841177Z WARNING - Compiler #4244 from d:\a\1\s\UefiCpuPkg\PiSmmCpuDxeSmm\CpuS3.c(659):   '=': conversion from 'UINTN' to 'volatile UINT32', possible loss of data

I've suppressed that by squashing the following hunks into the patch:

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index 1e0840119724..29e9ba92b453 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -621,7 +621,7 @@ InitializeCpuBeforeRebase (
>    } else {
>      ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);
>    }
> -  mNumberToFinish = mNumberOfCpus - 1;
> +  mNumberToFinish = (UINT32)(mNumberOfCpus - 1);
>    mExchangeInfo->ApFunction  = (VOID *) (UINTN) InitializeAp;
>
>    //
> @@ -656,7 +656,7 @@ InitializeCpuAfterRebase (
>    } else {
>      ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);
>    }
> -  mNumberToFinish = mNumberOfCpus - 1;
> +  mNumberToFinish = (UINT32)(mNumberOfCpus - 1);
>
>    //
>    // Signal that SMM base relocation is complete and to continue initialization for all APs.

Thanks,
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#55408): https://edk2.groups.io/g/devel/message/55408
Mute This Topic: https://groups.io/mt/71575170/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 02/16] UefiCpuPkg/PiSmmCpuDxeSmm: fix S3 Resume for CPU hotplug
Posted by Dong, Eric 5 years, 11 months ago 
Hi Laszlo,

Got it. Go ahead.

Thanks,
Eric

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: Wednesday, March 4, 2020 8:23 PM
To: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Igor Mammedov <imammedo@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; Ni, Ray <ray.ni@intel.com>; Boris Ostrovsky <boris.ostrovsky@oracle.com>
Subject: Re: [edk2-devel] [PATCH v2 02/16] UefiCpuPkg/PiSmmCpuDxeSmm: fix S3 Resume for CPU hotplug

Hi Eric,

On 02/28/20 04:05, Dong, Eric wrote:
> Hi Laszlo,
>
> Thanks for your patch. The change make sense base on the comments in 
> the data structure header file.
>
> I also checked all the code related to this data structure. The inputs 
> for this data structure are CpuS3DataDxe and RegisterCpuFeaturesLib.
> Both these two drivers not support CPU hot plug feature, so the real 
> inputs for mAcpiCpuData.NumberOfCpus is the enabled CPU number in this 
> system. So before and after your code change, the CPU values are same.
> But the data structure comments said it can support CPU hot plug, so I 
> agree your code change.
>
> Reviewed-by: Eric Dong <eric.dong@intel.com>

while merging this patch, the edk2 CI system reported the following build warning (in the "Windows VS2019 PR" check):

https://github.com/tianocore/edk2/pull/416/checks?check_run_id=484688972
https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=4568&view=logs&j=898a5c7a-7a49-5be1-c417-92a6761a8039&t=7c50f5c2-8a2c-50f9-5007-e26f12377af8&l=64

> 2020-03-04T11:06:29.7838532Z ERROR - Compiler #2220 from d:\a\1\s\UefiCpuPkg\PiSmmCpuDxeSmm\CpuS3.c(624):   the following warning is treated as an error
> 2020-03-04T11:06:29.7839570Z WARNING - Compiler #4244 from d:\a\1\s\UefiCpuPkg\PiSmmCpuDxeSmm\CpuS3.c(624):   '=': conversion from 'UINTN' to 'volatile UINT32', possible loss of data
> 2020-03-04T11:06:29.7841177Z WARNING - Compiler #4244 from d:\a\1\s\UefiCpuPkg\PiSmmCpuDxeSmm\CpuS3.c(659):   '=': conversion from 'UINTN' to 'volatile UINT32', possible loss of data

I've suppressed that by squashing the following hunks into the patch:

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index 1e0840119724..29e9ba92b453 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -621,7 +621,7 @@ InitializeCpuBeforeRebase (
>    } else {
>      ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);
>    }
> -  mNumberToFinish = mNumberOfCpus - 1;
> +  mNumberToFinish = (UINT32)(mNumberOfCpus - 1);
>    mExchangeInfo->ApFunction  = (VOID *) (UINTN) InitializeAp;
>
>    //
> @@ -656,7 +656,7 @@ InitializeCpuAfterRebase (
>    } else {
>      ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);
>    }
> -  mNumberToFinish = mNumberOfCpus - 1;
> +  mNumberToFinish = (UINT32)(mNumberOfCpus - 1);
>
>    //
>    // Signal that SMM base relocation is complete and to continue initialization for all APs.

Thanks,
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#55415): https://edk2.groups.io/g/devel/message/55415
Mute This Topic: https://groups.io/mt/71575170/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.