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

[Laszlo Ersek <lersek@redhat.com>]

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