Re: [Qemu-arm] [PATCH v5 2/3] ACPI: Add APEI GHES Table Generation support

[gengdongjiu]

Laszlo,
  Thanks for the comments.

On 2017/7/14 3:41, Laszlo Ersek wrote:
snip

> 
> snip
> 
>>> +
>>> +    error_source_table = (AcpiHardwareErrorSourceTable *)(table_data->data
>>> +                        + table_data->len - buffer->len);
>>> +    error_source_table->error_source_count = 
>>> GHES_ACPI_HEST_NOTIFY_RESERVED;
>>> +    error_source = (AcpiGenericHardwareErrorSource *)
>>> +        ((AcpiHardwareErrorSourceTable *)error_source_table + 1);
>>
>> Concern with all these casts and pointer math is that it is barely readable.
>> We can easily overflow the array and get hard to debug heap corruption
>> errors.
>>
>> What can I suggest?  Just add this to the array in steps. Then you will
>> not need all the math.
>>
>> Or again, you can just add things as you init them using 
>> build_append_int_noprefix.
>>
>>
>>> +
>>> +    bios_linker_loader_write_pointer(linker, GHES_DATA_ADDR_FW_CFG_FILE,
>>> +        0, sizeof(uint64_t), GHES_ERRORS_FW_CFG_FILE,
>>> +        GHES_ACPI_HEST_NOTIFY_RESERVED * sizeof(uint64_t));
>>
>> What does this uint64_t refer to? It's repeated everywhere.
>>
>>> +
>>> +    for (i = 0; i < GHES_ACPI_HEST_NOTIFY_RESERVED; i++) {
>>> +        error_source->type = ACPI_HEST_SOURCE_GENERIC_ERROR;
>>> +        error_source->source_id = cpu_to_le16(i);
>>> +        error_source->related_source_id = 0xffff;
>>> +        error_source->flags = 0;
>>> +        error_source->enabled = 1;
>>> +        /* The number of error status block per Generic Hardware Error 
>>> Source */
>>
>> number of ... blocks ?
>>
>>> +        error_source->number_of_records = 1;
>>> +        error_source->max_sections_per_record = 1;
>>> +        error_source->max_raw_data_length = GHES_MAX_RAW_DATA_LENGTH;
>>> +        error_source->error_status_address.space_id =
>>> +                                    AML_SYSTEM_MEMORY;
>>> +        error_source->error_status_address.bit_width = 64;
>>> +        error_source->error_status_address.bit_offset = 0;
>>> +        error_source->error_status_address.access_width = 4;
>>> +        error_source->notify.type = i;
>>> +        error_source->notify.length = sizeof(AcpiHestNotify);
>>> +
>>> +        error_source->error_status_block_length = GHES_MAX_RAW_DATA_LENGTH;
>>> +
>>> +        bios_linker_loader_add_pointer(linker,
>>> +            ACPI_BUILD_TABLE_FILE, address_registers_offset + i *
>>> +            sizeof(AcpiGenericHardwareErrorSource), sizeof(uint64_t),
>>> +            GHES_ERRORS_FW_CFG_FILE, i * sizeof(uint64_t));
>>
>> and what's this math for?
>>
>>> +
>>> +        error_source++;
>>> +    }
>>> +
>>> +    for (i = 0; i < GHES_ACPI_HEST_NOTIFY_RESERVED; i++) {
>>> +        bios_linker_loader_add_pointer(linker,
>>> +            GHES_ERRORS_FW_CFG_FILE, sizeof(uint64_t) * i, 
>>> sizeof(uint64_t),
>>> +            GHES_ERRORS_FW_CFG_FILE, GHES_ACPI_HEST_NOTIFY_RESERVED *
>>> +            sizeof(uint64_t) + i * GHES_MAX_RAW_DATA_LENGTH);
>>> +    }
> 
> So basically all this math exists to set up the pointers that are shown
> in the diagram in the commit message. It is a bit tricky because most of
> those pointer fields (all 8-bytes wide) are individually embedded into
> their own containing structures. In the previous version of this patch
> set, I painstakingly verified the math, and pointed out wherever I
> thought updates were necessary.
Laszlo, you are right. it is indeed a bit tricky. Michael also has a concern
about it. I am changing to make it clear

> 
> I agree the math is hard to read, the code is very "dense". My
> suggestion (supporting yours) would be to calculate the fw_cfg blob
> offsets that should be patched in more fine-grained steps, possibly with
> multiple separate increments, using:
> - structure type names,
> - sizeof operators,
> - offsetof macros,
> - and possibly a separate comment for each offset increment.
good suggestion. I will follow that.

> 
> Thanks,
> Laszlo
> 
> .
>