Re: [PATCH v5 5/7] qapi/ghes-cper: add an interface to do generic CPER error injection

[Mauro Carvalho Chehab]

Em Thu, 08 Aug 2024 10:50:33 +0200
Markus Armbruster <armbru@xxxxxxxxxx> escreveu:

> Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> writes:

> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 98eddf7ae155..655edcb6688c 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2075,6 +2075,13 @@ F: hw/acpi/ghes.c
> >  F: include/hw/acpi/ghes.h
> >  F: docs/specs/acpi_hest_ghes.rst
> >  
> > +ACPI/HEST/GHES/ARM processor CPER
> > +R: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> > +S: Maintained
> > +F: hw/arm/ghes_cper.c
> > +F: hw/acpi/ghes_cper_stub.c
> > +F: qapi/ghes-cper.json
> > +  
> 
> Here's the reason for creating a new QAPI module instead of adding to
> existing module acpi.json: different maintainers.
> 
> Hypothetical question: if we didn't care for that, would this go into
> qapi/acpi.json?

Independently of maintainers, GHES is part of ACPI APEI HEST, meaning
to report hardware errors. Such hardware errors are typically handled by 
the host OS, so quest doesn't need to be aware of that[1].

So, IMO the best would be to keep APEI/HEST/GHES in a separate file.

[1] still, I can foresee some scenarios were passing some errors to the
    guest could make sense.

> 
> If yes, then should we call it acpi-ghes-cper.json or acpi-ghes.json
> instead?

Naming it as acpi-ghes,acpi-hest or acpi-ghes-cper would equally work
from my side.

> 
> >  ppc4xx
> >  L: qemu-ppc@xxxxxxxxxx
> >  S: Orphan  
> 
> [...]
> 
> > diff --git a/qapi/ghes-cper.json b/qapi/ghes-cper.json
> > new file mode 100644
> > index 000000000000..3cc4f9f2aaa9
> > --- /dev/null
> > +++ b/qapi/ghes-cper.json
> > @@ -0,0 +1,55 @@
> > +# -*- Mode: Python -*-
> > +# vim: filetype=python
> > +
> > +##
> > +# = GHESv2 CPER Error Injection
> > +#
> > +# These are defined at
> > +# ACPI 6.2: 18.3.2.8 Generic Hardware Error Source version 2
> > +# (GHESv2 - Type 10)
> > +##  
> 
> Feels a bit terse.  These what?
> 
> The reference could be clearer: "defined in the ACPI Specification 6.2,
> section 18.3.2.8 Generic Hardware Error Source version 2".  A link would
> be nice, if it's stable.

I can add a link, but only newer ACPI versions are hosted in html format
(e. g. only versions 6.4 and 6.5 are available as html at uefi.org).

Can I place something like:

	Defined since ACPI Specification 6.2,
	section 18.3.2.8 Generic Hardware Error Source version 2. See:

	https://uefi.org/specs/ACPI/6.5/18_Platform_Error_Interfaces.html#generic-hardware-error-source-version-2-ghesv2-type-10

e. g. having the link pointing to ACPI 6.4 or 6.5, instead of 6.2?

>   # @raw-data: payload of the CPER encoded in base64
> 
> Have you considered naming this @payload instead?

Works for me.

Thanks,
Mauro