Re: [RESEND PATCH v21 3/6] ACPI: Add APEI GHES table generation support

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Nov 18, 2019 at 09:18:01PM +0800, gengdongjiu wrote:
> On 2019/11/18 20:49, gengdongjiu wrote:
> >>> +     */
> >>> +    build_append_int_noprefix(table_data, source_id, 2);
> >>> +    /* Related Source Id */
> >>> +    build_append_int_noprefix(table_data, 0xffff, 2);
> >>> +    /* Flags */
> >>> +    build_append_int_noprefix(table_data, 0, 1);
> >>> +    /* Enabled */
> >>> +    build_append_int_noprefix(table_data, 1, 1);
> >>> +
> >>> +    /* Number of Records To Pre-allocate */
> >>> +    build_append_int_noprefix(table_data, 1, 4);
> >>> +    /* Max Sections Per Record */
> >>> +    build_append_int_noprefix(table_data, 1, 4);
> >>> +    /* Max Raw Data Length */
> >>> +    build_append_int_noprefix(table_data, ACPI_GHES_MAX_RAW_DATA_LENGTH, 4);
> >>> +
> >>> +    /* Error Status Address */
> >>> +    build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 0x40, 0,
> >>> +                     4 /* QWord access */, 0);
> >>> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> >>> +        ACPI_GHES_ERROR_STATUS_ADDRESS_OFFSET(hest_start, source_id),
> >> it's fine only if GHESv2 is the only entries in HEST, but once
> >> other types are added this macro will silently fall apart and
> >> cause table corruption.
>    why  silently fall?
>    I think the acpi_ghes.c only support GHESv2 type, not support other type.
> 
> >>
> >> Instead of offset from hest_start, I suggest to use offset relative
> >> to GAS structure, here is an idea>>
> >> #define GAS_ADDR_OFFSET 4
> >>
> >>     off = table->len
> >>     build_append_gas()
> >>     bios_linker_loader_add_pointer(...,
> >>         off + GAS_ADDR_OFFSET, ...
> 
> If use offset relative to GAS structure, the code does not easily extend to support more Generic Hardware Error Source.
> if use offset relative to hest_start, just use a loop, the code can support  more error source, for example:
> for (source_id = 0; i<n; source_id++)
> {
>    ......
>     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>         ACPI_GHES_ERROR_STATUS_ADDRESS_OFFSET(hest_start, source_id),
>         sizeof(uint64_t), ACPI_GHES_ERRORS_FW_CFG_FILE,
>         source_id * sizeof(uint64_t));
>   .......
> }
> 
> My previous series patch support 2 error sources, but now only enable 'SEA' type Error Source

I'd try to merge this, worry about extending things later.
This is at v21 and the simpler you can keep things,
the faster it'll go in.