[Qemu-devel] [edk2 PATCH 0/1] OvmfPkg: grab ACPI tables from QEMU

[Qemu-devel] [edk2 PATCH 0/1] OvmfPkg: grab ACPI tables from QEMU

[Laszlo Ersek]

I'll put only some testing and usage notes in the blurb. The commit
message for the patch and the comment block in the code go into some
implementation detail.

First, this patch has no guest-visible consequences when upgrading from
qemu v1.6 or earlier to v1.7 or later, as long as the qemu machine type
(-M option) is kept intact. Qemu only makes the underlying feature
available for v1.7+ machine types available.

(The machine type is saved in the libvirt XML, so libvirt users are
automatically protected. Direct qemu users need to specify a machine
type with an exact version number.)

Second, I tested the patch under the following circumstances:
- 3.10-based host kernel,
- qemu v1.7.0-rc0, with additional patches that shrink the "pci-hole"
  memory range to just below "system.flash" (see the parallel discussion
  on qemu-devel),
- in the guest, OVMF from SVN r14831, with the following additional
  patches pending on edk2-devel:
  - [edk2] [PATCH v2 00/10] OVMF support for QEMU's PC System Flash
  - [edk2] [PATCH v3 0/2] OvmfPkg: don't restore NvVars from disk if we
    have working flash
  - (this patch)

Guest OSes:
(1) RHEL-7,
(2) Windows Server 2008 R2 SP1 (using additional CSM patches for OVMF),
(3) Windows Server 2012 R2

Details:

(1) RHEL-7

No noticeable changes in behavior.

I dumped and decompiled all ACPI tables before and after the patch. I
also saved the dmesgs. I can make them available at request.

I compared these. I noticed the following differences:

- OEM strings in tables (of course).

- Other small, apparently insignificant changes (for example, in the
  MADT the OVMF built-in tables set the ID of the IO-APIC to
  num_vcpus+1, while qemu exports it as a fixed 0 value, same as SeaBIOS
  used to prepare it). Similarly C[23] latencies in FADT (=FACP). OVMF
  also used to export a bunch of fixed fluff in FADT, which is now gone
  if we take the tables from qemu.

- Important stuff like PCI interrupt routing (_PRT, LNK[A-DS]) seems
  identical, according to the guest kernel dmesg.

- One small regression where OVMF used to provide RESET_REG in FADT
  (=FACP), but qemu currently doesn't say so. Should be fixed in qemu,
  probably.

- Giant leaps forward in the DSDT/SSDT area, especially wrt. PCI
  hotplug. (I didn't actually test PCI hotplug, that's for another
  time.)

- The S3/S4 packages are functionally identical. (Of course OVMF doesn't
  support S3 yet, that's a whole different story.)

- Qemu by default exports a HPET table. The base address described
  therein (0xFED00000) matches the fixed values we have in
  "OvmfPkg/PlatformPei/Platform.c" and
  "PcAtChipsetPkg/PcAtChipsetPkg.dec". I'll return to the HPET later.

- Boot progress bar and console (efifb) continue to work (tested with
  both 1GB and 5GB guest sizes).

(2) Windows Server 2008 R2 SP1 (using additional CSM patches for OVMF)

No changes that would have been visible to me as an interactive user.
Shutdown with the ACPI power button works.

(3) Windows Server 2012 R2

Installed a brand new VM for this guest.

It worked OK but at several points that all looked like reboot attempts
(even after installation, with the live OS) the VM was just waiting and
waiting for something to happen. I looked into the KVM trace as Gleb
had taught me, and I noticed accesses to the HPET. Which made me recall
(and google, and find) recommendations to disable the HPET in general
for virtual machines.

(Again, the OVMF built-in tables don't mention HPET at all, but qemu
does export one if the -no-hpet switch is absent.)

So, I added the corresponding stanza to my libvirt XML. It helped some,
but the problem didn't go away completely.

At this point the KVM trace overwhelmed me, and I just flipped the
machine type back to pc-i440fx-1.6. (I verified in the OVMF debug log
that this had the desired effect -- no tables from qemu.)

The hangs were gone. So I flipped the machine type to pc-i440fx-1.7
again, just to triple-check -- and I couldn't reproduce the hangs any
longer!

So, my testing in this part is inconclusive. The problem may or may not
be related to the ACPI tables; it might or might not be related to
recent changes in qemu itself, etc. Someone with more time and knowledge
than I have should look into the KVM trace.

The ACPI power button initiates the shutdown sequence.

Bonus testing: verified secure boot in this guest with the Microsoft
keys. Thanks to Jordan's series "OVMF support for QEMU's PC System
Flash" mentioned above, key enrollment is permanent now.

Thanks,
Laszlo

Laszlo Ersek (1):
  OvmfPkg/AcpiPlatformDxe: download ACPI tables from QEMU

 OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h |   7 +-
 OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c |  12 +-
 OvmfPkg/AcpiPlatformDxe/Qemu.c         | 204 +++++++++++++++++++++++++++++++++
 3 files changed, 215 insertions(+), 8 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [edk2 PATCH 1/1] OvmfPkg/AcpiPlatformDxe: download ACPI tables from QEMU

[Laszlo Ersek]

Qemu v1.7.0-rc0 features an ACPI linker/loader interface, available over
fw_cfg, written by Michael Tsirkin.

Qemu composes all ACPI tables on the host side, according to the target
hardware configuration, and makes the tables available to any guest
firmware over fw_cfg.

The feature moves the burden of keeping ACPI tables up-to-date from boot
firmware to qemu (which is the source of hardware configuration anyway).

This patch adds client code for this feature.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h |   7 +-
 OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c |  12 +-
 OvmfPkg/AcpiPlatformDxe/Qemu.c         | 204 +++++++++++++++++++++++++++++++++
 3 files changed, 215 insertions(+), 8 deletions(-)

diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
index 21107cd..c643fa1 100644
--- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
+++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
@@ -10,7 +10,7 @@
   THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
   WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 
-**/ 
+**/
 
 #ifndef _ACPI_PLATFORM_H_INCLUDED_
 #define _ACPI_PLATFORM_H_INCLUDED_
@@ -61,5 +61,10 @@ InstallXenTables (
   IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
   );
 
+EFI_STATUS
+EFIAPI
+InstallQemuLinkedTables (
+  IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
+  );
 #endif
 
diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
index 6e0b610..084c393 100644
--- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
+++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
@@ -256,16 +256,14 @@ AcpiPlatformEntryPoint (
 
   if (XenDetected ()) {
     Status = InstallXenTables (AcpiTable);
-    if (EFI_ERROR (Status)) {
-      Status = FindAcpiTablesInFv (AcpiTable);
-    }
   } else {
+    Status = InstallQemuLinkedTables (AcpiTable);
+  }
+
+  if (EFI_ERROR (Status)) {
     Status = FindAcpiTablesInFv (AcpiTable);
   }
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
 
-  return EFI_SUCCESS;
+  return Status;
 }
 
diff --git a/OvmfPkg/AcpiPlatformDxe/Qemu.c b/OvmfPkg/AcpiPlatformDxe/Qemu.c
index 06bd463..c572f8a 100644
--- a/OvmfPkg/AcpiPlatformDxe/Qemu.c
+++ b/OvmfPkg/AcpiPlatformDxe/Qemu.c
@@ -515,3 +515,207 @@ QemuInstallAcpiTable (
            );
 }
 
+
+/**
+  Download the ACPI table data file from QEMU and interpret it.
+
+  Starting with v1.7.0-rc0, QEMU provides the following three files for 1.7+
+  machine types:
+  - etc/acpi/rsdp
+  - etc/acpi/tables
+  - etc/table-loader
+
+  "etc/acpi/rsdp" and "etc/acpi/tables" are similar, they are only kept
+  separate because they have different allocation requirements in SeaBIOS.
+
+  Both of these fw_cfg files contain preformatted ACPI payload. "etc/acpi/rsdp"
+  contains only the RSDP table, while "etc/acpi/tables" contains all other
+  tables, concatenated.
+
+  The tables in these two files have been filled in by qemu, but two kinds of
+  fields are incomplete in each table: pointers to other tables, and checksums
+  (which depend on the pointers). The pointers are pre-initialized with
+  *relative* offsets, but their final values depend on where the actual files
+  will be placed in memory by the guest. That is, the pointer fields need to be
+  "relocated" (incremented) by the base addresses of where "/etc/acpi/rsdp" and
+  "/etc/acpi/tables" will be placed in guest memory.
+
+  This is where the third file, "/etc/table-loader" comes into the picture. It
+  is a linker/loader script that has several command types:
+
+    One command type instructs the guest to download the other two files.
+
+    Another command type instructs the guest to increment ("absolutize") a
+    pointer field (having a relative initial value) in some file, with the
+    dynamic base address of the same (or another) file.
+
+    The third command type instructs the guest to compute checksums over
+    ranges and to store them.
+
+  In edk2, EFI_ACPI_TABLE_PROTOCOL knows about table relationships -- it
+  handles linkage automatically when a table is installed. The protocol takes
+  care of checksumming too. RSDP is installed automatically. Hence we only need
+  to care about the "etc/acpi/tables" file, determining the boundaries of each
+  table and installing it.
+
+  @param[in] AcpiProtocol  The ACPI table protocol used to install tables.
+
+  @retval  EFI_UNSUPPORTED       Firmware configuration is unavailable.
+
+  @retval  EFI_NOT_FOUND         The host doesn't export the "etc/acpi/tables"
+                                 fw_cfg file.
+
+  @retval  EFI_OUT_OF_RESOURCES  Memory allocation failed.
+
+  @return                        Status codes returned by
+                                 AcpiProtocol->InstallAcpiTable().
+
+**/
+
+//
+// We'll be saving the keys of installed tables so that we can roll them back
+// in case of failure. 128 tables should be enough for anyone (TM).
+//
+#define INSTALLED_TABLES_MAX 128
+
+//
+// This macro produces three arguments for DEBUG(), for the format string
+// "%-*.*a" -- left-justify, take field width, take number of characters to
+// consume, ASCII character string. The argument must be a valid pointer.
+//
+#define DBGSTR(ptr) (sizeof *(ptr)), (sizeof *(ptr)), ((CONST CHAR8 *) (ptr))
+
+//
+// Introduce a macro for the format string as well, bracketed by embedded
+// double quotes.
+//
+#define DBGFMT "\"%-*.*a\""
+
+EFI_STATUS
+EFIAPI
+InstallQemuLinkedTables (
+  IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
+  )
+{
+  STATIC CONST CHAR8   Func[] = "InstallQemuLinkedTables";
+  EFI_STATUS           Status;
+  FIRMWARE_CONFIG_ITEM TablesFile;
+  UINTN                TablesFileSize;
+  UINT8                *Tables;
+  UINTN                *InstalledKey;
+  UINTN                Processed;
+  INT32                Installed;
+
+  Status = QemuFwCfgFindFile ("etc/acpi/tables", &TablesFile, &TablesFileSize);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((EFI_D_INFO, "%a: \"etc/acpi/tables\" interface unavailable: %r\n",
+      Func, Status));
+    return Status;
+  }
+
+  Tables = AllocatePool (TablesFileSize);
+  if (Tables == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  QemuFwCfgSelectItem (TablesFile);
+  QemuFwCfgReadBytes (TablesFileSize, Tables);
+
+  InstalledKey = AllocatePool (INSTALLED_TABLES_MAX * sizeof *InstalledKey);
+  if (InstalledKey == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    goto FreeTables;
+  }
+
+  Processed = 0;
+  Installed = 0;
+  while (Processed < TablesFileSize) {
+    UINTN                       Remaining;
+    EFI_ACPI_DESCRIPTION_HEADER *Probe;
+
+    Remaining = TablesFileSize - Processed;
+    if (Remaining < sizeof *Probe) {
+      DEBUG ((EFI_D_VERBOSE, "%a: truncated ACPI table header at offset "
+        "0x%Lx\n", Func, (UINT64) Processed));
+      Status = EFI_PROTOCOL_ERROR;
+      break;
+    }
+
+    Probe = (EFI_ACPI_DESCRIPTION_HEADER *) (Tables + Processed);
+    DEBUG ((EFI_D_VERBOSE, "%a: offset 0x%016Lx:"
+      " Signature=" DBGFMT
+      " Length=0x%08x"
+      " Revision=0x%02x"
+      " OemId=" DBGFMT
+      " OemTableId=" DBGFMT
+      " OemRevision=0x%08x"
+      " CreatorId=" DBGFMT
+      " CreatorRevision=0x%08x\n",
+      Func, (UINT64) Processed,
+      DBGSTR (&Probe->Signature),
+      Probe->Length,
+      Probe->Revision,
+      DBGSTR (&Probe->OemId),
+      DBGSTR (&Probe->OemTableId),
+      Probe->OemRevision,
+      DBGSTR (&Probe->CreatorId),
+      Probe->CreatorRevision));
+
+    if (Remaining < Probe->Length || Probe->Length < sizeof *Probe) {
+      DEBUG ((EFI_D_VERBOSE, "%a: invalid ACPI table header at offset 0x%Lx\n",
+        Func, (UINT64) Processed));
+      Status = EFI_PROTOCOL_ERROR;
+      break;
+    }
+
+    //
+    // skip automatically handled "root" tables: RSDT, XSDT
+    //
+    if (Probe->Signature !=
+                        EFI_ACPI_1_0_ROOT_SYSTEM_DESCRIPTION_TABLE_SIGNATURE &&
+        Probe->Signature !=
+                    EFI_ACPI_2_0_EXTENDED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
+      if (Installed == INSTALLED_TABLES_MAX) {
+        DEBUG ((EFI_D_ERROR, "%a: can't install more than %d tables\n", Func,
+          INSTALLED_TABLES_MAX));
+        Status = EFI_OUT_OF_RESOURCES;
+        break;
+      }
+
+      Status = AcpiProtocol->InstallAcpiTable (AcpiProtocol, Probe,
+                 Probe->Length, &InstalledKey[Installed]);
+      if (EFI_ERROR (Status)) {
+        DEBUG ((EFI_D_ERROR,
+          "%a: failed to install table " DBGFMT " at offset 0x%Lx: %r\n", Func,
+          DBGSTR (&Probe->Signature), (UINT64) Processed, Status));
+        break;
+      }
+
+      ++Installed;
+    }
+
+    Processed += Probe->Length;
+  }
+
+  if (Processed == TablesFileSize || Status == EFI_PROTOCOL_ERROR) {
+    DEBUG ((EFI_D_INFO, "%a: installed %d tables\n", Func, Installed));
+    Status = EFI_SUCCESS;
+  } else {
+    ASSERT (EFI_ERROR (Status));
+
+    //
+    // Roll back partial installation.
+    //
+    while (Installed > 0) {
+      --Installed;
+      AcpiProtocol->UninstallAcpiTable (AcpiProtocol, InstalledKey[Installed]);
+    }
+  }
+
+  FreePool (InstalledKey);
+
+FreeTables:
+  FreePool (Tables);
+
+  return Status;
+}
-- 
1.8.3.1

Re: [Qemu-devel] [edk2 PATCH 1/1] OvmfPkg/AcpiPlatformDxe: download ACPI tables from QEMU

[Jordan Justen]

On Tue, Nov 12, 2013 at 7:11 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> Qemu v1.7.0-rc0 features an ACPI linker/loader interface, available over
> fw_cfg, written by Michael Tsirkin.
>
> Qemu composes all ACPI tables on the host side, according to the target
> hardware configuration, and makes the tables available to any guest
> firmware over fw_cfg.
>
> The feature moves the burden of keeping ACPI tables up-to-date from boot
> firmware to qemu (which is the source of hardware configuration anyway).
>
> This patch adds client code for this feature.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h |   7 +-
>  OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c |  12 +-
>  OvmfPkg/AcpiPlatformDxe/Qemu.c         | 204 +++++++++++++++++++++++++++++++++
>  3 files changed, 215 insertions(+), 8 deletions(-)
>
> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
> index 21107cd..c643fa1 100644
> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
> @@ -10,7 +10,7 @@
>    THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>    WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>
> -**/
> +**/
>
>  #ifndef _ACPI_PLATFORM_H_INCLUDED_
>  #define _ACPI_PLATFORM_H_INCLUDED_
> @@ -61,5 +61,10 @@ InstallXenTables (
>    IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
>    );
>
> +EFI_STATUS
> +EFIAPI
> +InstallQemuLinkedTables (
> +  IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
> +  );
>  #endif
>
> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
> index 6e0b610..084c393 100644
> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
> @@ -256,16 +256,14 @@ AcpiPlatformEntryPoint (
>
>    if (XenDetected ()) {
>      Status = InstallXenTables (AcpiTable);
> -    if (EFI_ERROR (Status)) {
> -      Status = FindAcpiTablesInFv (AcpiTable);
> -    }
>    } else {
> +    Status = InstallQemuLinkedTables (AcpiTable);
> +  }
> +
> +  if (EFI_ERROR (Status)) {
>      Status = FindAcpiTablesInFv (AcpiTable);
>    }
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
>
> -  return EFI_SUCCESS;
> +  return Status;
>  }
>
> diff --git a/OvmfPkg/AcpiPlatformDxe/Qemu.c b/OvmfPkg/AcpiPlatformDxe/Qemu.c
> index 06bd463..c572f8a 100644
> --- a/OvmfPkg/AcpiPlatformDxe/Qemu.c
> +++ b/OvmfPkg/AcpiPlatformDxe/Qemu.c
> @@ -515,3 +515,207 @@ QemuInstallAcpiTable (
>             );
>  }
>
> +
> +/**
> +  Download the ACPI table data file from QEMU and interpret it.
> +
> +  Starting with v1.7.0-rc0, QEMU provides the following three files for 1.7+
> +  machine types:
> +  - etc/acpi/rsdp
> +  - etc/acpi/tables
> +  - etc/table-loader
> +
> +  "etc/acpi/rsdp" and "etc/acpi/tables" are similar, they are only kept
> +  separate because they have different allocation requirements in SeaBIOS.
> +
> +  Both of these fw_cfg files contain preformatted ACPI payload. "etc/acpi/rsdp"
> +  contains only the RSDP table, while "etc/acpi/tables" contains all other
> +  tables, concatenated.
> +
> +  The tables in these two files have been filled in by qemu, but two kinds of
> +  fields are incomplete in each table: pointers to other tables, and checksums
> +  (which depend on the pointers). The pointers are pre-initialized with
> +  *relative* offsets, but their final values depend on where the actual files
> +  will be placed in memory by the guest. That is, the pointer fields need to be
> +  "relocated" (incremented) by the base addresses of where "/etc/acpi/rsdp" and
> +  "/etc/acpi/tables" will be placed in guest memory.
> +
> +  This is where the third file, "/etc/table-loader" comes into the picture. It
> +  is a linker/loader script that has several command types:
> +
> +    One command type instructs the guest to download the other two files.
> +
> +    Another command type instructs the guest to increment ("absolutize") a
> +    pointer field (having a relative initial value) in some file, with the
> +    dynamic base address of the same (or another) file.
> +
> +    The third command type instructs the guest to compute checksums over
> +    ranges and to store them.
> +
> +  In edk2, EFI_ACPI_TABLE_PROTOCOL knows about table relationships -- it
> +  handles linkage automatically when a table is installed. The protocol takes
> +  care of checksumming too. RSDP is installed automatically. Hence we only need
> +  to care about the "etc/acpi/tables" file, determining the boundaries of each
> +  table and installing it.

I'm wondering if some of the information in this comment might have a
better home in the commit message. Will it help in maintaining the
code to have it here? Or, maybe a more terse version can live here?

Of course, I rarely comment my code enough, which is much worse. So,
feel free to ignore this feedback. :)

> +  @param[in] AcpiProtocol  The ACPI table protocol used to install tables.
> +
> +  @retval  EFI_UNSUPPORTED       Firmware configuration is unavailable.
> +
> +  @retval  EFI_NOT_FOUND         The host doesn't export the "etc/acpi/tables"
> +                                 fw_cfg file.
> +
> +  @retval  EFI_OUT_OF_RESOURCES  Memory allocation failed.
> +
> +  @return                        Status codes returned by
> +                                 AcpiProtocol->InstallAcpiTable().
> +
> +**/
> +
> +//
> +// We'll be saving the keys of installed tables so that we can roll them back
> +// in case of failure. 128 tables should be enough for anyone (TM).
> +//
> +#define INSTALLED_TABLES_MAX 128
> +
> +//
> +// This macro produces three arguments for DEBUG(), for the format string
> +// "%-*.*a" -- left-justify, take field width, take number of characters to
> +// consume, ASCII character string. The argument must be a valid pointer.
> +//
> +#define DBGSTR(ptr) (sizeof *(ptr)), (sizeof *(ptr)), ((CONST CHAR8 *) (ptr))
> +
> +//
> +// Introduce a macro for the format string as well, bracketed by embedded
> +// double quotes.
> +//
> +#define DBGFMT "\"%-*.*a\""

I think that needing these macros is a sign that perhaps there are
excessive debug messages. :)

> +EFI_STATUS
> +EFIAPI
> +InstallQemuLinkedTables (
> +  IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
> +  )
> +{
> +  STATIC CONST CHAR8   Func[] = "InstallQemuLinkedTables";
> +  EFI_STATUS           Status;
> +  FIRMWARE_CONFIG_ITEM TablesFile;
> +  UINTN                TablesFileSize;
> +  UINT8                *Tables;
> +  UINTN                *InstalledKey;
> +  UINTN                Processed;
> +  INT32                Installed;
> +
> +  Status = QemuFwCfgFindFile ("etc/acpi/tables", &TablesFile, &TablesFileSize);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((EFI_D_INFO, "%a: \"etc/acpi/tables\" interface unavailable: %r\n",
> +      Func, Status));
> +    return Status;
> +  }
> +
> +  Tables = AllocatePool (TablesFileSize);
> +  if (Tables == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  QemuFwCfgSelectItem (TablesFile);
> +  QemuFwCfgReadBytes (TablesFileSize, Tables);
> +
> +  InstalledKey = AllocatePool (INSTALLED_TABLES_MAX * sizeof *InstalledKey);
> +  if (InstalledKey == NULL) {
> +    Status = EFI_OUT_OF_RESOURCES;
> +    goto FreeTables;
> +  }
> +
> +  Processed = 0;
> +  Installed = 0;
> +  while (Processed < TablesFileSize) {
> +    UINTN                       Remaining;
> +    EFI_ACPI_DESCRIPTION_HEADER *Probe;
> +
> +    Remaining = TablesFileSize - Processed;
> +    if (Remaining < sizeof *Probe) {
> +      DEBUG ((EFI_D_VERBOSE, "%a: truncated ACPI table header at offset "
> +        "0x%Lx\n", Func, (UINT64) Processed));
> +      Status = EFI_PROTOCOL_ERROR;
> +      break;
> +    }
> +
> +    Probe = (EFI_ACPI_DESCRIPTION_HEADER *) (Tables + Processed);
> +    DEBUG ((EFI_D_VERBOSE, "%a: offset 0x%016Lx:"
> +      " Signature=" DBGFMT
> +      " Length=0x%08x"
> +      " Revision=0x%02x"
> +      " OemId=" DBGFMT
> +      " OemTableId=" DBGFMT
> +      " OemRevision=0x%08x"
> +      " CreatorId=" DBGFMT
> +      " CreatorRevision=0x%08x\n",
> +      Func, (UINT64) Processed,
> +      DBGSTR (&Probe->Signature),
> +      Probe->Length,
> +      Probe->Revision,
> +      DBGSTR (&Probe->OemId),
> +      DBGSTR (&Probe->OemTableId),
> +      Probe->OemRevision,
> +      DBGSTR (&Probe->CreatorId),
> +      Probe->CreatorRevision));

Yep. :)

Do we need to print all those fields?

Anyway, maybe a better centralized place for this would be:
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c

Also, I think we try to keep debug messages under 80 characters.

> +    if (Remaining < Probe->Length || Probe->Length < sizeof *Probe) {
> +      DEBUG ((EFI_D_VERBOSE, "%a: invalid ACPI table header at offset 0x%Lx\n",
> +        Func, (UINT64) Processed));
> +      Status = EFI_PROTOCOL_ERROR;
> +      break;
> +    }
> +
> +    //
> +    // skip automatically handled "root" tables: RSDT, XSDT
> +    //
> +    if (Probe->Signature !=
> +                        EFI_ACPI_1_0_ROOT_SYSTEM_DESCRIPTION_TABLE_SIGNATURE &&
> +        Probe->Signature !=
> +                    EFI_ACPI_2_0_EXTENDED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
> +      if (Installed == INSTALLED_TABLES_MAX) {
> +        DEBUG ((EFI_D_ERROR, "%a: can't install more than %d tables\n", Func,
> +          INSTALLED_TABLES_MAX));
> +        Status = EFI_OUT_OF_RESOURCES;
> +        break;
> +      }
> +
> +      Status = AcpiProtocol->InstallAcpiTable (AcpiProtocol, Probe,
> +                 Probe->Length, &InstalledKey[Installed]);

We do have a local InstallAcpiTable function. (Just remove "AcpiProtocol->")

-Jordan

Re: [Qemu-devel] [edk2 PATCH 1/1] OvmfPkg/AcpiPlatformDxe: download ACPI tables from QEMU

[Laszlo Ersek]

On 11/22/13 19:10, Jordan Justen wrote:
> On Tue, Nov 12, 2013 at 7:11 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>> Qemu v1.7.0-rc0 features an ACPI linker/loader interface, available over
>> fw_cfg, written by Michael Tsirkin.
>>
>> Qemu composes all ACPI tables on the host side, according to the target
>> hardware configuration, and makes the tables available to any guest
>> firmware over fw_cfg.
>>
>> The feature moves the burden of keeping ACPI tables up-to-date from boot
>> firmware to qemu (which is the source of hardware configuration anyway).
>>
>> This patch adds client code for this feature.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h |   7 +-
>>  OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c |  12 +-
>>  OvmfPkg/AcpiPlatformDxe/Qemu.c         | 204 +++++++++++++++++++++++++++++++++
>>  3 files changed, 215 insertions(+), 8 deletions(-)
>>
>> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>> index 21107cd..c643fa1 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>> @@ -10,7 +10,7 @@
>>    THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>>    WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>
>> -**/
>> +**/
>>
>>  #ifndef _ACPI_PLATFORM_H_INCLUDED_
>>  #define _ACPI_PLATFORM_H_INCLUDED_
>> @@ -61,5 +61,10 @@ InstallXenTables (
>>    IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
>>    );
>>
>> +EFI_STATUS
>> +EFIAPI
>> +InstallQemuLinkedTables (
>> +  IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
>> +  );
>>  #endif
>>
>> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
>> index 6e0b610..084c393 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
>> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
>> @@ -256,16 +256,14 @@ AcpiPlatformEntryPoint (
>>
>>    if (XenDetected ()) {
>>      Status = InstallXenTables (AcpiTable);
>> -    if (EFI_ERROR (Status)) {
>> -      Status = FindAcpiTablesInFv (AcpiTable);
>> -    }
>>    } else {
>> +    Status = InstallQemuLinkedTables (AcpiTable);
>> +  }
>> +
>> +  if (EFI_ERROR (Status)) {
>>      Status = FindAcpiTablesInFv (AcpiTable);
>>    }
>> -  if (EFI_ERROR (Status)) {
>> -    return Status;
>> -  }
>>
>> -  return EFI_SUCCESS;
>> +  return Status;
>>  }
>>
>> diff --git a/OvmfPkg/AcpiPlatformDxe/Qemu.c b/OvmfPkg/AcpiPlatformDxe/Qemu.c
>> index 06bd463..c572f8a 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/Qemu.c
>> +++ b/OvmfPkg/AcpiPlatformDxe/Qemu.c
>> @@ -515,3 +515,207 @@ QemuInstallAcpiTable (
>>             );
>>  }
>>
>> +
>> +/**
>> +  Download the ACPI table data file from QEMU and interpret it.
>> +
>> +  Starting with v1.7.0-rc0, QEMU provides the following three files for 1.7+
>> +  machine types:
>> +  - etc/acpi/rsdp
>> +  - etc/acpi/tables
>> +  - etc/table-loader
>> +
>> +  "etc/acpi/rsdp" and "etc/acpi/tables" are similar, they are only kept
>> +  separate because they have different allocation requirements in SeaBIOS.
>> +
>> +  Both of these fw_cfg files contain preformatted ACPI payload. "etc/acpi/rsdp"
>> +  contains only the RSDP table, while "etc/acpi/tables" contains all other
>> +  tables, concatenated.
>> +
>> +  The tables in these two files have been filled in by qemu, but two kinds of
>> +  fields are incomplete in each table: pointers to other tables, and checksums
>> +  (which depend on the pointers). The pointers are pre-initialized with
>> +  *relative* offsets, but their final values depend on where the actual files
>> +  will be placed in memory by the guest. That is, the pointer fields need to be
>> +  "relocated" (incremented) by the base addresses of where "/etc/acpi/rsdp" and
>> +  "/etc/acpi/tables" will be placed in guest memory.
>> +
>> +  This is where the third file, "/etc/table-loader" comes into the picture. It
>> +  is a linker/loader script that has several command types:
>> +
>> +    One command type instructs the guest to download the other two files.
>> +
>> +    Another command type instructs the guest to increment ("absolutize") a
>> +    pointer field (having a relative initial value) in some file, with the
>> +    dynamic base address of the same (or another) file.
>> +
>> +    The third command type instructs the guest to compute checksums over
>> +    ranges and to store them.
>> +
>> +  In edk2, EFI_ACPI_TABLE_PROTOCOL knows about table relationships -- it
>> +  handles linkage automatically when a table is installed. The protocol takes
>> +  care of checksumming too. RSDP is installed automatically. Hence we only need
>> +  to care about the "etc/acpi/tables" file, determining the boundaries of each
>> +  table and installing it.
> 
> I'm wondering if some of the information in this comment might have a
> better home in the commit message. Will it help in maintaining the
> code to have it here? Or, maybe a more terse version can live here?
> 
> Of course, I rarely comment my code enough, which is much worse. So,
> feel free to ignore this feedback. :)

I can move the text from "Starting with v1.7.0-rc0..." until here to the
commit message.

> 
>> +  @param[in] AcpiProtocol  The ACPI table protocol used to install tables.
>> +
>> +  @retval  EFI_UNSUPPORTED       Firmware configuration is unavailable.
>> +
>> +  @retval  EFI_NOT_FOUND         The host doesn't export the "etc/acpi/tables"
>> +                                 fw_cfg file.
>> +
>> +  @retval  EFI_OUT_OF_RESOURCES  Memory allocation failed.
>> +
>> +  @return                        Status codes returned by
>> +                                 AcpiProtocol->InstallAcpiTable().
>> +
>> +**/
>> +
>> +//
>> +// We'll be saving the keys of installed tables so that we can roll them back
>> +// in case of failure. 128 tables should be enough for anyone (TM).
>> +//
>> +#define INSTALLED_TABLES_MAX 128
>> +
>> +//
>> +// This macro produces three arguments for DEBUG(), for the format string
>> +// "%-*.*a" -- left-justify, take field width, take number of characters to
>> +// consume, ASCII character string. The argument must be a valid pointer.
>> +//
>> +#define DBGSTR(ptr) (sizeof *(ptr)), (sizeof *(ptr)), ((CONST CHAR8 *) (ptr))
>> +
>> +//
>> +// Introduce a macro for the format string as well, bracketed by embedded
>> +// double quotes.
>> +//
>> +#define DBGFMT "\"%-*.*a\""
> 
> I think that needing these macros is a sign that perhaps there are
> excessive debug messages. :)

There's no such thing as an excessive debug message, if you can disable
it :) When communication over a binary protocol goes wrong, the first
thing you add is a hexdump with some human-friendly parsing. I just
don't want to start from zero every time that happens.

> 
>> +EFI_STATUS
>> +EFIAPI
>> +InstallQemuLinkedTables (
>> +  IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
>> +  )
>> +{
>> +  STATIC CONST CHAR8   Func[] = "InstallQemuLinkedTables";
>> +  EFI_STATUS           Status;
>> +  FIRMWARE_CONFIG_ITEM TablesFile;
>> +  UINTN                TablesFileSize;
>> +  UINT8                *Tables;
>> +  UINTN                *InstalledKey;
>> +  UINTN                Processed;
>> +  INT32                Installed;
>> +
>> +  Status = QemuFwCfgFindFile ("etc/acpi/tables", &TablesFile, &TablesFileSize);
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((EFI_D_INFO, "%a: \"etc/acpi/tables\" interface unavailable: %r\n",
>> +      Func, Status));
>> +    return Status;
>> +  }
>> +
>> +  Tables = AllocatePool (TablesFileSize);
>> +  if (Tables == NULL) {
>> +    return EFI_OUT_OF_RESOURCES;
>> +  }
>> +
>> +  QemuFwCfgSelectItem (TablesFile);
>> +  QemuFwCfgReadBytes (TablesFileSize, Tables);
>> +
>> +  InstalledKey = AllocatePool (INSTALLED_TABLES_MAX * sizeof *InstalledKey);
>> +  if (InstalledKey == NULL) {
>> +    Status = EFI_OUT_OF_RESOURCES;
>> +    goto FreeTables;
>> +  }
>> +
>> +  Processed = 0;
>> +  Installed = 0;
>> +  while (Processed < TablesFileSize) {
>> +    UINTN                       Remaining;
>> +    EFI_ACPI_DESCRIPTION_HEADER *Probe;
>> +
>> +    Remaining = TablesFileSize - Processed;
>> +    if (Remaining < sizeof *Probe) {
>> +      DEBUG ((EFI_D_VERBOSE, "%a: truncated ACPI table header at offset "
>> +        "0x%Lx\n", Func, (UINT64) Processed));
>> +      Status = EFI_PROTOCOL_ERROR;
>> +      break;
>> +    }
>> +
>> +    Probe = (EFI_ACPI_DESCRIPTION_HEADER *) (Tables + Processed);
>> +    DEBUG ((EFI_D_VERBOSE, "%a: offset 0x%016Lx:"
>> +      " Signature=" DBGFMT
>> +      " Length=0x%08x"
>> +      " Revision=0x%02x"
>> +      " OemId=" DBGFMT
>> +      " OemTableId=" DBGFMT
>> +      " OemRevision=0x%08x"
>> +      " CreatorId=" DBGFMT
>> +      " CreatorRevision=0x%08x\n",
>> +      Func, (UINT64) Processed,
>> +      DBGSTR (&Probe->Signature),
>> +      Probe->Length,
>> +      Probe->Revision,
>> +      DBGSTR (&Probe->OemId),
>> +      DBGSTR (&Probe->OemTableId),
>> +      Probe->OemRevision,
>> +      DBGSTR (&Probe->CreatorId),
>> +      Probe->CreatorRevision));
> 
> Yep. :)
> 
> Do we need to print all those fields?
> 
> Anyway, maybe a better centralized place for this would be:
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> 
> Also, I think we try to keep debug messages under 80 characters.

We seem to think completely differently about debug messages :)

I can cut most of the fields though, and simply keep the signature (and
remove the format string macro too, because for the signature I'd only
use it once). Would that work for you?

I'd rather not try to patch MdeModulePkg without an actual bug to fix.

> 
>> +    if (Remaining < Probe->Length || Probe->Length < sizeof *Probe) {
>> +      DEBUG ((EFI_D_VERBOSE, "%a: invalid ACPI table header at offset 0x%Lx\n",
>> +        Func, (UINT64) Processed));
>> +      Status = EFI_PROTOCOL_ERROR;
>> +      break;
>> +    }
>> +
>> +    //
>> +    // skip automatically handled "root" tables: RSDT, XSDT
>> +    //
>> +    if (Probe->Signature !=
>> +                        EFI_ACPI_1_0_ROOT_SYSTEM_DESCRIPTION_TABLE_SIGNATURE &&
>> +        Probe->Signature !=
>> +                    EFI_ACPI_2_0_EXTENDED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
>> +      if (Installed == INSTALLED_TABLES_MAX) {
>> +        DEBUG ((EFI_D_ERROR, "%a: can't install more than %d tables\n", Func,
>> +          INSTALLED_TABLES_MAX));
>> +        Status = EFI_OUT_OF_RESOURCES;
>> +        break;
>> +      }
>> +
>> +      Status = AcpiProtocol->InstallAcpiTable (AcpiProtocol, Probe,
>> +                 Probe->Length, &InstalledKey[Installed]);
> 
> We do have a local InstallAcpiTable function. (Just remove "AcpiProtocol->")

Yes, I saw that. But, we don't have a wrapper for the rollback on the
error path (where we uninstall the tables). Since I used AcpiProtocol->
in the rollback part, I wanted to be consistent here.

Of course I can add a wrapper for the uninstall function too :)

Thanks
Laszlo

Re: [Qemu-devel] [edk2 PATCH 1/1] OvmfPkg/AcpiPlatformDxe: download ACPI tables from QEMU

[Jordan Justen]

On Fri, Nov 22, 2013 at 10:49 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 11/22/13 19:10, Jordan Justen wrote:
>> Do we need to print all those fields?
>>
>> Anyway, maybe a better centralized place for this would be:
>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>>
>> Also, I think we try to keep debug messages under 80 characters.
>
> We seem to think completely differently about debug messages :)

Maybe... ?

I think you can have too much debug if:
* it makes it debug builds excessively slow or large
* it makes it challenging to find items in the debug log
* it makes the code significantly more difficult to read
* it is in a code area that is just not valuable to log

> I can cut most of the fields though, and simply keep the signature (and
> remove the format string macro too, because for the signature I'd only
> use it once). Would that work for you?

Sure.

>> We do have a local InstallAcpiTable function. (Just remove "AcpiProtocol->")
>
> Yes, I saw that. But, we don't have a wrapper for the rollback on the
> error path (where we uninstall the tables). Since I used AcpiProtocol->
> in the rollback part, I wanted to be consistent here.

Good point.

-Jordan

Re: [Qemu-devel] [edk2 PATCH 0/1] OvmfPkg: grab ACPI tables from QEMU

[Igor Mammedov]

On Tue, 12 Nov 2013 16:11:49 +0100
Laszlo Ersek <lersek@redhat.com> wrote:


> Second, I tested the patch under the following circumstances:
> - 3.10-based host kernel,
> - qemu v1.7.0-rc0, with additional patches that shrink the "pci-hole"
>   memory range to just below "system.flash" (see the parallel discussion
>   on qemu-devel),
Is "pc: map PCI address space as catchall region for not
mapped addresses" works for you instead of shrinking?


-- 
Regards,
  Igor

Re: [Qemu-devel] [edk2 PATCH 0/1] OvmfPkg: grab ACPI tables from QEMU

[Laszlo Ersek]

On 11/12/13 17:05, Igor Mammedov wrote:
> On Tue, 12 Nov 2013 16:11:49 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
> 
>> Second, I tested the patch under the following circumstances:
>> - 3.10-based host kernel,
>> - qemu v1.7.0-rc0, with additional patches that shrink the "pci-hole"
>>   memory range to just below "system.flash" (see the parallel discussion
>>   on qemu-devel),
> Is "pc: map PCI address space as catchall region for not
> mapped addresses" works for you instead of shrinking?

Yes, it does. Thanks!
Laszlo

Re: [Qemu-devel] [edk2] [edk2 PATCH 0/1] OvmfPkg: grab ACPI tables from QEMU

[Laszlo Ersek]

On 11/12/13 16:11, Laszlo Ersek wrote:

> - Boot progress bar and console (efifb) continue to work (tested with
>   both 1GB and 5GB guest sizes).

Turns out one can't be diligent enough.

This patch causes (or exposes, dependent on your POV) breakage. It
breaks the cirrus video output for both RHEL-6 and Windows 2012 R2
guests, very early during boot, *if* the guest RAM size is 2560 MB.

(That amount is just an example, but a good example (= an esp. bad value).)

When the guest has this much RAM, then the built-in OVMF algorithm
advertises the following 32-bit PCI window (no 64-bit PCI window) in
\_SB.PCI0._CRS:

  ACPI PciWindow32: Base=0xA0000000 End=0xFEEFFFFF Length=0x5EF00000
  ACPI PciWindow64: Base=0x00000000 End=0x00000000 Length=0x00000000

(inclusive End). That is, the windows starts right above the
conventional memory; 0xA0000000 == 2560M.

However, the ACPI table exported by qemu says

  begin32=c0000000 end32=fec00000 begin64=0 end64=0

(exclusive end32).

Even though this latter range is a proper subset of the former, it
breaks the cirrus display in both said guests (didn't test more guests).

When I applied this PoC patch on qemu, the displays started to work:

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index edc974e..305b786 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -348,11 +348,11 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     /* Set PCI window size the way seabios has always done it. */
     /* Power of 2 so bios can cover it with a single MTRR */
     if (ram_size <= 0x80000000) {
         i440fx->pci_info.w32.begin = 0x80000000;
     } else if (ram_size <= 0xc0000000) {
-        i440fx->pci_info.w32.begin = 0xc0000000;
+        i440fx->pci_info.w32.begin = 0xa0000000;
     } else {
         i440fx->pci_info.w32.begin = 0xe0000000;
     }

     memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole",
f->pci_address_space,

I guess the range starting at 0xc0000000 is somehow "incompatible" with
the EFI memory map. (I can't actually explain this idea because, again,
this second range is a proper subset of the former, and its size is
still 1004MB.)

The comment before the w32.begin assignments is interesting. It dates
back to qemu commit 3459a625 ("pci: store PCI hole ranges in guestinfo
structure"). It could be incompatible with the *current* OVMF memory
map, but of course the OVMF memory map could be wrong as well.

I've no idea how to fix this disagreement.

Thanks
Laszlo

Re: [Qemu-devel] [edk2] [edk2 PATCH 0/1] OvmfPkg: grab ACPI tables from QEMU

[Igor Mammedov]

On Tue, 19 Nov 2013 00:34:31 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> On 11/12/13 16:11, Laszlo Ersek wrote:
> 
> > - Boot progress bar and console (efifb) continue to work (tested with
> >   both 1GB and 5GB guest sizes).
> 
> Turns out one can't be diligent enough.
> 
> This patch causes (or exposes, dependent on your POV) breakage. It
> breaks the cirrus video output for both RHEL-6 and Windows 2012 R2
> guests, very early during boot, *if* the guest RAM size is 2560 MB.
> 
> (That amount is just an example, but a good example (= an esp. bad value).)
> 
> When the guest has this much RAM, then the built-in OVMF algorithm
> advertises the following 32-bit PCI window (no 64-bit PCI window) in
> \_SB.PCI0._CRS:
> 
>   ACPI PciWindow32: Base=0xA0000000 End=0xFEEFFFFF Length=0x5EF00000
looks wrong, it should honor window advertised by QEMU exported ACPI table.

>   ACPI PciWindow64: Base=0x00000000 End=0x00000000 Length=0x00000000
> 
> (inclusive End). That is, the windows starts right above the
> conventional memory; 0xA0000000 == 2560M.
> 
> However, the ACPI table exported by qemu says
> 
>   begin32=c0000000 end32=fec00000 begin64=0 end64=0
> 
> (exclusive end32).
> 
> Even though this latter range is a proper subset of the former, it
> breaks the cirrus display in both said guests (didn't test more guests).
> 
> When I applied this PoC patch on qemu, the displays started to work:
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index edc974e..305b786 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -348,11 +348,11 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>      /* Set PCI window size the way seabios has always done it. */
>      /* Power of 2 so bios can cover it with a single MTRR */
>      if (ram_size <= 0x80000000) {
>          i440fx->pci_info.w32.begin = 0x80000000;
>      } else if (ram_size <= 0xc0000000) {
> -        i440fx->pci_info.w32.begin = 0xc0000000;
> +        i440fx->pci_info.w32.begin = 0xa0000000;
You cant just move it for compatibility reasons,
since similar code is hardcoded in Seabios since forever.


>      } else {
>          i440fx->pci_info.w32.begin = 0xe0000000;
>      }
> 
>      memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole",
> f->pci_address_space,
> 
> I guess the range starting at 0xc0000000 is somehow "incompatible" with
> the EFI memory map. (I can't actually explain this idea because, again,
> this second range is a proper subset of the former, and its size is
> still 1004MB.)
> 
> The comment before the w32.begin assignments is interesting. It dates
> back to qemu commit 3459a625 ("pci: store PCI hole ranges in guestinfo
> structure"). It could be incompatible with the *current* OVMF memory
> map, but of course the OVMF memory map could be wrong as well.
> 
> I've no idea how to fix this disagreement.
> 
> Thanks
> Laszlo
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [edk2] [edk2 PATCH 0/1] OvmfPkg: grab ACPI tables from QEMU

[Laszlo Ersek]

On 11/19/13 01:04, Igor Mammedov wrote:
> On Tue, 19 Nov 2013 00:34:31 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 11/12/13 16:11, Laszlo Ersek wrote:
>>
>>> - Boot progress bar and console (efifb) continue to work (tested with
>>>   both 1GB and 5GB guest sizes).
>>
>> Turns out one can't be diligent enough.
>>
>> This patch causes (or exposes, dependent on your POV) breakage. It
>> breaks the cirrus video output for both RHEL-6 and Windows 2012 R2
>> guests, very early during boot, *if* the guest RAM size is 2560 MB.
>>
>> (That amount is just an example, but a good example (= an esp. bad value).)
>>
>> When the guest has this much RAM, then the built-in OVMF algorithm
>> advertises the following 32-bit PCI window (no 64-bit PCI window) in
>> \_SB.PCI0._CRS:
>>
>>   ACPI PciWindow32: Base=0xA0000000 End=0xFEEFFFFF Length=0x5EF00000
> looks wrong, it should honor window advertised by QEMU exported ACPI table.

Sorry, I wasn't clear enough.

When you run OVMF without this patch on any qemu version, or when you
run OVMF with this patch on a qemu that doesn't export the
/etc/acpi/tables fw_cfg file, then OVMF relies on its own code to
determine the window boundaries, and installs DSDT+SSDT tables of its
own making. The values I quoted above originate from this OVMF
functionality.

>>   ACPI PciWindow64: Base=0x00000000 End=0x00000000 Length=0x00000000
>>
>> (inclusive End). That is, the windows starts right above the
>> conventional memory; 0xA0000000 == 2560M.
>>
>> However, the ACPI table exported by qemu says
>>
>>   begin32=c0000000 end32=fec00000 begin64=0 end64=0
>>
>> (exclusive end32).
>>
>> Even though this latter range is a proper subset of the former, it
>> breaks the cirrus display in both said guests (didn't test more guests).
>>
>> When I applied this PoC patch on qemu, the displays started to work:
>>
>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> index edc974e..305b786 100644
>> --- a/hw/pci-host/piix.c
>> +++ b/hw/pci-host/piix.c
>> @@ -348,11 +348,11 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>>      /* Set PCI window size the way seabios has always done it. */
>>      /* Power of 2 so bios can cover it with a single MTRR */
>>      if (ram_size <= 0x80000000) {
>>          i440fx->pci_info.w32.begin = 0x80000000;
>>      } else if (ram_size <= 0xc0000000) {
>> -        i440fx->pci_info.w32.begin = 0xc0000000;
>> +        i440fx->pci_info.w32.begin = 0xa0000000;
> You cant just move it for compatibility reasons,
> since similar code is hardcoded in Seabios since forever.

Only since commit b1c35f2b ("pciinit: Align start of PCI memory on i440
chipset."), Nov 26, 2012. The only justification given is "simplify mtrr
ranges".

The problem is, when OVMF forwards qemu's tables to the OS, rather than
creating and installing its own tables for the OS, the video output
breaks in the OS.

Anyway, as qemu owns (actually, qemu *is*) the hardware, and it deems
the above range appropriate, OVMF must accept it. The bug reproduces
with the RHEL-6 guest too, so I hope I can look into it. (Early dmesg on
serial would be a start.)

Thanks
Laszlo

Re: [Qemu-devel] [edk2] [edk2 PATCH 0/1] OvmfPkg: grab ACPI tables from QEMU

[Gerd Hoffmann]

  Hi,

>   ACPI PciWindow32: Base=0xA0000000 End=0xFEEFFFFF Length=0x5EF00000

>   begin32=c0000000 end32=fec00000 begin64=0 end64=0

qemu & seabios pick a 32bit window size which allows to cover it with a
single mtrr entry.  Size must be a power of two for that to work.

  [root@fedora ~]# cat /proc/mtrr 
  reg00: base=0x080000000 ( 2048MB), size= 2048MB, count=1: uncachable

So we have three cases for piix (as you've figured in the source code
already).  Start at 0x80000000 (2G window), 0xc0000000 (1G window) and
0xe0000000 (512M window).

btw: q35 has a fixed 1G window, max low ram addr is 0xb000000, the
remaining address space (0xb0000000 -> 0xc0000000) is used for mmconfig.

> I guess the range starting at 0xc0000000 is somehow "incompatible" with
> the EFI memory map. (I can't actually explain this idea because, again,
> this second range is a proper subset of the former, and its size is
> still 1004MB.)

Probably efi places the gfx memory bar somewhere between 0xa0000000 and
0xc0000000.  Sets up efifb accordingly.  Then the linux kernel loads and
figures "Oh, that bar is outside the 0xc0000000+ window" and goes remap
it -> efifb writes go into nowhere.

cheers,
  Gerd

Re: [Qemu-devel] [edk2] [edk2 PATCH 0/1] OvmfPkg: grab ACPI tables from QEMU

[Laszlo Ersek]

[-- Attachment #1: Type: text/plain, Size: 2863 bytes --]

On 11/19/13 09:54, Gerd Hoffmann wrote:
>   Hi,
>
>>   ACPI PciWindow32: Base=0xA0000000 End=0xFEEFFFFF Length=0x5EF00000
>
>>   begin32=c0000000 end32=fec00000 begin64=0 end64=0
>
> qemu & seabios pick a 32bit window size which allows to cover it with
> a single mtrr entry.  Size must be a power of two for that to work.
>
>   [root@fedora ~]# cat /proc/mtrr
>   reg00: base=0x080000000 ( 2048MB), size= 2048MB, count=1: uncachable
>
> So we have three cases for piix (as you've figured in the source code
> already).  Start at 0x80000000 (2G window), 0xc0000000 (1G window) and
> 0xe0000000 (512M window).
>
> btw: q35 has a fixed 1G window, max low ram addr is 0xb000000, the
> remaining address space (0xb0000000 -> 0xc0000000) is used for
> mmconfig.
>
>> I guess the range starting at 0xc0000000 is somehow "incompatible"
>> with the EFI memory map. (I can't actually explain this idea because,
>> again, this second range is a proper subset of the former, and its
>> size is still 1004MB.)
>
> Probably efi places the gfx memory bar somewhere between 0xa0000000
> and 0xc0000000.  Sets up efifb accordingly.  Then the linux kernel
> loads and figures "Oh, that bar is outside the 0xc0000000+ window" and
> goes remap it -> efifb writes go into nowhere.

Thank you for the explanation.

How do I fix it though? :) I could change the MMIO HOBs in PEI
(OvmfPkg/PlatformPei/Platform.c):

  //
  // Video memory + Legacy BIOS region
  //
  AddIoMemoryRangeHob (0x0A0000, BASE_1MB);

  //
  // address       purpose   size
  // ------------  --------  -------------------------
  // max(top, 2g)  PCI MMIO  0xFC000000 - max(top, 2g)
  // 0xFC000000    gap                           44 MB
  // 0xFEC00000    IO-APIC                        4 KB
  // 0xFEC01000    gap                         1020 KB
  // 0xFED00000    HPET                           1 KB
  // 0xFED00400    gap                         1023 KB
  // 0xFEE00000    LAPIC                          1 MB
  //
  AddIoMemoryRangeHob (TopOfMemory < BASE_2GB ? BASE_2GB : TopOfMemory, 0xFC000000);
  AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
  AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
  AddIoMemoryBaseSizeHob (PcdGet32(PcdCpuLocalApicBaseAddress), SIZE_1MB);

to imitate the same three cases. The HOB with the lowest address
produced here would affect the BAR placement as well.

... Yes, I tested the attached patch, and it makes the display work in
both 2560MB guests.

However, I don't like the idea of hardwiring those boundaries here. (The
current values are also hardwired, but they are better: they are not the
consequence of "SeaBIOS has done it like this forever" -- inter-firmware
dependency, especially when they aren't each other's payloads, is bad
IMHO.) We'd need something dynamic here, like a memory map from qemu.

... Which puts us in the same boat with Wei :)

Thanks
Laszlo

[-- Attachment #2: 0001-OvmfPkg-PlatformPei-follow-SeaBIOS-tradition-with-32.patch --]
[-- Type: text/plain, Size: 1679 bytes --]

From 9cf2af82399d7d7a9717ff6ac17860b66c705a64 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Tue, 19 Nov 2013 13:07:41 +0100
Subject: [PATCH] OvmfPkg/PlatformPei: follow SeaBIOS tradition with 32-bit PCI
 hole placement

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/PlatformPei/Platform.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index fb56e99..5bc0d74 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -197,7 +197,7 @@ MemMapInitialization (
   //
   // address       purpose   size
   // ------------  --------  -------------------------
-  // max(top, 2g)  PCI MMIO  0xFC000000 - max(top, 2g)
+  // 2G/3G/3.5G    PCI MMIO    0xFC000000 - 2G/3G/3.5G
   // 0xFC000000    gap                           44 MB
   // 0xFEC00000    IO-APIC                        4 KB
   // 0xFEC01000    gap                         1020 KB
@@ -205,7 +205,9 @@ MemMapInitialization (
   // 0xFED00400    gap                         1023 KB
   // 0xFEE00000    LAPIC                          1 MB
   //
-  AddIoMemoryRangeHob (TopOfMemory < BASE_2GB ? BASE_2GB : TopOfMemory, 0xFC000000);
+  AddIoMemoryRangeHob (TopOfMemory <= 0x80000000 ? 0x80000000 :
+                       TopOfMemory <= 0xC0000000 ? 0xC0000000 :
+                       0xE0000000, 0xFC000000);
   AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
   AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
   AddIoMemoryBaseSizeHob (PcdGet32(PcdCpuLocalApicBaseAddress), SIZE_1MB);
-- 
1.8.3.1

Re: [Qemu-devel] [edk2] [edk2 PATCH 0/1] OvmfPkg: grab ACPI tables from QEMU

[Michael S. Tsirkin]

On Tue, Nov 19, 2013 at 01:31:39PM +0100, Laszlo Ersek wrote:
> On 11/19/13 09:54, Gerd Hoffmann wrote:
> >   Hi,
> >
> >>   ACPI PciWindow32: Base=0xA0000000 End=0xFEEFFFFF Length=0x5EF00000
> >
> >>   begin32=c0000000 end32=fec00000 begin64=0 end64=0
> >
> > qemu & seabios pick a 32bit window size which allows to cover it with
> > a single mtrr entry.  Size must be a power of two for that to work.
> >
> >   [root@fedora ~]# cat /proc/mtrr
> >   reg00: base=0x080000000 ( 2048MB), size= 2048MB, count=1: uncachable
> >
> > So we have three cases for piix (as you've figured in the source code
> > already).  Start at 0x80000000 (2G window), 0xc0000000 (1G window) and
> > 0xe0000000 (512M window).
> >
> > btw: q35 has a fixed 1G window, max low ram addr is 0xb000000, the
> > remaining address space (0xb0000000 -> 0xc0000000) is used for
> > mmconfig.

Is it really fixed? My understanding is it's programmed by firmware,
you can put mmconfig whereever you want.

> >> I guess the range starting at 0xc0000000 is somehow "incompatible"
> >> with the EFI memory map. (I can't actually explain this idea because,
> >> again, this second range is a proper subset of the former, and its
> >> size is still 1004MB.)
> >
> > Probably efi places the gfx memory bar somewhere between 0xa0000000
> > and 0xc0000000.  Sets up efifb accordingly.  Then the linux kernel
> > loads and figures "Oh, that bar is outside the 0xc0000000+ window" and
> > goes remap it -> efifb writes go into nowhere.
> 
> Thank you for the explanation.
> 
> How do I fix it though? :) I could change the MMIO HOBs in PEI
> (OvmfPkg/PlatformPei/Platform.c):
> 
>   //
>   // Video memory + Legacy BIOS region
>   //
>   AddIoMemoryRangeHob (0x0A0000, BASE_1MB);
> 
>   //
>   // address       purpose   size
>   // ------------  --------  -------------------------
>   // max(top, 2g)  PCI MMIO  0xFC000000 - max(top, 2g)
>   // 0xFC000000    gap                           44 MB
>   // 0xFEC00000    IO-APIC                        4 KB
>   // 0xFEC01000    gap                         1020 KB
>   // 0xFED00000    HPET                           1 KB
>   // 0xFED00400    gap                         1023 KB
>   // 0xFEE00000    LAPIC                          1 MB
>   //
>   AddIoMemoryRangeHob (TopOfMemory < BASE_2GB ? BASE_2GB : TopOfMemory, 0xFC000000);
>   AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
>   AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
>   AddIoMemoryBaseSizeHob (PcdGet32(PcdCpuLocalApicBaseAddress), SIZE_1MB);
> 
> to imitate the same three cases. The HOB with the lowest address
> produced here would affect the BAR placement as well.
> 
> ... Yes, I tested the attached patch, and it makes the display work in
> both 2560MB guests.
> 
> However, I don't like the idea of hardwiring those boundaries here. (The
> current values are also hardwired, but they are better: they are not the
> consequence of "SeaBIOS has done it like this forever" -- inter-firmware
> dependency, especially when they aren't each other's payloads, is bad
> IMHO.) We'd need something dynamic here, like a memory map from qemu.
> 
> ... Which puts us in the same boat with Wei :)
> 
> Thanks
> Laszlo

> From 9cf2af82399d7d7a9717ff6ac17860b66c705a64 Mon Sep 17 00:00:00 2001
> From: Laszlo Ersek <lersek@redhat.com>
> Date: Tue, 19 Nov 2013 13:07:41 +0100
> Subject: [PATCH] OvmfPkg/PlatformPei: follow SeaBIOS tradition with 32-bit PCI
>  hole placement
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/PlatformPei/Platform.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index fb56e99..5bc0d74 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -197,7 +197,7 @@ MemMapInitialization (
>    //
>    // address       purpose   size
>    // ------------  --------  -------------------------
> -  // max(top, 2g)  PCI MMIO  0xFC000000 - max(top, 2g)
> +  // 2G/3G/3.5G    PCI MMIO    0xFC000000 - 2G/3G/3.5G
>    // 0xFC000000    gap                           44 MB
>    // 0xFEC00000    IO-APIC                        4 KB
>    // 0xFEC01000    gap                         1020 KB
> @@ -205,7 +205,9 @@ MemMapInitialization (
>    // 0xFED00400    gap                         1023 KB
>    // 0xFEE00000    LAPIC                          1 MB
>    //
> -  AddIoMemoryRangeHob (TopOfMemory < BASE_2GB ? BASE_2GB : TopOfMemory, 0xFC000000);
> +  AddIoMemoryRangeHob (TopOfMemory <= 0x80000000 ? 0x80000000 :
> +                       TopOfMemory <= 0xC0000000 ? 0xC0000000 :
> +                       0xE0000000, 0xFC000000);
>    AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
>    AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
>    AddIoMemoryBaseSizeHob (PcdGet32(PcdCpuLocalApicBaseAddress), SIZE_1MB);
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [edk2] [edk2 PATCH 0/1] OvmfPkg: grab ACPI tables from QEMU

[Gerd Hoffmann]

  Hi,

> > > btw: q35 has a fixed 1G window, max low ram addr is 0xb000000, the
> > > remaining address space (0xb0000000 -> 0xc0000000) is used for
> > > mmconfig.
> 
> Is it really fixed? My understanding is it's programmed by firmware,
> you can put mmconfig whereever you want.

Not really fixed, but it is a most useful configuration.  You can place
large bars (up to 512M) @ 0xc0000000 then, and you have a single region
for your bars which makes management easy.  If you place the mmconfig
somewhere else it only gets more complicated.

cheers,
  Gerd

Re: [Qemu-devel] [edk2] [edk2 PATCH 0/1] OvmfPkg: grab ACPI tables from QEMU

[Michael S. Tsirkin]

On Tue, Nov 19, 2013 at 01:31:39PM +0100, Laszlo Ersek wrote:
> On 11/19/13 09:54, Gerd Hoffmann wrote:
> >   Hi,
> >
> >>   ACPI PciWindow32: Base=0xA0000000 End=0xFEEFFFFF Length=0x5EF00000
> >
> >>   begin32=c0000000 end32=fec00000 begin64=0 end64=0
> >
> > qemu & seabios pick a 32bit window size which allows to cover it with
> > a single mtrr entry.  Size must be a power of two for that to work.
> >
> >   [root@fedora ~]# cat /proc/mtrr
> >   reg00: base=0x080000000 ( 2048MB), size= 2048MB, count=1: uncachable
> >
> > So we have three cases for piix (as you've figured in the source code
> > already).  Start at 0x80000000 (2G window), 0xc0000000 (1G window) and
> > 0xe0000000 (512M window).
> >
> > btw: q35 has a fixed 1G window, max low ram addr is 0xb000000, the
> > remaining address space (0xb0000000 -> 0xc0000000) is used for
> > mmconfig.
> >
> >> I guess the range starting at 0xc0000000 is somehow "incompatible"
> >> with the EFI memory map. (I can't actually explain this idea because,
> >> again, this second range is a proper subset of the former, and its
> >> size is still 1004MB.)
> >
> > Probably efi places the gfx memory bar somewhere between 0xa0000000
> > and 0xc0000000.  Sets up efifb accordingly.  Then the linux kernel
> > loads and figures "Oh, that bar is outside the 0xc0000000+ window" and
> > goes remap it -> efifb writes go into nowhere.
> 
> Thank you for the explanation.
> 
> How do I fix it though? :) I could change the MMIO HOBs in PEI
> (OvmfPkg/PlatformPei/Platform.c):
> 
>   //
>   // Video memory + Legacy BIOS region
>   //
>   AddIoMemoryRangeHob (0x0A0000, BASE_1MB);
> 
>   //
>   // address       purpose   size
>   // ------------  --------  -------------------------
>   // max(top, 2g)  PCI MMIO  0xFC000000 - max(top, 2g)
>   // 0xFC000000    gap                           44 MB
>   // 0xFEC00000    IO-APIC                        4 KB
>   // 0xFEC01000    gap                         1020 KB
>   // 0xFED00000    HPET                           1 KB
>   // 0xFED00400    gap                         1023 KB
>   // 0xFEE00000    LAPIC                          1 MB
>   //
>   AddIoMemoryRangeHob (TopOfMemory < BASE_2GB ? BASE_2GB : TopOfMemory, 0xFC000000);
>   AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
>   AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
>   AddIoMemoryBaseSizeHob (PcdGet32(PcdCpuLocalApicBaseAddress), SIZE_1MB);
> 
> to imitate the same three cases. The HOB with the lowest address
> produced here would affect the BAR placement as well.
> 
> ... Yes, I tested the attached patch, and it makes the display work in
> both 2560MB guests.
> 
> However, I don't like the idea of hardwiring those boundaries here. (The
> current values are also hardwired, but they are better: they are not the
> consequence of "SeaBIOS has done it like this forever" -- inter-firmware
> dependency, especially when they aren't each other's payloads, is bad
> IMHO.) We'd need something dynamic here, like a memory map from qemu.
> 
> ... Which puts us in the same boat with Wei :)
> 
> Thanks
> Laszlo

So the bug is really qemu being too conservative with it's ACPI
tables, isn't it?
The MTRRs in BIOS don't affect much: AFAIK they are completely ignored
if it's exiting to the host, but I'm not sure about assigned
devices.
Alex, can you remind us all what happens there?


> From 9cf2af82399d7d7a9717ff6ac17860b66c705a64 Mon Sep 17 00:00:00 2001
> From: Laszlo Ersek <lersek@redhat.com>
> Date: Tue, 19 Nov 2013 13:07:41 +0100
> Subject: [PATCH] OvmfPkg/PlatformPei: follow SeaBIOS tradition with 32-bit PCI
>  hole placement
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/PlatformPei/Platform.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index fb56e99..5bc0d74 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -197,7 +197,7 @@ MemMapInitialization (
>    //
>    // address       purpose   size
>    // ------------  --------  -------------------------
> -  // max(top, 2g)  PCI MMIO  0xFC000000 - max(top, 2g)
> +  // 2G/3G/3.5G    PCI MMIO    0xFC000000 - 2G/3G/3.5G
>    // 0xFC000000    gap                           44 MB
>    // 0xFEC00000    IO-APIC                        4 KB
>    // 0xFEC01000    gap                         1020 KB
> @@ -205,7 +205,9 @@ MemMapInitialization (
>    // 0xFED00400    gap                         1023 KB
>    // 0xFEE00000    LAPIC                          1 MB
>    //
> -  AddIoMemoryRangeHob (TopOfMemory < BASE_2GB ? BASE_2GB : TopOfMemory, 0xFC000000);
> +  AddIoMemoryRangeHob (TopOfMemory <= 0x80000000 ? 0x80000000 :
> +                       TopOfMemory <= 0xC0000000 ? 0xC0000000 :
> +                       0xE0000000, 0xFC000000);
>    AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
>    AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
>    AddIoMemoryBaseSizeHob (PcdGet32(PcdCpuLocalApicBaseAddress), SIZE_1MB);
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [edk2] [edk2 PATCH 0/1] OvmfPkg: grab ACPI tables from QEMU

[Laszlo Ersek]

On 11/19/13 22:13, Michael S. Tsirkin wrote:
> On Tue, Nov 19, 2013 at 01:31:39PM +0100, Laszlo Ersek wrote:
>> On 11/19/13 09:54, Gerd Hoffmann wrote:
>>>   Hi,
>>>
>>>>   ACPI PciWindow32: Base=0xA0000000 End=0xFEEFFFFF Length=0x5EF00000
>>>
>>>>   begin32=c0000000 end32=fec00000 begin64=0 end64=0
>>>
>>> qemu & seabios pick a 32bit window size which allows to cover it with
>>> a single mtrr entry.  Size must be a power of two for that to work.
>>>
>>>   [root@fedora ~]# cat /proc/mtrr
>>>   reg00: base=0x080000000 ( 2048MB), size= 2048MB, count=1: uncachable
>>>
>>> So we have three cases for piix (as you've figured in the source code
>>> already).  Start at 0x80000000 (2G window), 0xc0000000 (1G window) and
>>> 0xe0000000 (512M window).
>>>
>>> btw: q35 has a fixed 1G window, max low ram addr is 0xb000000, the
>>> remaining address space (0xb0000000 -> 0xc0000000) is used for
>>> mmconfig.
>>>
>>>> I guess the range starting at 0xc0000000 is somehow "incompatible"
>>>> with the EFI memory map. (I can't actually explain this idea because,
>>>> again, this second range is a proper subset of the former, and its
>>>> size is still 1004MB.)
>>>
>>> Probably efi places the gfx memory bar somewhere between 0xa0000000
>>> and 0xc0000000.  Sets up efifb accordingly.  Then the linux kernel
>>> loads and figures "Oh, that bar is outside the 0xc0000000+ window" and
>>> goes remap it -> efifb writes go into nowhere.
>>
>> Thank you for the explanation.
>>
>> How do I fix it though? :) I could change the MMIO HOBs in PEI
>> (OvmfPkg/PlatformPei/Platform.c):
>>
>>   //
>>   // Video memory + Legacy BIOS region
>>   //
>>   AddIoMemoryRangeHob (0x0A0000, BASE_1MB);
>>
>>   //
>>   // address       purpose   size
>>   // ------------  --------  -------------------------
>>   // max(top, 2g)  PCI MMIO  0xFC000000 - max(top, 2g)
>>   // 0xFC000000    gap                           44 MB
>>   // 0xFEC00000    IO-APIC                        4 KB
>>   // 0xFEC01000    gap                         1020 KB
>>   // 0xFED00000    HPET                           1 KB
>>   // 0xFED00400    gap                         1023 KB
>>   // 0xFEE00000    LAPIC                          1 MB
>>   //
>>   AddIoMemoryRangeHob (TopOfMemory < BASE_2GB ? BASE_2GB : TopOfMemory, 0xFC000000);
>>   AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
>>   AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
>>   AddIoMemoryBaseSizeHob (PcdGet32(PcdCpuLocalApicBaseAddress), SIZE_1MB);
>>
>> to imitate the same three cases. The HOB with the lowest address
>> produced here would affect the BAR placement as well.
>>
>> ... Yes, I tested the attached patch, and it makes the display work in
>> both 2560MB guests.
>>
>> However, I don't like the idea of hardwiring those boundaries here. (The
>> current values are also hardwired, but they are better: they are not the
>> consequence of "SeaBIOS has done it like this forever" -- inter-firmware
>> dependency, especially when they aren't each other's payloads, is bad
>> IMHO.) We'd need something dynamic here, like a memory map from qemu.
>>
>> ... Which puts us in the same boat with Wei :)
>>
>> Thanks
>> Laszlo
> 
> So the bug is really qemu being too conservative with it's ACPI
> tables, isn't it?

Here's what I think happens (and I'll shamelessly steal Gerd's explanation):

(1) OVMF configures its own memory map very early (in PEI). The ranges
configured are quite static. The only variable that goes into this
configuration is "top of RAM below 4G", which is capped at 3.5GB by qemu
(the variable comes from the CMOS):

  TopOfMemory <=  2G ---> PCI hole starts at 2G
  TopOfMemory >   2G ---> PCI hole starts at TopOfMemory
  TopOfMemory > 3.5G ---> not possible

The low bound of the 32-bit PCI hole determined thus further affects two
things (in a later phase (DXE)) of OVMF:

(2) PCI resource allocation -- in practice the video framebuffer gets
allocated right at the start of the hole,
(3) the value that OVMF's own ACPI table population code advertises in
the PCI bridge's _CRS.


              > (2) PCI resource alloc
             /
  (1) memmap
            \
             > (3) ACPI
                       \
                        > (4) guest OS access -- match (2)

So, until this patch, everything was in sync. With this patch, the low
bound exported over ACPI -- in (3) -- got replaced (whole-sale) by
qemu's ACPI table. Therefore (1) only determined (2), and a conflict
ensued between (2) and (3). (Which is where I refer back to Gerd's
explanation -- the guest OS tries to access the framebuffer according to
(3), the value in the ACPI table, but that's not where the framebuffer
has actually been configured in (2), which had keyed off (1).)

              > (2) PCI resource alloc
             /
  (1) memmap

   (X) qemu
           \
            > (3) ACPI
                      \
                       > (4) guest OS access -- should match (2),
                                                but doesn't


The *real* bug is that OVMF's memory map configuration (1) is not
*dynamically* synchronized with qemu's ACPI table population logic (X).
My patch that I attached earlier to this thread fixes this (by simply
duplicating the same "quantization" logic), but it's ugly:

              > (2) PCI resource alloc
             /
  (1) memmap
        ||
   (X) qemu
           \
            > (3) ACPI
                      \
                       > (4) guest OS access -- matches (2) by way of
                                                code duplication
                                                between (1) and (X)

What we really should implement is this:

                          > (2) PCI resource alloc
                         /
            > (1) memmap
           /
   (X) qemu
           \
            > (3) ACPI
                       \
                        > (4) guest OS access -- matches (2) by (X)
                                                 being the common root
                                                 of everything

That is, OVMF should compose its own memmap in (1) based on dynamic
information from qemu (importantly, the start address of the 32-bit PCI
hole that qemu is going to advertise later in the ACPI tables).

> The MTRRs in BIOS don't affect much: AFAIK they are completely ignored
> if it's exiting to the host, but I'm not sure about assigned
> devices.
> Alex, can you remind us all what happens there?

If you simply relax (drop) the stage-wise hole-setting, that will at
best create a situation where OVMF and qemu duplicate the logic. This
info needs to be passed down to OVMF (maybe it already is, somewhere in
fw_cfg!), in some form that's more approachable than an ACPI table. (We
absolutely don't want to parse ACPI (AML) in OVMF.)

Thanks!
Laszlo

Re: [Qemu-devel] [edk2] [edk2 PATCH 0/1] OvmfPkg: grab ACPI tables from QEMU

[Michael S. Tsirkin]

On Tue, Nov 19, 2013 at 10:39:06PM +0100, Laszlo Ersek wrote:
> On 11/19/13 22:13, Michael S. Tsirkin wrote:
> > On Tue, Nov 19, 2013 at 01:31:39PM +0100, Laszlo Ersek wrote:
> >> On 11/19/13 09:54, Gerd Hoffmann wrote:
> >>>   Hi,
> >>>
> >>>>   ACPI PciWindow32: Base=0xA0000000 End=0xFEEFFFFF Length=0x5EF00000
> >>>
> >>>>   begin32=c0000000 end32=fec00000 begin64=0 end64=0
> >>>
> >>> qemu & seabios pick a 32bit window size which allows to cover it with
> >>> a single mtrr entry.  Size must be a power of two for that to work.
> >>>
> >>>   [root@fedora ~]# cat /proc/mtrr
> >>>   reg00: base=0x080000000 ( 2048MB), size= 2048MB, count=1: uncachable
> >>>
> >>> So we have three cases for piix (as you've figured in the source code
> >>> already).  Start at 0x80000000 (2G window), 0xc0000000 (1G window) and
> >>> 0xe0000000 (512M window).
> >>>
> >>> btw: q35 has a fixed 1G window, max low ram addr is 0xb000000, the
> >>> remaining address space (0xb0000000 -> 0xc0000000) is used for
> >>> mmconfig.
> >>>
> >>>> I guess the range starting at 0xc0000000 is somehow "incompatible"
> >>>> with the EFI memory map. (I can't actually explain this idea because,
> >>>> again, this second range is a proper subset of the former, and its
> >>>> size is still 1004MB.)
> >>>
> >>> Probably efi places the gfx memory bar somewhere between 0xa0000000
> >>> and 0xc0000000.  Sets up efifb accordingly.  Then the linux kernel
> >>> loads and figures "Oh, that bar is outside the 0xc0000000+ window" and
> >>> goes remap it -> efifb writes go into nowhere.
> >>
> >> Thank you for the explanation.
> >>
> >> How do I fix it though? :) I could change the MMIO HOBs in PEI
> >> (OvmfPkg/PlatformPei/Platform.c):
> >>
> >>   //
> >>   // Video memory + Legacy BIOS region
> >>   //
> >>   AddIoMemoryRangeHob (0x0A0000, BASE_1MB);
> >>
> >>   //
> >>   // address       purpose   size
> >>   // ------------  --------  -------------------------
> >>   // max(top, 2g)  PCI MMIO  0xFC000000 - max(top, 2g)
> >>   // 0xFC000000    gap                           44 MB
> >>   // 0xFEC00000    IO-APIC                        4 KB
> >>   // 0xFEC01000    gap                         1020 KB
> >>   // 0xFED00000    HPET                           1 KB
> >>   // 0xFED00400    gap                         1023 KB
> >>   // 0xFEE00000    LAPIC                          1 MB
> >>   //
> >>   AddIoMemoryRangeHob (TopOfMemory < BASE_2GB ? BASE_2GB : TopOfMemory, 0xFC000000);
> >>   AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
> >>   AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
> >>   AddIoMemoryBaseSizeHob (PcdGet32(PcdCpuLocalApicBaseAddress), SIZE_1MB);
> >>
> >> to imitate the same three cases. The HOB with the lowest address
> >> produced here would affect the BAR placement as well.
> >>
> >> ... Yes, I tested the attached patch, and it makes the display work in
> >> both 2560MB guests.
> >>
> >> However, I don't like the idea of hardwiring those boundaries here. (The
> >> current values are also hardwired, but they are better: they are not the
> >> consequence of "SeaBIOS has done it like this forever" -- inter-firmware
> >> dependency, especially when they aren't each other's payloads, is bad
> >> IMHO.) We'd need something dynamic here, like a memory map from qemu.
> >>
> >> ... Which puts us in the same boat with Wei :)
> >>
> >> Thanks
> >> Laszlo
> > 
> > So the bug is really qemu being too conservative with it's ACPI
> > tables, isn't it?
> 
> Here's what I think happens (and I'll shamelessly steal Gerd's explanation):
> 
> (1) OVMF configures its own memory map very early (in PEI). The ranges
> configured are quite static. The only variable that goes into this
> configuration is "top of RAM below 4G", which is capped at 3.5GB by qemu
> (the variable comes from the CMOS):
> 
>   TopOfMemory <=  2G ---> PCI hole starts at 2G
>   TopOfMemory >   2G ---> PCI hole starts at TopOfMemory
>   TopOfMemory > 3.5G ---> not possible
> 
> The low bound of the 32-bit PCI hole determined thus further affects two
> things (in a later phase (DXE)) of OVMF:
> 
> (2) PCI resource allocation -- in practice the video framebuffer gets
> allocated right at the start of the hole,
> (3) the value that OVMF's own ACPI table population code advertises in
> the PCI bridge's _CRS.
> 
> 
>               > (2) PCI resource alloc
>              /
>   (1) memmap
>             \
>              > (3) ACPI
>                        \
>                         > (4) guest OS access -- match (2)
> 
> So, until this patch, everything was in sync. With this patch, the low
> bound exported over ACPI -- in (3) -- got replaced (whole-sale) by
> qemu's ACPI table. Therefore (1) only determined (2), and a conflict
> ensued between (2) and (3). (Which is where I refer back to Gerd's
> explanation -- the guest OS tries to access the framebuffer according to
> (3), the value in the ACPI table, but that's not where the framebuffer
> has actually been configured in (2), which had keyed off (1).)
> 
>               > (2) PCI resource alloc
>              /
>   (1) memmap
> 
>    (X) qemu
>            \
>             > (3) ACPI
>                       \
>                        > (4) guest OS access -- should match (2),
>                                                 but doesn't
> 
> 
> The *real* bug is that OVMF's memory map configuration (1) is not
> *dynamically* synchronized with qemu's ACPI table population logic (X).
> My patch that I attached earlier to this thread fixes this (by simply
> duplicating the same "quantization" logic), but it's ugly:
> 
>               > (2) PCI resource alloc
>              /
>   (1) memmap
>         ||
>    (X) qemu
>            \
>             > (3) ACPI
>                       \
>                        > (4) guest OS access -- matches (2) by way of
>                                                 code duplication
>                                                 between (1) and (X)
> 
> What we really should implement is this:
> 
>                           > (2) PCI resource alloc
>                          /
>             > (1) memmap
>            /
>    (X) qemu
>            \
>             > (3) ACPI
>                        \
>                         > (4) guest OS access -- matches (2) by (X)
>                                                  being the common root
>                                                  of everything
> 
> That is, OVMF should compose its own memmap in (1) based on dynamic
> information from qemu (importantly, the start address of the 32-bit PCI
> hole that qemu is going to advertise later in the ACPI tables).
> 
> > The MTRRs in BIOS don't affect much: AFAIK they are completely ignored
> > if it's exiting to the host, but I'm not sure about assigned
> > devices.
> > Alex, can you remind us all what happens there?
> 
> If you simply relax (drop) the stage-wise hole-setting, that will at
> best create a situation where OVMF and qemu duplicate the logic. This
> info needs to be passed down to OVMF (maybe it already is, somewhere in
> fw_cfg!), in some form that's more approachable than an ACPI table. (We
> absolutely don't want to parse ACPI (AML) in OVMF.)
> 
> Thanks!
> Laszlo


So the argument of whether we should make qemu control
PCI BAR mapping for guest is a completely separate issue
I think.
Even if we should, I think making ACPI depend on guest
obeying this logic is a mistake.

I think the whole confusion comes from the pci hole terminology.
There's no PCI hole, at least not on PIIX.
On PIIX what's not memory apic etc, is PCI.
So guest can use everything that isn't memory for PCI.

The only issue is _CRS in our ACPI is kind of seabios specific.
So let's just make it generic, make it really describe hardware:


Two possible fixes, both of them in QEMU:
1. declare all available regions (that resolve to PCI) in _CRS
2. detect where did guest put PCI devices and declare the result in _CRS

2 is exactly what we did for 64 bit BARs.

The only issue I worry about is MTRRs.
Maybe we are lucky and they do not matter for KVM.

-- 
MST

Re: [Qemu-devel] [edk2] [edk2 PATCH 0/1] OvmfPkg: grab ACPI tables from QEMU

[Gerd Hoffmann]

  Hi,

> > If you simply relax (drop) the stage-wise hole-setting, that will at
> > best create a situation where OVMF and qemu duplicate the logic. This
> > info needs to be passed down to OVMF (maybe it already is, somewhere in
> > fw_cfg!), in some form that's more approachable than an ACPI table. (We
> > absolutely don't want to parse ACPI (AML) in OVMF.)

No need to parse aml.  The acpi tables are not fixed, qemu tweaks them
according to the hardware programming done by the guest.

> Two possible fixes, both of them in QEMU:
> 1. declare all available regions (that resolve to PCI) in _CRS
> 2. detect where did guest put PCI devices and declare the result in _CRS
> 
> 2 is exactly what we did for 64 bit BARs.

Well, it's actually a mix, isn't it?  64bit window base address is
derived from guest pci bar mapping, 64bit window size is configurable to
leave room for hotplug (or maybe that is the future plan still ...).

I think we should simply declare end-of-lowram -> ioapic base as 32bit
window.  Looking at the pci bar locations programmed by the guest will
only make this smaller, leaving less space for hotplug, for no good
reason.

> The only issue I worry about is MTRRs.
> Maybe we are lucky and they do not matter for KVM.

I think they are largely cosmetic in virtual machines.  And if ovmf
wants use 0xa000000+ as pci io window it still has the option to create
two mtrr entries to cover the region (one 512m @ 0xa0000000 and one 1G @
0xc0000000).

cheers,
  Gerd