Re: [PATCH v5 13/16] acpi/ghes: better name the offset of the hardware error firmware

[Mauro Carvalho Chehab]

Em Wed, 4 Dec 2024 17:37:59 +0100
Igor Mammedov <imammedo@xxxxxxxxxx> escreveu:

> On Wed,  4 Dec 2024 08:41:21 +0100
> Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote:
> 
> > The hardware error firmware is where HEST error structures are  
>       ^^^^^^^^^^^^^^^^^^^^^^^ I can't parse this, suspect you've meant something else here
> 
> > stored. Those can be GHESv2, but they can also be other types.
> > 
> > Better name the location of the hardware error.
> > 
> > No functional changes.

I meant this fw_cfg file:
#define ACPI_HW_ERROR_FW_CFG_FILE           "etc/hardware_errors"
#define ACPI_HW_ERROR_ADDR_FW_CFG_FILE      "etc/hardware_errors_addr"

What about changing description to:

	The etc/hardware_errors fw_cfg file is where the HEST error
	source structures are stored. Those can be GHESv2, but they can also
	be other types.

	For more details about error source structure, see:
	https://uefi.org/specs/ACPI/6.5/18_Platform_Error_Interfaces.html#acpi-error-source

	Better name the address variable from ghes_error_le to hw_error_le
	to better reflect that.

	No functional changes.

> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Reviewed-by: Igor Mammedov <imammedo@xxxxxxxxxx>
> > ---
> >  hw/acpi/generic_event_device.c | 4 ++--
> >  hw/acpi/ghes.c                 | 4 ++--
> >  include/hw/acpi/ghes.h         | 2 +-
> >  3 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> > index 663d9cb09380..17baf36132a8 100644
> > --- a/hw/acpi/generic_event_device.c
> > +++ b/hw/acpi/generic_event_device.c
> > @@ -364,7 +364,7 @@ static const VMStateDescription vmstate_ghes = {
> >      .version_id = 1,
> >      .minimum_version_id = 1,
> >      .fields = (const VMStateField[]) {
> > -        VMSTATE_UINT64(ghes_addr_le, AcpiGhesState),
> > +        VMSTATE_UINT64(hw_error_le, AcpiGhesState),
> >          VMSTATE_END_OF_LIST()
> >      },
> >  };
> > @@ -372,7 +372,7 @@ static const VMStateDescription vmstate_ghes = {
> >  static bool ghes_needed(void *opaque)
> >  {
> >      AcpiGedState *s = opaque;
> > -    return s->ghes_state.ghes_addr_le;
> > +    return s->ghes_state.hw_error_le;
> >  }
> >  
> >  static const VMStateDescription vmstate_ghes_state = {
> > diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> > index 52c2b69d3664..90d76b9c2d8c 100644
> > --- a/hw/acpi/ghes.c
> > +++ b/hw/acpi/ghes.c
> > @@ -359,7 +359,7 @@ void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s,
> >  
> >      /* Create a read-write fw_cfg file for Address */
> >      fw_cfg_add_file_callback(s, ACPI_HW_ERROR_ADDR_FW_CFG_FILE, NULL, NULL,
> > -        NULL, &(ags->ghes_addr_le), sizeof(ags->ghes_addr_le), false);
> > +        NULL, &(ags->hw_error_le), sizeof(ags->hw_error_le), false);
> >  
> >      ags->present = true;
> >  }
> > @@ -385,7 +385,7 @@ void ghes_record_cper_errors(const void *cper, size_t len,
> >      }
> >      ags = &acpi_ged_state->ghes_state;
> >  
> > -    start_addr = le64_to_cpu(ags->ghes_addr_le);
> > +    start_addr = le64_to_cpu(ags->hw_error_le);
> >  
> >      start_addr += source_id * sizeof(uint64_t);
> >  
> > diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h
> > index 21666a4bcc8b..39619a2457cb 100644
> > --- a/include/hw/acpi/ghes.h
> > +++ b/include/hw/acpi/ghes.h
> > @@ -65,7 +65,7 @@ enum {
> >  };
> >  
> >  typedef struct AcpiGhesState {
> > -    uint64_t ghes_addr_le;
> > +    uint64_t hw_error_le;
> >      bool present; /* True if GHES is present at all on this board */
> >  } AcpiGhesState;
> >    
> 



Thanks,
Mauro