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

[Igor Mammedov]

On Mon, 18 Nov 2019 08:21:18 -0500
"Michael S. Tsirkin" <address@hidden> wrote:

> 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.
I don't think the series is ready for merging yet.
It has a number of issues (not stylistic ones) that need to be fixed first.

As for extending, I think I've suggested to simplify series
to account for single error source only in some places so it
would be easier on author and reviewers and worry about extending
it later.