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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jan 28, 2025 at 06:42:02PM +0100, Mauro Carvalho Chehab wrote:
> 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)

we do not do a lot of defines definetely not for 1 time used constants.

just comment on top of the value.

> > 
> > > +                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