Re: [PATCH v5 2/7] acpi/generic_event_device: add an APEI error device

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

Em Mon, 5 Aug 2024 17:39:46 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> escreveu:

> On Fri,  2 Aug 2024 23:43:57 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > Adds a Generic Event Device to handle generic hardware error
> > events, supporting General Purpose Event (GPE) 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 bios 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>
> > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> Much nicer with a GED event.
> Happy to give SoB on this as you requested due to changes.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One minor comment though.
> The pnp0c33 device isn't technically coupled to the generic_event_device.
> Perhaps that should be in aml_build.h/.c instead of where you
> have it here?
> 
> Maybe we can move it later though if anyone implements non GED signalling?

I opted to place it there at hw/acpi/generic_event_device.c, just after 
PNP0C0C, e. g.:

	void acpi_dsdt_add_power_button(Aml *scope)
	{
	    Aml *dev = aml_device(ACPI_POWER_BUTTON_DEVICE);
	    aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
	    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
	    aml_append(scope, dev);
	}
	
	void acpi_dsdt_add_error_device(Aml *scope)
	{
	    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)));
	    aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
	    aml_append(scope, dev);
	}

IMO this way it will be kept closer to other PNP devices. If this starts
to grow, then some later cleanup could move those to some separate file,
but, as now there are just two, I would just keep both there at GED
file.

> 
> Jonathan
> 
> 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> >  hw/acpi/generic_event_device.c         | 17 +++++++++++++++++
> >  include/hw/acpi/acpi_dev_interface.h   |  1 +
> >  include/hw/acpi/generic_event_device.h |  3 +++
> >  3 files changed, 21 insertions(+)
> > 
> > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> > index 15b4c3ebbf24..b9ad05e98c05 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)));
> > +                break;
> >              case ACPI_GED_NVDIMM_HOTPLUG_EVT:
> >                  aml_append(if_ctx,
> >                             aml_notify(aml_name("\\_SB.NVDR"),
> > @@ -153,6 +159,15 @@ void acpi_dsdt_add_power_button(Aml *scope)
> >      aml_append(scope, dev);
> >  }
> >  
> > +void acpi_dsdt_add_error_device(Aml *scope)
> > +{
> > +    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)));
> > +    aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
> > +    aml_append(scope, dev);
> > +}
> > +
> >  /* Memory read by the GED _EVT AML dynamic method */
> >  static uint64_t ged_evt_read(void *opaque, hwaddr addr, unsigned size)
> >  {
> > @@ -295,6 +310,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/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> > index 40af3550b56d..b8f2f1328e0c 100644
> > --- a/include/hw/acpi/generic_event_device.h
> > +++ b/include/hw/acpi/generic_event_device.h
> > @@ -66,6 +66,7 @@
> >  #include "qom/object.h"
> >  
> >  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> > +#define ACPI_APEI_ERROR_DEVICE   "GEDD"
> >  
> >  #define TYPE_ACPI_GED "acpi-ged"
> >  OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
> > @@ -98,6 +99,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;
> > @@ -120,5 +122,6 @@ struct AcpiGedState {
> >  void build_ged_aml(Aml *table, const char* name, HotplugHandler *hotplug_dev,
> >                     uint32_t ged_irq, AmlRegionSpace rs, hwaddr ged_base);
> >  void acpi_dsdt_add_power_button(Aml *scope);
> > +void acpi_dsdt_add_error_device(Aml *scope);
> >  
> >  #endif  
> 



Thanks,
Mauro