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

[Mauro Carvalho Chehab]

Em Fri, 21 Feb 2025 10:21:27 +0000
Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> escreveu:

> On Fri, 21 Feb 2025 07:38:23 +0100
> Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote:
> 
> > Em Mon, 3 Feb 2025 16:22:36 +0100
> > Igor Mammedov <imammedo@xxxxxxxxxx> escreveu:
> >   
> > > 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.    
> 
> To potentially save time I think Igor is asking that before you do anything
> at all you plug the existing test hole which is that we don't test HEST
> at all.   Even after this series I think we don't test HEST. 

On a previous review (v2, I guess), Igor requested me to do the DSDT
test just before and after the patch which is actually changing its
content (patch 11). The HEST table is inside DSDT firmware, and it is
already tested.

> You add
> a stub hest and exclusion but then in patch 12 the HEST stub is deleted whereas
> it should be replaced with the example data for the test.

This was actually a misinterpretation from my side: patch 10 adds the
etc/hardware_errors table (mistakenly naming it as HEST), but this
was never tested. For the next submission, I'll drop etc/hardware_errors
table from patches 10 and 12.

> That indeed doesn't address testing the error data storage which would be
> a different problem.
> > 
> > Not sure if I got this one. The HEST table is part of etc/acpi/tables,
> > which is already tested, as you pointed at the previous reviews. Doing
> > changes there is already detected. That's basically why we added patches
> > 10 and 12:
> > 
> > 	[PATCH v3 10/14] tests/acpi: virt: allow acpi table changes for a new table: HEST
> > 	[PATCH v3 12/14] tests/acpi: virt: add a HEST table to aarch64 virt and update DSDT
> > 
> > What tests don't have is a check for etc/hardware_errors firmware inside 
> > tests/data/acpi/aarch64/virt/, but, IMO, we shouldn't add it there.
> > 
> > See, hardware_errors table contains only some skeleton space to
> > store:
> > 
> > 	- 1 or more error block address offsets;
> > 	- 1 or more read ack register;
> > 	- 1 or more HEST source entries containing CPER blocks.
> > 
> > There's nothing there to be actually checked: it is just some
> > empty spaces with a variable number of fields.
> > 
> > With the new code, the actual number of CPER blocks and their
> > corresponding offsets and read ack registers can be different on 
> > different architectures. So, for instance, when we add x86 support,
> > we'll likely start with just one error source entry, while arm will
> > have two after this changeset.
> > 
> > Also, one possibility to address the issues reported by Gavin Shan at
> > https://lore.kernel.org/qemu-devel/20250214041635.608012-1-gshan@xxxxxxxxxx/
> > would be to have one entry per each CPU. So, the size of such firmware
> > could be dependent on the number of CPUs.
> > 
> > So, adding any validation to it would just cause pain and probably
> > won't detect any problems.  
> 
> If we did do this the test would use a fixed number of CPUs so
> would just verify we didn't break a small number of variants. Useful
> but to me a follow up to this series not something that needs to
> be part of it - particularly as Gavin's work may well change that!

I don't think that testing etc/hardware_errors would detect any
regressions. It will just create a test scenario that will require
constant changes, as adding any entry to HEST would hit it. 

Besides that, I don't think adding support for it would be a simple
matter of adding another table. See, after this series, there are two 
different scenarios for the /etc/hardware_errors:

- one with a single GHESv2 entry, for virt-9.2;
- another one with two GHESv2 entries for virt-10.0 and above that
  will dynamically change its size (starting from 2) depending on
  the features we add, and if we'll have one entry per CPU or not.

Right now, the tests there are only for "virt-latest": there's no
test directory for "virt-9.2". Adding support for virt-legacy will 
very likely require lots of changes there at the test infrastructure,
as it will require some virt migration support. 

> > What could be done instead is to have a different type of tests that
> > would use the error injection script to check if regressions are 
> > introduced after QEMU 10.0. Such new kind of test would require
> > this series to be merged first. It would also require the usage of
> > an OSPM image with some testing tools on it. This is easier said 
> > than done, as besides the complexity of having an OSPM test image,
> > such kind of tests would require extra logic, specially if it would
> > check regressions for SEA and other notification sources.
> >   
> Agreed that a more end to end test is even better, but those are
> quite a bit more complex so definitely a follow up.

Yes, but it could be simpler than modifying ACPI tests to handle
migration.

The way I see is that such kind of integration could be done by some
gitlab workflow that would run an error injection script inside a
pre-defined image emulating both virt-9.2 and virt-latest and checking
if the HEST tables were properly generated for both SEA and GED
sources.

This is probably easier for GED, as the QMP interface already
detects that the read ack register was changed by the OSPM. For
SEA, it may require either some additional instrumentation or to
capture OSPM logs.

Anyway, ether way, a change like that is IMO outside the escope of
this series, as it will require lots of unrelated changes.

Regards,
Mauro