[Qemu-devel] allocation zone extensions for the firmware linker/loader

[Qemu-devel] allocation zone extensions for the firmware linker/loader

[Laszlo Ersek]

Hi,

this message is cross-posted to three lists (qemu, seabios, edk2). I'll
follow up with three patch series, one series for each project. I'll
cross-post all of the patches as well, but I'll add the project name in
the "bag of tags" in the subject lines.

The QEMU series introduces two extensions to the ALLOCATE firmware
linker/loader command.

One extension is a new allocation zone, with value 3, for allowing the
firmware to allocate the fw_cfg blobs in 64-bit address space.

The other extension is a repurposing of the most significant bit (bit 7)
in the zone field. This bit becomes orthogonal to the rest of the zone
field. If the bit is set, it means that QEMU promises the firmware that
the blob referenced by the ALLOCATE command contains no ACPI tables at all.

After introducing these, the QEMU series puts them to use, covering all
of the currently generated ALLOCATE commands, as appropriate. Among the
benefits we can mention
- the removal of the OVMF ACPI SDT Header Probe suppressor from VMGENID
(and from any similar future devices),
- and the fact that the "virt" machine type (and maybe other machine
types) of the arm/aarch64 target will no longer require RAM under 4GB
for ACPI to work.

Both of these extensions are irrelevant for SeaBIOS, therefore the
SeaBIOS patches simply mask out bit 7 (for ignoring the "no ACPI
content" hint), and fall back to the HIGH zone (= 32-bit address space)
when the 64-bit zone is permitted.

In other words, SeaBIOS needs some patches to recognize the new zone
values, but beyond that, the behavior is unchanged.

Both extensions are important for virtual UEFI firmware (OVMF in x86
guests and ArmVirtQemu in aarch64 guests). The edk2 patches add support
to OvmfPkg/AcpiPlatformDxe for the extensions. Please see the commit
messages for details (all the extensions are explained in detail in the
relevant patches for all of the projects).

The patches can cause linker/loader breakage when old firmware is booted
on new QEMU. However, that's no problem (it's nothing new), the next
release of QEMU should bundle the new firmware binaries as always.

New firmware will continue running on old QEMU without issues.

(In case you have sent me emails about this in the last few tens of
hours, please know that I'm not ignoring them, I just haven't seen /
read them. Reading emails every five minutes makes focused work
impossible, so when I'm busy, I tend to read email once per day.)

Thanks
Laszlo

[Qemu-devel] [qemu PATCH 0/7] bios-linker-loader: introduce the NOACPI hint and the 64-bit zone for ALLOCATE

[Laszlo Ersek]

Please see the parent blurb
<http://mid.mail-archive.com/c76b36de-ebf9-c662-d454-0a95b43901e8@redhat.com>
for a high level description.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ben Warren <ben@skyportsystems.com>
Cc: Dongjiu Geng <gengdongjiu@huawei.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>

Thanks
Laszlo

Laszlo Ersek (7):
  hw/acpi/bios-linker-loader: expose allocation zone as an enum
  hw/acpi/bios-linker-loader: introduce "no ACPI tables" content hint
    for ALLOC
  hw/acpi/bios-linker-loader: introduce
    BIOS_LINKER_LOADER_ALLOC_ZONE_64BIT
  hw/acpi/nvdimm: ask the firmware to allocate NVDIMM_DSM_MEM_FILE as
    NOACPI
  hw/acpi/vmgenid: ask the fw to alloc VMGENID_GUID_FW_CFG_FILE as
    NOACPI
  hw/i386/acpi-build: ask the fw to alloc ACPI_BUILD_TPMLOG_FILE with
    64bit/NOACPI
  hw/arm/virt-acpi-build: make the fw alloc blobs with ACPI tables as
    64bit

 docs/specs/vmgenid.txt               | 49 ++++++++++++++++--------------------
 include/hw/acpi/bios-linker-loader.h | 22 +++++++++++++++-
 include/hw/acpi/vmgenid.h            |  3 ---
 hw/acpi/bios-linker-loader.c         | 18 ++++++-------
 hw/acpi/nvdimm.c                     |  4 ++-
 hw/acpi/vmgenid.c                    | 25 ++++++------------
 hw/arm/virt-acpi-build.c             |  6 +++--
 hw/i386/acpi-build.c                 |  9 ++++---
 8 files changed, 70 insertions(+), 66 deletions(-)

-- 
2.9.3

[Qemu-devel] [qemu PATCH 1/7] hw/acpi/bios-linker-loader: expose allocation zone as an enum

[Laszlo Ersek]

In a later patch, we'll introduce another allocation zone (which won't fit
in the "alloc_fseg" bool). For now, just move the enum constants from
"bios-linker-loader.c" to "bios-linker-loader.h", and update the
bios_linker_loader_alloc() function prototype so that callers can directly
pass in the enumeration constants.

This is all the more justified because at the bios_linker_loader_alloc()
call sites, the true/false arguments passed in to the current "alloc_fseg"
boolean parameter are always accompanied by a textual comment that spells
out the actual zone. So this patch improves clarity in itself.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ben Warren <ben@skyportsystems.com>
Cc: Dongjiu Geng <gengdongjiu@huawei.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 include/hw/acpi/bios-linker-loader.h | 10 +++++++++-
 hw/acpi/bios-linker-loader.c         | 14 ++++----------
 hw/acpi/nvdimm.c                     |  3 ++-
 hw/acpi/vmgenid.c                    |  5 +++--
 hw/arm/virt-acpi-build.c             |  4 ++--
 hw/i386/acpi-build.c                 |  6 +++---
 6 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/include/hw/acpi/bios-linker-loader.h b/include/hw/acpi/bios-linker-loader.h
index efe17b0b9cb0..8d55f1fab32b 100644
--- a/include/hw/acpi/bios-linker-loader.h
+++ b/include/hw/acpi/bios-linker-loader.h
@@ -5,17 +5,25 @@
 typedef struct BIOSLinker {
     GArray *cmd_blob;
     GArray *file_list;
 } BIOSLinker;
 
+typedef enum BIOSLinkerLoaderAllocZone {
+    /* request blob allocation in 32-bit memory */
+    BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH = 0x1,
+
+    /* request blob allocation in FSEG zone (useful for the RSDP ACPI table) */
+    BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG = 0x2,
+} BIOSLinkerLoaderAllocZone;
+
 BIOSLinker *bios_linker_loader_init(void);
 
 void bios_linker_loader_alloc(BIOSLinker *linker,
                               const char *file_name,
                               GArray *file_blob,
                               uint32_t alloc_align,
-                              bool alloc_fseg);
+                              BIOSLinkerLoaderAllocZone zone);
 
 void bios_linker_loader_add_checksum(BIOSLinker *linker, const char *file,
                                      unsigned start_offset, unsigned size,
                                      unsigned checksum_offset);
 
diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
index 046183a0f142..9754d98e7345 100644
--- a/hw/acpi/bios-linker-loader.c
+++ b/hw/acpi/bios-linker-loader.c
@@ -38,11 +38,11 @@ struct BiosLinkerLoaderEntry {
     uint32_t command;
     union {
         /*
          * COMMAND_ALLOCATE - allocate a table from @alloc.file
          * subject to @alloc.align alignment (must be power of 2)
-         * and @alloc.zone (can be HIGH or FSEG) requirements.
+         * and @alloc.zone (see BIOSLinkerLoaderAllocZone) requirements.
          *
          * Must appear exactly once for each file, and before
          * this file is referenced by any other command.
          */
         struct {
@@ -104,15 +104,10 @@ enum {
     BIOS_LINKER_LOADER_COMMAND_ADD_POINTER       = 0x2,
     BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM      = 0x3,
     BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER     = 0x4,
 };
 
-enum {
-    BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH = 0x1,
-    BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG = 0x2,
-};
-
 /*
  * BiosLinkerFileEntry:
  *
  * An internal type used for book-keeping file entries
  */
@@ -173,19 +168,19 @@ bios_linker_find_file(const BIOSLinker *linker, const char *name)
  *
  * @linker: linker object instance
  * @file_name: name of the file blob to be loaded
  * @file_blob: pointer to blob corresponding to @file_name
  * @alloc_align: required minimal alignment in bytes. Must be a power of 2.
- * @alloc_fseg: request allocation in FSEG zone (useful for the RSDP ACPI table)
+ * @zone: request allocation in this zone
  *
  * Note: this command must precede any other linker command using this file.
  */
 void bios_linker_loader_alloc(BIOSLinker *linker,
                               const char *file_name,
                               GArray *file_blob,
                               uint32_t alloc_align,
-                              bool alloc_fseg)
+                              BIOSLinkerLoaderAllocZone zone)
 {
     BiosLinkerLoaderEntry entry;
     BiosLinkerFileEntry file = { g_strdup(file_name), file_blob};
 
     assert(!(alloc_align & (alloc_align - 1)));
@@ -195,12 +190,11 @@ void bios_linker_loader_alloc(BIOSLinker *linker,
 
     memset(&entry, 0, sizeof entry);
     strncpy(entry.alloc.file, file_name, sizeof entry.alloc.file - 1);
     entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_ALLOCATE);
     entry.alloc.align = cpu_to_le32(alloc_align);
-    entry.alloc.zone = alloc_fseg ? BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG :
-                                    BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH;
+    entry.alloc.zone = zone;
 
     /* Alloc entries must come first, so prepend them */
     g_array_prepend_vals(linker->cmd_blob, &entry, sizeof entry);
 }
 
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 8e7d6ec03490..91dd0df4b128 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -1261,11 +1261,12 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
     mem_addr_offset = build_append_named_dword(table_data,
                                                NVDIMM_ACPI_MEM_ADDR);
 
     bios_linker_loader_alloc(linker,
                              NVDIMM_DSM_MEM_FILE, dsm_dma_arrea,
-                             sizeof(NvdimmDsmIn), false /* high memory */);
+                             sizeof(NvdimmDsmIn),
+                             BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH);
     bios_linker_loader_add_pointer(linker,
         ACPI_BUILD_TABLE_FILE, mem_addr_offset, sizeof(uint32_t),
         NVDIMM_DSM_MEM_FILE, 0);
     build_header(linker, table_data,
         (void *)(table_data->data + nvdimm_ssdt),
diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
index a32b847fe0df..315d3b3327ed 100644
--- a/hw/acpi/vmgenid.c
+++ b/hw/acpi/vmgenid.c
@@ -88,12 +88,13 @@ void vmgenid_build_acpi(VmGenIdState *vms, GArray *table_data, GArray *guid,
     aml_append(ssdt, method);
 
     g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
 
     /* Allocate guest memory for the Data fw_cfg blob */
-    bios_linker_loader_alloc(linker, VMGENID_GUID_FW_CFG_FILE, guid, 4096,
-                             false /* page boundary, high memory */);
+    bios_linker_loader_alloc(linker, VMGENID_GUID_FW_CFG_FILE, guid,
+                             4096 /* page boundary */,
+                             BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH);
 
     /* Patch address of GUID fw_cfg blob into the ADDR fw_cfg blob
      * so QEMU can write the GUID there.  The address is expected to be
      * < 4GB, but write 64 bits anyway.
      * The address that is patched in is offset in order to implement
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index e5852067f5bd..a378e18b0d97 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -370,11 +370,11 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
     unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
     unsigned xsdt_pa_offset =
         (char *)&rsdp->xsdt_physical_address - rsdp_table->data;
 
     bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16,
-                             true /* fseg memory */);
+                             BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG);
 
     memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
     memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
     rsdp->length = cpu_to_le32(sizeof(*rsdp));
     rsdp->revision = 0x02;
@@ -749,11 +749,11 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     table_offsets = g_array_new(false, true /* clear */,
                                         sizeof(uint32_t));
 
     bios_linker_loader_alloc(tables->linker,
                              ACPI_BUILD_TABLE_FILE, tables_blob,
-                             64, false /* high memory */);
+                             64, BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH);
 
     /* DSDT is pointed to by FADT */
     dsdt = tables_blob->len;
     build_dsdt(tables_blob, tables->linker, vms);
 
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index afcadacd2e7d..4e7b30b44d5a 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2285,11 +2285,11 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
     tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
     tcpa->log_area_minimum_length = cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
     acpi_data_push(tcpalog, le32_to_cpu(tcpa->log_area_minimum_length));
 
     bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE, tcpalog, 1,
-                             false /* high memory */);
+                             BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH);
 
     /* log area start address to be filled by Guest linker */
     bios_linker_loader_add_pointer(linker,
         ACPI_BUILD_TABLE_FILE, log_addr_offset, log_addr_size,
         ACPI_BUILD_TPMLOG_FILE, 0);
@@ -2570,11 +2570,11 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
     unsigned rsdt_pa_size = sizeof(rsdp->rsdt_physical_address);
     unsigned rsdt_pa_offset =
         (char *)&rsdp->rsdt_physical_address - rsdp_table->data;
 
     bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16,
-                             true /* fseg memory */);
+                             BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG);
 
     memcpy(&rsdp->signature, "RSD PTR ", 8);
     memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
     /* Address to be filled by Guest linker */
     bios_linker_loader_add_pointer(linker,
@@ -2649,11 +2649,11 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
     ACPI_BUILD_DPRINTF("init ACPI tables\n");
 
     bios_linker_loader_alloc(tables->linker,
                              ACPI_BUILD_TABLE_FILE, tables_blob,
                              64 /* Ensure FACS is aligned */,
-                             false /* high memory */);
+                             BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH);
 
     /*
      * FACS is pointed to by FADT.
      * We place it first since it's the only table that has alignment
      * requirements.
-- 
2.9.3

[Qemu-devel] [qemu PATCH 2/7] hw/acpi/bios-linker-loader: introduce "no ACPI tables" content hint for ALLOC

[Laszlo Ersek]

OvmfPkg/AcpiPlatformDxe, which implements the client for QEMU's
linker/loader in the OVMF and ArmVirtQemu virtual UEFI firmwares,
currently relies on a 2nd pass processing of the ADD_POINTER commands, to
identify potential ACPI tables in the pointed-to blobs. The reason for
this is that ACPI tables must be individually passed to
EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() for installation.

In order to tell apart ACPI tables from other operation region-like areas
within pointed-to blobs, OvmfPkg/AcpiPlatformDxe employs a heuristic
called "ACPI SDT header probe" at the target locations of the ADD_POINTER
commands. While all ACPI tables generated by QEMU satisfy this check
(i.e., there are no false negatives), blob content that is *not* an ACPI
table has a very slight chance to pass the test as well (i.e., there is a
small chance for false positives).

In order to suppress this small chance, we've historically formatted
opregion-like areas in blobs with a fixed size zero prefix (see e.g.
"docs/specs/vmgenid.txt"), which guarantees that the probe in
OvmfPkg/AcpiPlatformDxe will fail. However, this "suppressor prefix" has
had to be taken into account explicitly in generated AML code -- the
prefix size has had to be added to the patched integer object in AML, at
runtime --, leading to awkwardness.

Introduce a new hint for the ALLOC command, as the most significant bit of
the uint8_t "zone" field, for disabling the ACPI SDT header probe in
OvmfPkg/AcpiPlatformDxe, for all the pointers that point into the blob
downloaded with the ALLOC command. When the bit is set, the blob is
guaranteed to contain no ACPI tables. When the bit is clear, the behavior
is left unchanged.

In this initial patch, all bios_linker_loader_alloc() invocations are left
with intact behavior.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ben Warren <ben@skyportsystems.com>
Cc: Dongjiu Geng <gengdongjiu@huawei.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 include/hw/acpi/bios-linker-loader.h | 11 ++++++++++-
 hw/acpi/bios-linker-loader.c         |  8 ++++++--
 hw/acpi/nvdimm.c                     |  3 ++-
 hw/acpi/vmgenid.c                    |  3 ++-
 hw/arm/virt-acpi-build.c             |  6 ++++--
 hw/i386/acpi-build.c                 |  9 ++++++---
 6 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/include/hw/acpi/bios-linker-loader.h b/include/hw/acpi/bios-linker-loader.h
index 8d55f1fab32b..5202fd14977d 100644
--- a/include/hw/acpi/bios-linker-loader.h
+++ b/include/hw/acpi/bios-linker-loader.h
@@ -13,17 +13,26 @@ typedef enum BIOSLinkerLoaderAllocZone {
 
     /* request blob allocation in FSEG zone (useful for the RSDP ACPI table) */
     BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG = 0x2,
 } BIOSLinkerLoaderAllocZone;
 
+typedef enum BIOSLinkerLoaderAllocContent {
+    /* the blob may or may not contain ACPI tables */
+    BIOS_LINKER_LOADER_ALLOC_CONTENT_MIXED = 0x00,
+
+    /* the blob is guaranteed not to contain ACPI tables */
+    BIOS_LINKER_LOADER_ALLOC_CONTENT_NOACPI = 0x80,
+} BIOSLinkerLoaderAllocContent;
+
 BIOSLinker *bios_linker_loader_init(void);
 
 void bios_linker_loader_alloc(BIOSLinker *linker,
                               const char *file_name,
                               GArray *file_blob,
                               uint32_t alloc_align,
-                              BIOSLinkerLoaderAllocZone zone);
+                              BIOSLinkerLoaderAllocZone zone,
+                              BIOSLinkerLoaderAllocContent content);
 
 void bios_linker_loader_add_checksum(BIOSLinker *linker, const char *file,
                                      unsigned start_offset, unsigned size,
                                      unsigned checksum_offset);
 
diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
index 9754d98e7345..4ad9260fe72d 100644
--- a/hw/acpi/bios-linker-loader.c
+++ b/hw/acpi/bios-linker-loader.c
@@ -39,10 +39,12 @@ struct BiosLinkerLoaderEntry {
     union {
         /*
          * COMMAND_ALLOCATE - allocate a table from @alloc.file
          * subject to @alloc.align alignment (must be power of 2)
          * and @alloc.zone (see BIOSLinkerLoaderAllocZone) requirements.
+         * The most significant bit (bit 7) of @alloc.zone is used as a content
+         * hint for UEFI guest firmware, see BIOSLinkerLoaderAllocContent.
          *
          * Must appear exactly once for each file, and before
          * this file is referenced by any other command.
          */
         struct {
@@ -169,18 +171,20 @@ bios_linker_find_file(const BIOSLinker *linker, const char *name)
  * @linker: linker object instance
  * @file_name: name of the file blob to be loaded
  * @file_blob: pointer to blob corresponding to @file_name
  * @alloc_align: required minimal alignment in bytes. Must be a power of 2.
  * @zone: request allocation in this zone
+ * @content: information about the blob content for the firmware
  *
  * Note: this command must precede any other linker command using this file.
  */
 void bios_linker_loader_alloc(BIOSLinker *linker,
                               const char *file_name,
                               GArray *file_blob,
                               uint32_t alloc_align,
-                              BIOSLinkerLoaderAllocZone zone)
+                              BIOSLinkerLoaderAllocZone zone,
+                              BIOSLinkerLoaderAllocContent content)
 {
     BiosLinkerLoaderEntry entry;
     BiosLinkerFileEntry file = { g_strdup(file_name), file_blob};
 
     assert(!(alloc_align & (alloc_align - 1)));
@@ -190,11 +194,11 @@ void bios_linker_loader_alloc(BIOSLinker *linker,
 
     memset(&entry, 0, sizeof entry);
     strncpy(entry.alloc.file, file_name, sizeof entry.alloc.file - 1);
     entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_ALLOCATE);
     entry.alloc.align = cpu_to_le32(alloc_align);
-    entry.alloc.zone = zone;
+    entry.alloc.zone = zone | content;
 
     /* Alloc entries must come first, so prepend them */
     g_array_prepend_vals(linker->cmd_blob, &entry, sizeof entry);
 }
 
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 91dd0df4b128..81bd0214fb3e 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -1262,11 +1262,12 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
                                                NVDIMM_ACPI_MEM_ADDR);
 
     bios_linker_loader_alloc(linker,
                              NVDIMM_DSM_MEM_FILE, dsm_dma_arrea,
                              sizeof(NvdimmDsmIn),
-                             BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH);
+                             BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH,
+                             BIOS_LINKER_LOADER_ALLOC_CONTENT_MIXED);
     bios_linker_loader_add_pointer(linker,
         ACPI_BUILD_TABLE_FILE, mem_addr_offset, sizeof(uint32_t),
         NVDIMM_DSM_MEM_FILE, 0);
     build_header(linker, table_data,
         (void *)(table_data->data + nvdimm_ssdt),
diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
index 315d3b3327ed..dc97771de5f7 100644
--- a/hw/acpi/vmgenid.c
+++ b/hw/acpi/vmgenid.c
@@ -90,11 +90,12 @@ void vmgenid_build_acpi(VmGenIdState *vms, GArray *table_data, GArray *guid,
     g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
 
     /* Allocate guest memory for the Data fw_cfg blob */
     bios_linker_loader_alloc(linker, VMGENID_GUID_FW_CFG_FILE, guid,
                              4096 /* page boundary */,
-                             BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH);
+                             BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH,
+                             BIOS_LINKER_LOADER_ALLOC_CONTENT_MIXED);
 
     /* Patch address of GUID fw_cfg blob into the ADDR fw_cfg blob
      * so QEMU can write the GUID there.  The address is expected to be
      * < 4GB, but write 64 bits anyway.
      * The address that is patched in is offset in order to implement
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index a378e18b0d97..1c20b851a611 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -370,11 +370,12 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
     unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
     unsigned xsdt_pa_offset =
         (char *)&rsdp->xsdt_physical_address - rsdp_table->data;
 
     bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16,
-                             BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG);
+                             BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG,
+                             BIOS_LINKER_LOADER_ALLOC_CONTENT_MIXED);
 
     memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
     memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
     rsdp->length = cpu_to_le32(sizeof(*rsdp));
     rsdp->revision = 0x02;
@@ -749,11 +750,12 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     table_offsets = g_array_new(false, true /* clear */,
                                         sizeof(uint32_t));
 
     bios_linker_loader_alloc(tables->linker,
                              ACPI_BUILD_TABLE_FILE, tables_blob,
-                             64, BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH);
+                             64, BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH,
+                             BIOS_LINKER_LOADER_ALLOC_CONTENT_MIXED);
 
     /* DSDT is pointed to by FADT */
     dsdt = tables_blob->len;
     build_dsdt(tables_blob, tables->linker, vms);
 
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4e7b30b44d5a..3c4c28c6c2ca 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2285,11 +2285,12 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
     tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
     tcpa->log_area_minimum_length = cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
     acpi_data_push(tcpalog, le32_to_cpu(tcpa->log_area_minimum_length));
 
     bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE, tcpalog, 1,
-                             BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH);
+                             BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH,
+                             BIOS_LINKER_LOADER_ALLOC_CONTENT_MIXED);
 
     /* log area start address to be filled by Guest linker */
     bios_linker_loader_add_pointer(linker,
         ACPI_BUILD_TABLE_FILE, log_addr_offset, log_addr_size,
         ACPI_BUILD_TPMLOG_FILE, 0);
@@ -2570,11 +2571,12 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
     unsigned rsdt_pa_size = sizeof(rsdp->rsdt_physical_address);
     unsigned rsdt_pa_offset =
         (char *)&rsdp->rsdt_physical_address - rsdp_table->data;
 
     bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16,
-                             BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG);
+                             BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG,
+                             BIOS_LINKER_LOADER_ALLOC_CONTENT_MIXED);
 
     memcpy(&rsdp->signature, "RSD PTR ", 8);
     memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
     /* Address to be filled by Guest linker */
     bios_linker_loader_add_pointer(linker,
@@ -2649,11 +2651,12 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
     ACPI_BUILD_DPRINTF("init ACPI tables\n");
 
     bios_linker_loader_alloc(tables->linker,
                              ACPI_BUILD_TABLE_FILE, tables_blob,
                              64 /* Ensure FACS is aligned */,
-                             BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH);
+                             BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH,
+                             BIOS_LINKER_LOADER_ALLOC_CONTENT_MIXED);
 
     /*
      * FACS is pointed to by FADT.
      * We place it first since it's the only table that has alignment
      * requirements.
-- 
2.9.3

[Qemu-devel] [qemu PATCH 3/7] hw/acpi/bios-linker-loader: introduce BIOS_LINKER_LOADER_ALLOC_ZONE_64BIT

[Laszlo Ersek]

Using this allocation zone permits the guest firmware to allocate the blob
being downloaded anywhere in the 64-bit address space. QEMU code that
generates ADD_POINTER commands with @src_file set to such a blob is
responsible for using @dst_patched_offset_size=8.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ben Warren <ben@skyportsystems.com>
Cc: Dongjiu Geng <gengdongjiu@huawei.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 include/hw/acpi/bios-linker-loader.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/hw/acpi/bios-linker-loader.h b/include/hw/acpi/bios-linker-loader.h
index 5202fd14977d..621de7bd98e8 100644
--- a/include/hw/acpi/bios-linker-loader.h
+++ b/include/hw/acpi/bios-linker-loader.h
@@ -11,10 +11,13 @@ typedef enum BIOSLinkerLoaderAllocZone {
     /* request blob allocation in 32-bit memory */
     BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH = 0x1,
 
     /* request blob allocation in FSEG zone (useful for the RSDP ACPI table) */
     BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG = 0x2,
+
+    /* request blob allocation in 64-bit memory */
+    BIOS_LINKER_LOADER_ALLOC_ZONE_64BIT = 0x3,
 } BIOSLinkerLoaderAllocZone;
 
 typedef enum BIOSLinkerLoaderAllocContent {
     /* the blob may or may not contain ACPI tables */
     BIOS_LINKER_LOADER_ALLOC_CONTENT_MIXED = 0x00,
-- 
2.9.3

[Qemu-devel] [qemu PATCH 4/7] hw/acpi/nvdimm: ask the firmware to allocate NVDIMM_DSM_MEM_FILE as NOACPI

[Laszlo Ersek]

The "etc/acpi/nvdimm-mem" fw_cfg blob is guaranteed not to contain ACPI
tables, so turning off the ACPI SDT header probe in OVMF is the right
thing to do.

SeaBIOS needs a patch for recognizing (and masking out) the
BIOS_LINKER_LOADER_ALLOC_CONTENT_NOACPI bit, but its behavior will not
change.

Regarding the allocation zone, we cannot relax that to 64-bit, because the
"MEMA" object (NVDIMM_ACPI_MEM_ADDR), into which the address of
"etc/acpi/nvdimm-mem" is patched, is only a DWORD.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ben Warren <ben@skyportsystems.com>
Cc: Dongjiu Geng <gengdongjiu@huawei.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    I don't know how to test this device, so I didn't. Help from the
    device's maintainer would be highly appreciated. Thanks.

 hw/acpi/nvdimm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 81bd0214fb3e..34b9a0f39a02 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -1263,11 +1263,11 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
 
     bios_linker_loader_alloc(linker,
                              NVDIMM_DSM_MEM_FILE, dsm_dma_arrea,
                              sizeof(NvdimmDsmIn),
                              BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH,
-                             BIOS_LINKER_LOADER_ALLOC_CONTENT_MIXED);
+                             BIOS_LINKER_LOADER_ALLOC_CONTENT_NOACPI);
     bios_linker_loader_add_pointer(linker,
         ACPI_BUILD_TABLE_FILE, mem_addr_offset, sizeof(uint32_t),
         NVDIMM_DSM_MEM_FILE, 0);
     build_header(linker, table_data,
         (void *)(table_data->data + nvdimm_ssdt),
-- 
2.9.3

[Qemu-devel] [qemu PATCH 5/7] hw/acpi/vmgenid: ask the fw to alloc VMGENID_GUID_FW_CFG_FILE as NOACPI

[Laszlo Ersek]

The "etc/vmgenid_guid" fw_cfg blob is guaranteed not to contain ACPI
tables, so turning off the ACPI SDT header probe in OVMF is the right
thing to do.

SeaBIOS needs a patch for recognizing (and masking out) the
BIOS_LINKER_LOADER_ALLOC_CONTENT_NOACPI bit, but its behavior will not
change.

By setting the BIOS_LINKER_LOADER_ALLOC_CONTENT_NOACPI bit, we can
eliminate the "OVMF SDT Header probe suppressor" -- we can shift the GUID
to offset zero from offset 40 decimal (VMGENID_GUID_OFFSET).

Regarding the allocation zone, we cannot relax that to 64-bit, because the
"VGIA" object, into which the address of "etc/vmgenid_guid" is patched, is
only a DWORD.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ben Warren <ben@skyportsystems.com>
Cc: Dongjiu Geng <gengdongjiu@huawei.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    I tested this change extensively,
    - by repeating the steps written up in
      <http://mid.mail-archive.com/c052d05e-71a5-1a6a-f34f-17d14167c2f6@redhat.com>,
    - by doing the same after S3 suspend/resume,
    - by verifying firmware logs,
    - by directly checking ACPI content and dmesg in a Linux guest.
    
    I know Ben has a pending patch titled "[PATCH] tests: Add unit tests for
    the VM Generation ID feature", that one should be adapted as well
    (replace VMGENID_GUID_OFFSET with plain 0). I'd be happy to do that.

 docs/specs/vmgenid.txt    | 49 ++++++++++++++++++++---------------------------
 include/hw/acpi/vmgenid.h |  3 ---
 hw/acpi/vmgenid.c         | 21 ++++----------------
 3 files changed, 25 insertions(+), 48 deletions(-)

diff --git a/docs/specs/vmgenid.txt b/docs/specs/vmgenid.txt
index aa9f5186767c..fb9f372edbc8 100644
--- a/docs/specs/vmgenid.txt
+++ b/docs/specs/vmgenid.txt
@@ -63,76 +63,74 @@ Xen) put it in the main descriptor table (Differentiated System Description
 Table or DSDT).  For ease of debugging and implementation, we have decided to
 put it in its own Secondary System Description Table, or SSDT.
 
 The following is a dump of the contents from a running system:
 
-# iasl -p ./SSDT -d /sys/firmware/acpi/tables/SSDT
+# acpidump -n SSDT -b
+# iasl -d ssdt.dat
 
 Intel ACPI Component Architecture
-ASL+ Optimizing Compiler version 20150717-64
-Copyright (c) 2000 - 2015 Intel Corporation
+ASL+ Optimizing Compiler version 20160527-64
+Copyright (c) 2000 - 2016 Intel Corporation
 
-Reading ACPI table from file /sys/firmware/acpi/tables/SSDT - Length
-00000198 (0x0000C6)
+Input file ssdt.dat, Length 0xC6 (198) bytes
 ACPI: SSDT 0x0000000000000000 0000C6 (v01 BOCHS  VMGENID  00000001 BXPC
 00000001)
-Acpi table [SSDT] successfully installed and loaded
 Pass 1 parse of [SSDT]
 Pass 2 parse of [SSDT]
 Parsing Deferred Opcodes (Methods/Buffers/Packages/Regions)
 
 Parsing completed
 Disassembly completed
-ASL Output:    ./SSDT.dsl - 1631 bytes
-# cat SSDT.dsl
+ASL Output:    ssdt.dsl - 1559 bytes
+# cat ssdt.dsl
 /*
  * Intel ACPI Component Architecture
- * AML/ASL+ Disassembler version 20150717-64
- * Copyright (c) 2000 - 2015 Intel Corporation
+ * AML/ASL+ Disassembler version 20160527-64
+ * Copyright (c) 2000 - 2016 Intel Corporation
  *
  * Disassembling to symbolic ASL+ operators
  *
- * Disassembly of /sys/firmware/acpi/tables/SSDT, Sun Feb  5 00:19:37 2017
+ * Disassembly of ssdt.dat, Fri Jun  2 15:29:10 2017
  *
  * Original Table Header:
  *     Signature        "SSDT"
- *     Length           0x000000CA (202)
+ *     Length           0x000000C6 (198)
  *     Revision         0x01
- *     Checksum         0x4B
+ *     Checksum         0x38
  *     OEM ID           "BOCHS "
  *     OEM Table ID     "VMGENID"
  *     OEM Revision     0x00000001 (1)
  *     Compiler ID      "BXPC"
  *     Compiler Version 0x00000001 (1)
  */
-DefinitionBlock ("/sys/firmware/acpi/tables/SSDT.aml", "SSDT", 1, "BOCHS ",
-"VMGENID", 0x00000001)
+DefinitionBlock ("", "SSDT", 1, "BOCHS ", "VMGENID", 0x00000001)
 {
-    Name (VGIA, 0x07FFF000)
+    Name (VGIA, 0xBFFFF000)
     Scope (\_SB)
     {
         Device (VGEN)
         {
             Name (_HID, "QEMUVGID")  // _HID: Hardware ID
             Name (_CID, "VM_Gen_Counter")  // _CID: Compatible ID
             Name (_DDN, "VM_Gen_Counter")  // _DDN: DOS Device Name
             Method (_STA, 0, NotSerialized)  // _STA: Status
             {
                 Local0 = 0x0F
-                If ((VGIA == Zero))
+                If (VGIA == Zero)
                 {
                     Local0 = Zero
                 }
 
                 Return (Local0)
             }
 
             Method (ADDR, 0, NotSerialized)
             {
                 Local0 = Package (0x02) {}
-                Index (Local0, Zero) = (VGIA + 0x28)
-                Index (Local0, One) = Zero
+                Local0 [Zero] = VGIA /* \VGIA */
+                Local0 [One] = Zero
                 Return (Local0)
             }
         }
     }
 
@@ -197,12 +195,11 @@ GUID values displayed via monitor are shown in big-endian format.
 
 
 GUID Storage Format:
 --------------------
 
-In order to implement an OVMF "SDT Header Probe Suppressor", the contents of
-the vmgenid_guid fw_cfg blob are not simply a 128-bit GUID.  There is also
+The contents of the vmgenid_guid fw_cfg blob are a 128-bit GUID, followed by
 significant padding in order to align and fill a memory page, as shown in the
 following diagram:
 
 +----------------------------------+
 | SSDT with OEM Table ID = VMGENID |
@@ -210,17 +207,13 @@ following diagram:
 | ...                              |       TOP OF PAGE
 | VGIA dword object ---------------|-----> +---------------------------+
 | ...                              |       | fw-allocated array for    |
 | _STA method referring to VGIA    |       | "etc/vmgenid_guid"        |
 | ...                              |       +---------------------------+
-| ADDR method referring to VGIA    |       |  0: OVMF SDT Header probe |
-| ...                              |       |     suppressor            |
-+----------------------------------+       | 36: padding for 8-byte    |
-                                           |     alignment             |
-                                           | 40: GUID                  |
-                                           | 56: padding to page size  |
-                                           +---------------------------+
+| ADDR method referring to VGIA    |       |  0: GUID                  |
+| ...                              |       | 16: padding to page size  |
++----------------------------------+       +---------------------------+
                                            END OF PAGE
 
 
 Device Usage:
 -------------
diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h
index 7beb9592fb01..b7cde45c0dea 100644
--- a/include/hw/acpi/vmgenid.h
+++ b/include/hw/acpi/vmgenid.h
@@ -9,13 +9,10 @@
 #define VMGENID_GUID             "guid"
 #define VMGENID_GUID_FW_CFG_FILE      "etc/vmgenid_guid"
 #define VMGENID_ADDR_FW_CFG_FILE      "etc/vmgenid_addr"
 
 #define VMGENID_FW_CFG_SIZE      4096 /* Occupy a page of memory */
-#define VMGENID_GUID_OFFSET      40   /* allow space for
-                                       * OVMF SDT Header Probe Supressor
-                                       */
 
 #define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE)
 
 typedef struct VmGenIdState {
     DeviceClass parent_obj;
diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
index dc97771de5f7..92067bc52421 100644
--- a/hw/acpi/vmgenid.c
+++ b/hw/acpi/vmgenid.c
@@ -29,16 +29,11 @@ void vmgenid_build_acpi(VmGenIdState *vms, GArray *table_data, GArray *guid,
      * first, since that's what the guest expects
      */
     g_array_set_size(guid, VMGENID_FW_CFG_SIZE - ARRAY_SIZE(guid_le.data));
     guid_le = vms->guid;
     qemu_uuid_bswap(&guid_le);
-    /* The GUID is written at a fixed offset into the fw_cfg file
-     * in order to implement the "OVMF SDT Header probe suppressor"
-     * see docs/specs/vmgenid.txt for more details
-     */
-    g_array_insert_vals(guid, VMGENID_GUID_OFFSET, guid_le.data,
-                        ARRAY_SIZE(guid_le.data));
+    g_array_insert_vals(guid, 0, guid_le.data, ARRAY_SIZE(guid_le.data));
 
     /* Put this in a separate SSDT table */
     ssdt = init_aml_allocator();
 
     /* Reserve space for header */
@@ -70,12 +65,11 @@ void vmgenid_build_acpi(VmGenIdState *vms, GArray *table_data, GArray *guid,
     method = aml_method("ADDR", 0, AML_NOTSERIALIZED);
 
     addr = aml_local(0);
     aml_append(method, aml_store(aml_package(2), addr));
 
-    aml_append(method, aml_store(aml_add(aml_name("VGIA"),
-                                         aml_int(VMGENID_GUID_OFFSET), NULL),
+    aml_append(method, aml_store(aml_name("VGIA"),
                                  aml_index(addr, aml_int(0))));
     aml_append(method, aml_store(aml_int(0), aml_index(addr, aml_int(1))));
     aml_append(method, aml_return(addr));
 
     aml_append(dev, method);
@@ -91,22 +85,19 @@ void vmgenid_build_acpi(VmGenIdState *vms, GArray *table_data, GArray *guid,
 
     /* Allocate guest memory for the Data fw_cfg blob */
     bios_linker_loader_alloc(linker, VMGENID_GUID_FW_CFG_FILE, guid,
                              4096 /* page boundary */,
                              BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH,
-                             BIOS_LINKER_LOADER_ALLOC_CONTENT_MIXED);
+                             BIOS_LINKER_LOADER_ALLOC_CONTENT_NOACPI);
 
     /* Patch address of GUID fw_cfg blob into the ADDR fw_cfg blob
      * so QEMU can write the GUID there.  The address is expected to be
      * < 4GB, but write 64 bits anyway.
-     * The address that is patched in is offset in order to implement
-     * the "OVMF SDT Header probe suppressor"
-     * see docs/specs/vmgenid.txt for more details.
      */
     bios_linker_loader_write_pointer(linker,
         VMGENID_ADDR_FW_CFG_FILE, 0, sizeof(uint64_t),
-        VMGENID_GUID_FW_CFG_FILE, VMGENID_GUID_OFFSET);
+        VMGENID_GUID_FW_CFG_FILE, 0);
 
     /* Patch address of GUID fw_cfg blob into the AML so OSPM can retrieve
      * and read it.  Note that while we provide storage for 64 bits, only
      * the least-signficant 32 get patched into AML.
      */
@@ -150,14 +141,10 @@ static void vmgenid_update_guest(VmGenIdState *vms)
              * however, will expect the fields to be little-endian.
              * Perform a byte swap immediately before writing.
              */
             guid_le = vms->guid;
             qemu_uuid_bswap(&guid_le);
-            /* The GUID is written at a fixed offset into the fw_cfg file
-             * in order to implement the "OVMF SDT Header probe suppressor"
-             * see docs/specs/vmgenid.txt for more details.
-             */
             cpu_physical_memory_write(vmgenid_addr, guid_le.data,
                                       sizeof(guid_le.data));
             /* Send _GPE.E05 event */
             acpi_send_event(DEVICE(obj), ACPI_VMGENID_CHANGE_STATUS);
         }
-- 
2.9.3

[Qemu-devel] [qemu PATCH 6/7] hw/i386/acpi-build: ask the fw to alloc ACPI_BUILD_TPMLOG_FILE with 64bit/NOACPI

[Laszlo Ersek]

The "etc/tpm/log" fw_cfg blob is guaranteed not to contain ACPI tables, so
turning off the ACPI SDT header probe in OVMF is the right thing to do.

In addition, the address of the blob is patched into the
"TCPA.log_area_start_address" field, which has type "uint64_t". Therefore
we can change the allocation zone to 64-bit as well.

SeaBIOS needs a patch for recognizing (and masking out) the
BIOS_LINKER_LOADER_ALLOC_CONTENT_NOACPI bit, and also for handling
BIOS_LINKER_LOADER_ALLOC_ZONE_64BIT the same as
BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH, but its allocation behavior will not
change.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ben Warren <ben@skyportsystems.com>
Cc: Dongjiu Geng <gengdongjiu@huawei.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    I don't know how to test this device, so I didn't. Help from the
    device's maintainer would be highly appreciated. Thanks.

 hw/i386/acpi-build.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 3c4c28c6c2ca..1ec008ec5003 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2285,12 +2285,12 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
     tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
     tcpa->log_area_minimum_length = cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
     acpi_data_push(tcpalog, le32_to_cpu(tcpa->log_area_minimum_length));
 
     bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE, tcpalog, 1,
-                             BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH,
-                             BIOS_LINKER_LOADER_ALLOC_CONTENT_MIXED);
+                             BIOS_LINKER_LOADER_ALLOC_ZONE_64BIT,
+                             BIOS_LINKER_LOADER_ALLOC_CONTENT_NOACPI);
 
     /* log area start address to be filled by Guest linker */
     bios_linker_loader_add_pointer(linker,
         ACPI_BUILD_TABLE_FILE, log_addr_offset, log_addr_size,
         ACPI_BUILD_TPMLOG_FILE, 0);
-- 
2.9.3

[Qemu-devel] [qemu PATCH 7/7] hw/arm/virt-acpi-build: make the fw alloc blobs with ACPI tables as 64bit

[Laszlo Ersek]

Thanks to commit cb51ac2ffe36 ("hw/arm/virt: generate 64-bit addressable
ACPI objects", 2017-04-10), all pointer fields in the ACPI tables in the
"etc/acpi/rsdp" (ACPI_BUILD_RSDP_FILE) and "etc/acpi/tables"
(ACPI_BUILD_TABLE_FILE) fw_cfg blobs are 64-bit wide.

Therefore we can allow the guest firmware to allocate these blobs from
64-bit address space.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ben Warren <ben@skyportsystems.com>
Cc: Dongjiu Geng <gengdongjiu@huawei.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    I verified this change with firmware logs and a Linux guest's dmesg.

 hw/arm/virt-acpi-build.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 1c20b851a611..8648d89decb7 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -370,11 +370,11 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
     unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
     unsigned xsdt_pa_offset =
         (char *)&rsdp->xsdt_physical_address - rsdp_table->data;
 
     bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16,
-                             BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG,
+                             BIOS_LINKER_LOADER_ALLOC_ZONE_64BIT,
                              BIOS_LINKER_LOADER_ALLOC_CONTENT_MIXED);
 
     memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
     memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
     rsdp->length = cpu_to_le32(sizeof(*rsdp));
@@ -750,11 +750,11 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     table_offsets = g_array_new(false, true /* clear */,
                                         sizeof(uint32_t));
 
     bios_linker_loader_alloc(tables->linker,
                              ACPI_BUILD_TABLE_FILE, tables_blob,
-                             64, BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH,
+                             64, BIOS_LINKER_LOADER_ALLOC_ZONE_64BIT,
                              BIOS_LINKER_LOADER_ALLOC_CONTENT_MIXED);
 
     /* DSDT is pointed to by FADT */
     dsdt = tables_blob->len;
     build_dsdt(tables_blob, tables->linker, vms);
-- 
2.9.3

[Qemu-devel] [seabios PATCH 0/2] romfile_loader: cope with the UEFI-oriented allocation extensions

[Laszlo Ersek]

Please see the parent blurb
<http://mid.mail-archive.com/c76b36de-ebf9-c662-d454-0a95b43901e8@redhat.com>
for a high level description.

Cc: "Kevin O'Connor" <kevin@koconnor.net>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ben Warren <ben@skyportsystems.com>
Cc: Dongjiu Geng <gengdongjiu@huawei.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>

Thanks
Laszlo

Laszlo Ersek (2):
  romfile_loader: alloc: cope with the UEFI-oriented NOACPI content hint
  romfile_loader: alloc: cope with the UEFI-oriented 64BIT zone hint

 src/fw/romfile_loader.h | 14 +++++++++++---
 src/fw/romfile_loader.c |  6 +++++-
 2 files changed, 16 insertions(+), 4 deletions(-)

-- 
2.9.3

[Qemu-devel] [seabios PATCH 1/2] romfile_loader: alloc: cope with the UEFI-oriented NOACPI content hint

[Laszlo Ersek]

OvmfPkg/AcpiPlatformDxe, which implements the client for QEMU's
linker/loader in the OVMF and ArmVirtQemu virtual UEFI firmwares,
currently relies on a 2nd pass processing of the ADD_POINTER commands, to
identify potential ACPI tables in the pointed-to blobs. The reason for
this is that ACPI tables must be individually passed to
EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() for installation.

In order to tell apart ACPI tables from other operation region-like areas
within pointed-to blobs, OvmfPkg/AcpiPlatformDxe employs a heuristic
called "ACPI SDT header probe" at the target locations of the ADD_POINTER
commands. While all ACPI tables generated by QEMU satisfy this check
(i.e., there are no false negatives), blob content that is *not* an ACPI
table has a very slight chance to pass the test as well (i.e., there is a
small chance for false positives).

In order to suppress this small chance, in QEMU we've historically
formatted opregion-like areas in blobs with a fixed size zero prefix (see
e.g. "docs/specs/vmgenid.txt"), which guarantees that the probe in
OvmfPkg/AcpiPlatformDxe will fail. However, this "suppressor prefix" has
had to be taken into account explicitly in generated AML code -- the
prefix size has had to be added to the patched integer object in AML, at
runtime --, leading to awkwardness.

QEMU is introducing a new hint for the ALLOC command, as the most
significant bit of the uint8_t "zone" field, for disabling the ACPI SDT
header probe in OvmfPkg/AcpiPlatformDxe, for all the pointers that point
into the blob downloaded with the ALLOC command. When the bit is set, the
blob is guaranteed to contain no ACPI tables. When the bit is clear, the
behavior is left unchanged.

For SeaBIOS, this bit is irrelevant, thus mask it out.

Cc: "Kevin O'Connor" <kevin@koconnor.net>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ben Warren <ben@skyportsystems.com>
Cc: Dongjiu Geng <gengdongjiu@huawei.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 src/fw/romfile_loader.h | 7 +++++++
 src/fw/romfile_loader.c | 5 ++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/fw/romfile_loader.h b/src/fw/romfile_loader.h
index fcd4ab236b61..d90c3db24331 100644
--- a/src/fw/romfile_loader.h
+++ b/src/fw/romfile_loader.h
@@ -12,10 +12,12 @@ struct romfile_loader_entry_s {
     union {
         /*
          * COMMAND_ALLOCATE - allocate a table from @alloc.file
          * subject to @alloc.align alignment (must be power of 2)
          * and @alloc.zone (can be HIGH or FSEG) requirements.
+         * The most significant bit (bit 7) of @alloc.zone is used as a content
+         * hint for UEFI guest firmware, see ROMFILE_LOADER_ALLOC_CONTENT_*.
          *
          * Must appear exactly once for each file, and before
          * this file is referenced by any other command.
          */
         struct {
@@ -82,10 +84,15 @@ enum {
 enum {
     ROMFILE_LOADER_ALLOC_ZONE_HIGH = 0x1,
     ROMFILE_LOADER_ALLOC_ZONE_FSEG = 0x2,
 };
 
+enum {
+    ROMFILE_LOADER_ALLOC_CONTENT_MIXED  = 0x00,
+    ROMFILE_LOADER_ALLOC_CONTENT_NOACPI = 0x80,
+};
+
 int romfile_loader_execute(const char *name);
 
 void romfile_fw_cfg_resume(void);
 
 #endif
diff --git a/src/fw/romfile_loader.c b/src/fw/romfile_loader.c
index 18476e2075e3..6a457902a36a 100644
--- a/src/fw/romfile_loader.c
+++ b/src/fw/romfile_loader.c
@@ -55,19 +55,22 @@ void romfile_fw_cfg_resume(void)
 
 static void romfile_loader_allocate(struct romfile_loader_entry_s *entry,
                                     struct romfile_loader_files *files)
 {
     struct zone_s *zone;
+    unsigned zone_req;
     struct romfile_loader_file *file = &files->files[files->nfiles];
     void *data;
     int ret;
     unsigned alloc_align = le32_to_cpu(entry->alloc.align);
 
     if (alloc_align & (alloc_align - 1))
         goto err;
 
-    switch (entry->alloc.zone) {
+    zone_req = entry->alloc.zone;
+    zone_req &= ~(unsigned)ROMFILE_LOADER_ALLOC_CONTENT_NOACPI;
+    switch (zone_req) {
         case ROMFILE_LOADER_ALLOC_ZONE_HIGH:
             zone = &ZoneHigh;
             break;
         case ROMFILE_LOADER_ALLOC_ZONE_FSEG:
             zone = &ZoneFSeg;
-- 
2.9.3

[Qemu-devel] [seabios PATCH 2/2] romfile_loader: alloc: cope with the UEFI-oriented 64BIT zone hint

[Laszlo Ersek]

ROMFILE_LOADER_ALLOC_ZONE_64BIT permits the guest firmware to allocate the
blob being downloaded anywhere in the 64-bit address space. In SeaBIOS, we
can simply alias this zone request to ROMFILE_LOADER_ALLOC_ZONE_HIGH
(i.e., allocate the blob in 32-bit address space.)

Cc: "Kevin O'Connor" <kevin@koconnor.net>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ben Warren <ben@skyportsystems.com>
Cc: Dongjiu Geng <gengdongjiu@huawei.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 src/fw/romfile_loader.h | 7 ++++---
 src/fw/romfile_loader.c | 1 +
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/fw/romfile_loader.h b/src/fw/romfile_loader.h
index d90c3db24331..9828d4ad1094 100644
--- a/src/fw/romfile_loader.h
+++ b/src/fw/romfile_loader.h
@@ -11,11 +11,11 @@ struct romfile_loader_entry_s {
     u32 command;
     union {
         /*
          * COMMAND_ALLOCATE - allocate a table from @alloc.file
          * subject to @alloc.align alignment (must be power of 2)
-         * and @alloc.zone (can be HIGH or FSEG) requirements.
+         * and @alloc.zone (see ROMFILE_LOADER_ALLOC_ZONE_*) requirements.
          * The most significant bit (bit 7) of @alloc.zone is used as a content
          * hint for UEFI guest firmware, see ROMFILE_LOADER_ALLOC_CONTENT_*.
          *
          * Must appear exactly once for each file, and before
          * this file is referenced by any other command.
@@ -80,12 +80,13 @@ enum {
     ROMFILE_LOADER_COMMAND_ADD_CHECKSUM  = 0x3,
     ROMFILE_LOADER_COMMAND_WRITE_POINTER = 0x4,
 };
 
 enum {
-    ROMFILE_LOADER_ALLOC_ZONE_HIGH = 0x1,
-    ROMFILE_LOADER_ALLOC_ZONE_FSEG = 0x2,
+    ROMFILE_LOADER_ALLOC_ZONE_HIGH  = 0x1,
+    ROMFILE_LOADER_ALLOC_ZONE_FSEG  = 0x2,
+    ROMFILE_LOADER_ALLOC_ZONE_64BIT = 0x3,
 };
 
 enum {
     ROMFILE_LOADER_ALLOC_CONTENT_MIXED  = 0x00,
     ROMFILE_LOADER_ALLOC_CONTENT_NOACPI = 0x80,
diff --git a/src/fw/romfile_loader.c b/src/fw/romfile_loader.c
index 6a457902a36a..c0c476b58990 100644
--- a/src/fw/romfile_loader.c
+++ b/src/fw/romfile_loader.c
@@ -68,10 +68,11 @@ static void romfile_loader_allocate(struct romfile_loader_entry_s *entry,
 
     zone_req = entry->alloc.zone;
     zone_req &= ~(unsigned)ROMFILE_LOADER_ALLOC_CONTENT_NOACPI;
     switch (zone_req) {
         case ROMFILE_LOADER_ALLOC_ZONE_HIGH:
+        case ROMFILE_LOADER_ALLOC_ZONE_64BIT:
             zone = &ZoneHigh;
             break;
         case ROMFILE_LOADER_ALLOC_ZONE_FSEG:
             zone = &ZoneFSeg;
             break;
-- 
2.9.3

[Qemu-devel] [edk2 PATCH 0/3] OvmfPkg/AcpiPlatformDxe: NOACPI hint and 64-bit zone in fw_cfg blob alloc

[Laszlo Ersek]

Please see the parent blurb
<http://mid.mail-archive.com/c76b36de-ebf9-c662-d454-0a95b43901e8@redhat.com>
for a high level description.

Repo:   https://github.com/lersek/edk2.git
Branch: zone_hints

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ben Warren <ben@skyportsystems.com>
Cc: Dongjiu Geng <gengdongjiu@huawei.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>

Thanks
Laszlo

Laszlo Ersek (3):
  OvmfPkg/AcpiPlatformDxe: rename BLOB.HostsOnlyTableData to
    BLOB.Releasable
  OvmfPkg/AcpiPlatformDxe: support NOACPI content hint in ALLOCATE
    command
  OvmfPkg/AcpiPlatformDxe: support 64-bit zone in ALLOCATE command

 OvmfPkg/AcpiPlatformDxe/QemuLoader.h    | 12 ++++-
 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 53 +++++++++++++++-----
 2 files changed, 50 insertions(+), 15 deletions(-)

-- 
2.9.3

[Qemu-devel] [edk2 PATCH 1/3] OvmfPkg/AcpiPlatformDxe: rename BLOB.HostsOnlyTableData to BLOB.Releasable

[Laszlo Ersek]

The "BLOB.HostsOnlyTableData" field tracks whether the allocated &
downloaded fw_cfg blob should be released in the end. The current name
"HostsOnlyTableData" reflects only the original determinant for this,
namely whether the blob hosts ACPI table data only -- because in that case
EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() would create deep copies of all
referenced parts of the blob.

However, in commit 9965cbd424f2 ("OvmfPkg/AcpiPlatformDxe: implement the
QEMU_LOADER_WRITE_POINTER command", 2017-02-08) we flipped the field to
FALSE in ProcessCmdWritePointer() too, because the blob must also not be
released if we send its allocation address back to QEMU. Therefore we
should more generally call the field "Releasable".

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ben Warren <ben@skyportsystems.com>
Cc: Dongjiu Geng <gengdongjiu@huawei.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 22 ++++++++++----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
index 1bc5fe297a96..4a7b051288bc 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
@@ -34,13 +34,12 @@ typedef struct {
   UINT8   File[QEMU_LOADER_FNAME_SIZE]; // NUL-terminated name of the fw_cfg
                                         // blob. This is the ordering / search
                                         // key.
   UINTN   Size;                         // The number of bytes in this blob.
   UINT8   *Base;                        // Pointer to the blob data.
-  BOOLEAN HostsOnlyTableData;           // TRUE iff the blob has been found to
-                                        // only contain data that is directly
-                                        // part of ACPI tables.
+  BOOLEAN Releasable;                   // TRUE iff the blob should be released
+                                        // at the end of processing.
 } BLOB;
 
 
 /**
   Compare a standalone key against a user structure containing an embedded key.
@@ -206,11 +205,11 @@ ProcessCmdAllocate (
     goto FreePages;
   }
   CopyMem (Blob->File, Allocate->File, QEMU_LOADER_FNAME_SIZE);
   Blob->Size = FwCfgSize;
   Blob->Base = (VOID *)(UINTN)Address;
-  Blob->HostsOnlyTableData = TRUE;
+  Blob->Releasable = TRUE;
 
   Status = OrderedCollectionInsert (Tracker, NULL, Blob);
   if (Status == RETURN_ALREADY_STARTED) {
     DEBUG ((EFI_D_ERROR, "%a: duplicated file \"%a\"\n", __FUNCTION__,
       Allocate->File));
@@ -505,11 +504,11 @@ ProcessCmdWritePointer (
   //
   // Because QEMU has now learned PointeeBlob->Base, we must mark PointeeBlob
   // as unreleasable, for the case when the whole linker/loader script is
   // handled successfully.
   //
-  PointeeBlob->HostsOnlyTableData = FALSE;
+  PointeeBlob->Releasable = FALSE;
 
   DEBUG ((DEBUG_VERBOSE, "%a: PointerFile=\"%a\" PointeeFile=\"%a\" "
     "PointerOffset=0x%x PointeeOffset=0x%x PointerSize=%d\n", __FUNCTION__,
     WritePointer->PointerFile, WritePointer->PointeeFile,
     WritePointer->PointerOffset, WritePointer->PointeeOffset,
@@ -612,12 +611,13 @@ UndoCmdWritePointer (
                                  before, or an ACPI table different from RSDT
                                  and XSDT has been installed (reflected by
                                  InstalledKey and NumInstalled), or RSDT or
                                  XSDT has been identified but not installed, or
                                  the fw_cfg blob pointed-into by AddPointer has
-                                 been marked as hosting something else than
-                                 just direct ACPI table contents.
+                                 been marked as non-releasable due to hosting
+                                 something else than just direct ACPI table
+                                 contents.
 
   @return                        Error codes returned by
                                  AcpiProtocol->InstallAcpiTable().
 **/
 STATIC
@@ -740,11 +740,11 @@ Process2ndPassCmdAddPointer (
     }
   }
 
   if (TableSize == 0) {
     DEBUG ((EFI_D_VERBOSE, "not found; marking fw_cfg blob as opaque\n"));
-    Blob2->HostsOnlyTableData = FALSE;
+    Blob2->Releasable = FALSE;
     return EFI_SUCCESS;
   }
 
   if (*NumInstalled == INSTALLED_TABLES_MAX) {
     DEBUG ((EFI_D_ERROR, "%a: can't install more than %d tables\n",
@@ -982,23 +982,23 @@ RollbackWritePointersAndFreeTracker:
     }
   }
 
   //
   // Tear down the tracker infrastructure. Each fw_cfg blob will be left in
-  // place only if we're exiting with success and the blob hosts data that is
-  // not directly part of some ACPI table.
+  // place only if we're exiting with success and the blob has been marked
+  // non-releasable.
   //
   for (TrackerEntry = OrderedCollectionMin (Tracker); TrackerEntry != NULL;
        TrackerEntry = TrackerEntry2) {
     VOID *UserStruct;
     BLOB *Blob;
 
     TrackerEntry2 = OrderedCollectionNext (TrackerEntry);
     OrderedCollectionDelete (Tracker, TrackerEntry, &UserStruct);
     Blob = UserStruct;
 
-    if (EFI_ERROR (Status) || Blob->HostsOnlyTableData) {
+    if (EFI_ERROR (Status) || Blob->Releasable) {
       DEBUG ((EFI_D_VERBOSE, "%a: freeing \"%a\"\n", __FUNCTION__,
         Blob->File));
       gBS->FreePages ((UINTN)Blob->Base, EFI_SIZE_TO_PAGES (Blob->Size));
     }
     FreePool (Blob);
-- 
2.9.3

[Qemu-devel] [edk2 PATCH 2/3] OvmfPkg/AcpiPlatformDxe: support NOACPI content hint in ALLOCATE command

[Laszlo Ersek]

This driver currently relies on a 2nd pass processing of the ADD_POINTER
commands to identify potential ACPI tables in the pointed-to blobs.

In order to tell apart ACPI tables from other operation region-like areas
within pointed-to blobs, we employ a heuristic called "ACPI SDT header
probe" at the target locations of the ADD_POINTER commands. While all ACPI
tables generated by QEMU satisfy this check (i.e., there are no false
negatives), blob content that is *not* an ACPI table has a very slight
chance to pass the test as well (i.e., there is a small chance for false
positives).

In order to suppress this small chance, in QEMU we've historically
formatted opregion-like areas in blobs with a fixed size zero prefix (see
e.g. "docs/specs/vmgenid.txt"), which guarantees that the probe in
OvmfPkg/AcpiPlatformDxe will fail. However, this "suppressor prefix" has
had to be taken into account explicitly in generated AML code -- the
prefix size has had to be added to the patched integer object in AML, at
runtime --, leading to awkwardness.

QEMU is introducing a new hint for the ALLOCATE command, as the most
significant bit of the UINT8 "Zone" field, for disabling the ACPI SDT
header probe in OvmfPkg/AcpiPlatformDxe, for all the pointers that point
into the blob downloaded with the ALLOCATE command. When the bit is set,
the blob is guaranteed to contain no ACPI tables. When the bit is clear,
the behavior is left unchanged.

In ProcessCmdAllocate(), save the hint for later.

In Process2ndPassCmdAddPointer(), consult the saved hint. If QEMU reported
the blob as containing no ACPI table data, then omit the ACPI SDT header
probing and mark the pointed-to blob as unreleasable.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ben Warren <ben@skyportsystems.com>
Cc: Dongjiu Geng <gengdongjiu@huawei.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/AcpiPlatformDxe/QemuLoader.h    |  9 +++++-
 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 29 +++++++++++++++++++-
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/AcpiPlatformDxe/QemuLoader.h b/OvmfPkg/AcpiPlatformDxe/QemuLoader.h
index 437776d86d9a..fa558540e62b 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuLoader.h
+++ b/OvmfPkg/AcpiPlatformDxe/QemuLoader.h
@@ -34,19 +34,26 @@ typedef enum {
 typedef enum {
   QemuLoaderAllocHigh = 1,
   QemuLoaderAllocFSeg
 } QEMU_LOADER_ALLOC_ZONE;
 
+typedef enum {
+  QemuLoaderAllocContentMixed  = 0x00,
+  QemuLoaderAllocContentNoAcpi = 0x80,
+} QEMU_LOADER_ALLOC_CONTENT;
+
 #pragma pack (1)
 //
 // QemuLoaderCmdAllocate: download the fw_cfg file named File, to a buffer
 // allocated in the zone specified by Zone, aligned at a multiple of Alignment.
 //
 typedef struct {
   UINT8  File[QEMU_LOADER_FNAME_SIZE]; // NUL-terminated
   UINT32 Alignment;                    // power of two
-  UINT8  Zone;                         // QEMU_LOADER_ALLOC_ZONE values
+  UINT8  Zone;                         // One QEMU_LOADER_ALLOC_ZONE value
+                                       // OR-ed together with one
+                                       // QEMU_LOADER_ALLOC_CONTENT value
 } QEMU_LOADER_ALLOCATE;
 
 //
 // QemuLoaderCmdAddPointer: the bytes at
 // [PointerOffset..PointerOffset+PointerSize) in the file PointerFile contain a
diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
index 4a7b051288bc..23d543ffe361 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
@@ -36,10 +36,12 @@ typedef struct {
                                         // key.
   UINTN   Size;                         // The number of bytes in this blob.
   UINT8   *Base;                        // Pointer to the blob data.
   BOOLEAN Releasable;                   // TRUE iff the blob should be released
                                         // at the end of processing.
+  BOOLEAN AcpiTablesExcluded;           // TRUE iff QEMU guarantees that the
+                                        // blob contains no ACPI tables
 } BLOB;
 
 
 /**
   Compare a standalone key against a user structure containing an embedded key.
@@ -167,10 +169,12 @@ ProcessCmdAllocate (
   )
 {
   FIRMWARE_CONFIG_ITEM FwCfgItem;
   UINTN                FwCfgSize;
   EFI_STATUS           Status;
+  UINT32               Zone;
+  BOOLEAN              AcpiTablesExcluded;
   UINTN                NumPages;
   EFI_PHYSICAL_ADDRESS Address;
   BLOB                 *Blob;
 
   if (Allocate->File[QEMU_LOADER_FNAME_SIZE - 1] != '\0') {
@@ -189,10 +193,18 @@ ProcessCmdAllocate (
     DEBUG ((EFI_D_ERROR, "%a: QemuFwCfgFindFile(\"%a\"): %r\n", __FUNCTION__,
       Allocate->File, Status));
     return Status;
   }
 
+  Zone = Allocate->Zone;
+  if ((Zone & QemuLoaderAllocContentNoAcpi) != 0) {
+    Zone &= ~(UINT32)QemuLoaderAllocContentNoAcpi;
+    AcpiTablesExcluded = TRUE;
+  } else {
+    AcpiTablesExcluded = FALSE;
+  }
+
   NumPages = EFI_SIZE_TO_PAGES (FwCfgSize);
   Address = 0xFFFFFFFF;
   Status = gBS->AllocatePages (AllocateMaxAddress, EfiACPIMemoryNVS, NumPages,
                   &Address);
   if (EFI_ERROR (Status)) {
@@ -206,10 +218,11 @@ ProcessCmdAllocate (
   }
   CopyMem (Blob->File, Allocate->File, QEMU_LOADER_FNAME_SIZE);
   Blob->Size = FwCfgSize;
   Blob->Base = (VOID *)(UINTN)Address;
   Blob->Releasable = TRUE;
+  Blob->AcpiTablesExcluded = AcpiTablesExcluded;
 
   Status = OrderedCollectionInsert (Tracker, NULL, Blob);
   if (Status == RETURN_ALREADY_STARTED) {
     DEBUG ((EFI_D_ERROR, "%a: duplicated file \"%a\"\n", __FUNCTION__,
       Allocate->File));
@@ -595,11 +608,13 @@ UndoCmdWritePointer (
                                target address is encountered for the first
                                time, and it identifies an ACPI table that is
                                different from RDST and XSDT, the table is
                                installed. If a target address is seen for the
                                second or later times, it is skipped without
-                               taking any action.
+                               taking any action. Target addresses that fall
+                               into fw_cfg blobs that QEMU reported in advance
+                               as holding no ACPI content are not even tracked.
 
   @retval EFI_INVALID_PARAMETER  NumInstalled was outside the allowed range on
                                  input.
 
   @retval EFI_OUT_OF_RESOURCES   The AddPointer command identified an ACPI
@@ -651,10 +666,22 @@ Process2ndPassCmdAddPointer (
 
   TrackerEntry = OrderedCollectionFind (Tracker, AddPointer->PointerFile);
   TrackerEntry2 = OrderedCollectionFind (Tracker, AddPointer->PointeeFile);
   Blob = OrderedCollectionUserStruct (TrackerEntry);
   Blob2 = OrderedCollectionUserStruct (TrackerEntry2);
+
+  if (Blob2->AcpiTablesExcluded) {
+    DEBUG ((
+      DEBUG_VERBOSE,
+      "%a: marking blob \"%a\" with no ACPI content as unreleasable\n",
+      __FUNCTION__,
+      AddPointer->PointeeFile
+      ));
+    Blob2->Releasable = FALSE;
+    return EFI_SUCCESS;
+  }
+
   PointerField = Blob->Base + AddPointer->PointerOffset;
   PointerValue = 0;
   CopyMem (&PointerValue, PointerField, AddPointer->PointerSize);
 
   //
-- 
2.9.3

[Qemu-devel] [edk2 PATCH 3/3] OvmfPkg/AcpiPlatformDxe: support 64-bit zone in ALLOCATE command

[Laszlo Ersek]

The QemuLoaderAlloc64Bit (3) Zone value permits the guest firmware to
allocate the blob being downloaded anywhere in the 64-bit address space.
Set the maximum Address value in ProcessCmdAllocate() accordingly.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ben Warren <ben@skyportsystems.com>
Cc: Dongjiu Geng <gengdongjiu@huawei.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/AcpiPlatformDxe/QemuLoader.h    | 3 ++-
 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/AcpiPlatformDxe/QemuLoader.h b/OvmfPkg/AcpiPlatformDxe/QemuLoader.h
index fa558540e62b..1daa918ff9b7 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuLoader.h
+++ b/OvmfPkg/AcpiPlatformDxe/QemuLoader.h
@@ -31,11 +31,12 @@ typedef enum {
   QemuLoaderCmdWritePointer,
 } QEMU_LOADER_COMMAND_TYPE;
 
 typedef enum {
   QemuLoaderAllocHigh = 1,
-  QemuLoaderAllocFSeg
+  QemuLoaderAllocFSeg,
+  QemuLoaderAlloc64Bit,
 } QEMU_LOADER_ALLOC_ZONE;
 
 typedef enum {
   QemuLoaderAllocContentMixed  = 0x00,
   QemuLoaderAllocContentNoAcpi = 0x80,
diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
index 23d543ffe361..0b0b3f590f2b 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
@@ -202,11 +202,11 @@ ProcessCmdAllocate (
   } else {
     AcpiTablesExcluded = FALSE;
   }
 
   NumPages = EFI_SIZE_TO_PAGES (FwCfgSize);
-  Address = 0xFFFFFFFF;
+  Address = (Zone == QemuLoaderAlloc64Bit) ? MAX_UINT64 : MAX_UINT32;
   Status = gBS->AllocatePages (AllocateMaxAddress, EfiACPIMemoryNVS, NumPages,
                   &Address);
   if (EFI_ERROR (Status)) {
     return Status;
   }
-- 
2.9.3

Re: [Qemu-devel] allocation zone extensions for the firmware linker/loader

[Michael S. Tsirkin]

On Fri, Jun 02, 2017 at 05:45:21PM +0200, Laszlo Ersek wrote:
> Hi,
> 
> this message is cross-posted to three lists (qemu, seabios, edk2). I'll
> follow up with three patch series, one series for each project. I'll
> cross-post all of the patches as well, but I'll add the project name in
> the "bag of tags" in the subject lines.
> 
> The QEMU series introduces two extensions to the ALLOCATE firmware
> linker/loader command.
> 
> One extension is a new allocation zone, with value 3, for allowing the
> firmware to allocate the fw_cfg blobs in 64-bit address space.

Seems to make sense. I guess it's safe to do this if no
pointers to this table are 32 bit, right?
Is there a chance we'll ever be able to use this on PC
assuming the need to support 32 bit guests?

> The other extension is a repurposing of the most significant bit (bit 7)
> in the zone field. This bit becomes orthogonal to the rest of the zone
> field. If the bit is set, it means that QEMU promises the firmware that
> the blob referenced by the ALLOCATE command contains no ACPI tables at all.

This one is a bit strange in that it does not seem to be about
allocations - it seems to be about content.

I'd like to better understand what makes ACPI special.

I see two other things that make acpi special, but I'd like to make sure
1. I think that RSDT from qemu is more or less ignored by OVMF, it
   builds it from tables supplied. Thus pointers from RSDT only serve to
   find beginning of tables - they are not really patched in. So ACPI
   tables are special in that their actual addresses are unused. As a
   result they can be moved at will after linker runs.
2. Some tables can go into AddressRangeACPI, some into AddressRangeNVS,
   but they never need to be in AddressRangeReserved.
   It would be simple if we could just say "allocate this table
   from AddressRangeACPI". But I am not sure this is true
   since e.g. stuff used for power management can't go into
   AddressRangeACPI.  Thoughts?


> After introducing these, the QEMU series puts them to use, covering all
> of the currently generated ALLOCATE commands, as appropriate. Among the
> benefits we can mention
> - the removal of the OVMF ACPI SDT Header Probe suppressor from VMGENID
> (and from any similar future devices),
> - and the fact that the "virt" machine type (and maybe other machine
> types) of the arm/aarch64 target will no longer require RAM under 4GB
> for ACPI to work.
> 
> Both of these extensions are irrelevant for SeaBIOS, therefore the
> SeaBIOS patches simply mask out bit 7 (for ignoring the "no ACPI
> content" hint), and fall back to the HIGH zone (= 32-bit address space)
> when the 64-bit zone is permitted.
> 
> In other words, SeaBIOS needs some patches to recognize the new zone
> values, but beyond that, the behavior is unchanged.
> 
> Both extensions are important for virtual UEFI firmware (OVMF in x86
> guests and ArmVirtQemu in aarch64 guests). The edk2 patches add support
> to OvmfPkg/AcpiPlatformDxe for the extensions. Please see the commit
> messages for details (all the extensions are explained in detail in the
> relevant patches for all of the projects).
> 
> The patches can cause linker/loader breakage when old firmware is booted
> on new QEMU. However, that's no problem (it's nothing new), the next
> release of QEMU should bundle the new firmware binaries as always.
> 
> New firmware will continue running on old QEMU without issues.
> 
> (In case you have sent me emails about this in the last few tens of
> hours, please know that I'm not ignoring them, I just haven't seen /
> read them. Reading emails every five minutes makes focused work
> impossible, so when I'm busy, I tend to read email once per day.)
> 
> Thanks
> Laszlo

Re: [Qemu-devel] allocation zone extensions for the firmware linker/loader

[Laszlo Ersek]

On 06/02/17 18:30, Michael S. Tsirkin wrote:
> On Fri, Jun 02, 2017 at 05:45:21PM +0200, Laszlo Ersek wrote:
>> Hi,
>>
>> this message is cross-posted to three lists (qemu, seabios, edk2). I'll
>> follow up with three patch series, one series for each project. I'll
>> cross-post all of the patches as well, but I'll add the project name in
>> the "bag of tags" in the subject lines.
>>
>> The QEMU series introduces two extensions to the ALLOCATE firmware
>> linker/loader command.
>>
>> One extension is a new allocation zone, with value 3, for allowing the
>> firmware to allocate the fw_cfg blobs in 64-bit address space.
> 
> Seems to make sense. I guess it's safe to do this if no
> pointers to this table are 32 bit, right?

That's right. For example, the TCPA patch (6 of 7) in the QEMU series
does this, because the ACPI_BUILD_TPMLOG_FILE is only referenced by a
64-bit pointer.

> Is there a chance we'll ever be able to use this on PC
> assuming the need to support 32 bit guests?

Well, sticking with the TCPA example, if an ACPI table defines *only* an
8-byte pointer to some memory area, that seems to preclude support for
32-bit guests already, generally speaking, no?

But otherwise I agree, requiring support for 32-bit guests makes this
allocation zone a lot less useful.

>> The other extension is a repurposing of the most significant bit (bit 7)
>> in the zone field. This bit becomes orthogonal to the rest of the zone
>> field. If the bit is set, it means that QEMU promises the firmware that
>> the blob referenced by the ALLOCATE command contains no ACPI tables at all.
> 
> This one is a bit strange in that it does not seem to be about
> allocations - it seems to be about content.

Sure, I only stuffed it in the Zone field because that's where I found a
free bit :)

> 
> I'd like to better understand what makes ACPI special.
> 
> I see two other things that make acpi special, but I'd like to make sure
> 1. I think that RSDT from qemu is more or less ignored by OVMF, it
>    builds it from tables supplied. Thus pointers from RSDT only serve to
>    find beginning of tables - they are not really patched in. So ACPI
>    tables are special in that their actual addresses are unused. As a
>    result they can be moved at will after linker runs.

Sort of. OVMF performs two passes on the linker/loader script. The first
pass is fully identical to what SeaBIOS does, so the RSDT too is
allocated (as part of its containing fw_cfg blob, of course) and its
fields are patched like anything else.

In the second pass, the (now relocated) pointers are checked again,
based on the ADD_POINTER commands. Wherever the (now relocated) pointers
point, we probe for ACPI table headers. If the target passes the probe
(i.e., it looks like an ACPI table), we call
EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() with it. This installs a
separate copy of that table, maintaining RSDT and XSDT internally.

Now, the RSD PTR table has a pointer to the RSDT, so normally we'd
invoke InstallAcpiTable() with the RSDT as well, in the second pass. To
prevent that, we have a quirk that recognizes RSDT and XSDT, and skips them.

So you can say that the RSDT from QEMU is *ultimately* ignored in OVMF,
but for the first pass to complete (which is identical to SeaBIOS's only
pass), OVMF uses the RSDT (as embedded in its containing fw_cfg blob) too.

There are more details about the second pass. If a (relocated) pointer
points to some stuff that doesn't look like an ACPI table header, then
we don't install that thing with InstallAcpiTable(). Instead, we think
that at that location the containing fw_cfg blob contains an
opregion-like area, so its actual absolute address is relevant (remember
we are past the first pass, which completed all the relocations). So in
this case, the containing fw_cfg blob is marked as "non-releasable", and
we'll keep it forever (in AcpiNVS memory). Otherwise, if at the end of
processing a blob is marked as releasable (the default), because all
pointers into it pointed only at ACPI table headers, we free the blob.

There are more details about the second pass. There can be several
pointers that point to the same address (= same offset of the same
pointed-to fw_cfg blob). In such cases, only the first encounter is
honored with an InstallAcpiTable() call, further surfacings of the same
target address are skipped.

Now, the heuristics to determine whether a pointed-to location is an
ACPI table header or not, is not fool-proof. It recognizes all valid
headers (so no false negatives), I think, but it could also
mis-recognize random garbage in an opregion-like blob as an ACPI table
header (so there's a chance for false positives). In order to suppress
these false positives deterministically, there are two methods:

- prefix all such areas in the pointed-to blobs with a sufficient number
of zeroes so that the probe would fail, guaranteed. However, this means
that you have to keep the ADD_POINTER commands pointed at the zeroes,
not at the real opregion, so you need to add the offset in the AML code
at runtime, to actually access the opregion.

- the other method is the "no ACPI content" hint, proposed here. In this
case whenever an ADD_POINTER command is found, in the 2nd pass, to point
into a blob that had this "no ACPI content" hint at ALLOCATE time, the
probing is skipped immediately.

> 2. Some tables can go into AddressRangeACPI, some into AddressRangeNVS,
>    but they never need to be in AddressRangeReserved.
>    It would be simple if we could just say "allocate this table
>    from AddressRangeACPI". But I am not sure this is true
>    since e.g. stuff used for power management can't go into
>    AddressRangeACPI.  Thoughts?

For the first pass, OVMF allocates all the blobs in AcpiNVS memory. Most
of these blobs are ultimately released (because they only contain ACPI
tables, of which EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() creates deep
copies). The blobs that are marked as non-releasable (see above) are
preserved in AcpiNVS memory.

Regarding the tables that EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable()
installs -- this protocol member function has internal knowledge about
what table should be allocated in what kind of memory. It cannot be
influenced from the outside. Years ago I proposed and
EFI_ACPI_TABLE_PROTOCOL2 on the USWG list, in which the
InstallAcpiTable() member would additionally take the memory type to
place the copied / installed table into. I received zero feedback.

Thanks
Laszlo

>> After introducing these, the QEMU series puts them to use, covering all
>> of the currently generated ALLOCATE commands, as appropriate. Among the
>> benefits we can mention
>> - the removal of the OVMF ACPI SDT Header Probe suppressor from VMGENID
>> (and from any similar future devices),
>> - and the fact that the "virt" machine type (and maybe other machine
>> types) of the arm/aarch64 target will no longer require RAM under 4GB
>> for ACPI to work.
>>
>> Both of these extensions are irrelevant for SeaBIOS, therefore the
>> SeaBIOS patches simply mask out bit 7 (for ignoring the "no ACPI
>> content" hint), and fall back to the HIGH zone (= 32-bit address space)
>> when the 64-bit zone is permitted.
>>
>> In other words, SeaBIOS needs some patches to recognize the new zone
>> values, but beyond that, the behavior is unchanged.
>>
>> Both extensions are important for virtual UEFI firmware (OVMF in x86
>> guests and ArmVirtQemu in aarch64 guests). The edk2 patches add support
>> to OvmfPkg/AcpiPlatformDxe for the extensions. Please see the commit
>> messages for details (all the extensions are explained in detail in the
>> relevant patches for all of the projects).
>>
>> The patches can cause linker/loader breakage when old firmware is booted
>> on new QEMU. However, that's no problem (it's nothing new), the next
>> release of QEMU should bundle the new firmware binaries as always.
>>
>> New firmware will continue running on old QEMU without issues.
>>
>> (In case you have sent me emails about this in the last few tens of
>> hours, please know that I'm not ignoring them, I just haven't seen /
>> read them. Reading emails every five minutes makes focused work
>> impossible, so when I'm busy, I tend to read email once per day.)
>>
>> Thanks
>> Laszlo

Re: [Qemu-devel] allocation zone extensions for the firmware linker/loader

[Stefan Berger]

On 06/02/2017 07:20 PM, Laszlo Ersek wrote:
> On 06/02/17 18:30, Michael S. Tsirkin wrote:
>> On Fri, Jun 02, 2017 at 05:45:21PM +0200, Laszlo Ersek wrote:
>>> Hi,
>>>
>>> this message is cross-posted to three lists (qemu, seabios, edk2). I'll
>>> follow up with three patch series, one series for each project. I'll
>>> cross-post all of the patches as well, but I'll add the project name in
>>> the "bag of tags" in the subject lines.
>>>
>>> The QEMU series introduces two extensions to the ALLOCATE firmware
>>> linker/loader command.
>>>
>>> One extension is a new allocation zone, with value 3, for allowing the
>>> firmware to allocate the fw_cfg blobs in 64-bit address space.
>> Seems to make sense. I guess it's safe to do this if no
>> pointers to this table are 32 bit, right?
> That's right. For example, the TCPA patch (6 of 7) in the QEMU series
> does this, because the ACPI_BUILD_TPMLOG_FILE is only referenced by a
> 64-bit pointer.
>
>> Is there a chance we'll ever be able to use this on PC
>> assuming the need to support 32 bit guests?
> Well, sticking with the TCPA example, if an ACPI table defines *only* an
> 8-byte pointer to some memory area, that seems to preclude support for
> 32-bit guests already, generally speaking, no?

I just tested this, giving 8G of memory to a VM and running i386 Fedora 
in it. The memory allocated for the TCPA log seems to be in 32bit 
memory, so not out of reach of i386 guests. I guess it's important what 
the firmware does with it, whether it strictly follows the 64bit and 
allocates memory as far up as possible or provides compatibility. 
SeaBIOS (1.10.0) seems to do the right thing.

    Stefan

Re: [Qemu-devel] allocation zone extensions for the firmware linker/loader

[Laszlo Ersek]

On 06/02/17 17:45, Laszlo Ersek wrote:

> The patches can cause linker/loader breakage when old firmware is booted
> on new QEMU. However, that's no problem (it's nothing new), the next
> release of QEMU should bundle the new firmware binaries as always.

Dave made a good point (which I should have realized myself, really!),
namely if you launch old fw on old qemu, then migrate the guest to a new
qemu and then reboot the guest on the target host, within the migrated
VM, things will break.

So that makes this approach dead in the water.

Possible mitigations I could think of:
- Make it machine type dependent. Complicated (we don't usually bind
ACPI generation to machine types) and wouldn't help existing devices.
- Let the firmware negotiate these extensions. Very complicated (new
fw-cfg files are needed for negotiation) and wouldn't help existing devices.

So I guess I'll do what Igor and Gerd suggested: record in advance
whether any pointer field narrower than 8 bytes points into a given
blob, and if so, forbid allocating that blob from 64-bit address space.
This should solve Ard's needs purely within the firmware.

Regarding the NOACPI hint, I guess I'm dropping that. I only meant
NOACPI for addressing Igor's long-standing dislike for the "ACPI SDT
header probe suppression" in VMGENID (and future similar devices). But,
there's no actual *technical* need to eliminate that (unlike the
technical need for 64-bit blob allocations which should really be
solved), so I guess it's OK to postpone NOACPI indefinitely.

Self-nack for this set of sets.

Thanks for the feedback,
Laszlo

Re: [Qemu-devel] allocation zone extensions for the firmware linker/loader

[Dr. David Alan Gilbert]

* Laszlo Ersek (lersek@redhat.com) wrote:
> On 06/02/17 17:45, Laszlo Ersek wrote:
> 
> > The patches can cause linker/loader breakage when old firmware is booted
> > on new QEMU. However, that's no problem (it's nothing new), the next
> > release of QEMU should bundle the new firmware binaries as always.
> 
> Dave made a good point (which I should have realized myself, really!),
> namely if you launch old fw on old qemu, then migrate the guest to a new
> qemu and then reboot the guest on the target host, within the migrated
> VM, things will break.

Yep, sorry it always complicates things; as does the opposite case of
back-migrating a VM with an old machine type but newer bios to an old
qemu.

Dave

> So that makes this approach dead in the water.
> 
> Possible mitigations I could think of:
> - Make it machine type dependent. Complicated (we don't usually bind
> ACPI generation to machine types) and wouldn't help existing devices.
> - Let the firmware negotiate these extensions. Very complicated (new
> fw-cfg files are needed for negotiation) and wouldn't help existing devices.
> 
> So I guess I'll do what Igor and Gerd suggested: record in advance
> whether any pointer field narrower than 8 bytes points into a given
> blob, and if so, forbid allocating that blob from 64-bit address space.
> This should solve Ard's needs purely within the firmware.
> 
> Regarding the NOACPI hint, I guess I'm dropping that. I only meant
> NOACPI for addressing Igor's long-standing dislike for the "ACPI SDT
> header probe suppression" in VMGENID (and future similar devices). But,
> there's no actual *technical* need to eliminate that (unlike the
> technical need for 64-bit blob allocations which should really be
> solved), so I guess it's OK to postpone NOACPI indefinitely.
> 
> Self-nack for this set of sets.
> 
> Thanks for the feedback,
> Laszlo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] allocation zone extensions for the firmware linker/loader

[Igor Mammedov]

On Sat, 3 Jun 2017 09:36:23 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 06/02/17 17:45, Laszlo Ersek wrote:
> 
> > The patches can cause linker/loader breakage when old firmware is booted
> > on new QEMU. However, that's no problem (it's nothing new), the next
> > release of QEMU should bundle the new firmware binaries as always.  
> 
> Dave made a good point (which I should have realized myself, really!),
> namely if you launch old fw on old qemu, then migrate the guest to a new
> qemu and then reboot the guest on the target host, within the migrated
> VM, things will break.
[...]

> Regarding the NOACPI hint, I guess I'm dropping that. I only meant
> NOACPI for addressing Igor's long-standing dislike for the "ACPI SDT
> header probe suppression" in VMGENID (and future similar devices). But,
> there's no actual *technical* need to eliminate that 
I'd consider eliminating wasting space technical need,
but there are not a lot of tables that need it so far and
implementing NOACPI hint would lead to all out compat
logic impl. along with it (either negotiation or machine versioning).
The later seem to me too much so I wouldn't bother with it yet.

> (unlike the technical need for 64-bit blob allocations which should really be
> solved)
here I disagree, having 32bit pointer is sufficient enough hint.
Yep, firmware have to scan linker script to determine limits
before doing allocations but it's totally upto firmware to decide
where to allocate tables.
It also helps to avoid in qemu 2 points where we'd have to specify
that table is "64 bit-able" in add_pointer and allocate.

BTW:
how OVMF would deal with booting 32-bit OS if it would allocate
ACPI tables above 4Gb?
For BIOS on baremetal I'd expect some switch in settings, something
like "Disable 32-bit OS support".

> Self-nack for this set of sets.
> 
> Thanks for the feedback,
> Laszlo
> 

Re: [Qemu-devel] allocation zone extensions for the firmware linker/loader

[Laszlo Ersek]

On 06/05/17 11:54, Igor Mammedov wrote:

> BTW:
> how OVMF would deal with booting 32-bit OS if it would allocate
> ACPI tables above 4Gb?
> For BIOS on baremetal I'd expect some switch in settings, something
> like "Disable 32-bit OS support".

In order to answer your question exhaustively, I'd have to ponder this a
lot longer. For now my basic answer is the following:

- If you can allocate memory above 4GB in the DXE phase, that means your
DXE and BDS phases are 64-bit. Which in turn implies you *cannot* boot a
32-bit OS at all.

Generally speaking, with PI / UEFI, the bitness of the firmware (the DXE
and the BDS phases) and the bitness of the OS (incl. its UEFI boot
loader) must be identical.

As a trick, Linux can work around this, in *one* direction -- 64-bit
Linux can run on 32-bit UEFI firmware (on x86; not sure about other
arches). But, the other direction (32-bit UEFI OS on 64-bit firmware)
cannot work, minimally because you can only call the UEFI runtime
services in 64-bit mode.

In short (again, this is quite rudimentary), if your memory allocation
in the DXE and/or BDS phases ends up being served from above 4GB, you
won't be booting a 32-bit-only OS.

Thanks
Laszlo

Re: [Qemu-devel] allocation zone extensions for the firmware linker/loader

[Michael S. Tsirkin]

On Sat, Jun 03, 2017 at 09:36:23AM +0200, Laszlo Ersek wrote:
> On 06/02/17 17:45, Laszlo Ersek wrote:
> 
> > The patches can cause linker/loader breakage when old firmware is booted
> > on new QEMU. However, that's no problem (it's nothing new), the next
> > release of QEMU should bundle the new firmware binaries as always.
> 
> Dave made a good point (which I should have realized myself, really!),
> namely if you launch old fw on old qemu, then migrate the guest to a new
> qemu and then reboot the guest on the target host, within the migrated
> VM, things will break.
> 
> So that makes this approach dead in the water.
> 
> Possible mitigations I could think of:
> - Make it machine type dependent. Complicated (we don't usually bind
> ACPI generation to machine types) and wouldn't help existing devices.
> - Let the firmware negotiate these extensions. Very complicated (new
> fw-cfg files are needed for negotiation) and wouldn't help existing devices.

This last option *shouldn't* be complicated. If it is something's wrong.

Maybe we made a mistake when we added etc/smi/*features*.

It's not too late to move these to etc/*features* for new
machine types if we want to and if you can do the firmware
work. Then you'd just take out a bit and be done with it.

I don't insist on doing the ACPI thing now but I do think
infrastructure for negotiating extensions should be there.

-- 
MST

Re: [Qemu-devel] allocation zone extensions for the firmware linker/loader

[Laszlo Ersek]

On 06/05/17 18:02, Michael S. Tsirkin wrote:
> On Sat, Jun 03, 2017 at 09:36:23AM +0200, Laszlo Ersek wrote:
>> On 06/02/17 17:45, Laszlo Ersek wrote:
>>
>>> The patches can cause linker/loader breakage when old firmware is booted
>>> on new QEMU. However, that's no problem (it's nothing new), the next
>>> release of QEMU should bundle the new firmware binaries as always.
>>
>> Dave made a good point (which I should have realized myself, really!),
>> namely if you launch old fw on old qemu, then migrate the guest to a new
>> qemu and then reboot the guest on the target host, within the migrated
>> VM, things will break.
>>
>> So that makes this approach dead in the water.
>>
>> Possible mitigations I could think of:
>> - Make it machine type dependent. Complicated (we don't usually bind
>> ACPI generation to machine types) and wouldn't help existing devices.
>> - Let the firmware negotiate these extensions. Very complicated (new
>> fw-cfg files are needed for negotiation) and wouldn't help existing devices.
> 
> This last option *shouldn't* be complicated. If it is something's wrong.
> 
> Maybe we made a mistake when we added etc/smi/*features*.
> 
> It's not too late to move these to etc/*features* for new
> machine types if we want to and if you can do the firmware
> work. Then you'd just take out a bit and be done with it.
> 
> I don't insist on doing the ACPI thing now but I do think
> infrastructure for negotiating extensions should be there.

Different drivers in the firmware would need to negotiate different
questions / features with QEMU independently of each other. The "thing"
in OVMF that negotiates (and uses) the SMI broadcast is very-very
different and separate from the "thing" in OVMF that handles the ACPI
linker/loader script.

As one example, the first "thing" mentioned above is not even built into
ArmVirtQemu (only into OVMF, i.e. x86), while the second "thing" is
built into both aarch64 and x86 firmware.

So, I think we couldn't share the same fw_cfg files (if they needed
write access & lock-down too, i.e. actual negotiation from the firmware)
between wildly unrelated features.

The virtio feature negotiation is different because each device gets its
own negotiation, and that maps very well to UEFI concepts too.

BTW, do we have a specific concern relating to the number of fw_cfg
files? That count can now be raised from machine type to machine type,
but Paolo didn't seem to like raising the current value (or maybe I
misunderstood him):

http://mid.mail-archive.com/2e6dec37-8b69-979b-c856-406233273066@redhat.com

... Also, above I wrote, with regard to feature negotiation, that it
"wouldn't help existing devices". By that I mean this:

Consider the NOACPI content hint as an example. If the firmware doesn't
negotiate it (before selecting and downloading the ACPI payload), then
QEMU cannot generate the NOACPI content hint. In turn QEMU must keep the
OVMF SDT Header Probe suppressor (those paddings and AML additions) enabled.

But, for the QEMU developers it means that the suppressor code has to be
kept around forever, for compatibility with old machine types. And if
you do that, then why add a negotiable NOACPI hint at all? That would
just further complicate device code (because now you have to generate
two different AML payloads), where the old one (the one with the
explicit suppressor) would work just fine "forever".

Thanks,
Laszlo

Re: [Qemu-devel] allocation zone extensions for the firmware linker/loader

[Michael S. Tsirkin]

On Tue, Jun 06, 2017 at 08:10:17PM +0200, Laszlo Ersek wrote:
> On 06/05/17 18:02, Michael S. Tsirkin wrote:
> > On Sat, Jun 03, 2017 at 09:36:23AM +0200, Laszlo Ersek wrote:
> >> On 06/02/17 17:45, Laszlo Ersek wrote:
> >>
> >>> The patches can cause linker/loader breakage when old firmware is booted
> >>> on new QEMU. However, that's no problem (it's nothing new), the next
> >>> release of QEMU should bundle the new firmware binaries as always.
> >>
> >> Dave made a good point (which I should have realized myself, really!),
> >> namely if you launch old fw on old qemu, then migrate the guest to a new
> >> qemu and then reboot the guest on the target host, within the migrated
> >> VM, things will break.
> >>
> >> So that makes this approach dead in the water.
> >>
> >> Possible mitigations I could think of:
> >> - Make it machine type dependent. Complicated (we don't usually bind
> >> ACPI generation to machine types) and wouldn't help existing devices.
> >> - Let the firmware negotiate these extensions. Very complicated (new
> >> fw-cfg files are needed for negotiation) and wouldn't help existing devices.
> > 
> > This last option *shouldn't* be complicated. If it is something's wrong.
> > 
> > Maybe we made a mistake when we added etc/smi/*features*.
> > 
> > It's not too late to move these to etc/*features* for new
> > machine types if we want to and if you can do the firmware
> > work. Then you'd just take out a bit and be done with it.
> > 
> > I don't insist on doing the ACPI thing now but I do think
> > infrastructure for negotiating extensions should be there.
> 
> Different drivers in the firmware would need to negotiate different
> questions / features with QEMU independently of each other. The "thing"
> in OVMF that negotiates (and uses) the SMI broadcast is very-very
> different and separate from the "thing" in OVMF that handles the ACPI
> linker/loader script.


They both could call a common library.

Also, we don't need separate fw cfg files - we could
reserve ranges of bits in a single file.
E.g. bits 0-31 - smi, 32-63 - tseg, etc.

-- 
MST

Re: [Qemu-devel] allocation zone extensions for the firmware linker/loader

[Paolo Bonzini]

On 08/06/2017 19:44, Michael S. Tsirkin wrote:
> On Tue, Jun 06, 2017 at 08:10:17PM +0200, Laszlo Ersek wrote:
>> On 06/05/17 18:02, Michael S. Tsirkin wrote:
>>> On Sat, Jun 03, 2017 at 09:36:23AM +0200, Laszlo Ersek wrote:
>>>> On 06/02/17 17:45, Laszlo Ersek wrote:
>>>>
>>>>> The patches can cause linker/loader breakage when old firmware is booted
>>>>> on new QEMU. However, that's no problem (it's nothing new), the next
>>>>> release of QEMU should bundle the new firmware binaries as always.
>>>>
>>>> Dave made a good point (which I should have realized myself, really!),
>>>> namely if you launch old fw on old qemu, then migrate the guest to a new
>>>> qemu and then reboot the guest on the target host, within the migrated
>>>> VM, things will break.
>>>>
>>>> So that makes this approach dead in the water.
>>>>
>>>> Possible mitigations I could think of:
>>>> - Make it machine type dependent. Complicated (we don't usually bind
>>>> ACPI generation to machine types) and wouldn't help existing devices.
>>>> - Let the firmware negotiate these extensions. Very complicated (new
>>>> fw-cfg files are needed for negotiation) and wouldn't help existing devices.
>>>
>>> This last option *shouldn't* be complicated. If it is something's wrong.
>>>
>>> Maybe we made a mistake when we added etc/smi/*features*.
>>>
>>> It's not too late to move these to etc/*features* for new
>>> machine types if we want to and if you can do the firmware
>>> work. Then you'd just take out a bit and be done with it.
>>>
>>> I don't insist on doing the ACPI thing now but I do think
>>> infrastructure for negotiating extensions should be there.
>>
>> Different drivers in the firmware would need to negotiate different
>> questions / features with QEMU independently of each other. The "thing"
>> in OVMF that negotiates (and uses) the SMI broadcast is very-very
>> different and separate from the "thing" in OVMF that handles the ACPI
>> linker/loader script.
> 
> They both could call a common library.
> 
> Also, we don't need separate fw cfg files - we could
> reserve ranges of bits in a single file.
> E.g. bits 0-31 - smi, 32-63 - tseg, etc.

TSEG size is an integer, not a boolean...

Paolo