Re: [PATCH 02/11] acpi/ghes: add a firmware file with HEST address

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

Em Thu, 23 Jan 2025 10:02:17 +0000
Jonathan Cameron <Jonathan.Cameron@huawei.com> escreveu:

> On Wed, 22 Jan 2025 16:46:19 +0100
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > Store HEST table address at GPA, placing its content at
> > hest_addr_le variable.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >   
> A few trivial things inline.
> 
> Jonathan
> 
> > ---
> > 
> > Change from v8:
> > - hest_addr_lr is now pointing to the error source size and data.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> Bonus.  I guess you really like this patch :)
> > ---
> >  hw/acpi/ghes.c         | 17 ++++++++++++++++-
> >  include/hw/acpi/ghes.h |  1 +
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> > index 3f519ccab90d..34e3364d3fd8 100644
> > --- a/hw/acpi/ghes.c
> > +++ b/hw/acpi/ghes.c
> > @@ -30,6 +30,7 @@
> >  
> >  #define ACPI_HW_ERROR_FW_CFG_FILE           "etc/hardware_errors"
> >  #define ACPI_HW_ERROR_ADDR_FW_CFG_FILE      "etc/hardware_errors_addr"
> > +#define ACPI_HEST_ADDR_FW_CFG_FILE          "etc/acpi_table_hest_addr"
> >  
> >  /* The max size in bytes for one error block */
> >  #define ACPI_GHES_MAX_RAW_DATA_LENGTH   (1 * KiB)
> > @@ -261,7 +262,7 @@ static void build_ghes_error_table(GArray *hardware_errors, BIOSLinker *linker,
> >      }
> >  
> >      /*
> > -     * tell firmware to write hardware_errors GPA into
> > +     * Tell firmware to write hardware_errors GPA into  
> 
> Sneaky tidy up.  No problem with it in general but adding noise here, so if there
> are others in the series maybe gather them up in a cleanup patch.

There are no other cleanups pending. Besides, as you noticed, this
aligns with the comment below. So, I'm opting to add a note at the
patch's description.

> 
> >       * hardware_errors_addr fw_cfg, once the former has been initialized.
> >       */
> >      bios_linker_loader_write_pointer(linker, ACPI_HW_ERROR_ADDR_FW_CFG_FILE, 0,
> > @@ -355,6 +356,8 @@ void acpi_build_hest(GArray *table_data, GArray *hardware_errors,
> >  
> >      acpi_table_begin(&table, table_data);
> >  
> > +    int hest_offset = table_data->len;  
> 
> Local style looks to be traditional C with definitions at top.  Maybe define
> hest_offset up a few lines and just set it here?

Ok. I'll follow Igor's suggestion of using uint32_t.

> > +
> >      /* Error Source Count */
> >      build_append_int_noprefix(table_data, num_sources, 4);
> >      for (i = 0; i < num_sources; i++) {
> > @@ -362,6 +365,15 @@ void acpi_build_hest(GArray *table_data, GArray *hardware_errors,
> >      }
> >  
> >      acpi_table_end(linker, &table);
> > +
> > +    /*
> > +     * tell firmware to write into GPA the address of HEST via fw_cfg,  
> 
> Given the tidy up above, fix this one to have a capital T, or was this
> where you meant to change it?

OK.

> > +     * once initialized.
> > +     */
> > +    bios_linker_loader_write_pointer(linker,
> > +                                     ACPI_HEST_ADDR_FW_CFG_FILE, 0,  
> 
> Could wrap less and stay under 80 chars as both lines above add up to 70 something

Why? This follows QEMU coding style and lines aren't longer than 80
columns. Besides, at least for my eyes and some experience doing maintainership
on other projects over the years, it is a lot quicker to identify function
parameters if they're properly aligned with the parenthesis.

Thanks,
Mauro