Re: [PATCH v4 2/6] arm/virt: Wire up GPIO error source for ACPI / GHES

[Mauro Carvalho Chehab]

Em Mon, 29 Jul 2024 17:08:40 +0100
Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> escreveu:

> On Mon, 29 Jul 2024 15:21:06 +0200
> Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > 
> > Creates a Generic Event Device (GED) as specified at  
> 
> I wrote this a while back and wasn't aware of the naming
> mess around GED in the ACPI spec.  This one is just
> referred to as 'error device' whereas there is also
> a Generic Event Device.
> 
> Linux solved this clash by going with Hardware Error Device
> I think we should do the same here.

I opted to do it a little bit different to stay closer to ACPI 6.5
18.3.2.7.2. - Event Notification For Generic Error Sources.

There, it is actually talking about a General Purpose Event (GPE).
Current ACPI spec doesn't mention "GED", so maybe such term was fixed
on some previous ACPI spec revision.

Basically, it currently mentions:
	- error device
	- GPE / General Purpose Event
	- Generic Hardware Error Source Structure 

I guess Linux crafted the term Hardware Error device by mixing
those.

As we don't need to really preserve such names here, as this appears
only at the patch description, I opted to rewrite the patch description
to:

    arm/virt: Wire up GPIO error source for ACPI / GHES
    
    Creates a hardware event device to support 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).
    
    It is aligned with Linux Kernel patch:
    https://lore.kernel.org/lkml/1272350481-27951-8-git-send-email-ying.huang@xxxxxxxxx/
    
    [mchehab: use a define for the generic event pin number and do some cleanups]
    Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
    Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>

Clearly associating "hardware event device" with ACPI GPE. That sounds
good enough to be stored at the git description associated with such
change.

> > 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
> > with HID PNP0C33.
> > 
> > The PNP0C33 device is used to report hardware errors to
> > the bios via ACPI APEI Generic Hardware Error Source (GHES).
> > 
> > It is aligned with Linux Kernel patch:
> > https://lore.kernel.org/lkml/1272350481-27951-8-git-send-email-ying.huang@xxxxxxxxx/
> > 
> > [mchehab: use a define for the generic event pin number and do some cleanups]
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>  
> 
> > ---
> >  hw/arm/virt-acpi-build.c | 30 ++++++++++++++++++++++++++----
> >  hw/arm/virt.c            | 14 ++++++++++++--
> >  include/hw/arm/virt.h    |  1 +
> >  include/hw/boards.h      |  1 +
> >  4 files changed, 40 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index f76fb117adff..c502ccf40909 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -63,6 +63,7 @@
> >  
> >  #define ARM_SPI_BASE 32
> >  
> > +#define ACPI_GENERIC_EVENT_DEVICE "GEDD"  
> 
> Ah. My mistake. This is the confusing named GENERIC_ERROR_DEVICE
> or HARDWARE_ERROR_DEVICE (which is what Linux called it because
> in the ACPI Spec it is just (all lower case) error device).

I opted to use a different name there, using just error device,
together with the name of the PNP device. So:

	#define PNP0C33_ERROR_DEVICE "GEDD"

This is clear enough for people just looking at the driver, and
even clearer for people familiar with session 18.3.2.7.2 of the
ACPI spec.

> 
> >  #define ACPI_BUILD_TABLE_SIZE             0x20000  
> 
> >  /* DSDT */
> >  static void
> >  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > @@ -841,10 +863,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >                        HOTPLUG_HANDLER(vms->acpi_dev),
> >                        irqmap[VIRT_ACPI_GED] + ARM_SPI_BASE, AML_SYSTEM_MEMORY,
> >                        memmap[VIRT_ACPI_GED].base);
> > -    } else {
> > -        acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
> > -                           (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
> >      }
> > +    acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
> > +                       (irqmap[VIRT_GPIO] + ARM_SPI_BASE));  
> 
> Arguably excess brackets, but obviously this is just a code move
> so fine to keep it the same.

I'll drop the extra brackets.

> >  
> >      if (vms->acpi_dev) {
> >          uint32_t event = object_property_get_uint(OBJECT(vms->acpi_dev),
> > @@ -858,6 +879,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >      }
> >  
> >      acpi_dsdt_add_power_button(scope);

> > +    acpi_dsdt_add_generic_event_device(scope);

I'm also renaming this function/function call to run away from GED,
calling it as:

	 acpi_dsdt_add_error_device(scope);

> >  #ifdef CONFIG_TPM
> >      acpi_dsdt_add_tpm(scope, vms);
> >  #endif  
> 

Thanks,
Mauro