Re: [RFC PATCH] MdeModulePkg/AcpiTableDxe: inhibit table publication until we have a FADT

[Leif Lindholm <leif.lindholm@linaro.org>]

On Sat, Mar 18, 2017 at 08:58:24PM +0000, Ard Biesheuvel wrote:
> Since commit 78c41ff519b1 ("ArmVirtPkg/FdtClientDxe: make DT table
> installation !ACPI dependent"), we inhibit the installation of the core
> ACPI tables when booting in DT mode, to relieve the OS from having to
> decide which hardware description to use. However, as it turns out,
> in presence of the RamdiskDxe or BGRT drivers, some ACPI tables are
> still registered with the protocol, and published under the ACPI entry
> point configuration tables, ignoring the fact that there is no way the
> OS can boot with only NFIT and BGRT tables present.
> 
> So let's only publish the ACPI tables if we can reasonably assume that
> the tables that the OS requires have been registered with the protocol,
> by checking that we have either FADT1 or FADT3 table (or both) before
> installing the configuration tables.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> 
> This is a counterproposal for Laszlo's series [0], which is technically
> sound, but a bit unwieldy. Instead of making ARM the odd one out, and
> tweaking universal modules via NULL library class resolution, we can
> prevent AcpiTableDxe from publishing the entry points when only NFIT
> or BGRT tables are present (which sounds like a strange thing to do
> in any case)
> 
> I am aware that this still leaves some loose ends when it comes to ordering
> and the ReadyToBoot callback, which I agree we should solve regardless, but
> I hope we can do so without putting the burden on the ARM platforms to
> always carry a set of NULL class libraries to keep sane behavior.
> 
> [0] https://lists.01.org/pipermail/edk2-devel/2017-March/008684.html
> 
>  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> index 4bb848df5203..4c9921f69a8a 100644
> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> @@ -130,6 +130,21 @@ PublishTables (
>    UINT64                    Buffer64;
>  
>    //
> +  // Don't publish anything yet if we don't have a FADT table. This table is
> +  // mandatory for all ACPI compatible OSes, and installing the ACPI entry
> +  // point configuration table without a full set of ACPI tables may confuse
> +  // some OSes (Linux/arm64). (This may happen when the BGRT or ramdisk drivers
> +  // publish their respective ACPI tables without regard for whether ACPI boot
> +  // is currently enabled.)
> +  //
> +  if (AcpiTableInstance->Fadt1 == NULL && AcpiTableInstance->Fadt3 == NULL) {

While I realise that this argument is the weakest it can possibly be
in this source file and function ... this type of late-stage content
inspection feels to me like it breaks the modular design intended.

I guess more importantly, permitting modules to successfully install
tables and then not publish them obstructs useful/helpful handling of
known-to-be-fatal errors.

Or, in short, I would much rather see a solution that makes
AcpiTableDxe unavailable. If there was a way to prevent installation
of alternative implementations with gEfiAcpiTableProtocolGuid, that
would be even better.

Regards,

Leif

> +    DEBUG ((DEBUG_INFO,
> +      "%a: not publishing ACPI tables [yet], no FADT table installed\n",
> +      __FUNCTION__));
> +    return EFI_SUCCESS;
> +  }
> +
> +  //
>    // Reorder tables as some operating systems don't seem to find the
>    // FADT correctly if it is not in the first few entries
>    //
> -- 
> 2.9.3
>