Re: [PATCH v3 00/14] Change ghes to use HEST-based offsets and add support for error inject

[Igor Mammedov]

On Mon, 3 Feb 2025 11:09:34 +0000
Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote:

> On Fri, 31 Jan 2025 18:42:41 +0100
> Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote:
> 
> > Now that the ghes preparation patches were merged, let's add support
> > for error injection.
> > 
> > On this series, the first 6 patches chang to the math used to calculate offsets at HEST
> > table and hardware_error firmware file, together with its migration code. Migration tested
> > with both latest QEMU released kernel and upstream, on both directions.
> > 
> > The next patches add a new QAPI to allow injecting GHESv2 errors, and a script using such QAPI
> >    to inject ARM Processor Error records.
> > 
> > If I'm counting well, this is the 19th submission of my error inject patches.  
> 
> Looks good to me. All remaining trivial things are in the category
> of things to consider only if you are doing another spin.  The code
> ends up how I'd like it at the end of the series anyway, just
> a question of the precise path to that state!

if you look at series as a whole it's more or less fine (I guess you
and me got used to it)

however if you take it patch by patch (as if you've never seen it)
ordering is messed up (the same would apply to everyone after a while
when it's forgotten)

So I'd strongly suggest to restructure the series (especially 2-6/14).
re sum up my comments wrt ordering:

0  add testcase for HEST table with current HEST as expected blob
   (currently missing), so that we can be sure that we haven't messed
   existing tables during refactoring.
1. Introduce use_hest_addr (disabled) for now so we could place all
   legacy code to !use_hest_addr branch
2. then patches that do the part of switching to HEST addr lookup,
    * ged lookup (preferably at the place it should end up eventually)
    * legacy bios_linker/fwcfg fencing patches
    * on top of that new hest bios_linker/fwcfg ones
    * and then the rest 
    (everything that belongs to the 2nd error source should _not_ be a part of that)
3. add 2nd error source incl. necessary tests procedures introduce
   and update DSDT/HEST



> 
> Jonathan
>