From: Shannon Zhao <shannon.zhao@linaro.org>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
Shannon Zhao <zhaoshenglong@huawei.com>
Cc: edk2-devel-01 <edk2-devel@lists.01.org>,
Laszlo Ersek <lersek@redhat.com>,
"Huangpeng (Peter)" <peter.huangpeng@huawei.com>,
"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Subject: Re: [PATCH v2] ArmVirtPkg/ArmVirtXen: Add ACPI support for Virt Xen ARM
Date: Thu, 23 Jun 2016 22:27:50 +0800 [thread overview]
Message-ID: <576BF1E6.6060103@linaro.org> (raw)
In-Reply-To: <CAKv+Gu8_RhmX-tK9z6DmU0KRA9O22pdN8HcpOwpoo6q+jmhogw@mail.gmail.com>
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
next prev parent reply other threads:[~2016-06-23 14:27 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1466681480-14500-1-git-send-email-zhaoshenglong@huawei.com>
2016-06-23 13:42 ` [PATCH v2] ArmVirtPkg/ArmVirtXen: Add ACPI support for Virt Xen ARM Ard Biesheuvel
[not found] ` <CAKv+Gu8_RhmX-tK9z6DmU0KRA9O22pdN8HcpOwpoo6q+jmhogw@mail.gmail.com>
2016-06-23 14:27 ` Shannon Zhao [this message]
2016-06-25 2:50 ` Shannon Zhao
[not found] ` <576DF192.4040307@huawei.com>
2016-06-25 3:05 ` [edk2] " Andrew Fish
[not found] ` <62300FCB-B297-4A1E-B984-12FB9AF4311D@apple.com>
2016-06-25 4:46 ` Shannon Zhao
2016-06-23 11:31 Shannon Zhao
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-all 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=576BF1E6.6060103@linaro.org \
--to=shannon.zhao@linaro.org \
--cc=ard.biesheuvel@linaro.org \
--cc=edk2-devel@lists.01.org \
--cc=lersek@redhat.com \
--cc=peter.huangpeng@huawei.com \
--cc=xen-devel@lists.xen.org \
--cc=zhaoshenglong@huawei.com \
/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;
as well as URLs for NNTP newsgroup(s).