Hi Shannon,
On 05/30/2018 03:14 AM, Shannon Zhao wrote:
>
>
> 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
nit: duringa typo
>>>>> + * 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 checked against the v1 in my branch thinking you did not change
anything besides the comment (your log history?).
>
> I think you mean
> iort_node_offset = sizeof(*iort);
Yes sorry wrong copy/paste
> 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.
OK checking again v2, this looks OK to me.
Thanks
Eric
>
> 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
>>>
>>>
>>
>> .
>>
>
