public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: edk2-devel-groups-io <devel@edk2.groups.io>
Cc: "Ard Biesheuvel" <ard.biesheuvel@linaro.org>,
	"Igor Mammedov" <imammedo@redhat.com>,
	"Jiewen Yao" <jiewen.yao@intel.com>,
	"Jordan Justen" <jordan.l.justen@intel.com>,
	"Michael Kinney" <michael.d.kinney@intel.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: [PATCH 16/16] OvmfPkg/CpuS3DataDxe: enable S3 resume after CPU hotplug
Date: Sun, 23 Feb 2020 18:25:37 +0100	[thread overview]
Message-ID: <20200223172537.28464-17-lersek@redhat.com> (raw)
In-Reply-To: <20200223172537.28464-1-lersek@redhat.com>

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>
---
 OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf |  4 +
 OvmfPkg/CpuS3DataDxe/CpuS3Data.c      | 91 ++++++++++++++------
 2 files changed, 70 insertions(+), 25 deletions(-)

diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
index f9679e0c33b3..ceae1d4078c7 100644
--- a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
+++ b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
@@ -16,46 +16,50 @@
 #
 ##
 
 [Defines]
   INF_VERSION                    = 1.29
   BASE_NAME                      = CpuS3DataDxe
   FILE_GUID                      = 229B7EFD-DA02-46B9-93F4-E20C009F94E9
   MODULE_TYPE                    = DXE_DRIVER
   VERSION_STRING                 = 1.0
   ENTRY_POINT                    = CpuS3DataInitialize
 
 # The following information is for reference only and not required by the build
 # tools.
 #
 #  VALID_ARCHITECTURES           = IA32 X64
 
 [Sources]
   CpuS3Data.c
 
 [Packages]
   MdeModulePkg/MdeModulePkg.dec
   MdePkg/MdePkg.dec
+  OvmfPkg/OvmfPkg.dec
   UefiCpuPkg/UefiCpuPkg.dec
 
 [LibraryClasses]
   BaseLib
   BaseMemoryLib
   DebugLib
+  IoLib
   MemoryAllocationLib
   MtrrLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
 
 [Guids]
   gEfiEndOfDxeEventGroupGuid         ## CONSUMES   ## Event
 
 [Protocols]
   gEfiMpServiceProtocolGuid          ## CONSUMES
 
 [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable                    ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                       ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber         ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress                     ## PRODUCES
+  gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase             ## CONSUMES
 
 [Depex]
   gEfiMpServiceProtocolGuid
diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
index 8bb9807cd501..bac7285aa2f3 100644
--- a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
+++ b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
@@ -4,51 +4,55 @@ ACPI CPU Data initialization module
 This module initializes the ACPI_CPU_DATA structure and registers the address
 of this structure in the PcdCpuS3DataAddress PCD.  This is a generic/simple
 version of this module.  It does not provide a machine check handler or CPU
 register initialization tables for ACPI S3 resume.  It also only supports the
 number of CPUs reported by the MP Services Protocol, so 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.
 
 Copyright (c) 2013 - 2017, Intel Corporation. All rights reserved.<BR>
 Copyright (c) 2015 - 2020, Red Hat, Inc.
 
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
 
 #include <PiDxe.h>
 
 #include <AcpiCpuData.h>
 
 #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>
 
 #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
 //
 typedef struct {
   ACPI_CPU_DATA             AcpiCpuData;
   MTRR_SETTINGS             MtrrTable;
   IA32_DESCRIPTOR           GdtrProfile;
   IA32_DESCRIPTOR           IdtrProfile;
 } ACPI_CPU_DATA_EX;
 
 /**
   Allocate EfiACPIMemoryNVS memory.
 
   @param[in] Size   Size of memory to allocate.
 
   @return       Allocated address for output.
 
 **/
 VOID *
 AllocateAcpiNvsMemory (
   IN UINTN  Size
   )
@@ -144,89 +148,101 @@ CpuS3DataOnEndOfDxe (
    to the address that ACPI_CPU_DATA is allocated at.
 
    @param[in] ImageHandle  The firmware allocated handle for the EFI image.
    @param[in] SystemTable  A pointer to the EFI System Table.
 
    @retval EFI_SUCCESS     The entry point is executed successfully.
    @retval EFI_UNSUPPORTED Do not support ACPI S3.
    @retval other           Some error occurs when executing this entry point.
 
 **/
 EFI_STATUS
 EFIAPI
 CpuS3DataInitialize (
   IN EFI_HANDLE        ImageHandle,
   IN EFI_SYSTEM_TABLE  *SystemTable
   )
 {
   EFI_STATUS                 Status;
   ACPI_CPU_DATA_EX           *AcpiCpuDataEx;
   ACPI_CPU_DATA              *AcpiCpuData;
   EFI_MP_SERVICES_PROTOCOL   *MpServices;
   UINTN                      NumberOfCpus;
-  UINTN                      NumberOfEnabledProcessors;
   VOID                       *Stack;
   UINTN                      TableSize;
   CPU_REGISTER_TABLE         *RegisterTable;
   UINTN                      Index;
   EFI_PROCESSOR_INFORMATION  ProcessorInfoBuffer;
   UINTN                      GdtSize;
   UINTN                      IdtSize;
   VOID                       *Gdt;
   VOID                       *Idt;
   EFI_EVENT                  Event;
   ACPI_CPU_DATA              *OldAcpiCpuData;
+  BOOLEAN                    FetchPossibleApicIds;
 
   if (!PcdGetBool (PcdAcpiS3Enable)) {
     return EFI_UNSUPPORTED;
   }
 
   //
   // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA structure
   //
   OldAcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64 (PcdCpuS3DataAddress);
 
   AcpiCpuDataEx = AllocateZeroPages (sizeof (ACPI_CPU_DATA_EX));
   ASSERT (AcpiCpuDataEx != NULL);
   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;
 
   //
   // Initialize ACPI_CPU_DATA fields
   //
   AcpiCpuData->StackSize                 = PcdGet32 (PcdCpuApStackSize);
   AcpiCpuData->ApMachineCheckHandlerBase = 0;
   AcpiCpuData->ApMachineCheckHandlerSize = 0;
   AcpiCpuData->GdtrProfile  = (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDataEx->GdtrProfile;
   AcpiCpuData->IdtrProfile  = (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDataEx->IdtrProfile;
   AcpiCpuData->MtrrTable    = (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDataEx->MtrrTable;
 
   //
   // Allocate stack space for all CPUs.
   // Use ACPI NVS memory type because this data will be directly used by APs
   // in S3 resume phase in long mode. Also during S3 resume, the stack buffer
   // will only be used as scratch space. i.e. we won't read anything from it
   // before we write to it, in PiSmmCpuDxeSmm.
   //
   Stack = AllocateAcpiNvsMemory (NumberOfCpus * AcpiCpuData->StackSize);
   ASSERT (Stack != NULL);
   AcpiCpuData->StackAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)Stack;
@@ -244,58 +260,83 @@ CpuS3DataInitialize (
   IdtSize = AcpiCpuDataEx->IdtrProfile.Limit + 1;
   Gdt = AllocateZeroPages (GdtSize + IdtSize);
   ASSERT (Gdt != NULL);
   Idt = (VOID *)((UINTN)Gdt + GdtSize);
   CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize);
   CopyMem (Idt, (VOID *)AcpiCpuDataEx->IdtrProfile.Base, IdtSize);
   AcpiCpuDataEx->GdtrProfile.Base = (UINTN)Gdt;
   AcpiCpuDataEx->IdtrProfile.Base = (UINTN)Idt;
 
   if (OldAcpiCpuData != NULL) {
     AcpiCpuData->RegisterTable           = OldAcpiCpuData->RegisterTable;
     AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData->PreSmmInitRegisterTable;
     AcpiCpuData->ApLocation              = OldAcpiCpuData->ApLocation;
     CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus, sizeof (CPU_STATUS_INFORMATION));
   } else {
     //
     // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
     //
     TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
     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);
+      UINT32 InitialApicId;
 
-      RegisterTable[Index].InitialApicId      = (UINT32)ProcessorInfoBuffer.ProcessorId;
+      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;
     }
     AcpiCpuData->RegisterTable           = (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
     AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
   }
 
   //
   // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA structure
   //
   Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
   ASSERT_EFI_ERROR (Status);
 
   //
   // Register EFI_END_OF_DXE_EVENT_GROUP_GUID event.
   // The notification function allocates StartupVector and saves MTRRs for ACPI_CPU_DATA
   //
   Status = gBS->CreateEventEx (
                   EVT_NOTIFY_SIGNAL,
                   TPL_CALLBACK,
                   CpuS3DataOnEndOfDxe,
-- 
2.19.1.3.g30247aa5d201


  parent reply	other threads:[~2020-02-23 17:26 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-23 17:25 [PATCH 00/16] OvmfPkg: support VCPU hotplug with -D SMM_REQUIRE Laszlo Ersek
2020-02-23 17:25 ` [PATCH 01/16] MdeModulePkg/PiSmmCore: log SMM image start failure Laszlo Ersek
2020-02-23 17:25 ` [PATCH 02/16] UefiCpuPkg/PiSmmCpuDxeSmm: fix S3 Resume for CPU hotplug Laszlo Ersek
2020-02-23 17:25 ` [PATCH 03/16] OvmfPkg: clone SmmCpuPlatformHookLib from UefiCpuPkg Laszlo Ersek
2020-02-23 17:25 ` [PATCH 04/16] OvmfPkg: enable SMM Monarch Election in PiSmmCpuDxeSmm Laszlo Ersek
2020-02-23 17:25 ` [PATCH 05/16] OvmfPkg: enable CPU hotplug support " Laszlo Ersek
2020-02-23 17:25 ` [PATCH 06/16] OvmfPkg/CpuHotplugSmm: introduce skeleton for CPU Hotplug SMM driver Laszlo Ersek
2020-02-23 17:25 ` [PATCH 07/16] OvmfPkg/CpuHotplugSmm: add hotplug register block helper functions Laszlo Ersek
2020-02-23 17:25 ` [PATCH 08/16] OvmfPkg/CpuHotplugSmm: define the QEMU_CPUHP_CMD_GET_ARCH_ID macro Laszlo Ersek
2020-02-23 17:25 ` [PATCH 09/16] OvmfPkg/CpuHotplugSmm: add function for collecting CPUs with events Laszlo Ersek
2020-02-23 17:25 ` [PATCH 10/16] OvmfPkg/CpuHotplugSmm: collect " Laszlo Ersek
2020-02-23 17:25 ` [PATCH 11/16] OvmfPkg/CpuHotplugSmm: introduce Post-SMM Pen for hot-added CPUs Laszlo Ersek
2020-02-23 17:25 ` [PATCH 12/16] OvmfPkg/CpuHotplugSmm: introduce First SMI Handler " Laszlo Ersek
2020-02-24  9:10   ` [edk2-devel] " Laszlo Ersek
2020-02-26 21:22     ` Laszlo Ersek
2020-02-23 17:25 ` [PATCH 13/16] OvmfPkg/CpuHotplugSmm: complete root MMI handler for CPU hotplug Laszlo Ersek
2020-02-23 17:25 ` [PATCH 14/16] OvmfPkg: clone CpuS3DataDxe from UefiCpuPkg Laszlo Ersek
2020-02-23 17:25 ` [PATCH 15/16] OvmfPkg/CpuS3DataDxe: superficial cleanups Laszlo Ersek
2020-02-23 17:25 ` Laszlo Ersek [this message]
2020-02-24 16:31 ` [PATCH 00/16] OvmfPkg: support VCPU hotplug with -D SMM_REQUIRE Ard Biesheuvel
2020-07-24  6:26 ` [edk2-devel] " Wu, Jiaxin
2020-07-24 16:01   ` Laszlo Ersek
2020-07-29  8:37     ` Wu, Jiaxin
2020-10-01  9:59       ` [ann] " Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200223172537.28464-17-lersek@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox