From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x22b.google.com (mail-io0-x22b.google.com [IPv6:2607:f8b0:4001:c06::22b]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 901A5803B0 for ; Sat, 11 Mar 2017 01:19:38 -0800 (PST) Received: by mail-io0-x22b.google.com with SMTP id z13so61838582iof.2 for ; Sat, 11 Mar 2017 01:19:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=rxUsOyrY4UIWtOs04xzuPEqJ/d3Y/CATENR4LNVSxm4=; b=IS/5U9PYZ2h1fBu0slmXMsPt1vPVZOTVrC3XEKKYRyrLVfuX9VKDDTx6eSx+aEV02O eJnwPFiL1FAL11Ro2nhBUYzt4RyiUjsEtdtkZAFn22xdPdwTBQW9jDnKpXsfwiK49z9k K2p286tNHxSGCL1hKb6knCxFB25sjYMkzhs9A= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=rxUsOyrY4UIWtOs04xzuPEqJ/d3Y/CATENR4LNVSxm4=; b=Ok4Hf3IdJlWGaEbAX7yqYVqldyujUEU+LKHY2iTSWMZQo97pVFj6B+3hBJhgGSPM6E gmUQtnmeit82zviWBkd8QocrqwDMrkPnGN/S1LBHnNckqmu0AvMktZOY887QvHMtAiIC IJsJ4M+LB8zktzB2zPZCGWV5+HFx8sTmgpUSYNdD+UrVCSvjzay7I2lx1YD7No1JKmIz NIikjasmgPh/F5q28/zY2r5iBCTkwMOrOpfxYTyuNgHK4BBfW40Pewrr5lIldqDj23Eg nhqz/oJt37W8LXK1sagf0t16E3iScfsHnu4dwjROLGVByBOWwhokl5n76yZOMYZetxg5 NmmQ== X-Gm-Message-State: AMke39kCE8Ra7BaZYygIxaICJD+WEGPkyecwmbJ1NThp3YxsIiia/MwbK+2FlZbdwm9yv1fwRUZcFgt1KY1FJZPA X-Received: by 10.107.132.155 with SMTP id o27mr19019578ioi.138.1489223977662; Sat, 11 Mar 2017 01:19:37 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Sat, 11 Mar 2017 01:19:37 -0800 (PST) In-Reply-To: <9efa3d90-f6b0-3772-d4ca-5a3ed2546a42@redhat.com> 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> <5A0A8801-DC29-4F20-B9C7-323B7E7D55EA@linaro.org> <9efa3d90-f6b0-3772-d4ca-5a3ed2546a42@redhat.com> From: Ard Biesheuvel Date: Sat, 11 Mar 2017 10:19:37 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Andrew Jones , Leif Lindholm 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 09:19:38 -0000 Content-Type: text/plain; charset=UTF-8 On 11 March 2017 at 08:41, Laszlo Ersek wrote: > On 03/11/17 08:30, Ard Biesheuvel wrote: >> >>> On 11 Mar 2017, at 08:21, Laszlo Ersek wrote: >>> >>>> On 03/11/17 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. >>> >>> ... BTW this is exactly the kind of hard-to-predict unreliability of >>> ReadyToBoot callbacks that I was worried about. The patches that I >>> posted looked for the "etc/table-loader" fw_cfg file, which directly >>> corresponds to the absence of the "-no-acpi" switch. With a ReadyToBoot >>> callback, we depend on a larger, and fuzzier, pile of state. >>> >>> I'm not suggesting that we return to my series -- I think this one can >>> be fixed up by looking for the FADT in particular --, I'd just like if >>> we all grew a healthy aversion and distrust to ReadyToBoot callbacks. >>> Their perceived simplicity is deceptive. >>> >> >> Point taken. But couldn't we still check the existence of that at ReadyToBoot? > > We could, but that would bring back the new INF file for QemuFwCfgLib, > and the explicit constructor call in FdtClientDxe (assuming we continue > to register the callback in FdtClientDxe -- I think it does belong there). > > So that would be worst of both worlds. > Ah, of course. Silly me :-) So my primary concern here, and I am glad we spotted it now, is that the presence of neutered ACPI tables and no DT makes the system unbootable. The existence of such firmware will, in turn, make it even more difficult to convince the arm64 maintainers that ACPI should be preferred over DT by default if both h/w descriptions are available. So IIUC, firmwares the predate this series can never be affected in such a way, right? (Unless you use a PURE_ACPI_BOOT build and pass -no-acpi, in which case you will probably get the neutered tables but you wouldn't have been able to boot in the first place) So I will look into the FADT check on Monday, I agree that is the most appropriate approach here.