1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH v2] Virt: ACPI: fix qemu assert due to re-assigned table data address

Shannon Zhao posted 1 patch 7 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1514362572-1624-1-git-send-email-zhaoshenglong@huawei.com
Test checkpatch passed
Test docker passed
Test ppc passed
Test s390x passed
There is a newer version of this series
hw/arm/virt-acpi-build.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
[Qemu-devel] [PATCH v2] Virt: ACPI: fix qemu assert due to re-assigned table data address
Posted by Shannon Zhao 7 years, 10 months ago 
From: Zhaoshenglong <zhaoshenglong@huawei.com>

acpi_data_push uses g_array_set_size to resize the memory size. If there
is no enough contiguous memory, the address will be changed. If we use
the old value, it will assert.
qemu-kvm: hw/acpi/bios-linker-loader.c:214: bios_linker_loader_add_checksum:
Assertion `start_offset < file->blob->len' failed.`

This issue only happens in building SRAT table now but here we unify the
pattern for other tables as well to avoid possible issues in the future.

Signed-off-by: Zhaoshenglong <zhaoshenglong@huawei.com>
---
v2: update the commit message according to Andrew's comments
---
 hw/arm/virt-acpi-build.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 3d78ff6..5901142 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -453,6 +453,7 @@ build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     AcpiSerialPortConsoleRedirection *spcr;
     const MemMapEntry *uart_memmap = &vms->memmap[VIRT_UART];
     int irq = vms->irqmap[VIRT_UART] + ARM_SPI_BASE;
+    int spcr_start = table_data->len;
 
     spcr = acpi_data_push(table_data, sizeof(*spcr));
 
@@ -476,8 +477,8 @@ build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     spcr->pci_device_id = 0xffff;  /* PCI Device ID: not a PCI device */
     spcr->pci_vendor_id = 0xffff;  /* PCI Vendor ID: not a PCI device */
 
-    build_header(linker, table_data, (void *)spcr, "SPCR", sizeof(*spcr), 2,
-                 NULL, NULL);
+    build_header(linker, table_data, (void *)(table_data->data + spcr_start),
+                 "SPCR", table_data->len - spcr_start, 2, NULL, NULL);
 }
 
 static void
@@ -512,8 +513,8 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         mem_base += numa_info[i].node_mem;
     }
 
-    build_header(linker, table_data, (void *)srat, "SRAT",
-                 table_data->len - srat_start, 3, NULL, NULL);
+    build_header(linker, table_data, (void *)(table_data->data + srat_start),
+                 "SRAT", table_data->len - srat_start, 3, NULL, NULL);
 }
 
 static void
@@ -522,6 +523,7 @@ build_mcfg(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     AcpiTableMcfg *mcfg;
     const MemMapEntry *memmap = vms->memmap;
     int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]);
+    int mcfg_start = table_data->len;
 
     mcfg = acpi_data_push(table_data, len);
     mcfg->allocation[0].address = cpu_to_le64(memmap[VIRT_PCIE_ECAM].base);
@@ -532,7 +534,8 @@ build_mcfg(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     mcfg->allocation[0].end_bus_number = (memmap[VIRT_PCIE_ECAM].size
                                           / PCIE_MMCFG_SIZE_MIN) - 1;
 
-    build_header(linker, table_data, (void *)mcfg, "MCFG", len, 1, NULL, NULL);
+    build_header(linker, table_data, (void *)(table_data->data + mcfg_start),
+                 "MCFG", len, 1, NULL, NULL);
 }
 
 /* GTDT */
@@ -651,6 +654,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 static void build_fadt(GArray *table_data, BIOSLinker *linker,
                        VirtMachineState *vms, unsigned dsdt_tbl_offset)
 {
+    int fadt_start = table_data->len;
     AcpiFadtDescriptorRev5_1 *fadt = acpi_data_push(table_data, sizeof(*fadt));
     unsigned xdsdt_entry_offset = (char *)&fadt->x_dsdt - table_data->data;
     uint16_t bootflags;
@@ -681,8 +685,8 @@ static void build_fadt(GArray *table_data, BIOSLinker *linker,
         ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, sizeof(fadt->x_dsdt),
         ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
 
-    build_header(linker, table_data,
-                 (void *)fadt, "FACP", sizeof(*fadt), 5, NULL, NULL);
+    build_header(linker, table_data, (void *)(table_data->data + fadt_start),
+                 "FACP", table_data->len - fadt_start, 5, NULL, NULL);
 }
 
 /* DSDT */
-- 
2.0.4
Re: [Qemu-devel] [PATCH v2] Virt: ACPI: fix qemu assert due to re-assigned table data address
Posted by Andrew Jones 7 years, 10 months ago 
On Wed, Dec 27, 2017 at 04:16:12PM +0800, Shannon Zhao wrote:
> From: Zhaoshenglong <zhaoshenglong@huawei.com>
> 
> acpi_data_push uses g_array_set_size to resize the memory size. If there
> is no enough contiguous memory, the address will be changed. If we use
> the old value, it will assert.
> qemu-kvm: hw/acpi/bios-linker-loader.c:214: bios_linker_loader_add_checksum:
> Assertion `start_offset < file->blob->len' failed.`
> 
> This issue only happens in building SRAT table now but here we unify the
> pattern for other tables as well to avoid possible issues in the future.
> 
> Signed-off-by: Zhaoshenglong <zhaoshenglong@huawei.com>
> ---
> v2: update the commit message according to Andrew's comments
> ---
>  hw/arm/virt-acpi-build.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 3d78ff6..5901142 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -453,6 +453,7 @@ build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      AcpiSerialPortConsoleRedirection *spcr;
>      const MemMapEntry *uart_memmap = &vms->memmap[VIRT_UART];
>      int irq = vms->irqmap[VIRT_UART] + ARM_SPI_BASE;
> +    int spcr_start = table_data->len;
>  
>      spcr = acpi_data_push(table_data, sizeof(*spcr));
>  
> @@ -476,8 +477,8 @@ build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      spcr->pci_device_id = 0xffff;  /* PCI Device ID: not a PCI device */
>      spcr->pci_vendor_id = 0xffff;  /* PCI Vendor ID: not a PCI device */
>  
> -    build_header(linker, table_data, (void *)spcr, "SPCR", sizeof(*spcr), 2,
> -                 NULL, NULL);
> +    build_header(linker, table_data, (void *)(table_data->data + spcr_start),
> +                 "SPCR", table_data->len - spcr_start, 2, NULL, NULL);
>  }
>  
>  static void
> @@ -512,8 +513,8 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          mem_base += numa_info[i].node_mem;
>      }
>  
> -    build_header(linker, table_data, (void *)srat, "SRAT",
> -                 table_data->len - srat_start, 3, NULL, NULL);
> +    build_header(linker, table_data, (void *)(table_data->data + srat_start),
> +                 "SRAT", table_data->len - srat_start, 3, NULL, NULL);
>  }
>  
>  static void
> @@ -522,6 +523,7 @@ build_mcfg(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      AcpiTableMcfg *mcfg;
>      const MemMapEntry *memmap = vms->memmap;
>      int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]);
> +    int mcfg_start = table_data->len;
>  
>      mcfg = acpi_data_push(table_data, len);
>      mcfg->allocation[0].address = cpu_to_le64(memmap[VIRT_PCIE_ECAM].base);
> @@ -532,7 +534,8 @@ build_mcfg(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      mcfg->allocation[0].end_bus_number = (memmap[VIRT_PCIE_ECAM].size
>                                            / PCIE_MMCFG_SIZE_MIN) - 1;
>  
> -    build_header(linker, table_data, (void *)mcfg, "MCFG", len, 1, NULL, NULL);
> +    build_header(linker, table_data, (void *)(table_data->data + mcfg_start),
> +                 "MCFG", len, 1, NULL, NULL);

If we're going to change all of them for the new pattern, then len should
be changed here to table_data->len - mcfg_start

>  }
>  
>  /* GTDT */
> @@ -651,6 +654,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  static void build_fadt(GArray *table_data, BIOSLinker *linker,
>                         VirtMachineState *vms, unsigned dsdt_tbl_offset)
>  {
> +    int fadt_start = table_data->len;
>      AcpiFadtDescriptorRev5_1 *fadt = acpi_data_push(table_data, sizeof(*fadt));
>      unsigned xdsdt_entry_offset = (char *)&fadt->x_dsdt - table_data->data;
>      uint16_t bootflags;
> @@ -681,8 +685,8 @@ static void build_fadt(GArray *table_data, BIOSLinker *linker,
>          ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, sizeof(fadt->x_dsdt),
>          ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
>  
> -    build_header(linker, table_data,
> -                 (void *)fadt, "FACP", sizeof(*fadt), 5, NULL, NULL);
> +    build_header(linker, table_data, (void *)(table_data->data + fadt_start),
> +                 "FACP", table_data->len - fadt_start, 5, NULL, NULL);
>  }
>  
>  /* DSDT */
> -- 
> 2.0.4
> 
> 
>

Thanks,
drew

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.