From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web12.7824.1621405560111979910 for ; Tue, 18 May 2021 23:26:00 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=KeDzFhwd; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1621405559; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=HIpsUgVKaL8dSjfbME84vRtceFjAueV44dGCRAM9tp4=; b=KeDzFhwdCNOxufXQmsxgG2RhsE76/6LBW/QD25thC+30OhTphJa/sIxVnB3nGx+Tj3DLMp msqm/DwblWrnOXrUJhWghra47GkkMp1be7WyMFjjtHUtvr1DeFi3om4pIgCO7Wge5vWL0P JclO7sTZmBEAJSg5APsJ4nn4lUA79zc= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-284-FE4kOheAOTKrhPRksdzjfg-1; Wed, 19 May 2021 02:25:55 -0400 X-MC-Unique: FE4kOheAOTKrhPRksdzjfg-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id C44A4107ACC7; Wed, 19 May 2021 06:25:54 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-238.ams2.redhat.com [10.36.112.238]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 42D8060C04; Wed, 19 May 2021 06:25:53 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 3/5] ArmVirtPkg: enable ACPI for cloud hypervisor To: devel@edk2.groups.io, jianyong.wu@arm.com, ardb+tianocore@kernel.org, sami.mujawar@arm.com Cc: hao.a.wu@intel.com, justin.he@arm.com References: <20210517065032.82423-1-jianyong.wu@arm.com> <20210517065032.82423-4-jianyong.wu@arm.com> From: "Laszlo Ersek" Message-ID: <2e997009-55bd-7648-f817-6dcd1d857408@redhat.com> Date: Wed, 19 May 2021 08:25:51 +0200 MIME-Version: 1.0 In-Reply-To: <20210517065032.82423-4-jianyong.wu@arm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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. > > Signed-off-by: Jianyong Wu > --- > .../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. > > 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.
(3) Missing ARM (C). > +# 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. > + 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? > +# > + > +[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. > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +## > + > +[Defines] > + INF_VERSION = 1.25 > + BASE_NAME = ClhPlatformHasAcpiDtDxe (7) Should be "CloudHvHasAcpiDtDxe". > + 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 > +#include > +#include > +#include > +#include > +#include > +#include (8) File-top comment block missing altogether, including the @file Doxygen directive plus short explanation, ARM (C) notice, "SPDX-License-Identifier". > + > +#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. > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +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 Laszlo