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

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

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, but in this case, the handover state when invoking
the OS is much more important than clean handling in the firmware
itself.


> 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