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.
Thanks
Laszlo
>
>> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
>> b/ArmVirtPkg/FdtClientDxe/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
>>
>> [Protocols]
>> gFdtClientProtocolGuid ## PRODUCES
>>
>> [Guids]
>> + gEfiAcpi20TableGuid
>> gEfiEventReadyToBootGuid
>> gFdtHobGuid
>> gFdtTableGuid
>>
>> -[FeaturePcd]
>> - gArmVirtTokenSpaceGuid.PcdPureAcpiBoot
>> -
>> [Depex]
>> TRUE
>>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
