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

[Laszlo Ersek <lersek@redhat.com>]

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.

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.

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?

Thanks
Laszlo

> 
> 
>> We've done something like in the past with PcdAcpiS3Enable. It required IntelFrameworkModulePkg, MdeModulePkg and UefiCpuPkg changes.
>>
>> $ git grep -l -w InstallAcpiTable
>>
>> ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/AcpiTables.c
>> ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
>> EdkCompatibilityPkg/Foundation/Efi/Protocol/AcpiTable/AcpiTable.h
>> EmbeddedPkg/Library/AcpiLib/AcpiLib.c
>> IntelFrameworkModulePkg/Universal/Acpi/AcpiSupportDxe/AcpiSupportAcpiSupportProtocol.c
>> MdeModulePkg/Universal/Acpi/AcpiPlatformDxe/AcpiPlatform.c
>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c
>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.h
>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>> MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c
>> MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c
>> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskProtocol.c
>> MdeModulePkg/Universal/Network/IScsiDxe/IScsiIbft.c
>> MdePkg/Include/Protocol/AcpiSystemDescriptionTable.h
>> MdePkg/Include/Protocol/AcpiTable.h
>> NetworkPkg/IScsiDxe/IScsiIbft.c
>> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
>> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>> OvmfPkg/AcpiPlatformDxe/Qemu.c
>> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>> OvmfPkg/AcpiPlatformDxe/Xen.c
>> QuarkPlatformPkg/Acpi/Dxe/AcpiPlatform/AcpiPlatform.c
>> QuarkPlatformPkg/Acpi/DxeSmm/SmmPowerManagement/Ppm.c
>> SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c
>> SecurityPkg/Tcg/TcgDxe/TcgDxe.c
>> SecurityPkg/Tcg/TcgSmm/TcgSmm.c
>> SecurityPkg/Tcg/TrEEDxe/TrEEDxe.c
>> SecurityPkg/Tcg/TrEESmm/TrEESmm.c
>> UefiCpuPkg/PiSmmCommunication/PiSmmCommunicationSmm.c
>>
>> (Some of these hits don't belong to EFI_ACPI_TABLE_PROTOCOL clients, of course.)
>>
>> Thanks
>> Laszlo