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

[Michael S. Tsirkin]

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