Re: [PATCH 08/11] acpi/generic_event_device: add an APEI error device

[Mauro Carvalho Chehab]

Em Fri, 24 Jan 2025 13:30:54 +0100
Igor Mammedov <imammedo@redhat.com> escreveu:

> On Wed, 22 Jan 2025 16:46:25 +0100
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > Adds a generic error device to handle generic hardware error
> > events as specified at ACPI 6.5 specification at 18.3.2.7.2:
> > https://uefi.org/specs/ACPI/6.5/18_Platform_Error_Interfaces.html#event-notification-for-generic-error-sources
> > using HID PNP0C33.
> > 
> > The PNP0C33 device is used to report hardware errors to
> > the guest via ACPI APEI Generic Hardware Error Source (GHES).
> > 
> > Co-authored-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > Co-authored-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/acpi/aml-build.c                    | 10 ++++++++++
> >  hw/acpi/generic_event_device.c         |  8 ++++++++
> >  include/hw/acpi/acpi_dev_interface.h   |  1 +
> >  include/hw/acpi/aml-build.h            |  2 ++
> >  include/hw/acpi/generic_event_device.h |  1 +
> >  5 files changed, 22 insertions(+)
> > 
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index f8f93a9f66c8..e4bd7b611372 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -2614,3 +2614,13 @@ Aml *aml_i2c_serial_bus_device(uint16_t address, 
> > const char *resource_source)
> >  
> >      return var;
> >  }
> > +
> > +/* ACPI 5.0b: 18.3.2.6.2 Event Notification For Generic Error Sources */
> > +Aml *aml_error_device(void)
> > +{
> > +    Aml *dev = aml_device(ACPI_APEI_ERROR_DEVICE);
> > +    aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C33")));
> > +    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> > +
> > +    return dev;
> > +}
> > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> > index fe537ed05c66..ce00c80054f4 100644
> > --- a/hw/acpi/generic_event_device.c
> > +++ b/hw/acpi/generic_event_device.c
> > @@ -26,6 +26,7 @@ static const uint32_t ged_supported_events[] = {
> >      ACPI_GED_PWR_DOWN_EVT,
> >      ACPI_GED_NVDIMM_HOTPLUG_EVT,
> >      ACPI_GED_CPU_HOTPLUG_EVT,
> > +    ACPI_GED_ERROR_EVT,
> >  };
> >  
> >  /*
> > @@ -116,6 +117,11 @@ void build_ged_aml(Aml *table, const char *name, 
> > HotplugHandler *hotplug_dev,
> >                             aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
> >                                        aml_int(0x80)));
> >                  break;
> > +            case ACPI_GED_ERROR_EVT:
> > +                aml_append(if_ctx,
> > +                           aml_notify(aml_name(ACPI_APEI_ERROR_DEVICE),
> > +                                      aml_int(0x80)));  
>                                                  ^^^^^
> nit: perhaps add a comment with intent and ref to spec wrt above  value

Will add this as with a define:

        /*
         * ACPI 5.0b: 5.6.6 Device Object Notifications
         * Table 5-135 Error Device Notification Values
         */
        #define ERROR_DEVICE_NOTIFICATION   0x80

(the spec here is the same as we used on this patch for aml_error_device()
function)

> 
> > +                break;
> >              case ACPI_GED_NVDIMM_HOTPLUG_EVT:
> >                  aml_append(if_ctx,
> >                             aml_notify(aml_name("\\_SB.NVDR"),
> > @@ -295,6 +301,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, 
> > AcpiEventStatusBits ev)
> >          sel = ACPI_GED_MEM_HOTPLUG_EVT;
> >      } else if (ev & ACPI_POWER_DOWN_STATUS) {
> >          sel = ACPI_GED_PWR_DOWN_EVT;
> > +    } else if (ev & ACPI_GENERIC_ERROR) {
> > +        sel = ACPI_GED_ERROR_EVT;
> >      } else if (ev & ACPI_NVDIMM_HOTPLUG_STATUS) {
> >          sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
> >      } else if (ev & ACPI_CPU_HOTPLUG_STATUS) {
> > diff --git a/include/hw/acpi/acpi_dev_interface.h 
> > b/include/hw/acpi/acpi_dev_interface.h
> > index 68d9d15f50aa..8294f8f0ccca 100644
> > --- a/include/hw/acpi/acpi_dev_interface.h
> > +++ b/include/hw/acpi/acpi_dev_interface.h
> > @@ -13,6 +13,7 @@ typedef enum {
> >      ACPI_NVDIMM_HOTPLUG_STATUS = 16,
> >      ACPI_VMGENID_CHANGE_STATUS = 32,
> >      ACPI_POWER_DOWN_STATUS = 64,
> > +    ACPI_GENERIC_ERROR = 128,
> >  } AcpiEventStatusBits;
> >  
> >  #define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
> > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > index c18f68134246..f38e12971932 100644
> > --- a/include/hw/acpi/aml-build.h
> > +++ b/include/hw/acpi/aml-build.h
> > @@ -252,6 +252,7 @@ struct CrsRangeSet {
> >  /* Consumer/Producer */
> >  #define AML_SERIAL_BUS_FLAG_CONSUME_ONLY        (1 << 1)
> >  
> > +#define ACPI_APEI_ERROR_DEVICE   "GEDD"
> >  /**
> >   * init_aml_allocator:
> >   *
> > @@ -382,6 +383,7 @@ Aml *aml_dma(AmlDmaType typ, AmlDmaBusMaster bm, 
> > AmlTransferSize sz,
> >               uint8_t channel);
> >  Aml *aml_sleep(uint64_t msec);
> >  Aml *aml_i2c_serial_bus_device(uint16_t address, const char 
> > *resource_source);
> > +Aml *aml_error_device(void);
> >  
> >  /* Block AML object primitives */
> >  Aml *aml_scope(const char *name_format, ...) G_GNUC_PRINTF(1, 2);
> > diff --git a/include/hw/acpi/generic_event_device.h 
> > b/include/hw/acpi/generic_event_device.h
> > index d2dac87b4a9f..1c18ac296fcb 100644
> > --- a/include/hw/acpi/generic_event_device.h
> > +++ b/include/hw/acpi/generic_event_device.h
> > @@ -101,6 +101,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
> >  #define ACPI_GED_PWR_DOWN_EVT      0x2
> >  #define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
> >  #define ACPI_GED_CPU_HOTPLUG_EVT    0x8
> > +#define ACPI_GED_ERROR_EVT          0x10
> >  
> >  typedef struct GEDState {
> >      MemoryRegion evt;  
> 



Thanks,
Mauro