hw/arm/virt-acpi-build.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)
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.`
Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
---
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
On Fri, Dec 22, 2017 at 02:52:47PM +0800, Shannon Zhao wrote:
> 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.`
>
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> ---
> 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);
> }
We don't need to change build_spcr(), as acpi_data_push() is only called
once, so spcr == new table_data->data + old table_data->len and new
table_data->len - spcr == sizeof(*spcr) (the size used in the only
acpi_data_push() call)
>
> 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);
Yes, we need this fix, as there are many acpi_data_push() calls in this
function. I guess this was the table that triggered the assert.
> }
>
> 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);
> }
No need to change this one.
>
> /* 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);
> }
No need to change this one either.
>
> /* DSDT */
> --
> 2.0.4
>
>
Please respin only changing the one that needs the fix.
Thanks,
drew
On 2017/12/26 19:48, Andrew Jones wrote:
> On Fri, Dec 22, 2017 at 02:52:47PM +0800, Shannon Zhao wrote:
>> 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.`
>>
>> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> ---
>> 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);
>> }
>
> We don't need to change build_spcr(), as acpi_data_push() is only called
> once, so spcr == new table_data->data + old table_data->len and new
> table_data->len - spcr == sizeof(*spcr) (the size used in the only
> acpi_data_push() call)
>
>>
>> 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);
>
> Yes, we need this fix, as there are many acpi_data_push() calls in this
> function. I guess this was the table that triggered the assert.
>
>> }
>>
>> 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);
>> }
>
> No need to change this one.
>
>>
>> /* 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);
>> }
>
> No need to change this one either.
>
>>
>> /* DSDT */
>> --
>> 2.0.4
>>
>>
>
> Please respin only changing the one that needs the fix.
>
Hi Andrew,
Thanks for your comments. What you said is right that only the
build_srat needs to be fixed but I thought we need to unify the style
and avoid new issues if add some acpi_data_push in other functions in
the future.
Thanks,
--
Shannon
On Tue, Dec 26, 2017 at 07:54:15PM +0800, Shannon Zhao wrote:
>
>
> On 2017/12/26 19:48, Andrew Jones wrote:
> > On Fri, Dec 22, 2017 at 02:52:47PM +0800, Shannon Zhao wrote:
> >> 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.`
> >>
> >> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> >> ---
> >> 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);
> >> }
> >
> > We don't need to change build_spcr(), as acpi_data_push() is only called
> > once, so spcr == new table_data->data + old table_data->len and new
> > table_data->len - spcr == sizeof(*spcr) (the size used in the only
> > acpi_data_push() call)
> >
> >>
> >> 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);
> >
> > Yes, we need this fix, as there are many acpi_data_push() calls in this
> > function. I guess this was the table that triggered the assert.
> >
> >> }
> >>
> >> 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);
> >> }
> >
> > No need to change this one.
> >
> >>
> >> /* 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);
> >> }
> >
> > No need to change this one either.
> >
> >>
> >> /* DSDT */
> >> --
> >> 2.0.4
> >>
> >>
> >
> > Please respin only changing the one that needs the fix.
> >
> Hi Andrew,
>
> Thanks for your comments. What you said is right that only the
> build_srat needs to be fixed but I thought we need to unify the style
> and avoid new issues if add some acpi_data_push in other functions in
> the future.
Our style is unified with the x86 style which doesn't bother with the
_start offsets when only one push is used. I agree it's a bit error
prone, though, so I won't insist on not changing the ones that don't
need it. However if you choose to keep them, then I think the commit
message should be updated to explain that only SRAT needs the change,
and the other changes are for future-proofing and pattern consistency.
Thanks,
drew
© 2016 - 2024 Red Hat, Inc.