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

[Markus Armbruster]

Igor Mammedov <imammedo@xxxxxxxxxx> writes:

> On Thu, 8 Aug 2024 16:11:41 +0200
> Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote:
>
>> 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.
>
> if we going to keep it generic, acpi-hest would do

Works for me.

>> > >  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).
>
> 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 :)

Thanks!

[...]