From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 26D9E803BF for ; Fri, 10 Mar 2017 23:38:23 -0800 (PST) Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B216785545; Sat, 11 Mar 2017 07:38:23 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-18.phx2.redhat.com [10.3.116.18]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v2B7cKI7021074; Sat, 11 Mar 2017 02:38:21 -0500 To: Ard Biesheuvel References: <1489075441-23745-1-git-send-email-ard.biesheuvel@linaro.org> <1489075441-23745-4-git-send-email-ard.biesheuvel@linaro.org> <19f70c3d-4220-957b-24cd-bd2d41e45909@redhat.com> Cc: "edk2-devel@lists.01.org" , Andrew Jones , Leif Lindholm From: Laszlo Ersek Message-ID: <5bdc3cc1-b292-8da3-f858-83c752016941@redhat.com> Date: Sat, 11 Mar 2017 08:38:19 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Sat, 11 Mar 2017 07:38:23 +0000 (UTC) Subject: Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 11 Mar 2017 07:38:23 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 03/11/17 07:57, Ard Biesheuvel wrote: > On 11 March 2017 at 06:55, Laszlo Ersek wrote: >> On 03/09/17 17:04, Ard Biesheuvel wrote: >>> Instead of having a build time switch to prevent the FDT configuration >>> table from being installed, make this behavior dependent on whether we >>> are passing ACPI tables to the OS. This is done by looking for the >>> ACPI 2.0 configuration table, and only installing the FDT one if the >>> ACPI one cannot be found. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Ard Biesheuvel >>> --- >>> ArmVirtPkg/ArmVirtPkg.dec | 10 ---------- >>> ArmVirtPkg/ArmVirtQemu.dsc | 5 ----- >>> ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 15 ++++++++++----- >>> ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf | 5 ++--- >>> 4 files changed, 12 insertions(+), 23 deletions(-) >>> >>> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec >>> index a5ec42166445..efe83a383d55 100644 >>> --- a/ArmVirtPkg/ArmVirtPkg.dec >>> +++ b/ArmVirtPkg/ArmVirtPkg.dec >>> @@ -58,13 +58,3 @@ [PcdsFixedAtBuild, PcdsPatchableInModule] >>> # EFI_VT_100_GUID. >>> # >>> gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007 >>> - >>> -[PcdsFeatureFlag] >>> - # >>> - # Pure ACPI boot >>> - # >>> - # Inhibit installation of the FDT as a configuration table if this feature >>> - # PCD is TRUE. Otherwise, the OS is presented with both a DT and an ACPI >>> - # description of the platform, and it is up to the OS to choose. >>> - # >>> - gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|FALSE|BOOLEAN|0x0000000a >>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc >>> index 477dfdcfc764..7b266b98b949 100644 >>> --- a/ArmVirtPkg/ArmVirtQemu.dsc >>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc >>> @@ -34,7 +34,6 @@ [Defines] >>> # -D FLAG=VALUE >>> # >>> DEFINE SECURE_BOOT_ENABLE = FALSE >>> - DEFINE PURE_ACPI_BOOT_ENABLE = FALSE >>> >>> !include ArmVirtPkg/ArmVirt.dsc.inc >>> >>> @@ -94,10 +93,6 @@ [PcdsFeatureFlag.common] >>> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE >>> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE >>> >>> -!if $(PURE_ACPI_BOOT_ENABLE) == TRUE >>> - gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE >>> -!endif >>> - >>> [PcdsFixedAtBuild.common] >>> gArmPlatformTokenSpaceGuid.PcdCoreCount|1 >>> !if $(ARCH) == AARCH64 >>> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c >>> index 0327af5739f2..2981977f3d20 100644 >>> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c >>> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c >>> @@ -17,6 +17,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> #include >>> #include >>> >>> @@ -312,12 +313,16 @@ OnReadyToBoot ( >>> ) >>> { >>> EFI_STATUS Status; >>> + VOID *Table; >>> >>> - if (!FeaturePcdGet (PcdPureAcpiBoot)) { >>> - // >>> - // Only install the FDT as a configuration table if we want to leave it up >>> - // to the OS to decide whether it prefers ACPI over DT. >>> - // >>> + // >>> + // Only install the FDT as a configuration table if we are not exposing >>> + // ACPI 2.0 (or later) tables. Note that the legacy ACPI table GUID has >>> + // no meaning on ARM since we need at least ACPI 5.0 support, and the >>> + // 64-bit ACPI 2.0 table GUID is mandatory in that case. >>> + // >>> + Status = EfiGetSystemConfigurationTable (&gEfiAcpi20TableGuid, &Table); >>> + if (EFI_ERROR (Status) || Table == NULL) { >>> Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTreeBase); >>> ASSERT_EFI_ERROR (Status); >>> } >> >> This code breaks the DT-only ("-no-acpi") boot with boot graphics >> (virtio-gpu) enabled. >> >> Namely, we recently included BootGraphicsResourceTableDxe in the build. >> That driver calls InstallAcpiTable() for BGRT, which in turn causes >> AcpiTableDxe to call InstallConfigurationTable(), via PublishTables(). >> >> We all missed that just because QEMU doesn't produce ACPI payload (and >> we consequently don't install it), other drivers in edk2 may >> unconditionally install "auxiliary" ACPI tables, which minimally trigger >> the presence of the RSD PTR, RSDT and XSDT, and prevent DT installation. >> Such a crippled set of ACPI tables isn't sufficient for booting however. >> >> We have functions like FindAcpiFacsTableByAcpiGuid(), ScanTableInXSDT() >> and ScanTableInRSDT() in >> "MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c". I >> think the above check should be reworked to look for the FADT >> (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) with code lifted >> from these helper functions. No driver outside of >> QemuFwCfgAcpiPlatformDxe will install the FADT. And, the FADT will >> always be part of QEMU's ACPI payload, if it generates one. >> > > OK, that would get things working again, I suppose. But do we want > neutered ACPI tables to be exposed at all, even if there is a DT in > that case to boot from? I think the neutered ACPI tables (on -no-acpi) should be fine. The upstream Linux guest will prefer DT if it is present; it won't look at the crippled ACPI tables. And if we only provide ACPI, then the tables won't be crippled. And for Linux guests that favor ACPI over DT -- well, just don't boot them with -no-acpi. ( It would be hard to eliminate any and all ACPI tables on "-no-acpi" -- it could be necessary to modify individual / independent drivers that install tables. Excluding AcpiTableDxe (the one that produces EFI_ACPI_TABLE_PROTOCOL) dynamically, or causing it to exit early, would be messy. For some drivers, it could have the intended effect (if the driver handles the protocol's absence gracefully), while some other drivers (that have a depex on the protocol) wouldn't even load (possibly causing further problems). I think this just goes on to show how broken an ACPI-less UEFI setup is, in the first place. ) Laszlo