Re: [PATCH v3 6/7] acpi/ghes: update comments to point to newer ACPI specs

[Igor Mammedov <imammedo@redhat.com>]

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);