| Message ID | ba1864f1aa7073abe090eec0c31915f187967140.1723793768.git.mchehab+huawei@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add ACPI CPER firmware first error injection on ARM emulation | expand |
On Fri, 16 Aug 2024 09:37:35 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > As a GED error device is now defined, add another type > of notification. > > Add error notification to GHES v2 using >a GED error device GED triggered via interrupt. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This is hard to parse, perhaps update so it would be more clear what does what > > [mchehab: do some cleanups at ACPI_HEST_SRC_ID_* checks and > rename HEST event to better identify GED interrupt OSPM] > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- in addition to change log in cover letter, I'd suggest to keep per patch change log as well (after ---), it helps reviewer to notice intended changes. [...] > + case ACPI_HEST_SRC_ID_GED: > + build_ghes_hw_error_notification(table_data, ACPI_GHES_NOTIFY_GPIO); While GPIO works for arm, it's not the case for other machines. I recall a suggestion to use ACPI_GHES_NOTIFY_EXTERNAL instead of GPIO one, but that got lost somewhere... > + break; > default: > error_report("Not support this error source"); > abort(); > @@ -370,6 +376,7 @@ void acpi_build_hest(GArray *table_data, BIOSLinker *linker, > /* Error Source Count */ > build_append_int_noprefix(table_data, ACPI_GHES_ERROR_SOURCE_COUNT, 4); > build_ghes_v2(table_data, ACPI_HEST_SRC_ID_SEA, linker); > + build_ghes_v2(table_data, ACPI_HEST_SRC_ID_GED, linker); > > acpi_table_end(linker, &table); > } > diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h > index fb80897e7eac..419a97d5cbd9 100644 > --- a/include/hw/acpi/ghes.h > +++ b/include/hw/acpi/ghes.h > @@ -59,9 +59,10 @@ enum AcpiGhesNotifyType { > ACPI_GHES_NOTIFY_RESERVED = 12 > }; > > +/* Those are used as table indexes when building GHES tables */ > enum { > ACPI_HEST_SRC_ID_SEA = 0, > - /* future ids go here */ > + ACPI_HEST_SRC_ID_GED, > ACPI_HEST_SRC_ID_RESERVED, > }; >
Em Mon, 19 Aug 2024 13:43:04 +0200 Igor Mammedov <imammedo@redhat.com> escreveu: > On Fri, 16 Aug 2024 09:37:35 +0200 > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > As a GED error device is now defined, add another type > > of notification. > > > > Add error notification to GHES v2 using > >a GED error device GED triggered via interrupt. > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > This is hard to parse, perhaps update so it would be > more clear what does what > > > > > [mchehab: do some cleanups at ACPI_HEST_SRC_ID_* checks and > > rename HEST event to better identify GED interrupt OSPM] > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > --- > > in addition to change log in cover letter, > I'd suggest to keep per patch change log as well (after ---), > it helps reviewer to notice intended changes. > > > [...] > > + case ACPI_HEST_SRC_ID_GED: > > + build_ghes_hw_error_notification(table_data, ACPI_GHES_NOTIFY_GPIO); > While GPIO works for arm, it's not the case for other machines. > I recall a suggestion to use ACPI_GHES_NOTIFY_EXTERNAL instead of GPIO one, > but that got lost somewhere... True, but the same also applies to SEA, which is ARMv8+. After having everything in place, I confined the source ID into this code inside ghes.c: 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 ghes_notify_to_source_id(enum AcpiGhesNotifyType notify, enum AcpiHestSourceId *source_id) { switch (notify) { case ACPI_GHES_NOTIFY_SEA: /* ARMv8 */ *source_id = ACPI_HEST_SRC_ID_SEA; return false; case ACPI_GHES_NOTIFY_GPIO: *source_id = ACPI_HEST_SRC_ID_GED; return false; default: /* Unsupported notification types */ return true; } } The only place where the source ID number is used is at ghes_notify_to_source_id() - still we use ACPI_HEST_SRC_ID_COUNT on other places to initialize and fill in the HEST table and its error source structures. On other words, the source ID field is filled from the notification types as defined at include/hw/acpi/ghes.h: ACPI_GHES_NOTIFY_POLLED = 0, ACPI_GHES_NOTIFY_EXTERNAL = 1, ACPI_GHES_NOTIFY_LOCAL = 2, ACPI_GHES_NOTIFY_SCI = 3, ACPI_GHES_NOTIFY_NMI = 4, ACPI_GHES_NOTIFY_CMCI = 5, ACPI_GHES_NOTIFY_MCE = 6, ACPI_GHES_NOTIFY_GPIO = 7, ACPI_GHES_NOTIFY_SEA = 8, ACPI_GHES_NOTIFY_SEI = 9, ACPI_GHES_NOTIFY_GSIV = 10, ACPI_GHES_NOTIFY_SDEI = 11, (please notice that ACPI already defines "EXTERNAL" as being something else) Now, if we want to add support for x86, we could either add some ifdefs inside ghes.c, e. g. something like: enum AcpiHestSourceId { #ifdef TARGET_ARM ACPI_HEST_SRC_ID_SEA, ACPI_HEST_SRC_ID_GED, #endif #ifdef TARGET_I386 ACPI_HEST_SRC_ID_MCE, #endif /* Shall be the last one */ ACPI_HEST_SRC_ID_COUNT } AcpiHestSourceId; and something similar at ghes_notify_to_source_id(): static bool ghes_notify_to_source_id(enum AcpiGhesNotifyType notify, enum AcpiHestSourceId *source_id) { switch (notify) { #ifdef TARGET_ARM case ACPI_GHES_NOTIFY_SEA: /* ARMv8 */ *source_id = ACPI_HEST_SRC_ID_SEA; return false; case ACPI_GHES_NOTIFY_GPIO: *source_id = ACPI_HEST_SRC_ID_GED; return false; #endif #ifdef TARGET_I386 case ACPI_GHES_NOTIFY_MCE: *source_id = ACPI_HEST_SRC_ID_MCE; return false; #endif default: /* Unsupported notification types */ return true; } } An alternative would be to move source id/notification code out, placing them at hw/arm, hw/i386, but a more complex binding logic will be needed. If we're willing to do something like that, I would prefer to not do such redesign now. Better to do such change when we'll be ready to add some notification support that works on x86 (MCE? SCI? NMI?). Regards, Mauro
diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c index 13b105c5d02d..df59fd35568c 100644 --- a/hw/acpi/ghes.c +++ b/hw/acpi/ghes.c @@ -34,8 +34,8 @@ /* The max size in bytes for one error block */ #define ACPI_GHES_MAX_RAW_DATA_LENGTH (1 * KiB) -/* Now only support ARMv8 SEA notification type error source */ -#define ACPI_GHES_ERROR_SOURCE_COUNT 1 +/* Support ARMv8 SEA notification type error source and GPIO interrupt. */ +#define ACPI_GHES_ERROR_SOURCE_COUNT 2 /* Generic Hardware Error Source version 2 */ #define ACPI_GHES_SOURCE_GENERIC_ERROR_V2 10 @@ -290,6 +290,9 @@ void build_ghes_error_table(GArray *hardware_errors, BIOSLinker *linker) static void build_ghes_v2(GArray *table_data, int source_id, BIOSLinker *linker) { uint64_t address_offset; + + assert(source_id < ACPI_HEST_SRC_ID_RESERVED); + /* * Type: * Generic Hardware Error Source version 2(GHESv2 - Type 10) @@ -327,6 +330,9 @@ static void build_ghes_v2(GArray *table_data, int source_id, BIOSLinker *linker) */ build_ghes_hw_error_notification(table_data, ACPI_GHES_NOTIFY_SEA); break; + case ACPI_HEST_SRC_ID_GED: + build_ghes_hw_error_notification(table_data, ACPI_GHES_NOTIFY_GPIO); + break; default: error_report("Not support this error source"); abort(); @@ -370,6 +376,7 @@ void acpi_build_hest(GArray *table_data, BIOSLinker *linker, /* Error Source Count */ build_append_int_noprefix(table_data, ACPI_GHES_ERROR_SOURCE_COUNT, 4); build_ghes_v2(table_data, ACPI_HEST_SRC_ID_SEA, linker); + build_ghes_v2(table_data, ACPI_HEST_SRC_ID_GED, linker); acpi_table_end(linker, &table); } diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h index fb80897e7eac..419a97d5cbd9 100644 --- a/include/hw/acpi/ghes.h +++ b/include/hw/acpi/ghes.h @@ -59,9 +59,10 @@ enum AcpiGhesNotifyType { ACPI_GHES_NOTIFY_RESERVED = 12 }; +/* Those are used as table indexes when building GHES tables */ enum { ACPI_HEST_SRC_ID_SEA = 0, - /* future ids go here */ + ACPI_HEST_SRC_ID_GED, ACPI_HEST_SRC_ID_RESERVED, };