On 02/16/15 03:06, Jordan Justen wrote:
> if DownloadAndInstallTables EarlyTablesOnly is TRUE:
(somewhat unfinished English :))
> Publish FACS and FACP tables
> else
> Publish all tables othere than FACS and FACP
"other[s]", typo
also, what about the stuff that we never install anyway, RSDT, XSDT?
> We immediately call DownloadAndInstallTables with EarlyTablesOnly set
> to TRUE, and then FALSE in this patch, so all of the tables will still
> be published at the driver entry point.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jordan Justen <jordan.l.jus...@intel.com>
> ---
> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 55
> ++++++++++++++++++++++++++++-----
> 1 file changed, 47 insertions(+), 8 deletions(-)
>
> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> index 15e1bc2..b3be65d 100644
> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> @@ -468,9 +468,18 @@ Process2ndPassCmdAddPointer (
> Facs->Length <= Blob2Remaining &&
> Facs->Signature ==
> EFI_ACPI_1_0_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
> - DEBUG ((EFI_D_VERBOSE, "found \"%-4.4a\" size 0x%x; installing\n",
> + DEBUG ((EFI_D_VERBOSE, "found \"%-4.4a\" size 0x%x; ",
> (CONST CHAR8 *)&Facs->Signature, Facs->Length));
> TableSize = Facs->Length;
> + if (!EarlyTablesOnly) {
> + //
> + // The early tables were already installed.
> + //
> + DEBUG ((EFI_D_VERBOSE, "already installed\n"));
> + return EFI_SUCCESS;
> + } else {
> + DEBUG ((EFI_D_VERBOSE, "installing\n"));
> + }
Please drop the "else"; the last statement in the "if" is a return.
You could also move the "TableSize = Facs->Length" assignment below the return.
> }
> }
>
> @@ -492,15 +501,41 @@ Process2ndPassCmdAddPointer (
> (CONST CHAR8 *)&Header->Signature, Header->Length));
> TableSize = Header->Length;
>
> - //
> - // Skip RSDT and XSDT because those are handled by
> - // EFI_ACPI_TABLE_PROTOCOL automatically.
> - if (Header->Signature ==
> - EFI_ACPI_1_0_ROOT_SYSTEM_DESCRIPTION_TABLE_SIGNATURE ||
> - Header->Signature ==
> -
> EFI_ACPI_2_0_EXTENDED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
> + switch (Header->Signature) {
> + case EFI_ACPI_1_0_ROOT_SYSTEM_DESCRIPTION_TABLE_SIGNATURE:
> + case EFI_ACPI_2_0_EXTENDED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE:
> + //
> + // Skip RSDT and XSDT because those are handled by
> + // EFI_ACPI_TABLE_PROTOCOL automatically.
> + //
> DEBUG ((EFI_D_VERBOSE, "skipping\n"));
> return EFI_SUCCESS;
> + case EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE:
> + //
> + // Skip RSDT and XSDT because those are handled by
> + // EFI_ACPI_TABLE_PROTOCOL automatically.
> + //
Stale comment.
> + if (!EarlyTablesOnly) {
> + //
> + // The early tables were already installed.
> + //
> + DEBUG ((EFI_D_VERBOSE, "already installed\n"));
> + return EFI_SUCCESS;
> + }
> + break;
> + default:
> + //
> + // This table is in the set of tables that we install later.
> + //
> + if (EarlyTablesOnly) {
> + //
> + // We are only installing the early tables at this point, so skip
> + // installing this table for now.
> + //
> + DEBUG ((EFI_D_VERBOSE, "skipping for now\n"));
> + return EFI_SUCCESS;
> + }
> + break;
> }
> }
>
> @@ -736,6 +771,10 @@ InstallQemuFwCfgTables (
> if (EFI_ERROR (Status)) {
> return Status;
> }
> + Status = DownloadAndInstallTables (AcpiProtocol, FALSE);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
>
> return Status;
> }
>
I'm afraid this will not work. Please see my original patch:
https://github.com/tianocore/edk2/commit/66b280df28
Commit message excerpt:
commit 66b280df282ae82888d2eb416bfeda3f65afa386
Author: Laszlo Ersek <ler...@redhat.com>
Date: Thu Nov 20 09:58:28 2014 +0000
OvmfPkg: AcpiPlatformDxe: make dependency on PCI enumeration explicit
The ACPI payload that OVMF downloads from QEMU via fw_cfg depends on the
PCI enumaration and resource assignment performed by
MdeModulePkg/Bus/Pci/PciBusDxe.
Namely, although the ACPI payload is pre-generated in qemu during machine
initialization, in
main() [vl.c]
qemu_run_machine_init_done_notifiers()
pc_guest_info_machine_done() [hw/i386/pc.c]
acpi_setup() [hw/i386/acpi-build.c]
acpi_build()
acpi_add_rom_blob()
rom_add_blob(... acpi_build_update ...) [hw/core/loader.c]
fw_cfg_add_file_callback() [hw/nvram/fw_cfg.c]
the ACPI data is rebuilt at the *first time* *any* of the related fw_cfg
files
are read, through the acpi_build_update() fw_cfg read-callback function:
fw_cfg_read() [hw/nvram/fw_cfg.c]
acpi_build_update() [hw/i386/acpi-build.c]
acpi_build()
(See qemu commit d87072ceeccf4f84a64d4bc59124bcd64286c070 and its
containing series.)
For this reason we must not dispatch AcpiPlatformDxe before PciBusDxe
completes the enumeration.
(Emphasis added here.)
The first pass (that is supposed to install the FACP and FACS, and *not* to
wait for PCI enumeration) will be called right at the entry point:
InstallQemuFwCfgTables()
DownloadAndInstallTables(..., TRUE)
QemuFwCfgFindFile("etc/table-loader")
QemuFwCfgSelectItem()
QemuFwCfgReadBytes()
It will invoke the acpi_build_update() callback in QEMU
[hw/i386/acpi-build.c], which will set "build_state->patched" to 1.
In the proposed second phase, which should occur after PCI enumeration,
acpi_build_update() will bail out early, due to "build_state->patched" being
nonzero:
/* No state to update or already patched? Nothing to do. */
if (!build_state || build_state->patched) {
return;
}
And acpi_build() will not be invoked again.
Which means that the first phase here will miss the PCI dependent updates
(there will be updates, they just won't reflect PCI changes, because they will
not have happened yet), and the second phase will see no change.
This dependency is extremely idiosyncratic to the QEMU platform, which is why
we're having a hard time accommodating it in OVMF.
My proposal with gOvmfAcpiPlatformNoPciEnumerationProtocolGuid can be seen in
- http://thread.gmane.org/gmane.comp.bios.tianocore.devel/12640/focus=12645
- http://thread.gmane.org/gmane.comp.bios.tianocore.devel/12640/focus=12648
Otherwise, I think I'd be okay with this series. Initially I expected that the
first (and only) download ever would be delayed until ready-to-boot. The S3
dependencies have stricken me as a surprise, but after all they do make sense.
Thanks
Laszlo
------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=190641631&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
