Re: [PATCH v5 6/7] acpi/ghes: add support for generic error injection via QAPI

[Mauro Carvalho Chehab]

Em Tue, 6 Aug 2024 16:31:13 +0200
Igor Mammedov <imammedo@redhat.com> escreveu:

> > +    /* 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)

That sounds a better approach, however...

>   2nd:  sort of hack based on build_ghes_v2() Error Status Address/Read Ack 
> Register
>         patching instructions
>                bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,  
>               
>                    address_offset + GAS_ADDR_OFFSET, sizeof(uint64_t),        
>               
>                    ACPI_GHES_ERRORS_FW_CFG_FILE, source_id * 
> sizeof(uint64_t));
>                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^
>         during build_ghes_v2() also store on a side mapping
>              source_id -> error address offset : read ack address
> 
>         so when you are injecting error, you'd at least use offsets
>         used at start time, to get rid of risk where injection code
>         diverge from HEST:etc/hardware_errors layout at start time.
> 
>         However to make migration safe, one would need to add a fat
>         comment not to change order ghest error sources in HEST _and_
>         a dedicated unit test to make sure we catch it when that happens.
>         bios_tables_test should be able to catch the change, but it won't
>         say what's wrong, hence a test case that explicitly checks order
>         and loudly & clear complains when we will break order assumptions.
> 
>         downside:
>            * we are are limiting ways HEST could be composed/reshuffled in 
> future
>            * consumption of extra CI resources
>            * and well, it relies on above duct tape holding all pieces 
> together

I ended opting to do approach (2) on this changeset, as the current code
is already using bios_linker_loader_add_pointer() for ghes, being deeply 
relying on the block address/ack and cper calculus.

To avoid troubles on this duct tape, I opted to move all offset math
to a single function at ghes.c:

        /*
         * ID numbers used to fill HEST source ID field
         */
        enum AcpiHestSourceId {
            ACPI_HEST_SRC_ID_SEA,
            ACPI_HEST_SRC_ID_GED,
        
            /* Shall be the last one */
            ACPI_HEST_SRC_ID_COUNT
        } AcpiHestSourceId;

        ...

        static bool acpi_hest_address_offset(enum AcpiGhesNotifyType notify,
                                             uint64_t *error_block_offset,
                                             uint64_t *ack_offset,
                                             uint64_t *cper_offset,
                                             enum AcpiHestSourceId *source_id)
        {
            enum AcpiHestSourceId source;
            uint64_t offset;

            switch (notify) {
            case ACPI_GHES_NOTIFY_SEA:      /* Only on ARMv8 */
                source = ACPI_HEST_SRC_ID_SEA;
                break;
            case ACPI_GHES_NOTIFY_GPIO:
                source = ACPI_HEST_SRC_ID_GED;
                break;
            default:
                return true;
            }

            if (source_id) {
                *source_id = source;
            }

            /*
             * Please see docs/specs/acpi_hest_ghes.rst for the memory layout.
             * In summary, memory starts with error addresses, then acks and
             * finally CPER blocks.
             */

            offset = source * sizeof(uint64_t);

            if (error_block_offset) {
                *error_block_offset = offset;
            }
            if (ack_offset) {
                *ack_offset = offset + ACPI_HEST_SRC_ID_COUNT * 
sizeof(uint64_t);
            }
            if (cper_offset) {
                *cper_offset = 2 * ACPI_HEST_SRC_ID_COUNT * sizeof(uint64_t) +
                               source * ACPI_GHES_MAX_RAW_DATA_LENGTH;
            }

            return false;
        }

I also removed the anonymous enum with SEA/GPIO source IDs, using
only the ACPI notify type as arguments at the function calls.

As there's now a single point where the offsets from
docs/specs/acpi_hest_ghes.rst are enforced, this should be error
prone.

The code could later be changed to use approach (2), on a separate
cleanup.

Thanks,
Mauro