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>

diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
index 8bb9807cd501d4351bd16280ab0f8cf304be79c7..bac7285aa2f350ec37edc27e7f06d11f283f60c9 100644 (file)
--- 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>\r
 #include <Library/BaseMemoryLib.h>\r
 #include <Library/DebugLib.h>\r
 #include <Library/BaseLib.h>\r
 #include <Library/BaseMemoryLib.h>\r
 #include <Library/DebugLib.h>\r

+#include <Library/IoLib.h>\r

 #include <Library/MemoryAllocationLib.h>\r
 #include <Library/MtrrLib.h>\r
 #include <Library/UefiBootServicesTableLib.h>\r
 #include <Library/MemoryAllocationLib.h>\r
 #include <Library/MtrrLib.h>\r
 #include <Library/UefiBootServicesTableLib.h>\r


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

+#include <IndustryStandard/Q35MchIch9.h>\r
+#include <IndustryStandard/QemuCpuHotplug.h>\r
+\r

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


@@ -163,7 +167,6 @@ CpuS3DataInitialize (
   ACPI_CPU_DATA              *AcpiCpuData;\r
   EFI_MP_SERVICES_PROTOCOL   *MpServices;\r
   UINTN                      NumberOfCpus;\r
   ACPI_CPU_DATA              *AcpiCpuData;\r
   EFI_MP_SERVICES_PROTOCOL   *MpServices;\r
   UINTN                      NumberOfCpus;\r

-  UINTN                      NumberOfEnabledProcessors;\r

   VOID                       *Stack;\r
   UINTN                      TableSize;\r
   CPU_REGISTER_TABLE         *RegisterTable;\r
   VOID                       *Stack;\r
   UINTN                      TableSize;\r
   CPU_REGISTER_TABLE         *RegisterTable;\r


@@ -175,6 +178,7 @@ CpuS3DataInitialize (
   VOID                       *Idt;\r
   EFI_EVENT                  Event;\r
   ACPI_CPU_DATA              *OldAcpiCpuData;\r
   VOID                       *Idt;\r
   EFI_EVENT                  Event;\r
   ACPI_CPU_DATA              *OldAcpiCpuData;\r

+  BOOLEAN                    FetchPossibleApicIds;\r

 \r
   if (!PcdGetBool (PcdAcpiS3Enable)) {\r
     return EFI_UNSUPPORTED;\r
 \r
   if (!PcdGetBool (PcdAcpiS3Enable)) {\r
     return EFI_UNSUPPORTED;\r


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

-  // Get MP Services Protocol\r
+  // The "SMRAM at default SMBASE" feature guarantees that\r
+  // QEMU_CPUHP_CMD_GET_ARCH_ID too is available.\r

   //\r
   //\r

-  Status = gBS->LocateProtocol (\r
-                  &gEfiMpServiceProtocolGuid,\r
-                  NULL,\r
-                  (VOID **)&MpServices\r
-                  );\r
-  ASSERT_EFI_ERROR (Status);\r
+  FetchPossibleApicIds = PcdGetBool (PcdQ35SmramAtDefaultSmbase);\r

 \r
 \r

-  //\r
-  // Get the number of CPUs\r
-  //\r
-  Status = MpServices->GetNumberOfProcessors (\r
-                         MpServices,\r
-                         &NumberOfCpus,\r
-                         &NumberOfEnabledProcessors\r
-                         );\r
-  ASSERT_EFI_ERROR (Status);\r
+  if (FetchPossibleApicIds) {\r
+    NumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);\r
+  } else {\r
+    UINTN NumberOfEnabledProcessors;\r
+\r
+    //\r
+    // Get MP Services Protocol\r
+    //\r
+    Status = gBS->LocateProtocol (\r
+                    &gEfiMpServiceProtocolGuid,\r
+                    NULL,\r
+                    (VOID **)&MpServices\r
+                    );\r
+    ASSERT_EFI_ERROR (Status);\r
+\r
+    //\r
+    // Get the number of CPUs\r
+    //\r
+    Status = MpServices->GetNumberOfProcessors (\r
+                           MpServices,\r
+                           &NumberOfCpus,\r
+                           &NumberOfEnabledProcessors\r
+                           );\r
+    ASSERT_EFI_ERROR (Status);\r
+  }\r

   AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;\r
 \r
   //\r
   AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;\r
 \r
   //\r


@@ -263,20 +279,45 @@ CpuS3DataInitialize (
     RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize);\r
     ASSERT (RegisterTable != NULL);\r
 \r
     RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize);\r
     ASSERT (RegisterTable != NULL);\r
 \r

+    if (FetchPossibleApicIds) {\r
+      //\r
+      // Write a valid selector so that other hotplug registers can be\r
+      // accessed.\r
+      //\r
+      IoWrite32 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CPU_SEL, 0);\r
+      //\r
+      // We'll be fetching the APIC IDs.\r
+      //\r
+      IoWrite8 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CMD,\r
+        QEMU_CPUHP_CMD_GET_ARCH_ID);\r
+    }\r

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

-      Status = MpServices->GetProcessorInfo (\r
-                           MpServices,\r
-                           Index,\r
-                           &ProcessorInfoBuffer\r
-                           );\r
-      ASSERT_EFI_ERROR (Status);\r
-\r
-      RegisterTable[Index].InitialApicId      = (UINT32)ProcessorInfoBuffer.ProcessorId;\r
+      UINT32 InitialApicId;\r
+\r
+      if (FetchPossibleApicIds) {\r
+        IoWrite32 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CPU_SEL,\r
+          (UINT32)Index);\r
+        InitialApicId = IoRead32 (\r
+                          ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_RW_CMD_DATA);\r
+      } else {\r
+        Status = MpServices->GetProcessorInfo (\r
+                             MpServices,\r
+                             Index,\r
+                             &ProcessorInfoBuffer\r
+                             );\r
+        ASSERT_EFI_ERROR (Status);\r
+        InitialApicId = (UINT32)ProcessorInfoBuffer.ProcessorId;\r
+      }\r
+\r
+      DEBUG ((DEBUG_VERBOSE, "%a: Index=%05Lu ApicId=0x%08x\n", __FUNCTION__,\r
+        (UINT64)Index, InitialApicId));\r
+\r
+      RegisterTable[Index].InitialApicId      = InitialApicId;\r

       RegisterTable[Index].TableLength        = 0;\r
       RegisterTable[Index].AllocatedSize      = 0;\r
       RegisterTable[Index].RegisterTableEntry = 0;\r
 \r
       RegisterTable[Index].TableLength        = 0;\r
       RegisterTable[Index].AllocatedSize      = 0;\r
       RegisterTable[Index].RegisterTableEntry = 0;\r
 \r

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

       RegisterTable[NumberOfCpus + Index].TableLength        = 0;\r
       RegisterTable[NumberOfCpus + Index].AllocatedSize      = 0;\r
       RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0;\r
       RegisterTable[NumberOfCpus + Index].TableLength        = 0;\r
       RegisterTable[NumberOfCpus + Index].AllocatedSize      = 0;\r
       RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0;\r

diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
index f9679e0c33b3f1aa3ed1fc7a2ca49d45fae3e1ba..ceae1d4078c79793f65f8d9a7657ddc798eb87ef 100644 (file)
--- a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
+++ b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
@@ -35,12 +35,14 @@
 [Packages]\r
   MdeModulePkg/MdeModulePkg.dec\r
   MdePkg/MdePkg.dec\r
 [Packages]\r
   MdeModulePkg/MdeModulePkg.dec\r
   MdePkg/MdePkg.dec\r

+  OvmfPkg/OvmfPkg.dec\r

   UefiCpuPkg/UefiCpuPkg.dec\r
 \r
 [LibraryClasses]\r
   BaseLib\r
   BaseMemoryLib\r
   DebugLib\r
   UefiCpuPkg/UefiCpuPkg.dec\r
 \r
 [LibraryClasses]\r
   BaseLib\r
   BaseMemoryLib\r
   DebugLib\r

+  IoLib\r

   MemoryAllocationLib\r
   MtrrLib\r
   UefiBootServicesTableLib\r
   MemoryAllocationLib\r
   MtrrLib\r
   UefiBootServicesTableLib\r


@@ -55,7 +57,9 @@
 [Pcd]\r
   gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable                    ## CONSUMES\r
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                       ## CONSUMES\r
 [Pcd]\r
   gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable                    ## CONSUMES\r
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                       ## CONSUMES\r

+  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber         ## CONSUMES\r

   gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress                     ## PRODUCES\r
   gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress                     ## PRODUCES\r

+  gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase             ## CONSUMES\r

 \r
 [Depex]\r
   gEfiMpServiceProtocolGuid\r
 \r
 [Depex]\r
   gEfiMpServiceProtocolGuid\r