On 2018/5/30 3:53, Auger Eric wrote:
> Hi Shannon,
>
> On 05/29/2018 04:09 PM, Shannon Zhao wrote:
>>
>>
>>> 在 2018年5月29日，21:53，Peter Maydell <peter.mayd...@linaro.org> 写道：
>>>
>>>> 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?
>> I think it’s the new one. I found this bug later and wanted to send another
>> patch to fix it. But to address Philippe’s comment I folder them together.
> Shouldn't we have
> iort_node_offset = iort->node_offset;
> iort->node_offset = cpu_to_le32(iort_node_offset);
> instead?
>
Hi Eric,
Have you read this patch and code carefully?
I think you mean
iort_node_offset = sizeof(*iort);
iort->node_offset = cpu_to_le32(iort_node_offset);
Right? If so it's almost same with what I write. I just use sizeof twice.
iort->node_offset = cpu_to_le32(sizeof(*iort));
iort_node_offset = sizeof(*iort);
> Otherwise subsequent computations are done with the node_offset already
> converted to le32 whereas the cpu mode may be "be"?
>
What I did here is to avoid the case you mentioned.
The iort_node_offset is host order, right? And I use iort_node_offset
instead of iort->node_offset for subsequent computations not the le32
one. So smmu_offset = iort_node_offset + node_size; will get the right
value as well as below ones.
Thanks,
> Shouldn't conversions to le32 being done at the last stage when
> populating the structs?
>
> May be worth splitting this into 2 patches then or at least mentioning
> this other fix in the commit message?
>
> Thanks
>
> Eric
>
>>
>>>
>>>> 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?
>> Right, but for consistent as other places I think it’s fine to remain
>> unchanged.
>>>
>>>> --
>>>
>>> thanks
>>> -- PMM
>>
>>
>
> .
>
--
Shannon
