Re: [PATCH v2] ArmVirtPkg/ArmVirtXen: Add ACPI support for Virt Xen ARM

[Shannon Zhao <shannon.zhao@linaro.org>]

On 2016年06月23日 21:42, Ard Biesheuvel wrote:
> On 23 June 2016 at 13:31, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> Add ACPI support for Virt Xen ARM and only for aarch64. It gets the
>> ACPI tables through Xen ARM multiboot protocol.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>> Changes since v1:
>> * move the codes into ArmVirtPkg
>> * use FdtClient
>> * don't rely on OvmfPkg/AcpiPlatformDxe/EntryPoint.c while implement own
>>   entry point since it's minor
>> * use compatible string to find the DT node instead of node path
>>
>> If you want to test, the corresponding Xen patches can be fetched from:
>> https://git.linaro.org/people/shannon.zhao/xen.git  domu_acpi_v2
>> ---
>>  ArmVirtPkg/ArmVirtXen.dsc                          |   8 +
>>  ArmVirtPkg/ArmVirtXen.fdf                          |   8 +
>>  ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c | 241 +++++++++++++++++++++
>>  .../XenAcpiPlatformDxe/XenAcpiPlatformDxe.inf      |  49 +++++
>>  4 files changed, 306 insertions(+)
>>  create mode 100644 ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
>>  create mode 100644 ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.inf
>>
>> diff --git a/ArmVirtPkg/ArmVirtXen.dsc b/ArmVirtPkg/ArmVirtXen.dsc
>> index 594ca64..a869986 100644
>> --- a/ArmVirtPkg/ArmVirtXen.dsc
>> +++ b/ArmVirtPkg/ArmVirtXen.dsc
>> @@ -216,3 +216,11 @@
>>
>>    OvmfPkg/XenBusDxe/XenBusDxe.inf
>>    OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
>> +
>> +  #
>> +  # ACPI support
>> +  #
>> +!if $(ARCH) == AARCH64
>> +  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
>> +  ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.inf
>> +!endif
>> diff --git a/ArmVirtPkg/ArmVirtXen.fdf b/ArmVirtPkg/ArmVirtXen.fdf
>> index 13412f9..b1e00e5 100644
>> --- a/ArmVirtPkg/ArmVirtXen.fdf
>> +++ b/ArmVirtPkg/ArmVirtXen.fdf
>> @@ -179,6 +179,14 @@ READ_LOCK_STATUS   = TRUE
>>    INF OvmfPkg/XenBusDxe/XenBusDxe.inf
>>    INF OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
>>
>> +  #
>> +  # ACPI support
>> +  #
>> +!if $(ARCH) == AARCH64
>> +  INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
>> +  INF ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.inf
>> +!endif
>> +
>>  [FV.FVMAIN_COMPACT]
>>  FvAlignment        = 16
>>  ERASE_POLARITY     = 1
>> diff --git a/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c b/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
>> new file mode 100644
>> index 0000000..9258be8
>> --- /dev/null
>> +++ b/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
>> @@ -0,0 +1,241 @@
>> +/** @file
>> +  Xen ARM ACPI Platform Driver using Xen ARM multiboot protocol
>> +
>> +  Copyright (C) 2016, Linaro Ltd. All rights reserved.
>> +
>> +  This program and the accompanying materials
>> +  are licensed and made available under the terms and conditions of the BSD License
>> +  which accompanies this distribution.  The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#include <Library/BaseLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/UefiDriverEntryPoint.h>
>> +
>> +#include <Protocol/AcpiTable.h>
>> +#include <Protocol/FdtClient.h>
>> +
>> +#include <IndustryStandard/Acpi.h>
>> +
>> +EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER  *XenAcpiRsdpStructurePtr = NULL;
>> +
> 
> Does this need to be a global? If yes, please add STATIC, and prefix
> the name with 'm'. Otherwise, move it into InstallXenArmTables ().
> 
Ok, I'll move it to InstallXenArmTables().

>> +/**
>> +  Get the address of Xen ACPI Root System Description Pointer (RSDP)
>> +  structure.
>> +
>> +  @param  RsdpStructurePtr   Return pointer to RSDP structure
>> +
>> +  @return EFI_SUCCESS        Find Xen RSDP structure successfully.
>> +  @return EFI_NOT_FOUND      Don't find Xen RSDP structure.
>> +  @return EFI_ABORTED        Find Xen RSDP structure, but it's not integrated.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +GetXenArmAcpiRsdp (
> 
> ... and here
> 
Ok.

>> +  OUT   EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER   **RsdpPtr
>> +  )
>> +{
>> +  EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER   *RsdpStructurePtr;
>> +  EFI_STATUS                                     Status;
>> +  FDT_CLIENT_PROTOCOL                            *FdtClient;
>> +  CONST UINT64                                   *Reg;
>> +  UINT32                                         RegElemSize, RegSize;
>> +  UINT64                                         RegBase;
>> +  UINT8                                          Sum;
>> +
>> +  RsdpStructurePtr = NULL;
> 
> Please initialize FdtClient to NULL here.
> 
Sure.

>> +  //
>> +  // Get the RSDP structure address from DeviceTree
>> +  //
>> +  Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
> 
> Please add gFdtClientProtocolGuid to the [Depex] section of this module
> 
Ok.

>> +                  (VOID **)&FdtClient);
>> +  ASSERT_EFI_ERROR (Status);
>> +
>> +  Status = FdtClient->FindCompatibleNodeReg (FdtClient, "xen,guest-acpi",
>> +                        (CONST VOID **)&Reg, &RegElemSize, &RegSize);
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((EFI_D_WARN, "%a: No 'xen,guest-acpi' compatible DT node found\n",
>> +      __FUNCTION__));
>> +    return EFI_NOT_FOUND;
>> +  }
>> +
>> +  ASSERT (RegSize == 2 * sizeof (UINT64));
>> +
>> +  RegBase = SwapBytes64(Reg[0]);
>> +  RsdpStructurePtr = (EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER *)RegBase;
>> +
>> +  if (RsdpStructurePtr && RsdpStructurePtr->Revision >= 2) {
>> +    Sum = CalculateSum8 ((CONST UINT8 *)RsdpStructurePtr,
>> +            sizeof (EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER));
>> +    if (Sum != 0) {
>> +      return EFI_ABORTED;
>> +    }
>> +  }
>> +
>> +  *RsdpPtr = RsdpStructurePtr;
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +/**
>> +  Get Xen Acpi tables from the RSDP structure. And installs Xen ACPI tables
>> +  into the RSDT/XSDT using InstallAcpiTable. Some signature of the installed
>> +  ACPI tables are: FACP, APIC, GTDT, DSDT.
>> +
>> +  @param  AcpiProtocol           Protocol instance pointer.
>> +
>> +  @return EFI_SUCCESS            The table was successfully inserted.
>> +  @return EFI_INVALID_PARAMETER  Either AcpiTableBuffer is NULL, TableHandle is
>> +                                 NULL, or AcpiTableBufferSize and the size
>> +                                 field embedded in the ACPI table pointed to
>> +                                 by AcpiTableBuffer are not in sync.
>> +  @return EFI_OUT_OF_RESOURCES   Insufficient resources exist to complete the request.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +InstallXenArmTables (
> 
> ... and here
> 
Ok.

>> +  IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
>> +  )
>> +{
>> +  EFI_STATUS                                       Status;
>> +  UINTN                                            TableHandle;
>> +
>> +  EFI_ACPI_DESCRIPTION_HEADER                      *Xsdt;
>> +  VOID                                             *CurrentTableEntry;
>> +  UINTN                                            CurrentTablePointer;
>> +  EFI_ACPI_DESCRIPTION_HEADER                      *CurrentTable;
>> +  UINTN                                            Index;
>> +  UINTN                                            NumberOfTableEntries;
>> +  EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE        *FadtTable;
>> +  EFI_ACPI_DESCRIPTION_HEADER                      *DsdtTable;
>> +
>> +  FadtTable   = NULL;
>> +  DsdtTable   = NULL;
>> +  TableHandle = 0;
>> +  NumberOfTableEntries = 0;
>> +
>> +  //
>> +  // Try to find Xen ARM ACPI tables
>> +  //
>> +  Status = GetXenArmAcpiRsdp (&XenAcpiRsdpStructurePtr);
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((EFI_D_INFO, "%a: No RSDP table found\n", __FUNCTION__));
>> +    return Status;
>> +  }
>> +
>> +  //
>> +  // If XSDT table is find, just install its tables.
>> +  //
>> +  if (XenAcpiRsdpStructurePtr->XsdtAddress) {
>> +    //
>> +    // Retrieve the addresses of XSDT and
>> +    // calculate the number of its table entries.
>> +    //
>> +    Xsdt = (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN)
>> +             XenAcpiRsdpStructurePtr->XsdtAddress;
>> +    NumberOfTableEntries = (Xsdt->Length -
>> +                             sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
>> +                             sizeof (UINT64);
>> +    //
>> +    // Install ACPI tables found in XSDT.
>> +    //
>> +    for (Index = 0; Index < NumberOfTableEntries; Index++) {
>> +      //
>> +      // Get the table entry from XSDT
>> +      //
>> +      CurrentTableEntry = (VOID *) ((UINT8 *) Xsdt +
>> +                            sizeof (EFI_ACPI_DESCRIPTION_HEADER) +
>> +                            Index * sizeof (UINT64));
>> +      CurrentTablePointer = (UINTN) *(UINT64 *)CurrentTableEntry;
>> +      CurrentTable = (EFI_ACPI_DESCRIPTION_HEADER *) CurrentTablePointer;
>> +
>> +      //
>> +      // Install the XSDT tables
>> +      //
>> +      Status = AcpiProtocol->InstallAcpiTable (
>> +                 AcpiProtocol,
>> +                 CurrentTable,
>> +                 CurrentTable->Length,
>> +                 &TableHandle
>> +                 );
>> +
>> +      if (EFI_ERROR (Status)) {
>> +        return Status;
>> +      }
>> +
>> +      //
>> +      // Get the FACS and DSDT table address from the table FADT
>> +      //
>> +      if (!AsciiStrnCmp ((CHAR8 *) &CurrentTable->Signature, "FACP", 4)) {
>> +        FadtTable = (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *)
>> +                      (UINTN) CurrentTablePointer;
>> +        DsdtTable  = (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN) FadtTable->Dsdt;
>> +      }
>> +    }
>> +  }
>> +
>> +  //
>> +  // Install DSDT table.
>> +  //
>> +  Status = AcpiProtocol->InstallAcpiTable (
>> +             AcpiProtocol,
>> +             DsdtTable,
>> +             DsdtTable->Length,
>> +             &TableHandle
>> +             );
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +STATIC
>> +EFI_ACPI_TABLE_PROTOCOL *
>> +FindAcpiTableProtocol (
>> +  VOID
>> +  )
>> +{
>> +  EFI_STATUS              Status;
>> +  EFI_ACPI_TABLE_PROTOCOL *AcpiTable;
>> +
> 
> The assert below is compiled out in RELEASE build, in which case you
> are returning whatever value was on the stack if LocateProtocol ()
> fails. Even if that should never happen, due to the depex, could you
> please still initialize AcpiTable to NULL here?
> 
Ok, will initialize AcpiTable to NULL.

Thanks a lot for your review comments.

-- 
Shannon

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel