On 29 May 2018 at 04:08, Shannon Zhao <zhaoshengl...@huawei.com> 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. So previous
> pointer could not be used any more. It must update the pointer and use
> the new one.
>
> Reviewed-by: Eric Auger <eric.au...@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> Signed-off-by: Shannon Zhao <zhaoshengl...@huawei.com>
> ---
> V2: add comments for iort_node_offset and reviewed tags
> ---
> hw/arm/virt-acpi-build.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 92ceee9..6209138 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -400,7 +400,7 @@ build_iort(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
> AcpiIortItsGroup *its;
> AcpiIortTable *iort;
> AcpiIortSmmu3 *smmu;
> - size_t node_size, iort_length, smmu_offset = 0;
> + size_t node_size, iort_node_offset, iort_length, smmu_offset = 0;
> AcpiIortRC *rc;
>
> iort = acpi_data_push(table_data, sizeof(*iort));
> @@ -415,6 +415,12 @@ build_iort(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
> iort->node_count = cpu_to_le32(nb_nodes);
> iort->node_offset = cpu_to_le32(sizeof(*iort));
>
> + /*
> + * Use a copy in case table_data->data moves duringa acpi_data_push
> + * operations.
> + */
> + iort_node_offset = sizeof(*iort);
> +
> /* ITS group node */
> node_size = sizeof(*its) + sizeof(uint32_t);
> iort_length += node_size;
> @@ -429,7 +435,7 @@ build_iort(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
> int irq = vms->irqmap[VIRT_SMMU];
>
> /* SMMUv3 node */
> - smmu_offset = iort->node_offset + node_size;
> + smmu_offset = iort_node_offset + node_size;
In the old code, we used iort->node_offset directly as a CPU
endianness order bitmap, which is initialized above using
iort->node_offset = cpu_to_le32(sizeof(*iort));
In this version, we use iort_node_offset, which is
initialized using
iort_node_offset = sizeof(*iort);
So we've lost an endianness conversion on big-endian systems.
Which is correct, the old code or the new?
> node_size = sizeof(*smmu) + sizeof(*idmap);
> iort_length += node_size;
> smmu = acpi_data_push(table_data, node_size);
> @@ -450,7 +456,7 @@ build_iort(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
> idmap->id_count = cpu_to_le32(0xFFFF);
> idmap->output_base = 0;
> /* output IORT node is the ITS group node (the first node) */
> - idmap->output_reference = cpu_to_le32(iort->node_offset);
> + idmap->output_reference = cpu_to_le32(iort_node_offset);
Here we're doing an endianness conversion on iort_node_offset.
Overall something is weird here, even in the previous version:
if we wrote iort->node_offset with cpu_to_le32(), that implies
that it's little-endian; so why are we reading it with cpu_to_le32()
here rather than le32_to_cpu() ?
Both cpu_to_le32() and le32_to_cpu() are the same operation,
mathematically, but we should use the version which indicates
our intent, ie which of source and destination is the always-LE
data, and which is the host-order data.
> }
>
> /* Root Complex Node */
> @@ -479,9 +485,14 @@ build_iort(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
> idmap->output_reference = cpu_to_le32(smmu_offset);
> } else {
> /* output IORT node is the ITS group node (the first node) */
> - idmap->output_reference = cpu_to_le32(iort->node_offset);
> + idmap->output_reference = cpu_to_le32(iort_node_offset);
> }
>
> + /*
> + * Update the pointer address in case table_data->data moves during above
> + * acpi_data_push operations.
> + */
> + iort = (AcpiIortTable *)(table_data->data + iort_start);
> iort->length = cpu_to_le32(iort_length);
>
> build_header(linker, table_data, (void *)(table_data->data + iort_start),
This could just use 'iort' now, right?
> --
thanks
-- PMM
