Re: [PATCH v3 4/7] acpi/ghes: Add a logic to handle block addresses and FW first ARM processor error injection

From: Mauro Carvalho Chehab
Date: Mon Jul 29 2024 - 07:40:46 EST

Next message: Aleksa Sarai: "Re: Testing if two open descriptors refer to the same inode"
Previous message: Thomas Gleixner: "Re: [PATCH] x86/aperfmperf: Fix deadlock on cpu_hotplug_lock"
In reply to: Jonathan Cameron: "Re: [PATCH v3 4/7] acpi/ghes: Add a logic to handle block addresses and FW first ARM processor error injection"
Next in thread: Igor Mammedov: "Re: [PATCH v3 4/7] acpi/ghes: Add a logic to handle block addresses and FW first ARM processor error injection"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Em Fri, 26 Jul 2024 13:44:12 +0100
Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> escreveu:

> On Mon, 22 Jul 2024 08:45:56 +0200
> Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote:
>
> > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >
> > 1. Some GHES functions require handling addresses. Add a helper function
> > to support it.
> >
> > 2. Add support for ACPI CPER (firmware-first) ARM processor error injection.
> >
> > Compliance with N.2.4.4 ARM Processor Error Section in UEFI 2.6 and
> > upper specs, using error type bit encoding as detailed at UEFI 2.9A
> > errata.
> >
> > Error injection examples:
> >
> > { "execute": "qmp_capabilities" }
> >
> > { "execute": "arm-inject-error",
> > "arguments": {
> > "errortypes": ['cache-error']
> > }
> > }
> >
> > { "execute": "arm-inject-error",
> > "arguments": {
> > "errortypes": ['tlb-error']
> > }
> > }
> >
> > { "execute": "arm-inject-error",
> > "arguments": {
> > "errortypes": ['bus-error']
> > }
> > }
> >
> > { "execute": "arm-inject-error",
> > "arguments": {
> > "errortypes": ['cache-error', 'tlb-error']
> > }
> > }
> >
> > { "execute": "arm-inject-error",
> > "arguments": {
> > "errortypes": ['cache-error', 'tlb-error', 'bus-error', 'micro-arch-error']
> > }
> > }
> > ...
> >
> > Co-authored-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> > Co-authored-by: Shiju Jose <shiju.jose@xxxxxxxxxx>
> > For Add a logic to handle block addresses,
> # before comments I think?
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> > For FW first ARM processor error injection,
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> > Signed-off-by: Shiju Jose <shiju.jose@xxxxxxxxxx>
> I can't remember what I wrote in here so may well be commenting on
> my past self ;)
>
> > ---
> > configs/targets/aarch64-softmmu.mak | 1 +
> > hw/acpi/ghes.c | 258 ++++++++++++++++++++++++++--
> > hw/arm/Kconfig | 4 +
> > hw/arm/arm_error_inject.c | 35 ++++
> > hw/arm/arm_error_inject_stubs.c | 18 ++
> > hw/arm/meson.build | 3 +
> > include/hw/acpi/ghes.h | 2 +
> > qapi/arm-error-inject.json | 49 ++++++
> > qapi/meson.build | 1 +
> > qapi/qapi-schema.json | 1 +
> > 10 files changed, 361 insertions(+), 11 deletions(-)
> > create mode 100644 hw/arm/arm_error_inject.c
> > create mode 100644 hw/arm/arm_error_inject_stubs.c
> > create mode 100644 qapi/arm-error-inject.json
> >
> > diff --git a/configs/targets/aarch64-softmmu.mak b/configs/targets/aarch64-softmmu.mak
> > index 84cb32dc2f4f..b4b3cd97934a 100644
> > --- a/configs/targets/aarch64-softmmu.mak
> > +++ b/configs/targets/aarch64-softmmu.mak
> > @@ -5,3 +5,4 @@ TARGET_KVM_HAVE_GUEST_DEBUG=y
> > TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml gdb-xml/arm-core.xml gdb-xml/arm-vfp.xml gdb-xml/arm-vfp3.xml gdb-xml/arm-vfp-sysregs.xml gdb-xml/arm-neon.xml gdb-xml/arm-m-profile.xml gdb-xml/arm-m-profile-mve.xml gdb-xml/aarch64-pauth.xml
> > # needed by boot.c
> > TARGET_NEED_FDT=y
> > +CONFIG_ARM_EINJ=y
> > diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> > index 5b8bc6eeb437..6075ef5893ce 100644
> > --- a/hw/acpi/ghes.c
> > +++ b/hw/acpi/ghes.c
> > @@ -27,6 +27,7 @@
> > #include "hw/acpi/generic_event_device.h"
> > #include "hw/nvram/fw_cfg.h"
> > #include "qemu/uuid.h"
> > +#include "qapi/qapi-types-arm-error-inject.h"
> >
> > #define ACPI_GHES_ERRORS_FW_CFG_FILE "etc/hardware_errors"
> > #define ACPI_GHES_DATA_ADDR_FW_CFG_FILE "etc/hardware_errors_addr"
> > @@ -53,6 +54,12 @@
> > /* The memory section CPER size, UEFI 2.6: N.2.5 Memory Error Section */
> > #define ACPI_GHES_MEM_CPER_LENGTH 80
> >
> > +/*
> > + * ARM Processor section CPER size, UEFI 2.10: N.2.4.4
> > + * ARM Processor Error Section
> > + */
> > +#define ACPI_GHES_ARM_CPER_LENGTH (72 + 600)
> > +
> > /* Masks for block_status flags */
> > #define ACPI_GEBS_UNCORRECTABLE 1
> >
> > @@ -231,6 +238,142 @@ static int acpi_ghes_record_mem_error(uint64_t error_block_address,
> > return 0;
> > }
> >
> > +/* UEFI 2.9: N.2.4.4 ARM Processor Error Section */
> > +static void acpi_ghes_build_append_arm_cper(uint8_t error_types, GArray *table)
> > +{
> > + /*
> > + * ARM Processor Error Record
> > + */
> > +
> > + /* Validation Bits */
> > + build_append_int_noprefix(table,
> > + (1ULL << 3) | /* Vendor specific info Valid */
> > + (1ULL << 2) | /* Running status Valid */
> > + (1ULL << 1) | /* Error affinity level Valid */
> > + (1ULL << 0), /* MPIDR Valid */
> > + 4);
> > + /* Error Info Num */
> > + build_append_int_noprefix(table, 1, 2);
> > + /* Context Info Num */
> > + build_append_int_noprefix(table, 1, 2);
> > + /* Section length */
> > + build_append_int_noprefix(table, ACPI_GHES_ARM_CPER_LENGTH, 4);
> > + /* Error affinity level */
> > + build_append_int_noprefix(table, 2, 1);
> > + /* Reserved */
> > + build_append_int_noprefix(table, 0, 3);
> > + /* MPIDR_EL1 */
> > + build_append_int_noprefix(table, 0xAB12, 8);
>
> These need to be real - I see you fix that in later
> patches, but I'd be tempted to pull it back here. Or maybe just
> add a comment to say you will rewrite this later.
>
> I know you aren't keen to smash patches with different authorship
> together, but here I think you should just have this
> correct from the start (so combine this and 5-7)
> perhaps with some links back to the version where they are split?

I folded this with patch 7. I kept patch 5 as a separate one,
as it is a different logical change.

After folding, this field is filled from the emulation value for
it (by default, as it can be overridden via QMP).

> > + /* MIDR_EL1 */
> > + build_append_int_noprefix(table, 0xCD24, 8);
> > + /* Running state */
> > + build_append_int_noprefix(table, 0x1, 4);
> > + /* PSCI state */
> > + build_append_int_noprefix(table, 0x1234, 4);
> > +
> > + /* ARM Propcessor error information */
> > + /* Version */
> > + build_append_int_noprefix(table, 0, 1);
> > + /* Length */
> > + build_append_int_noprefix(table, 32, 1);
> > + /* Validation Bits */
> > + build_append_int_noprefix(table,
> > + (1ULL << 4) | /* Physical fault address Valid */
>
> Some tabs hiding in here that need to be spaces.

Solved when folding with patch 7.

> > + (1ULL << 3) | /* Virtual fault address Valid */
> > + (1ULL << 2) | /* Error information Valid */
> > + (1ULL << 1) | /* Flags Valid */
> > + (1ULL << 0), /* Multiple error count Valid */
> > + 2);
> > + /* Type */
> > + if (error_types & BIT(ARM_PROCESSOR_ERROR_TYPE_CACHE_ERROR) ||
> > + error_types & BIT(ARM_PROCESSOR_ERROR_TYPE_TLB_ERROR) ||
> > + error_types & BIT(ARM_PROCESSOR_ERROR_TYPE_BUS_ERROR) ||
> > + error_types & BIT(ARM_PROCESSOR_ERROR_TYPE_MICRO_ARCH_ERROR)) {
> > + build_append_int_noprefix(table, error_types, 1);
> > + } else {
> > + return;
> > + }
> > + /* Multiple error count */
> > + build_append_int_noprefix(table, 2, 2);
> > + /* Flags */
> > + build_append_int_noprefix(table, 0xD, 1);
> > + /* Error information */
> > + if (error_types & BIT(ARM_PROCESSOR_ERROR_TYPE_CACHE_ERROR)) {
> > + build_append_int_noprefix(table, 0x0091000F, 8);
> > + } else if (error_types & BIT(ARM_PROCESSOR_ERROR_TYPE_TLB_ERROR)) {
> > + build_append_int_noprefix(table, 0x0054007F, 8);
> > + } else if (error_types & BIT(ARM_PROCESSOR_ERROR_TYPE_BUS_ERROR)) {
> > + build_append_int_noprefix(table, 0x80D6460FFF, 8);
> > + } else if (error_types & BIT(ARM_PROCESSOR_ERROR_TYPE_MICRO_ARCH_ERROR)) {
> > + build_append_int_noprefix(table, 0x78DA03FF, 8);
> > + } else {
> > + return;
> > + }
> > + /* Virtual fault address */
> > + build_append_int_noprefix(table, 0x67320230, 8);
> > + /* Physical fault address */
> > + build_append_int_noprefix(table, 0x5CDFD492, 8);
> > +
> > + /* ARM Propcessor error context information */
> > + /* Version */
> > + build_append_int_noprefix(table, 0, 2);
> > + /* Validation Bits */
> > + /* AArch64 EL1 context registers Valid */
> > + build_append_int_noprefix(table, 5, 2);
> > + /* Register array size */
> > + build_append_int_noprefix(table, 592, 4);
> > + /* Register array */
> > + build_append_int_noprefix(table, 0x12ABDE67, 8);
> > +}
> > +
> > +static int acpi_ghes_record_arm_error(uint8_t error_types,
> > + uint64_t error_block_address)
> > +{
> > + GArray *block;
> > +
> > + /* ARM processor Error Section Type */
> > + const uint8_t uefi_cper_arm_sec[] =
> > + UUID_LE(0xE19E3D16, 0xBC11, 0x11E4, 0x9C, 0xAA, 0xC2, 0x05, \
> > + 0x1D, 0x5D, 0x46, 0xB0);
> > +
> > + /*
> > + * Invalid fru id: ACPI 4.0: 17.3.2.6.1 Generic Error Data,
> > + * Table 17-13 Generic Error Data Entry
> > + */
> > + QemuUUID fru_id = {};
> > + uint32_t data_length;
> > +
> > + block = g_array_new(false, true /* clear */, 1);
> > +
> > + /* This is the length if adding a new generic error data entry*/
>
> space before *

Fixed.

>
> > + data_length = ACPI_GHES_DATA_LENGTH + ACPI_GHES_ARM_CPER_LENGTH;
> > + /*
> > + * It should not run out of the preallocated memory if adding a new generic
> > + * error data entry
> > + */
> > + assert((data_length + ACPI_GHES_GESB_SIZE) <=
> > + ACPI_GHES_MAX_RAW_DATA_LENGTH);
> > +
> > + /* Build the new generic error status block header */
> > + acpi_ghes_generic_error_status(block, ACPI_GEBS_UNCORRECTABLE,
> > + 0, 0, data_length, ACPI_CPER_SEV_RECOVERABLE);
> > +
> > + /* Build this new generic error data entry header */
> > + acpi_ghes_generic_error_data(block, uefi_cper_arm_sec,
> > + ACPI_CPER_SEV_RECOVERABLE, 0, 0,
> > + ACPI_GHES_ARM_CPER_LENGTH, fru_id, 0);
> > +
> > + /* Build the ARM processor error section CPER */
> > + acpi_ghes_build_append_arm_cper(error_types, block);
> > +
> > + /* Write the generic error data entry into guest memory */
> > + cpu_physical_memory_write(error_block_address, block->data, block->len);
> > +
> > + g_array_free(block, true);
> > +
> > + return 0;
> > +}
>
>
> > +bool ghes_record_arm_errors(uint8_t error_types, uint32_t notify)
> > +{
> > + int read_ack_register = 0;
> > + uint64_t read_ack_register_addr = 0;
> > + uint64_t error_block_addr = 0;
> > +
> > + if (!ghes_get_addr(notify, &error_block_addr, &read_ack_register_addr)) {
> > + return false;
> > + }
> > +
> > + cpu_physical_memory_read(read_ack_register_addr,
> > + &read_ack_register, sizeof(uint64_t));
>
> longer but I'd prefer sizeof(read_ack_register)
> Maybe we can shorten to read_ack and read_ack_addr?
>
> > + /* zero means OSPM does not acknowledge the error */
> > + if (!read_ack_register) {
> > + error_report("Last time OSPM does not acknowledge the error,"
> > + " record CPER failed this time, set the ack value to"
> > + " avoid blocking next time CPER record! exit");
> > + read_ack_register = 1;
> > + cpu_physical_memory_write(read_ack_register_addr,
> > + &read_ack_register, sizeof(uint64_t));
> sizeof(read_ack_register)
>
> > + return false;
> > + }
> > +
> > + read_ack_register = cpu_to_le64(0);
> > + cpu_physical_memory_write(read_ack_register_addr,
> > + &read_ack_register, sizeof(uint64_t));
>
> sizeof(read_ack_register)
>
> > + return acpi_ghes_record_arm_error(error_types, error_block_addr);
> > +}
> > +

Changed as suggested.

>
> > diff --git a/qapi/arm-error-inject.json b/qapi/arm-error-inject.json
> > new file mode 100644
> > index 000000000000..430e6cea6b60
> > --- /dev/null
> > +++ b/qapi/arm-error-inject.json
>
> > +##
> > +# @arm-inject-error:
> > +#
> > +# Inject ARM Processor error.
> > +#
> > +# @errortypes: ARM processor error types to inject
> > +#
> > +# Features:
> > +#
> > +# @unstable: This command is experimental.
> > +#
> > +# Since: 9.1
> Update to 9.2 on next version.

Ok.

Thanks,
Mauro

Next message: Aleksa Sarai: "Re: Testing if two open descriptors refer to the same inode"
Previous message: Thomas Gleixner: "Re: [PATCH] x86/aperfmperf: Fix deadlock on cpu_hotplug_lock"
In reply to: Jonathan Cameron: "Re: [PATCH v3 4/7] acpi/ghes: Add a logic to handle block addresses and FW first ARM processor error injection"
Next in thread: Igor Mammedov: "Re: [PATCH v3 4/7] acpi/ghes: Add a logic to handle block addresses and FW first ARM processor error injection"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]