From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x232.google.com (mail-wm0-x232.google.com [IPv6:2a00:1450:400c:c09::232]) (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 F11C3803BD for ; Fri, 10 Mar 2017 23:30:55 -0800 (PST) Received: by mail-wm0-x232.google.com with SMTP id t189so9266893wmt.1 for ; Fri, 10 Mar 2017 23:30:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=V5F+ZEWBuQ+6663OA7WYCQ+/cVtDDXr49zsmRR7TRMs=; b=bKIFair9TD/qwYX1v4paQfvMeiSBNMSuQcUS1zjGsHsoGx8DKweVpoDkQ5qr21D/a0 /UP+g2Qxrpiv3JGn3G/3EjNS8j6yVVV3D8QMCAmnM6UGWqKAT6fUD4LhGGAwEzjA1JM/ atC0/lAu8YXDFvYLUwkOCOackyIwjxVAIZXJE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=V5F+ZEWBuQ+6663OA7WYCQ+/cVtDDXr49zsmRR7TRMs=; b=Of6r53XSeXat51U2Ed+qYZ6CYHTRbHxahE/eUgV0vZAwCeGtUATLvvSO/vOajA8Kwa ExgkpeuOHeKu0rsTKWzqpx9AeRk973qns2T4eyLjHRKKVb7dNdWOebn7Ffv1HgHEbqfi CAbLeMYgt6cWVH2oh2w2WZyBUjzec6xiQYJQdvJAv68ZefhXTg2oTR9f26TCHZhD4oUI ROr0NVdXTH55IFtqKVl3cyDfSVnZIe/wviEuGhK5PngDTlTC2nPGM2xQ0pZ6Al5e/h0a XYBzi5fLkw3M5rQJmB8mv5xm34JpZ2WLxG7gJizCZGVPVifFSCybkKB5LSKGtK+Jvly1 nrLA== X-Gm-Message-State: AFeK/H2WdCajbX8YVpqBMyP3ZxWnurCVgcK0vO1WTBoYizs09k7Lm73tI4b5PDw7E/H6MY3/ X-Received: by 10.28.23.66 with SMTP id 63mr2157497wmx.46.1489217454094; Fri, 10 Mar 2017 23:30:54 -0800 (PST) Received: from [100.116.233.70] (31.16.90.92.rev.sfr.net. [92.90.16.31]) by smtp.gmail.com with ESMTPSA id w30sm16338528wrb.5.2017.03.10.23.30.53 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 10 Mar 2017 23:30:53 -0800 (PST) Mime-Version: 1.0 (1.0) From: Ard Biesheuvel X-Mailer: iPhone Mail (14D27) In-Reply-To: Date: Sat, 11 Mar 2017 08:30:53 +0100 Cc: edk2-devel@ml01.01.org, drjones@redhat.com, leif.lindholm@linaro.org Message-Id: <5A0A8801-DC29-4F20-B9C7-323B7E7D55EA@linaro.org> 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> To: Laszlo Ersek 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:30:56 -0000 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable > On 11 Mar 2017, at 08:21, Laszlo Ersek wrote: >=20 >> 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. >>>=20 >>> 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(-) >>>=20 >>> 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, 0x= DF, 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 fe= ature >>> - # PCD is TRUE. Otherwise, the OS is presented with both a DT and an A= CPI >>> - # 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=3DVALUE >>> # >>> DEFINE SECURE_BOOT_ENABLE =3D FALSE >>> - DEFINE PURE_ACPI_BOOT_ENABLE =3D FALSE >>>=20 >>> !include ArmVirtPkg/ArmVirt.dsc.inc >>>=20 >>> @@ -94,10 +93,6 @@ [PcdsFeatureFlag.common] >>> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE >>> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE >>>=20 >>> -!if $(PURE_ACPI_BOOT_ENABLE) =3D=3D TRUE >>> - gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE >>> -!endif >>> - >>> [PcdsFixedAtBuild.common] >>> gArmPlatformTokenSpaceGuid.PcdCoreCount|1 >>> !if $(ARCH) =3D=3D AARCH64 >>> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClie= ntDxe/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 >>>=20 >>> @@ -312,12 +313,16 @@ OnReadyToBoot ( >>> ) >>> { >>> EFI_STATUS Status; >>> + VOID *Table; >>>=20 >>> - if (!FeaturePcdGet (PcdPureAcpiBoot)) { >>> - // >>> - // Only install the FDT as a configuration table if we want to leav= e 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 exposi= ng >>> + // ACPI 2.0 (or later) tables. Note that the legacy ACPI table GUID h= as >>> + // 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 =3D EfiGetSystemConfigurationTable (&gEfiAcpi20TableGuid, &Tab= le); >>> + if (EFI_ERROR (Status) || Table =3D=3D NULL) { >>> Status =3D gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTr= eeBase); >>> ASSERT_EFI_ERROR (Status); >>> } >>=20 >> This code breaks the DT-only ("-no-acpi") boot with boot graphics >> (virtio-gpu) enabled. >>=20 >> Namely, we recently included BootGraphicsResourceTableDxe in the build. >> That driver calls InstallAcpiTable() for BGRT, which in turn causes >> AcpiTableDxe to call InstallConfigurationTable(), via PublishTables(). >>=20 >> 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. >>=20 >> 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. >=20 > ... 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. >=20 > 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. >=20 Point taken. But couldn't we still check the existence of that at ReadyToBoo= t? >=20 >>=20 >>> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf b/ArmVirtPkg/FdtCl= ientDxe/FdtClientDxe.inf >>> index 00017727c32c..9861f41e968b 100644 >>> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf >>> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf >>> @@ -37,17 +37,16 @@ [LibraryClasses] >>> HobLib >>> UefiBootServicesTableLib >>> UefiDriverEntryPoint >>> + UefiLib >>>=20 >>> [Protocols] >>> gFdtClientProtocolGuid ## PRODUCES >>>=20 >>> [Guids] >>> + gEfiAcpi20TableGuid >>> gEfiEventReadyToBootGuid >>> gFdtHobGuid >>> gFdtTableGuid >>>=20 >>> -[FeaturePcd] >>> - gArmVirtTokenSpaceGuid.PcdPureAcpiBoot >>> - >>> [Depex] >>> TRUE >>>=20 >>=20 >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel >>=20 >=20