Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

> On 11 Mar 2017, at 08:21, Laszlo Ersek <lersek@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.biesheuvel@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?

> 
>> 
>>> 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
>> 
>