On Tue, 6 Aug 2024 16:31:13 +0200
Igor Mammedov <imamm...@redhat.com> wrote:
> On Fri, 2 Aug 2024 23:44:01 +0200
> Mauro Carvalho Chehab <mchehab+hua...@kernel.org> wrote:
>
> > Provide a generic interface for error injection via GHESv2.
> >
> > This patch is co-authored:
> > - original ghes logic to inject a simple ARM record by Shiju Jose;
> > - generic logic to handle block addresses by Jonathan Cameron;
> > - generic GHESv2 error inject by Mauro Carvalho Chehab;
> >
> > Co-authored-by: Jonathan Cameron <jonathan.came...@huawei.com>
> > Co-authored-by: Shiju Jose <shiju.j...@huawei.com>
> > Co-authored-by: Mauro Carvalho Chehab <mchehab+hua...@kernel.org>
> > Cc: Jonathan Cameron <jonathan.came...@huawei.com>
> > Cc: Shiju Jose <shiju.j...@huawei.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+hua...@kernel.org>
> > ---
> > hw/acpi/ghes.c | 159 ++++++++++++++++++++++++++++++++++++++---
> > hw/acpi/ghes_cper.c | 2 +-
> > include/hw/acpi/ghes.h | 3 +
> > 3 files changed, 152 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> > index a745dcc7be5e..e125c9475773 100644
> > --- a/hw/acpi/ghes.c
> > +++ b/hw/acpi/ghes.c
> > @@ -395,23 +395,22 @@ void acpi_ghes_add_fw_cfg(AcpiGhesState *ags,
> > FWCfgState *s,
> > ags->present = true;
> > }
> >
> > +static uint64_t ghes_get_state_start_address(void)
>
> ghes_get_hardware_errors_address() might better reflect what address it will
> return
>
> > +{
> > + AcpiGedState *acpi_ged_state =
> > + ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED, NULL));
> > + AcpiGhesState *ags = &acpi_ged_state->ghes_state;
> > +
> > + return le64_to_cpu(ags->ghes_addr_le);
> > +}
> > +
> > int acpi_ghes_record_errors(uint8_t source_id, uint64_t physical_address)
> > {
> > uint64_t error_block_addr, read_ack_register_addr, read_ack_register =
> > 0;
> > - uint64_t start_addr;
> > + uint64_t start_addr = ghes_get_state_start_address();
> > bool ret = -1;
> > - AcpiGedState *acpi_ged_state;
> > - AcpiGhesState *ags;
> > -
> > assert(source_id < ACPI_HEST_SRC_ID_RESERVED);
> >
> > - acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED,
> > - NULL));
> > - g_assert(acpi_ged_state);
> > - ags = &acpi_ged_state->ghes_state;
> > -
> > - start_addr = le64_to_cpu(ags->ghes_addr_le);
> > -
> > if (physical_address) {
> > start_addr += source_id * sizeof(uint64_t);
>
> above should be a separate patch
>
> >
> > @@ -448,9 +447,147 @@ int acpi_ghes_record_errors(uint8_t source_id,
> > uint64_t physical_address)
> > return ret;
> > }
> >
> > +/*
> > + * Error register block data layout
> > + *
> > + * | +---------------------+ ges.ghes_addr_le
> > + * | |error_block_address0 |
> > + * | +---------------------+
> > + * | |error_block_address1 |
> > + * | +---------------------+ --+--
> > + * | | ............. | GHES_ADDRESS_SIZE
> > + * | +---------------------+ --+--
> > + * | |error_block_addressN |
> > + * | +---------------------+
> > + * | | read_ack0 |
> > + * | +---------------------+ --+--
> > + * | | read_ack1 | GHES_ADDRESS_SIZE
> > + * | +---------------------+ --+--
> > + * | | ............. |
> > + * | +---------------------+
> > + * | | read_ackN |
> > + * | +---------------------+ --+--
> > + * | | CPER | |
> > + * | | .... | GHES_MAX_RAW_DATA_LENGT
> > + * | | CPER | |
> > + * | +---------------------+ --+--
> > + * | | .......... |
> > + * | +---------------------+
> > + * | | CPER |
> > + * | | .... |
> > + * | | CPER |
> > + * | +---------------------+
> > + */
>
> no need to duplicate docs/specs/acpi_hest_ghes.rst,
> I'd just reffer to it and maybe add short comment as to why it's mentioned.
>
> > +/* Map from uint32_t notify to entry offset in GHES */
> > +static const uint8_t error_source_to_index[] = { 0xff, 0xff, 0xff, 0xff,
> > + 0xff, 0xff, 0xff, 1, 0};
> > +
> > +static bool ghes_get_addr(uint32_t notify, uint64_t *error_block_addr,
> > + uint64_t *read_ack_addr)
> > +{
> > + uint64_t base;
> > +
> > + if (notify >= ACPI_GHES_NOTIFY_RESERVED) {
> > + return false;
> > + }
> > +
> > + /* Find and check the source id for this new CPER */
> > + if (error_source_to_index[notify] == 0xff) {
> > + return false;
> > + }
> > +
> > + base = ghes_get_state_start_address();
> > +
> > + *read_ack_addr = base +
> > + ACPI_GHES_ERROR_SOURCE_COUNT * sizeof(uint64_t) +
> > + error_source_to_index[notify] * sizeof(uint64_t);
> > +
> > + /* Could also be read back from the error_block_address register */
> > + *error_block_addr = base +
> > + ACPI_GHES_ERROR_SOURCE_COUNT * sizeof(uint64_t) +
> > + ACPI_GHES_ERROR_SOURCE_COUNT * sizeof(uint64_t) +
> > + error_source_to_index[notify] * ACPI_GHES_MAX_RAW_DATA_LENGTH;
> > +
> > + return true;
> > +}
>
> I don't like all this pointer math, which is basically a reverse engineered
> QEMU actions on startup + guest provided etc/hardware_errors address.
>
> For once, it assumes error_source_to_index[] matches order in which HEST
> error sources were described, which is fragile.
>
> 2nd: migration-wive it's disaster, since old/new HEST/hardware_errors tables
> in RAM migrated from older version might not match above assumptions
> of target QEMU.
>
> I see 2 ways to rectify it:
> 1st: preferred/cleanest would be to tell QEMU (via fw_cfg) address of HEST
> table
> in guest RAM, like we do with etc/hardware_errors, see
> build_ghes_error_table()
> ...
> tell firmware to write hardware_errors GPA into
> and then fetch from HEST table in RAM, the guest patched error/ack
> addresses
> for given source_id
>
> code-wise: relatively simple once one wraps their own head over
> how this whole APEI thing works in QEMU
> workflow is described in docs/specs/acpi_hest_ghes.rst
> look to me as sufficient to grasp it.
> (but my view is very biased given my prior knowledge,
> aka: docs/comments/examples wrt acpi patching are good
> enough)
> (if it's not clear how to do it, ask me for pointers)
Hi Igor, I think I follow what you mean but maybe this question will reveal
otherwise. HEST is currently in ACPI_BUILD_TABLE_FILE.
Would you suggest splitting it to it's own file, or using table_offsets
to get the offset in ACPI_BUILD_TABLE_FILE GPA?
