OvmfPkg/CpuS3DataDxe: enable S3 resume after CPU hotplug

During normal boot, CpuS3DataDxe allocates

- an empty CPU_REGISTER_TABLE entry in the
  "ACPI_CPU_DATA.PreSmmInitRegisterTable" array, and

- an empty CPU_REGISTER_TABLE entry in the "ACPI_CPU_DATA.RegisterTable"
  array,

for every CPU whose APIC ID CpuS3DataDxe can learn.

Currently EFI_MP_SERVICES_PROTOCOL is used for both determining the number
of CPUs -- the protocol reports the present-at-boot CPU count --, and for
retrieving the APIC IDs of those CPUs.

Consequently, if a CPU is hot-plugged at OS runtime, then S3 resume
breaks. That's because PiSmmCpuDxeSmm will not find the hot-added CPU's
APIC ID associated with any CPU_REGISTER_TABLE object, in the SMRAM copies
of either of the "RegisterTable" and "PreSmmInitRegisterTable" arrays. The
failure to match the hot-added CPU's APIC ID trips the ASSERT() in
SetRegister() [UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c].

If "PcdQ35SmramAtDefaultSmbase" is TRUE, then:

- prepare CPU_REGISTER_TABLE objects for all possible CPUs, not just the
  present-at-boot CPUs (PlatformPei stored the possible CPU count to
  "PcdCpuMaxLogicalProcessorNumber");

- use QEMU_CPUHP_CMD_GET_ARCH_ID for filling in the "InitialApicId" fields
  of the CPU_REGISTER_TABLE objects.

This provides full APIC ID coverage for PiSmmCpuDxeSmm during S3 resume,
accommodating CPUs hot-added at OS runtime.

This patch is best reviewed with

$ git show -b

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
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>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Message-Id: <20200226221156.29589-17-lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Tested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

2 files changed, 71 insertions, 26 deletions

diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
index 8bb9807cd5..bac7285aa2 100644
--- a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
+++ b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
@@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/BaseLib.h>

 #include <Library/BaseMemoryLib.h>

 #include <Library/DebugLib.h>

+#include <Library/IoLib.h>

 #include <Library/MemoryAllocationLib.h>

 #include <Library/MtrrLib.h>

 #include <Library/UefiBootServicesTableLib.h>

@@ -30,6 +31,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Protocol/MpService.h>

 #include <Guid/EventGroup.h>

 

+#include <IndustryStandard/Q35MchIch9.h>

+#include <IndustryStandard/QemuCpuHotplug.h>

+

 //

 // Data structure used to allocate ACPI_CPU_DATA and its supporting structures

 //

@@ -163,7 +167,6 @@ CpuS3DataInitialize (
   ACPI_CPU_DATA              *AcpiCpuData;

   EFI_MP_SERVICES_PROTOCOL   *MpServices;

   UINTN                      NumberOfCpus;

-  UINTN                      NumberOfEnabledProcessors;

   VOID                       *Stack;

   UINTN                      TableSize;

   CPU_REGISTER_TABLE         *RegisterTable;

@@ -175,6 +178,7 @@ CpuS3DataInitialize (
   VOID                       *Idt;

   EFI_EVENT                  Event;

   ACPI_CPU_DATA              *OldAcpiCpuData;

+  BOOLEAN                    FetchPossibleApicIds;

 

   if (!PcdGetBool (PcdAcpiS3Enable)) {

     return EFI_UNSUPPORTED;

@@ -190,24 +194,36 @@ CpuS3DataInitialize (
   AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;

 

   //

-  // Get MP Services Protocol

+  // The "SMRAM at default SMBASE" feature guarantees that

+  // QEMU_CPUHP_CMD_GET_ARCH_ID too is available.

   //

-  Status = gBS->LocateProtocol (

-                  &gEfiMpServiceProtocolGuid,

-                  NULL,

-                  (VOID **)&MpServices

-                  );

-  ASSERT_EFI_ERROR (Status);

+  FetchPossibleApicIds = PcdGetBool (PcdQ35SmramAtDefaultSmbase);

 

-  //

-  // Get the number of CPUs

-  //

-  Status = MpServices->GetNumberOfProcessors (

-                         MpServices,

-                         &NumberOfCpus,

-                         &NumberOfEnabledProcessors

-                         );

-  ASSERT_EFI_ERROR (Status);

+  if (FetchPossibleApicIds) {

+    NumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);

+  } else {

+    UINTN NumberOfEnabledProcessors;

+

+    //

+    // Get MP Services Protocol

+    //

+    Status = gBS->LocateProtocol (

+                    &gEfiMpServiceProtocolGuid,

+                    NULL,

+                    (VOID **)&MpServices

+                    );

+    ASSERT_EFI_ERROR (Status);

+

+    //

+    // Get the number of CPUs

+    //

+    Status = MpServices->GetNumberOfProcessors (

+                           MpServices,

+                           &NumberOfCpus,

+                           &NumberOfEnabledProcessors

+                           );

+    ASSERT_EFI_ERROR (Status);

+  }

   AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;

 

   //

@@ -263,20 +279,45 @@ CpuS3DataInitialize (
     RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize);

     ASSERT (RegisterTable != NULL);

 

+    if (FetchPossibleApicIds) {

+      //

+      // Write a valid selector so that other hotplug registers can be

+      // accessed.

+      //

+      IoWrite32 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CPU_SEL, 0);

+      //

+      // We'll be fetching the APIC IDs.

+      //

+      IoWrite8 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CMD,

+        QEMU_CPUHP_CMD_GET_ARCH_ID);

+    }

     for (Index = 0; Index < NumberOfCpus; Index++) {

-      Status = MpServices->GetProcessorInfo (

-                           MpServices,

-                           Index,

-                           &ProcessorInfoBuffer

-                           );

-      ASSERT_EFI_ERROR (Status);

-

-      RegisterTable[Index].InitialApicId      = (UINT32)ProcessorInfoBuffer.ProcessorId;

+      UINT32 InitialApicId;

+

+      if (FetchPossibleApicIds) {

+        IoWrite32 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CPU_SEL,

+          (UINT32)Index);

+        InitialApicId = IoRead32 (

+                          ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_RW_CMD_DATA);

+      } else {

+        Status = MpServices->GetProcessorInfo (

+                             MpServices,

+                             Index,

+                             &ProcessorInfoBuffer

+                             );

+        ASSERT_EFI_ERROR (Status);

+        InitialApicId = (UINT32)ProcessorInfoBuffer.ProcessorId;

+      }

+

+      DEBUG ((DEBUG_VERBOSE, "%a: Index=%05Lu ApicId=0x%08x\n", __FUNCTION__,

+        (UINT64)Index, InitialApicId));

+

+      RegisterTable[Index].InitialApicId      = InitialApicId;

       RegisterTable[Index].TableLength        = 0;

       RegisterTable[Index].AllocatedSize      = 0;

       RegisterTable[Index].RegisterTableEntry = 0;

 

-      RegisterTable[NumberOfCpus + Index].InitialApicId      = (UINT32)ProcessorInfoBuffer.ProcessorId;

+      RegisterTable[NumberOfCpus + Index].InitialApicId      = InitialApicId;

       RegisterTable[NumberOfCpus + Index].TableLength        = 0;

       RegisterTable[NumberOfCpus + Index].AllocatedSize      = 0;

       RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0;

diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
index f9679e0c33..ceae1d4078 100644
--- a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
+++ b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
@@ -35,12 +35,14 @@
 [Packages]

   MdeModulePkg/MdeModulePkg.dec

   MdePkg/MdePkg.dec

+  OvmfPkg/OvmfPkg.dec

   UefiCpuPkg/UefiCpuPkg.dec

 

 [LibraryClasses]

   BaseLib

   BaseMemoryLib

   DebugLib

+  IoLib

   MemoryAllocationLib

   MtrrLib

   UefiBootServicesTableLib

@@ -55,7 +57,9 @@
 [Pcd]

   gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable                    ## CONSUMES

   gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                       ## CONSUMES

+  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber         ## CONSUMES

   gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress                     ## PRODUCES

+  gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase             ## CONSUMES

 

 [Depex]

   gEfiMpServiceProtocolGuid