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

[Laszlo Ersek <lersek@redhat.com>]

On 03/11/17 10:19, Ard Biesheuvel wrote:
> On 11 March 2017 at 08:41, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 03/11/17 08:30, Ard Biesheuvel wrote:
>>>
>>>> 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?
>>
>> 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