Re: [edk2-devel] [PATCH v2 3/5] ArmVirtPkg: enable ACPI for cloud hypervisor

["Jianyong Wu" <jianyong.wu@arm.com>]

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, May 19, 2021 2:26 PM
> To: devel@edk2.groups.io; Jianyong Wu <Jianyong.Wu@arm.com>;
> ardb+tianocore@kernel.org; Sami Mujawar <Sami.Mujawar@arm.com>
> Cc: hao.a.wu@intel.com; Justin He <Justin.He@arm.com>
> Subject: Re: [edk2-devel] [PATCH v2 3/5] ArmVirtPkg: enable ACPI for cloud
> hypervisor
>
> On 05/17/21 08:50, Jianyong Wu wrote:
> > The current implementation of PlatformHasAcpiDt is not a common
> > library and is on behalf of qemu. So give a specific version for Cloud
> > Hypervisor here.
> >
> > There is no device like Fw-cfg in qemu in Cloud Hypervisor, so a
> > specific Acpi handler is introduced here.
> >
> > The handler implemented here is in a very simple way:
> > firstly, aquire the Rsdp address from the PCD varaible in the top
> > ".dsc"; secondly, get the Xsdp address from Rsdp structure; thirdly,
> > get the Acpi tables following the Xsdp structrue and install it
>
> (1) Please consider running a spell checker on the commit message ("aquire"
> should be "acquire", "varaible" should be "variable", "structrue" should be
> "structure"). Having this many typos in a short commit message gives the
> patch a rushed vibe.
>
> > one by one.

Thanks for reminder, I will do the spell check before sending out next time.

> >
> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> > ---
> >  .../CloudHvAcpiPlatformDxe.inf                | 51 +++++++++++++
> >  .../CloudHvHasAcpiDtDxe.inf                   | 43 +++++++++++
> >  .../CloudHvAcpiPlatformDxe/CloudHvAcpi.c      | 73
> +++++++++++++++++++
> >  .../CloudHvHasAcpiDtDxe.c                     | 69 ++++++++++++++++++
> >  4 files changed, 236 insertions(+)
> >  create mode 100644
> > ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
> >  create mode 100644
> > ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.inf
> >  create mode 100644 ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
> >  create mode 100644
> > ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.c
>
> (2) Unless there is a specific reason for adding both drivers in the same patch,
> please split them to separate patches.

Ok
>
> >
> > diff --git
> > a/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
> > b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
> > new file mode 100644
> > index 000000000000..63c74e84eb27
> > --- /dev/null
> > +++ b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
> > @@ -0,0 +1,51 @@
> > +## @file
> > +#  OVMF ACPI Platform Driver for Cloud Hypervisor # #  Copyright (c)
> > +2008 - 2014, Intel Corporation. All rights reserved.<BR>
>
> (3) Missing ARM (C).

Yeah, I will add it.

>
> > +#  SPDX-License-Identifier: BSD-2-Clause-Patent # ##
> > +
> > +[Defines]
> > +  INF_VERSION                    = 0x00010005
> > +  BASE_NAME                      = ClhFwCfgAcpiPlatform
>
> (4) This should be "CloudHvAcpiPlatformDxe", matching the basename of the
> INF file.
Yeah,

>
> > +  FILE_GUID                      = 6c76e407-73f2-dc1c-938f-5d6c4691ea93
> > +  MODULE_TYPE                    = DXE_DRIVER
> > +  VERSION_STRING                 = 1.0
> > +  ENTRY_POINT                    = CloudHvAcpiPlatformEntryPoint
> > +
> > +#
> > +# The following information is for reference only and not required by the
> build tools.
> > +#
> > +#  VALID_ARCHITECTURES           = IA32 X64 ARM AARCH64
>
> (5) Do you really want this driver to be used on, say, IA32?
>
No, I will only keep AArch64 here as I'm sure arm32 can use it.

> > +#
> > +
> > +[Sources]
> > +  CloudHvAcpi.c
> > +
> > +[Packages]
> > +  MdePkg/MdePkg.dec
> > +  MdeModulePkg/MdeModulePkg.dec
> > +  OvmfPkg/OvmfPkg.dec
> > +
> > +[LibraryClasses]
> > +  BaseLib
> > +  DebugLib
> > +  MemoryAllocationLib
> > +  OrderedCollectionLib
> > +  UefiBootServicesTableLib
> > +  UefiDriverEntryPoint
> > +
> > +[Protocols]
> > +  gEfiAcpiTableProtocolGuid                     # PROTOCOL ALWAYS_CONSUMED
> > +  gEfiPciIoProtocolGuid                         # PROTOCOL SOMETIMES_CONSUMED
> > +
> > +[Guids]
> > +  gRootBridgesConnectedEventGroupGuid
> > +
> > +[Pcd]
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiRsdpBaseAddress
> > +
> > +[Depex]
> > +  gEfiAcpiTableProtocolGuid
> > diff --git
> > a/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.inf
> > b/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.inf
> > new file mode 100644
> > index 000000000000..f511a4f5064c
> > --- /dev/null
> > +++
> b/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.inf
> > @@ -0,0 +1,43 @@
> > +## @file
> > +# Decide whether the firmware should expose an ACPI- and/or a Device
> > +Tree-based # hardware description to the operating system.
> > +#
> > +# Copyright (c) 2017, Red Hat, Inc.
>
> (6) ARM (C) missing.
Sure,

>
> > +#
> > +# SPDX-License-Identifier: BSD-2-Clause-Patent ##
> > +
> > +[Defines]
> > +  INF_VERSION                    = 1.25
> > +  BASE_NAME                      = ClhPlatformHasAcpiDtDxe
>
> (7) Should be "CloudHvHasAcpiDtDxe".

Ok
>
> > +  FILE_GUID                      = 71fe72f9-6dc1-199d-5054-13b4200ee88d
> > +  MODULE_TYPE                    = DXE_DRIVER
> > +  VERSION_STRING                 = 1.0
> > +  ENTRY_POINT                    = PlatformHasAcpiDt
> > +
> > +[Sources]
> > +  CloudHvHasAcpiDtDxe.c
> > +
> > +[Packages]
> > +  ArmVirtPkg/ArmVirtPkg.dec
> > +  EmbeddedPkg/EmbeddedPkg.dec
> > +  MdeModulePkg/MdeModulePkg.dec
> > +  MdePkg/MdePkg.dec
> > +  OvmfPkg/OvmfPkg.dec
> > +
> > +[LibraryClasses]
> > +  BaseLib
> > +  DebugLib
> > +  PcdLib
> > +  UefiBootServicesTableLib
> > +  UefiDriverEntryPoint
> > +
> > +[Guids]
> > +  gEdkiiPlatformHasAcpiGuid       ## SOMETIMES_PRODUCES ## PROTOCOL
> > +  gEdkiiPlatformHasDeviceTreeGuid ## SOMETIMES_PRODUCES ##
> PROTOCOL
> > +
> > +[Pcd]
> > +  gArmVirtTokenSpaceGuid.PcdForceNoAcpi
> > +
> > +[Depex]
> > +  gEfiVariableArchProtocolGuid
> > diff --git a/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
> > b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
> > new file mode 100644
> > index 000000000000..c2344e7b29fa
> > --- /dev/null
> > +++ b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
> > @@ -0,0 +1,73 @@
> > +#include <Library/BaseLib.h>
> > +#include <Library/MemoryAllocationLib.h> #include
> > +<IndustryStandard/Acpi63.h> #include <Protocol/AcpiTable.h> #include
> > +<Library/UefiBootServicesTableLib.h>
> > +#include <Library/UefiDriverEntryPoint.h> #include
> > +<Library/DebugLib.h>
>
> (8) File-top comment block missing altogether, including the @file Doxygen
> directive plus short explanation, ARM (C) notice, "SPDX-License-Identifier".
>
Yeah, will add it.

> > +
> > +#define ACPI_ENTRY_SIZE 8
> > +#define XSDT_LEN 36
> > +
> > +STATIC
> > +EFI_ACPI_TABLE_PROTOCOL *
> > +FindAcpiTableProtocol (
> > +  VOID
> > +  )
> > +{
> > +  EFI_STATUS              Status;
> > +  EFI_ACPI_TABLE_PROTOCOL *AcpiTable;
> > +
> > +  Status = gBS->LocateProtocol (
> > +                  &gEfiAcpiTableProtocolGuid,
> > +                  NULL,
> > +                  (VOID**)&AcpiTable
> > +                  );
> > +  ASSERT_EFI_ERROR (Status);
> > +  return AcpiTable;
> > +}
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +InstallCloudHvAcpiTables (
> > + IN     EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
> > + )
> > +{
> > +  UINTN InstalledKey, TableSize;
> > +  UINT64 Rsdp, Xsdt, table_offset, PointerValue;
> > +  EFI_STATUS Status = 0;
> > +  int size;
> > +
> > +  Rsdp = PcdGet64 (PcdAcpiRsdpBaseAddress);  Xsdt =
> > + ((EFI_ACPI_6_3_ROOT_SYSTEM_DESCRIPTION_POINTER *)Rsdp)-
> >XsdtAddress;
> > + PointerValue = Xsdt;  table_offset = Xsdt;  size =
> > + ((EFI_ACPI_COMMON_HEADER *)PointerValue)->Length - XSDT_LEN;
> > + table_offset += XSDT_LEN;
> > +
> > +  while(!Status && size > 0) {
> > +    PointerValue = *(UINT64 *)table_offset;
> > +    TableSize = ((EFI_ACPI_COMMON_HEADER *)PointerValue)->Length;
> > +    Status = AcpiProtocol->InstallAcpiTable (AcpiProtocol,
> > +             (VOID *)(UINT64)PointerValue, TableSize,
> > +             &InstalledKey);
> > +    table_offset += ACPI_ENTRY_SIZE;
> > +    size -= ACPI_ENTRY_SIZE;
> > +  }
> > +
> > +  return Status;
> > +}
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +CloudHvAcpiPlatformEntryPoint (
> > +  IN EFI_HANDLE         ImageHandle,
> > +  IN EFI_SYSTEM_TABLE   *SystemTable
> > +  )
> > +{
> > +  EFI_STATUS                         Status;
> > +
> > +  Status = InstallCloudHvAcpiTables (FindAcpiTableProtocol ());
> > +  return Status;
> > +}
> > +
> > diff --git
> > a/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.c
> > b/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.c
> > new file mode 100644
> > index 000000000000..ae07c91f5705
> > --- /dev/null
> > +++
> b/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.c
> > @@ -0,0 +1,69 @@
> > +/** @file
> > +  Decide whether the firmware should expose an ACPI- and/or a Device
> > +Tree-based
> > +  hardware description to the operating system.
> > +
> > +  Copyright (c) 2017, Red Hat, Inc.
>
> (9) ARM (C) missing.
>
Sure

> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> > +
> > +#include <Guid/PlatformHasAcpi.h>
> > +#include <Guid/PlatformHasDeviceTree.h> #include <Library/BaseLib.h>
> > +#include <Library/DebugLib.h> #include <Library/PcdLib.h> #include
> > +<Library/UefiBootServicesTableLib.h>
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +PlatformHasAcpiDt (
> > +  IN EFI_HANDLE       ImageHandle,
> > +  IN EFI_SYSTEM_TABLE *SystemTable
> > +  )
> > +{
> > +  EFI_STATUS           Status;
> > +
> > +  //
> > +  // If we fail to install any of the necessary protocols below, the
> > + OS will be  // unbootable anyway (due to lacking hardware
> > + description), so tolerate no  // errors here.
> > +  //
> > +  if (MAX_UINTN == MAX_UINT64 &&
> > +      !PcdGetBool (PcdForceNoAcpi))
> > +  {
> > +    Status = gBS->InstallProtocolInterface (
> > +                    &ImageHandle,
> > +                    &gEdkiiPlatformHasAcpiGuid,
> > +                    EFI_NATIVE_INTERFACE,
> > +                    NULL
> > +                    );
> > +    if (EFI_ERROR (Status)) {
> > +      goto Failed;
> > +    }
> > +
> > +    return Status;
> > +  }
> > +
> > +  //
> > +  // Expose the Device Tree otherwise.
> > +  //
> > +  Status = gBS->InstallProtocolInterface (
> > +                  &ImageHandle,
> > +                  &gEdkiiPlatformHasDeviceTreeGuid,
> > +                  EFI_NATIVE_INTERFACE,
> > +                  NULL
> > +                  );
> > +  if (EFI_ERROR (Status)) {
> > +    goto Failed;
> > +  }
> > +
> > +  return Status;
> > +
> > +Failed:
> > +  ASSERT_EFI_ERROR (Status);
> > +  CpuDeadLoop ();
> > +  //
> > +  // Keep compilers happy.
> > +  //
> > +  return Status;
> > +}
> >
>
> I've only pointed out what I consider the bare minimum for my ACK; the
> actual logic in the patch will still need an R-b from Ard and/or Leif and/or Sami.
>
Thanks
Jianyong Wu


> Thanks
> Laszlo

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.