| Message ID | 66c1ab4988589be99ae925c6361548f55fea58b0.1721630625.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add ACPI CPER firmware first error injection for Arm Processor | expand |
On Mon, 22 Jul 2024 08:45:58 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > There is one reference to ACPI 4.0 and several references > to ACPI 6.x versions. > > Update them to point to ACPI 6.5 whenever possible. when it comes to APCI doc comments, they should point to the 1st (earliest) revision that provides given feature/value/field/table. > There's one reference that was kept pointing to ACPI 6.4, > though, with HEST revision 1. > > ACPI 6.5 now defines HEST revision 2, and defined a new > way to handle source types starting from 12. According > with ACPI 6.5 revision history: > > 2312 Update to the HEST table and adding new error > source descriptor - Table 18.2. > > Yet, the spec doesn't define yet any new source > descriptors. It just defines a different behavior when > source type is above 11. > > I also double-checked GHES implementation on an open > source project (Linux Kernel). Currently upstream > doesn't currently handle HEST revision, ignoring such > field. > > In any case, revision 2 seems to be backward-compatible > with revison 1 when type <= 11 and just one error is > contained on a HEST record. > > So, while it is probably safe to update it, there's no > real need. So, let's keep the implementation using > an ACPI 6.4 compatible table, e. g. HEST revision 1. > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > --- > hw/acpi/ghes.c | 48 ++++++++++++++++++++++++++++-------------------- > 1 file changed, 28 insertions(+), 20 deletions(-) > > diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c > index 6075ef5893ce..ebf1b812aaaa 100644 > --- a/hw/acpi/ghes.c > +++ b/hw/acpi/ghes.c > @@ -45,9 +45,9 @@ > #define GAS_ADDR_OFFSET 4 > > /* > - * The total size of Generic Error Data Entry > - * ACPI 6.1/6.2: 18.3.2.7.1 Generic Error Data, > - * Table 18-343 Generic Error Data Entry > + * The total size of Generic Error Data Entry before data field > + * ACPI 6.5: 18.3.2.7.1 Generic Error Data, > + * Table 18.12 Generic Error Data Entry > */ > #define ACPI_GHES_DATA_LENGTH 72 > > @@ -65,8 +65,8 @@ > > /* > * Total size for Generic Error Status Block except Generic Error Data Entries > - * ACPI 6.2: 18.3.2.7.1 Generic Error Data, > - * Table 18-380 Generic Error Status Block > + * ACPI 6.5: 18.3.2.7.1 Generic Error Data, > + * Table 18.11 Generic Error Status Block > */ > #define ACPI_GHES_GESB_SIZE 20 > > @@ -82,7 +82,8 @@ enum AcpiGenericErrorSeverity { > > /* > * Hardware Error Notification > - * ACPI 4.0: 17.3.2.7 Hardware Error Notification > + * ACPI 6.5: 18.3.2.9 Hardware Error Notification, > + * Table 18.14 - Hardware Error Notification Structure > * Composes dummy Hardware Error Notification descriptor of specified type > */ > static void build_ghes_hw_error_notification(GArray *table, const uint8_t type) > @@ -112,7 +113,8 @@ static void build_ghes_hw_error_notification(GArray *table, const uint8_t type) > > /* > * Generic Error Data Entry > - * ACPI 6.1: 18.3.2.7.1 Generic Error Data > + * ACPI 6.5: 18.3.2.7.1 Generic Error Data, > + * Table 18.12 - Generic Error Data Entry > */ > static void acpi_ghes_generic_error_data(GArray *table, > const uint8_t *section_type, uint32_t error_severity, > @@ -148,7 +150,8 @@ static void acpi_ghes_generic_error_data(GArray *table, > > /* > * Generic Error Status Block > - * ACPI 6.1: 18.3.2.7.1 Generic Error Data > + * ACPI 6.5: 18.3.2.7.1 Generic Error Data, > + * Table 18.11 - Generic Hardware Error Source Structure > */ > static void acpi_ghes_generic_error_status(GArray *table, uint32_t block_status, > uint32_t raw_data_offset, uint32_t raw_data_length, > @@ -429,15 +432,18 @@ void build_ghes_error_table(GArray *hardware_errors, BIOSLinker *linker) > 0, sizeof(uint64_t), ACPI_GHES_ERRORS_FW_CFG_FILE, 0); > } > > -/* Build Generic Hardware Error Source version 2 (GHESv2) */ > +/* > + * Build Generic Hardware Error Source version 2 (GHESv2) > + * ACPI 6.5: 18.3.2.8 Generic Hardware Error Source version 2 (GHESv2 - Type 10), > + * Table 18.13: Generic Hardware Error Source version 2 (GHESv2) > + */ > static void build_ghes_v2(GArray *table_data, int source_id, BIOSLinker *linker) > { > uint64_t address_offset; > - /* > - * Type: > - * Generic Hardware Error Source version 2(GHESv2 - Type 10) > - */ > + /* Type: (GHESv2 - Type 10) */ > build_append_int_noprefix(table_data, ACPI_GHES_SOURCE_GENERIC_ERROR_V2, 2); > + > + /* ACPI 6.5: Table 18.10 - Generic Hardware Error Source Structure */ > /* Source Id */ > build_append_int_noprefix(table_data, source_id, 2); > /* Related Source Id */ > @@ -481,11 +487,8 @@ static void build_ghes_v2(GArray *table_data, int source_id, BIOSLinker *linker) > /* Error Status Block Length */ > build_append_int_noprefix(table_data, ACPI_GHES_MAX_RAW_DATA_LENGTH, 4); > > - /* > - * Read Ack Register > - * ACPI 6.1: 18.3.2.8 Generic Hardware Error Source > - * version 2 (GHESv2 - Type 10) > - */ > + /* ACPI 6.5: fields defined at GHESv2 table */ > + /* Read Ack Register */ > address_offset = table_data->len; > build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 0x40, 0, > 4 /* QWord access */, 0); > @@ -504,11 +507,16 @@ static void build_ghes_v2(GArray *table_data, int source_id, BIOSLinker *linker) > build_append_int_noprefix(table_data, 0x1, 8); > } > > -/* Build Hardware Error Source Table */ > +/* > + * Build Hardware Error Source Table > + * ACPI 6.4: 18.3.2 ACPI Error Source > + * Table 18.2: Hardware Error Source Table (HEST) > + */ > void acpi_build_hest(GArray *table_data, BIOSLinker *linker, > const char *oem_id, const char *oem_table_id) > { > - AcpiTable table = { .sig = "HEST", .rev = 1, > + AcpiTable table = { .sig = "HEST", > + .rev = 1, /* ACPI 4.0 to 6.4 */ > .oem_id = oem_id, .oem_table_id = oem_table_id }; > > acpi_table_begin(&table, table_data);
On Tue, Jul 30, 2024 at 01:24:30PM +0200, Igor Mammedov wrote: > On Mon, 22 Jul 2024 08:45:58 +0200 > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > There is one reference to ACPI 4.0 and several references > > to ACPI 6.x versions. > > > > Update them to point to ACPI 6.5 whenever possible. > > when it comes to APCI doc comments, they should point to > the 1st (earliest) revision that provides given feature/value/field/table. Yes. And the motivation is twofold. First, guests are built against old acpi versions. knowing in which version things appeared helps us know which guests support a feature. Second, acpi guys keep churning out new versions. It makes no sense to try and update to latest one, it will soon get out of date again. > > > There's one reference that was kept pointing to ACPI 6.4, > > though, with HEST revision 1. > > > > ACPI 6.5 now defines HEST revision 2, and defined a new > > way to handle source types starting from 12. According > > with ACPI 6.5 revision history: > > > > 2312 Update to the HEST table and adding new error > > source descriptor - Table 18.2. > > > > Yet, the spec doesn't define yet any new source > > descriptors. It just defines a different behavior when > > source type is above 11. > > > > I also double-checked GHES implementation on an open > > source project (Linux Kernel). Currently upstream > > doesn't currently handle HEST revision, ignoring such > > field. > > > > In any case, revision 2 seems to be backward-compatible > > with revison 1 when type <= 11 and just one error is > > contained on a HEST record. > > > > So, while it is probably safe to update it, there's no > > real need. So, let's keep the implementation using > > an ACPI 6.4 compatible table, e. g. HEST revision 1. > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > --- > > hw/acpi/ghes.c | 48 ++++++++++++++++++++++++++++-------------------- > > 1 file changed, 28 insertions(+), 20 deletions(-) > > > > diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c > > index 6075ef5893ce..ebf1b812aaaa 100644 > > --- a/hw/acpi/ghes.c > > +++ b/hw/acpi/ghes.c > > @@ -45,9 +45,9 @@ > > #define GAS_ADDR_OFFSET 4 > > > > /* > > - * The total size of Generic Error Data Entry > > - * ACPI 6.1/6.2: 18.3.2.7.1 Generic Error Data, > > - * Table 18-343 Generic Error Data Entry > > + * The total size of Generic Error Data Entry before data field > > + * ACPI 6.5: 18.3.2.7.1 Generic Error Data, > > + * Table 18.12 Generic Error Data Entry > > */ > > #define ACPI_GHES_DATA_LENGTH 72 > > > > @@ -65,8 +65,8 @@ > > > > /* > > * Total size for Generic Error Status Block except Generic Error Data Entries > > - * ACPI 6.2: 18.3.2.7.1 Generic Error Data, > > - * Table 18-380 Generic Error Status Block > > + * ACPI 6.5: 18.3.2.7.1 Generic Error Data, > > + * Table 18.11 Generic Error Status Block > > */ > > #define ACPI_GHES_GESB_SIZE 20 > > > > @@ -82,7 +82,8 @@ enum AcpiGenericErrorSeverity { > > > > /* > > * Hardware Error Notification > > - * ACPI 4.0: 17.3.2.7 Hardware Error Notification > > + * ACPI 6.5: 18.3.2.9 Hardware Error Notification, > > + * Table 18.14 - Hardware Error Notification Structure > > * Composes dummy Hardware Error Notification descriptor of specified type > > */ > > static void build_ghes_hw_error_notification(GArray *table, const uint8_t type) > > @@ -112,7 +113,8 @@ static void build_ghes_hw_error_notification(GArray *table, const uint8_t type) > > > > /* > > * Generic Error Data Entry > > - * ACPI 6.1: 18.3.2.7.1 Generic Error Data > > + * ACPI 6.5: 18.3.2.7.1 Generic Error Data, > > + * Table 18.12 - Generic Error Data Entry > > */ > > static void acpi_ghes_generic_error_data(GArray *table, > > const uint8_t *section_type, uint32_t error_severity, > > @@ -148,7 +150,8 @@ static void acpi_ghes_generic_error_data(GArray *table, > > > > /* > > * Generic Error Status Block > > - * ACPI 6.1: 18.3.2.7.1 Generic Error Data > > + * ACPI 6.5: 18.3.2.7.1 Generic Error Data, > > + * Table 18.11 - Generic Hardware Error Source Structure > > */ > > static void acpi_ghes_generic_error_status(GArray *table, uint32_t block_status, > > uint32_t raw_data_offset, uint32_t raw_data_length, > > @@ -429,15 +432,18 @@ void build_ghes_error_table(GArray *hardware_errors, BIOSLinker *linker) > > 0, sizeof(uint64_t), ACPI_GHES_ERRORS_FW_CFG_FILE, 0); > > } > > > > -/* Build Generic Hardware Error Source version 2 (GHESv2) */ > > +/* > > + * Build Generic Hardware Error Source version 2 (GHESv2) > > + * ACPI 6.5: 18.3.2.8 Generic Hardware Error Source version 2 (GHESv2 - Type 10), > > + * Table 18.13: Generic Hardware Error Source version 2 (GHESv2) > > + */ > > static void build_ghes_v2(GArray *table_data, int source_id, BIOSLinker *linker) > > { > > uint64_t address_offset; > > - /* > > - * Type: > > - * Generic Hardware Error Source version 2(GHESv2 - Type 10) > > - */ > > + /* Type: (GHESv2 - Type 10) */ > > build_append_int_noprefix(table_data, ACPI_GHES_SOURCE_GENERIC_ERROR_V2, 2); > > + > > + /* ACPI 6.5: Table 18.10 - Generic Hardware Error Source Structure */ > > /* Source Id */ > > build_append_int_noprefix(table_data, source_id, 2); > > /* Related Source Id */ > > @@ -481,11 +487,8 @@ static void build_ghes_v2(GArray *table_data, int source_id, BIOSLinker *linker) > > /* Error Status Block Length */ > > build_append_int_noprefix(table_data, ACPI_GHES_MAX_RAW_DATA_LENGTH, 4); > > > > - /* > > - * Read Ack Register > > - * ACPI 6.1: 18.3.2.8 Generic Hardware Error Source > > - * version 2 (GHESv2 - Type 10) > > - */ > > + /* ACPI 6.5: fields defined at GHESv2 table */ > > + /* Read Ack Register */ > > address_offset = table_data->len; > > build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 0x40, 0, > > 4 /* QWord access */, 0); > > @@ -504,11 +507,16 @@ static void build_ghes_v2(GArray *table_data, int source_id, BIOSLinker *linker) > > build_append_int_noprefix(table_data, 0x1, 8); > > } > > > > -/* Build Hardware Error Source Table */ > > +/* > > + * Build Hardware Error Source Table > > + * ACPI 6.4: 18.3.2 ACPI Error Source > > + * Table 18.2: Hardware Error Source Table (HEST) > > + */ > > void acpi_build_hest(GArray *table_data, BIOSLinker *linker, > > const char *oem_id, const char *oem_table_id) > > { > > - AcpiTable table = { .sig = "HEST", .rev = 1, > > + AcpiTable table = { .sig = "HEST", > > + .rev = 1, /* ACPI 4.0 to 6.4 */ > > .oem_id = oem_id, .oem_table_id = oem_table_id }; > > > > acpi_table_begin(&table, table_data);
Em Tue, 30 Jul 2024 07:36:32 -0400 "Michael S. Tsirkin" <mst@redhat.com> escreveu: > On Tue, Jul 30, 2024 at 01:24:30PM +0200, Igor Mammedov wrote: > > On Mon, 22 Jul 2024 08:45:58 +0200 > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > > > There is one reference to ACPI 4.0 and several references > > > to ACPI 6.x versions. > > > > > > Update them to point to ACPI 6.5 whenever possible. > > > > when it comes to APCI doc comments, they should point to > > the 1st (earliest) revision that provides given feature/value/field/table. > > Yes. And the motivation is twofold. > First, guests are built against > old acpi versions. knowing in which version things appeared > helps us know which guests support a feature. Good point, but IMO, a comment like "since: ACPI 4.0" would be better, as the comment may not reflect the first version supporting such features, but, instead, when someone added support to a particular feature set. > Second, acpi guys keep churning out new versions. > It makes no sense to try and update to latest one, > it will soon get out of date again. True, but having it updated helps people adding new code to get things right. Anyway, I got your point, I'll drop this patch. > > > void acpi_build_hest(GArray *table_data, BIOSLinker *linker, > > > const char *oem_id, const char *oem_table_id) > > > { > > > - AcpiTable table = { .sig = "HEST", .rev = 1, > > > + AcpiTable table = { .sig = "HEST", > > > + .rev = 1, /* ACPI 4.0 to 6.4 */ > > > .oem_id = oem_id, .oem_table_id = oem_table_id }; > > > > > > acpi_table_begin(&table, table_data); This hunk might still make sense, though. When double-checking the links against ACPI 6.5, I noticed that HEST now requires .rev = 2. There are some future incompatibilities, but the current implementation of acpi/ghes satisfies both rev 1 and ref 2 of HEST. Also, this is not relevant on Linux, as the revision is not checked there. So, currently this is not a problem. Thanks, Mauro
diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c index 6075ef5893ce..ebf1b812aaaa 100644 --- a/hw/acpi/ghes.c +++ b/hw/acpi/ghes.c @@ -45,9 +45,9 @@ #define GAS_ADDR_OFFSET 4 /* - * The total size of Generic Error Data Entry - * ACPI 6.1/6.2: 18.3.2.7.1 Generic Error Data, - * Table 18-343 Generic Error Data Entry + * The total size of Generic Error Data Entry before data field + * ACPI 6.5: 18.3.2.7.1 Generic Error Data, + * Table 18.12 Generic Error Data Entry */ #define ACPI_GHES_DATA_LENGTH 72 @@ -65,8 +65,8 @@ /* * Total size for Generic Error Status Block except Generic Error Data Entries - * ACPI 6.2: 18.3.2.7.1 Generic Error Data, - * Table 18-380 Generic Error Status Block + * ACPI 6.5: 18.3.2.7.1 Generic Error Data, + * Table 18.11 Generic Error Status Block */ #define ACPI_GHES_GESB_SIZE 20 @@ -82,7 +82,8 @@ enum AcpiGenericErrorSeverity { /* * Hardware Error Notification - * ACPI 4.0: 17.3.2.7 Hardware Error Notification + * ACPI 6.5: 18.3.2.9 Hardware Error Notification, + * Table 18.14 - Hardware Error Notification Structure * Composes dummy Hardware Error Notification descriptor of specified type */ static void build_ghes_hw_error_notification(GArray *table, const uint8_t type) @@ -112,7 +113,8 @@ static void build_ghes_hw_error_notification(GArray *table, const uint8_t type) /* * Generic Error Data Entry - * ACPI 6.1: 18.3.2.7.1 Generic Error Data + * ACPI 6.5: 18.3.2.7.1 Generic Error Data, + * Table 18.12 - Generic Error Data Entry */ static void acpi_ghes_generic_error_data(GArray *table, const uint8_t *section_type, uint32_t error_severity, @@ -148,7 +150,8 @@ static void acpi_ghes_generic_error_data(GArray *table, /* * Generic Error Status Block - * ACPI 6.1: 18.3.2.7.1 Generic Error Data + * ACPI 6.5: 18.3.2.7.1 Generic Error Data, + * Table 18.11 - Generic Hardware Error Source Structure */ static void acpi_ghes_generic_error_status(GArray *table, uint32_t block_status, uint32_t raw_data_offset, uint32_t raw_data_length, @@ -429,15 +432,18 @@ void build_ghes_error_table(GArray *hardware_errors, BIOSLinker *linker) 0, sizeof(uint64_t), ACPI_GHES_ERRORS_FW_CFG_FILE, 0); } -/* Build Generic Hardware Error Source version 2 (GHESv2) */ +/* + * Build Generic Hardware Error Source version 2 (GHESv2) + * ACPI 6.5: 18.3.2.8 Generic Hardware Error Source version 2 (GHESv2 - Type 10), + * Table 18.13: Generic Hardware Error Source version 2 (GHESv2) + */ static void build_ghes_v2(GArray *table_data, int source_id, BIOSLinker *linker) { uint64_t address_offset; - /* - * Type: - * Generic Hardware Error Source version 2(GHESv2 - Type 10) - */ + /* Type: (GHESv2 - Type 10) */ build_append_int_noprefix(table_data, ACPI_GHES_SOURCE_GENERIC_ERROR_V2, 2); + + /* ACPI 6.5: Table 18.10 - Generic Hardware Error Source Structure */ /* Source Id */ build_append_int_noprefix(table_data, source_id, 2); /* Related Source Id */ @@ -481,11 +487,8 @@ static void build_ghes_v2(GArray *table_data, int source_id, BIOSLinker *linker) /* Error Status Block Length */ build_append_int_noprefix(table_data, ACPI_GHES_MAX_RAW_DATA_LENGTH, 4); - /* - * Read Ack Register - * ACPI 6.1: 18.3.2.8 Generic Hardware Error Source - * version 2 (GHESv2 - Type 10) - */ + /* ACPI 6.5: fields defined at GHESv2 table */ + /* Read Ack Register */ address_offset = table_data->len; build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 0x40, 0, 4 /* QWord access */, 0); @@ -504,11 +507,16 @@ static void build_ghes_v2(GArray *table_data, int source_id, BIOSLinker *linker) build_append_int_noprefix(table_data, 0x1, 8); } -/* Build Hardware Error Source Table */ +/* + * Build Hardware Error Source Table + * ACPI 6.4: 18.3.2 ACPI Error Source + * Table 18.2: Hardware Error Source Table (HEST) + */ void acpi_build_hest(GArray *table_data, BIOSLinker *linker, const char *oem_id, const char *oem_table_id) { - AcpiTable table = { .sig = "HEST", .rev = 1, + AcpiTable table = { .sig = "HEST", + .rev = 1, /* ACPI 4.0 to 6.4 */ .oem_id = oem_id, .oem_table_id = oem_table_id }; acpi_table_begin(&table, table_data);
There is one reference to ACPI 4.0 and several references to ACPI 6.x versions. Update them to point to ACPI 6.5 whenever possible. There's one reference that was kept pointing to ACPI 6.4, though, with HEST revision 1. ACPI 6.5 now defines HEST revision 2, and defined a new way to handle source types starting from 12. According with ACPI 6.5 revision history: 2312 Update to the HEST table and adding new error source descriptor - Table 18.2. Yet, the spec doesn't define yet any new source descriptors. It just defines a different behavior when source type is above 11. I also double-checked GHES implementation on an open source project (Linux Kernel). Currently upstream doesn't currently handle HEST revision, ignoring such field. In any case, revision 2 seems to be backward-compatible with revison 1 when type <= 11 and just one error is contained on a HEST record. So, while it is probably safe to update it, there's no real need. So, let's keep the implementation using an ACPI 6.4 compatible table, e. g. HEST revision 1. Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> --- hw/acpi/ghes.c | 48 ++++++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 20 deletions(-)