Re: [PATCH v7 04/10] qapi/acpi-hest: add an interface to do generic CPER error injection

[Mauro Carvalho Chehab]

Em Wed, 14 Aug 2024 14:53:22 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> escreveu:

> On Wed, 14 Aug 2024 01:23:26 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > Creates a QMP command to be used for generic ACPI APEI hardware error
> > injection (HEST) via GHESv2.
> > 
> > The actual GHES code will be added at the followup patch.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> A few trivial things from a quick glance at this
> (to remind myself of how this fits together).
> 
> > diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
> > index e07d3204eb36..73ffbb82c150 100644
> > --- a/hw/acpi/Kconfig
> > +++ b/hw/acpi/Kconfig
> > @@ -51,6 +51,11 @@ config ACPI_APEI
> >      bool
> >      depends on ACPI
> >  
> > +config GHES_CPER
> > +    bool
> > +    depends on ACPI_APEI
> > +    default y
> > +
> >  config ACPI_PCI
> >      bool
> >      depends on ACPI && PCI
> > diff --git a/hw/acpi/ghes_cper.c b/hw/acpi/ghes_cper.c
> > new file mode 100644
> > index 000000000000..92ca84d738de
> > --- /dev/null
> > +++ b/hw/acpi/ghes_cper.c
> > @@ -0,0 +1,33 @@  
> 
> > +#include "qapi/qapi-commands-acpi-hest.h"
> > +#include "hw/acpi/ghes.h"
> > +
> > +void qmp_ghes_cper(const char *qmp_cper,
> > +                   Error **errp)  

Heh, with all code changes, this is not a lot simpler than before ;-)

I'll address it on a next spin.

> That's a very short line wrap.
> 
> > +{
> > +
> > +    uint8_t *cper;
> > +    size_t  len;
> > +
> > +    cper = qbase64_decode(qmp_cper, -1, &len, errp);
> > +    if (!cper) {
> > +        error_setg(errp, "missing GHES CPER payload");
> > +        return;
> > +    }
> > +
> > +    /* TODO: call a function at ghes */
> > +}  
> 
> > diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h
> > index 419a97d5cbd9..99d12d69c864 100644
> > --- a/include/hw/acpi/ghes.h
> > +++ b/include/hw/acpi/ghes.h
> > @@ -23,6 +23,7 @@
> >  #define ACPI_GHES_H
> >  
> >  #include "hw/acpi/bios-linker-loader.h"
> > +#include "qapi/error.h"  
> Odd to have an include added with no other changes in file?
> Wrong patch maybe?  Or should it be included by a c file instead?

Removing it would cause a compilation breakage. It might be moved
to a c file, but patch 5/10 requires it at ghes.h, as it adds
this to ghes.h:

        void ghes_record_cper_errors(uint8_t *cper, size_t len,
                                     enum AcpiGhesNotifyType notify,Error 
**errp);

So, instead of poking around moving this to/from .c/.h, I opted to
place it on its final place.

> >  #include "qemu/notify.h"
> >  
> >  extern NotifierList acpi_generic_error_notifiers;
> > diff --git a/qapi/acpi-hest.json b/qapi/acpi-hest.json  
> 
> 



Thanks,
Mauro