Re: [PATCH v3 03/14] acpi/ghes: Use HEST table offsets when preparing GHES records

[Igor Mammedov]

On Fri, 21 Feb 2025 07:02:21 +0100
Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote:

> Em Mon, 3 Feb 2025 15:34:23 +0100
> Igor Mammedov <imammedo@xxxxxxxxxx> escreveu:
> 
> > On Fri, 31 Jan 2025 18:42:44 +0100
> > Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote:
> >   
> > > There are two pointers that are needed during error injection:
> > > 
> > > 1. The start address of the CPER block to be stored;
> > > 2. The address of the ack.
> > > 
> > > It is preferable to calculate them from the HEST table.  This allows
> > > checking the source ID, the size of the table and the type of the
> > > HEST error block structures.
> > > 
> > > Yet, keep the old code, as this is needed for migration purposes.
> > > 
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> > > ---
> > >  hw/acpi/ghes.c         | 132 ++++++++++++++++++++++++++++++++++++-----
> > >  include/hw/acpi/ghes.h |   1 +
> > >  2 files changed, 119 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> > > index 27478f2d5674..8f284fd191a6 100644
> > > --- a/hw/acpi/ghes.c
> > > +++ b/hw/acpi/ghes.c
> > > @@ -41,6 +41,12 @@
> > >  /* Address offset in Generic Address Structure(GAS) */
> > >  #define GAS_ADDR_OFFSET 4
> > >  
> > > +/*
> > > + * ACPI spec 1.0b
> > > + * 5.2.3 System Description Table Header
> > > + */
> > > +#define ACPI_DESC_HEADER_OFFSET     36
> > > +
> > >  /*
> > >   * The total size of Generic Error Data Entry
> > >   * ACPI 6.1/6.2: 18.3.2.7.1 Generic Error Data,
> > > @@ -61,6 +67,25 @@
> > >   */
> > >  #define ACPI_GHES_GESB_SIZE                 20
> > >  
> > > +/*
> > > + * Offsets with regards to the start of the HEST table stored at
> > > + * ags->hest_addr_le,    
> > 
> > If I read this literary, then offsets above are not what
> > declared later in this patch.
> > I'd really drop this comment altogether as it's confusing,
> > and rather get variables/macro naming right
> >   
> > > according with the memory layout map at
> > > + * docs/specs/acpi_hest_ghes.rst.
> > > + */    
> > 
> > what we need is update to above doc, describing new and old ways.
> > a separate patch.  
> 
> I can't see anything that should be changed at
> docs/specs/acpi_hest_ghes.rst, as this series doesn't change the
> firmware layout: we're still using two firmware tables:
> 
> - etc/acpi/tables, with HEST on it;
> - etc/hardware_errors, with:
> 	- error block addresses;
> 	- read_ack registers;
> 	- CPER records.
> 
> The only changes that this series introduce are related to how
> the error generation logic navigates between HEST and hw_errors
> firmware. This is not described at acpi_hest_ghes.rst, and both
> ways follow ACPI specs to the letter.
> 
> The only difference is that the code which populates the CPER
> record and the error/read offsets doesn't require to know how
> the HEST table generation placed offsets, as it will basically
> reproduce what OSPM firmware does when handling	HEST events.

section 8 describes old way to get to address to record old CPER,
so it needs to amended to also describe a new approach and say
which way is used for which version.

possibly section 11 might need some messaging as well.


> 
> Thanks,
> Mauro
>