| Message ID | 20170318205824.13326-1-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
In fact, I prefer the Laszlo's proposal to prevent the production of EFI_ACPI_TABLE_PROTOCOL by NULL library class, but not the special handling in core module AcpiTableDxe. Then AcpiTableDxe could be not run(by adding DEPEX in NULL library class) or be unloaded(by returning unsuccessful in constructor of NULL library class), and the modules or codes depend on EFI_ACPI_TABLE_PROTOCOL could be not run. Thanks, Star -----Original Message----- From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] Sent: Sunday, March 19, 2017 4:58 AM To: edk2-devel@lists.01.org; lersek@redhat.com; leif.lindholm@linaro.org; Zeng, Star <star.zeng@intel.com> Cc: Tian, Feng <feng.tian@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org> Subject: [RFC PATCH] MdeModulePkg/AcpiTableDxe: inhibit table publication until we have a FADT 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(+) -- 2.9.3 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-develdiff --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) { + 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 //
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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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) { + 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 //
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(+) -- 2.9.3 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel