On 03/11/17 10:19, Ard Biesheuvel wrote: > On 11 March 2017 at 08:41, Laszlo Ersek <ler...@redhat.com> wrote: >> On 03/11/17 08:30, Ard Biesheuvel wrote: >>> >>>> On 11 Mar 2017, at 08:21, Laszlo Ersek <ler...@redhat.com> 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 <ard.biesheu...@linaro.org> >>>>>> --- >>>>>> 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 <Library/DebugLib.h> >>>>>> #include <Library/UefiDriverEntryPoint.h> >>>>>> #include <Library/UefiBootServicesTableLib.h> >>>>>> +#include <Library/UefiLib.h> >>>>>> #include <Library/HobLib.h> >>>>>> #include <libfdt.h> >>>>>> >>>>>> @@ -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.
Well, in the longer term, it shouldn't be necessary to convince the arm64 kernel maintainers -- it's enough to convince the firmware providers :) QEMU and libvirt produce ACPI by default, and ArmVirtQemu will stop forwarding DT. > > 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) Correct. > > So I will look into the FADT check on Monday, I agree that is the most > appropriate approach here. > Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel