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.