On 07/13/17 19:32, Michael S. Tsirkin wrote:
> On Wed, Jul 12, 2017 at 10:08:16AM +0800, Dongjiu Geng wrote:
snip
>> etc/acpi/tables etc/hardware_errors
>> ================ ==========================================
>> +-----------+
>> +--------------+ | address | +-> +--------------+
>> | HEST + | registers | | | Error Status |
>> + +------------+ | +---------+ | | Data Block 0 |
>> | | GHES0 | --> | |address0 | --------+ | +------------+
>> | | GHES1 | --> | |address1 | ------+ | | CPER |
>> | | GHES2 | --> | |address2 | ----+ | | | CPER |
>> | | .... | --> | | ....... | | | | | CPER |
>> | | GHES10 | --> | |address10| -+ | | | | CPER |
>> +-+------------+ +-+---------+ | | | +-+------------+
>> | | |
>> | | +---> +--------------+
>> | | | Error Status |
>> | | | Data Block 1 |
>> | | | +------------+
>> | | | | CPER |
>> | | | | CPER |
>> | | +-+------------+
>> | |
>> | +-----> +--------------+
>> | | Error Status |
>> | | Data Block 2 |
>> | | +------------+
>> | | | CPER |
>> | +-+------------+
>> | ...........
>> +--------> +--------------+
>> | Error Status |
>> | Data Block 10|
>> | +------------+
>> | | CPER |
>> | | CPER |
>> | | CPER |
>> +-+------------+
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.
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.
Thanks,
Laszlo
