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

[Laszlo Ersek <lersek@redhat.com>]

On 03/13/17 21:39, Laszlo Ersek 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.
> 
> 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.

NB, you could use the same approach in firmware for physical system; the
only component you'd have to replace would be AcpiDtSystemDxe. Namely,
it would not depend on fw_cfg, but on an HII setting. It would

(a) install an HII form with a checkbox, so that the knob (stored in a
non-volatile UEFI variable) could be toggled for the next boot, and

(b) in the entry point, fetch the variable, and install either
gAcpiSystemProtocolGuid or gDtSystemProtocolGuid, as appropriate.

With this -- that is, with physical systems / HII -- in mind, I think
that the following components should be introduced under ArmPkg, not
ArmVirtPkg:

- gAcpiSystemProtocolGuid and gDtSystemProtocolGuid
- the AcpiSystemLib plugin (with the DEPEX)

Hm, well, okay, FdtClientDxe is also platform specific, so whatever
driver the physical system uses to expose the DT to the OS would have to
be adapted to gDtSystemProtocolGuid, with a protocol notify.

This sounds good to me (if I may say so about my own idea, sorry). Thoughts?

Thanks,
Laszlo

> 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
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>