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

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

On 13 March 2017 at 20:39, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/13/17 15:59, Ard Biesheuvel wrote:
>> On 13 March 2017 at 14:53, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 03/13/17 11:23, Ard Biesheuvel wrote:
>>>> On 11 March 2017 at 10:26, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>>>> On Sat, Mar 11, 2017 at 08:38:19AM +0100, Laszlo Ersek wrote:
>>>>>>>> "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.
>>>>>>>
>>>>>>> OK, that would get things working again, I suppose. But do we want
>>>>>>> neutered ACPI tables to be exposed at all, even if there is a DT in
>>>>>>> that case to boot from?
>>>>>>
>>>>>> I think the neutered ACPI tables (on -no-acpi) should be fine. The
>>>>>> upstream Linux guest will prefer DT if it is present;
>>>>>
>>>>> Yes, but we've already determined that this situation is suboptimal,
>>>>> which was what triggered this changeset to begin with.
>>>>>
>>>>
>>>> Indeed. Having an ACPI entry point config table installed, but not
>>>> having the essential tables that allow you to boot is a situation that
>>>> we should try very hard to avoid IMO
>>>>
>>>
>>> That requires dynamic disabling of AcpiTableDxe -- so that EFI_ACPI_TABLE_PROTOCOL is not produced, despite the driver being loaded --, and equipping all clients of EFI_ACPI_TABLE_PROTOCOL to fail gracefully when the protocol is not found. Alternatively, let AcpiTableDxe install the protocol, but prevent all clients from calling it.
>>>
>>
>> Not really. We only care about what the OS gets to see, not what the
>> firmware ends up doing internally. So, say, at ReadyToBoot, we could
>> check that the ACPI entry point points to what we need minimally to
>> boot successfully, otherwise we rip it out. This will trigger the
>> recently added change to install the DT if no ACPI entry point was
>> found.
>>
>> Not pretty, I know,
>
> That's an understatement.
>
>> but in this case, the handover state when invoking
>> the OS is much more important than clean handling in the firmware
>> itself.
>
> The goal of the firmware is to boot the OS. Therefore the above argument
> could be used to justify anything and everything at all in the firmware.
>
> Anyway, here's a technical counter-argument too (i.e., against ripping
> out ACPI altogether at ReadyToBoot): the ArmVirtPkg platforms include
>
>   MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
>
> In this driver, in function RamDiskDxeEntryPoint()
> [MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDriver.c], we have a call
> to EfiCreateEventReadyToBootEx(), which registers the RamDiskAcpiCheck()
> callback function.
>
> The leading comment on the RamDiskAcpiCheck() function (rewrapped here
> for readability) is:
>
>   Check whether EFI_ACPI_TABLE_PROTOCOL and EFI_ACPI_SDT_PROTOCOL are
>   produced. If both protocols are produced, publish all the reserved
>   memory type RAM disks to the NVDIMM Firmware Interface Table (NFIT).
>
> We do have both protocols available; see commit d36447418d32
> ("ArmVirtPkg: Add Ramdisk support to ArmVirtPkg platforms", 2016-08-19).
>
> In turn, RamDiskPublishNfit()
> [MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskProtocol.c]:
> - removes the NFIT and installs an updated one, if an NFIT exists to
> begin with,
> - creates both an SSDT (with RamDiskPublishSsdt()) and an NFIT, otherwise.
>
> If RamDiskAcpiCheck() --> RamDiskPublishNfit() run after our callback --
> in which we'd null the ACPI root pointer in the sysconfig table, IIUC
> --, then further use of the ACPI protocols
> - might crash,
> - might transparently undo our nulling,
> - or might even do what we want (that is, "nothing").
>
> To me the behavior looks unpredictable.
>

Talk about understatements :-)

> Based on RamDiskDxe's workings, perhaps it is safe to say that the edk2
> drivers equate the presence of the ACPI protocols to saying "this is an
> ACPI system". If that's indeed the case, perhaps we need to make the
> ACPI protocols *not* appear, dynamically.
>
> Here's an idea how to implement that, cleanly:
>
> (1) Introduce two new (empty / NULL) protocols with GUIDs
> gAcpiSystemProtocolGuid and gDtSystemProtocolGuid.
>
> (2) Introduce a plugin library instance (== no explicit class) that has
> an empty constructor function (returning RETURN_SUCCESS), a DEPEX on
> gAcpiSystemProtocolGuid, and nothing else. Let's call this AcpiSystemLib.
>
> (3) In "ArmVirt.dsc.inc", hook AcpiSystemLib into
> "MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf", via NULL
> class resolution. This will imbue AcpiTableDxe with a DEPEX on
> gAcpiSystemProtocolGuid.
>
> (4) In FdtClientDxe, register a protocol notify callback for
> gDtSystemProtocolGuid. In the callback, install the DT as a sysconfig table.
>
> (5) Introduce a new DXE driver, called AcpiDtSystemDxe, which only
> depends on QemuFwCfgLib (cleanly, again), which in turn makes it depend
> on gFdtClientProtocolGuid. In AcpiDtSystemDxe's entry point, look up the
> "etc/table-loader" fw_cfg file.
>
> If the file exists (don't select or download it! just check the
> existence), install gAcpiSystemProtocolGuid, and exit. That will unblock
> AcpiTableDxe, and everything that depends on the ACPI protocols (via
> depex or protocol notify).
>
> Otherwise, install gDtSystemProtocolGuid in the entry point, and exit.
> That will cause FdtClientDxe to install the sysconfig table.
>
>
> This design takes a few new modules, but:
> - it operates only with valid edk2 tools and means, zero hacks,
> - it depends on no ReadyToBoot callbacks,
> - it needs no modifications for core drivers,
> - it does not try to remove resources once published.
>
> Note that step (5) will turn Xen guests into constantly DT-only systems.
> I don't know if that's appropriate. I guess Xen can expose another knob
> (different from fw_cfg) to make this switch work.
>

I think the desire is to support ACPI on Xen/arm64 hosts, but it does
not seem to me that the current code would do anything useful in that
regard.

> Alternatively, if you want to stick with the status quo for Xen (both
> ACPI and DT), implement XenAcpiDtSystemDxe, which would install *both*
> protocols unconditionally. Then you'll get both DT and ACPI in the Xen
> guest, same as now, and XenAcpiDtSystemDxe would be subject for further
> customization.
>
> If you want me to, I can work on this (I have a downstream RHBZ for the
> feature, so I can justify spending time on this, even in the current
> phase/state of RHEL development). I think I would start with reverting
> patches #3 and #2 in this series.
>
> If you agree with the idea and prefer to work on it yourself, that's
> even better.
>
> What do you think?
>

I like it! There is very little new logic or processing required at
runtime, and everything else is managed by DxeCore's protocol
dispatch.

I will look into this tomorrow.