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

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

Em Thu, 08 Aug 2024 16:45:51 +0200
Markus Armbruster <armbru@redhat.com> escreveu:

> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > On Thu, 8 Aug 2024 16:11:41 +0200
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> >  
> >> Em Thu, 08 Aug 2024 10:50:33 +0200
> >> Markus Armbruster <armbru@redhat.com> escreveu:
> >>   
> >> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> 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@kernel.org>
> >> > > +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.  
> >
> > if we going to keep it generic, acpi-hest would do  
> 
> Works for me.

Ok, I'll do the rename. With regards to the files implementing
support for it: 

	hw/acpi/ghes_cper.c
	hw/acpi/ghes_cper_stub.c

I guess there's no need to rename them, right? IMO such names
are better than acpi/hest.c, specially since the actual implementation
for HEST is inside acpi/ghes.c.

> 
> >> > >  ppc4xx
> >> > >  L: qemu-ppc@nongnu.org
> >> > >  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).  
> >
> > some years earlier it could be said 'stable link' about acpi spec hosted
> > elsewhere. Not the case anymore after umbrella change.
> >
> > spec name, rev, chapter worked fine for acpi code (it's easy to find wherever spec is hosted).
> > Probably the same would work for QAPI, I'm not QAPI maintainer though,
> > so preffered approach here is absolutely up to you.  
> 
> A link is strictly optional.  Stable links are nice, stale links are
> annoying.  Mauro, you decide :)

Well, I guess I'll add a link then, keeping it in text mode as well.

Changing umbrella is something that doesn't happen too often. Hopefully 
those will stay for a long time, if not forever, under uefi.org. 

If not, we can always drop the link.

Thanks,
Mauro