Re: [PATCH v2 16/16] OvmfPkg/CpuS3DataDxe: enable S3 resume after CPU hotplug

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Wed, 26 Feb 2020 at 23:12, Laszlo Ersek <lersek@redhat.com> wrote:
>
> 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>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-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.)
>
>  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
>