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.