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

Commit Message

Mauro Carvalho Chehab July 29, 2024, 1:21 p.m. UTC

From: Jonathan Cameron <[e-mail]>

Creates a Generic Event Device (GED) 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
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/[e-mail]/

[mchehab: use a define for the generic event pin number and do some cleanups]
Signed-off-by: Jonathan Cameron <[e-mail]>
Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
---
 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(-)

Comments

Jonathan Cameron July 29, 2024, 4:08 p.m. UTC | #1

On Mon, 29 Jul 2024 15:21:06 +0200
Mauro Carvalho Chehab <[e-mail]> wrote:

> From: Jonathan Cameron <[e-mail]>
> 
> 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.

> 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/[e-mail]/
> 
> [mchehab: use a define for the generic event pin number and do some cleanups]
> Signed-off-by: Jonathan Cameron <[e-mail]>
> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>

> ---
>  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).

>  #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.

>  
>      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);
>  #ifdef CONFIG_TPM
>      acpi_dsdt_add_tpm(scope, vms);
>  #endif

Mauro Carvalho Chehab July 30, 2024, 5:13 a.m. UTC | #2

Em Mon, 29 Jul 2024 17:08:40 +0100
Jonathan Cameron <[e-mail]> escreveu:

> On Mon, 29 Jul 2024 15:21:06 +0200
> Mauro Carvalho Chehab <[e-mail]> wrote:
> 
> > From: Jonathan Cameron <[e-mail]>
> > 
> > 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/[e-mail]/
    
    [mchehab: use a define for the generic event pin number and do some cleanups]
    Signed-off-by: Jonathan Cameron <[e-mail]>
    Signed-off-by: Mauro Carvalho Chehab <[e-mail]>

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/[e-mail]/
> > 
> > [mchehab: use a define for the generic event pin number and do some cleanups]
> > Signed-off-by: Jonathan Cameron <[e-mail]>
> > Signed-off-by: Mauro Carvalho Chehab <[e-mail]>  
> 
> > ---
> >  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

Zhao Liu July 30, 2024, 8:11 a.m. UTC | #3

Hi Mauro,

On Mon, Jul 29, 2024 at 03:21:06PM +0200, Mauro Carvalho Chehab wrote:
> Date: Mon, 29 Jul 2024 15:21:06 +0200
> From: Mauro Carvalho Chehab <[e-mail]>
> Subject: [PATCH v4 2/6] arm/virt: Wire up GPIO error source for ACPI / GHES
> X-Mailer: git-send-email 2.45.2
> 
> From: Jonathan Cameron <[e-mail]>
> 
> Creates a Generic Event Device (GED) 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
> 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/[e-mail]/
> 
> [mchehab: use a define for the generic event pin number and do some cleanups]
> Signed-off-by: Jonathan Cameron <[e-mail]>
> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> ---
>  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(-)

[snip]

> +static void virt_set_error(void)
> +{
> +    qemu_set_irq(qdev_get_gpio_in(gpio_error_dev, 0), 1);
> +}
> +

[snip]

> +    mc->generic_error_device_notify = virt_set_error;

[snip]

> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 48ff6d8b93f7..991f99138e57 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -308,6 +308,7 @@ struct MachineClass {
>      int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx);
>      ram_addr_t (*fixup_ram_size)(ram_addr_t size);
>      uint64_t smbios_memory_device_size;
> +    void (*generic_error_device_notify)(void);

The name looks inconsistent with the style of other MachineClass virtual
methods. What about the name like "notify_xxx"? And pls add the comment
about this new method.

BTW, I found this method is called in generic_error_device_notify() of
Patch 6. And the mc->generic_error_device_notify() - as the virtual
metchod of MachineClass looks just to implement a hook, and it doesn't
seem to have anything to do with MachineClass/MachineState, so my
question is why do we need to add this method to MachineClass?

Could we maintain a notifier list in ghes.c and expose an interface
to allow arm code register a notifier? This eliminates the need to add
the “notify” method to MachineClass.

Regards,
Zhao

Jonathan Cameron July 30, 2024, 8:39 a.m. UTC | #4

On Tue, 30 Jul 2024 07:13:30 +0200
Mauro Carvalho Chehab <[e-mail]> wrote:

> Em Mon, 29 Jul 2024 17:08:40 +0100
> Jonathan Cameron <[e-mail]> escreveu:
> 
> > On Mon, 29 Jul 2024 15:21:06 +0200
> > Mauro Carvalho Chehab <[e-mail]> wrote:
> >   
> > > From: Jonathan Cameron <[e-mail]>
> > > 
> > > 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/[e-mail]/
>     
>     [mchehab: use a define for the generic event pin number and do some cleanups]
>     Signed-off-by: Jonathan Cameron <[e-mail]>
>     Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> 
> Clearly associating "hardware event device" with ACPI GPE. That sounds
> good enough to be stored at the git description associated with such
> change.
All looks good to me.

Thanks,

Jonathan

> 
> > > 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/[e-mail]/
> > > 
> > > [mchehab: use a define for the generic event pin number and do some cleanups]
> > > Signed-off-by: Jonathan Cameron <[e-mail]>
> > > Signed-off-by: Mauro Carvalho Chehab <[e-mail]>    
> >   
> > > ---
> > >  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

Mauro Carvalho Chehab July 31, 2024, 5:21 a.m. UTC | #5

Em Tue, 30 Jul 2024 16:11:42 +0800
Zhao Liu <[e-mail]> escreveu:

> Hi Mauro,
> 
> On Mon, Jul 29, 2024 at 03:21:06PM +0200, Mauro Carvalho Chehab wrote:
> > Date: Mon, 29 Jul 2024 15:21:06 +0200
> > From: Mauro Carvalho Chehab <[e-mail]>
> > Subject: [PATCH v4 2/6] arm/virt: Wire up GPIO error source for ACPI / GHES
> > X-Mailer: git-send-email 2.45.2
> > 
> > From: Jonathan Cameron <[e-mail]>
> > 
> > Creates a Generic Event Device (GED) 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
> > 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/[e-mail]/
> > 
> > [mchehab: use a define for the generic event pin number and do some cleanups]
> > Signed-off-by: Jonathan Cameron <[e-mail]>
> > Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> > ---
> >  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(-)  
> 
> [snip]
> 
> > +static void virt_set_error(void)
> > +{
> > +    qemu_set_irq(qdev_get_gpio_in(gpio_error_dev, 0), 1);
> > +}
> > +  
> 
> [snip]
> 
> > +    mc->generic_error_device_notify = virt_set_error;  
> 
> [snip]
> 
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 48ff6d8b93f7..991f99138e57 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -308,6 +308,7 @@ struct MachineClass {
> >      int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx);
> >      ram_addr_t (*fixup_ram_size)(ram_addr_t size);
> >      uint64_t smbios_memory_device_size;
> > +    void (*generic_error_device_notify)(void);  
> 
> The name looks inconsistent with the style of other MachineClass virtual
> methods. What about the name like "notify_xxx"? And pls add the comment
> about this new method.
> 
> BTW, I found this method is called in generic_error_device_notify() of
> Patch 6. And the mc->generic_error_device_notify() - as the virtual
> metchod of MachineClass looks just to implement a hook, and it doesn't
> seem to have anything to do with MachineClass/MachineState, so my
> question is why do we need to add this method to MachineClass?
> 
> Could we maintain a notifier list in ghes.c and expose an interface
> to allow arm code register a notifier? This eliminates the need to add
> the “notify” method to MachineClass.

Makes sense. I'll change the logic to use this notifier list code inside
ghes.c, and drop generic_error_device_notify():

	NotifierList generic_error_notifiers =
	    NOTIFIER_LIST_INITIALIZER(error_device_notifiers);

	/* Notify BIOS about an error via Generic Error Device - GED */
	static void generic_error_device_notify(void)
	{
	    notifier_list_notify(&generic_error_notifiers, NULL);
	}

Regards,
Mauro

Zhao Liu July 31, 2024, 8 a.m. UTC | #6

On Wed, Jul 31, 2024 at 07:21:58AM +0200, Mauro Carvalho Chehab wrote:

[snip]

> > The name looks inconsistent with the style of other MachineClass virtual
> > methods. What about the name like "notify_xxx"? And pls add the comment
> > about this new method.
> > 
> > BTW, I found this method is called in generic_error_device_notify() of
> > Patch 6. And the mc->generic_error_device_notify() - as the virtual
> > metchod of MachineClass looks just to implement a hook, and it doesn't
> > seem to have anything to do with MachineClass/MachineState, so my
> > question is why do we need to add this method to MachineClass?
> > 
> > Could we maintain a notifier list in ghes.c and expose an interface
> > to allow arm code register a notifier? This eliminates the need to add
> > the “notify” method to MachineClass.
> 
> Makes sense. I'll change the logic to use this notifier list code inside
> ghes.c, and drop generic_error_device_notify():
> 
> 	NotifierList generic_error_notifiers =
> 	    NOTIFIER_LIST_INITIALIZER(error_device_notifiers);
> 
> 	/* Notify BIOS about an error via Generic Error Device - GED */
> 	static void generic_error_device_notify(void)
> 	{
> 	    notifier_list_notify(&generic_error_notifiers, NULL);
> 	}

Fine for me.

Regards,
Zhao

Patch

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"
 #define ACPI_BUILD_TABLE_SIZE             0x20000
 
 static void acpi_dsdt_add_cpus(Aml *scope, VirtMachineState *vms)
@@ -142,6 +143,8 @@  static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
 static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
                                            uint32_t gpio_irq)
 {
+    uint32_t pin;
+
     Aml *dev = aml_device("GPO0");
     aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
     aml_append(dev, aml_name_decl("_UID", aml_int(0)));
@@ -155,7 +158,12 @@  static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
 
     Aml *aei = aml_resource_template();
 
-    const uint32_t pin = GPIO_PIN_POWER_BUTTON;
+    pin = GPIO_PIN_POWER_BUTTON;
+    aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
+                                 AML_EXCLUSIVE, AML_PULL_UP, 0, &pin, 1,
+                                 "GPO0", NULL, 0));
+    /* Pin for generic error */
+    pin = GPIO_PIN_GENERIC_ERROR;
     aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
                                  AML_EXCLUSIVE, AML_PULL_UP, 0, &pin, 1,
                                  "GPO0", NULL, 0));
@@ -166,6 +174,11 @@  static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
     aml_append(method, aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
                                   aml_int(0x80)));
     aml_append(dev, method);
+    method = aml_method("_E06", 0, AML_NOTSERIALIZED);
+    aml_append(method, aml_notify(aml_name(ACPI_GENERIC_EVENT_DEVICE),
+                                  aml_int(0x80)));
+    aml_append(dev, method);
+
     aml_append(scope, dev);
 }
 
@@ -800,6 +813,15 @@  static void build_fadt_rev6(GArray *table_data, BIOSLinker *linker,
     build_fadt(table_data, linker, &fadt, vms->oem_id, vms->oem_table_id);
 }
 
+static void acpi_dsdt_add_generic_event_device(Aml *scope)
+{
+    Aml *dev = aml_device(ACPI_GENERIC_EVENT_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);
+}
+
 /* 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));
 
     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);
 #ifdef CONFIG_TPM
     acpi_dsdt_add_tpm(scope, vms);
 #endif
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 687fe0bb8bc9..5a11691f29f6 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -997,6 +997,13 @@  static void create_rtc(const VirtMachineState *vms)
 }
 
 static DeviceState *gpio_key_dev;
+
+static DeviceState *gpio_error_dev;
+static void virt_set_error(void)
+{
+    qemu_set_irq(qdev_get_gpio_in(gpio_error_dev, 0), 1);
+}
+
 static void virt_powerdown_req(Notifier *n, void *opaque)
 {
     VirtMachineState *s = container_of(n, VirtMachineState, powerdown_notifier);
@@ -1015,6 +1022,9 @@  static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
     gpio_key_dev = sysbus_create_simple("gpio-key", -1,
                                         qdev_get_gpio_in(pl061_dev,
                                                          GPIO_PIN_POWER_BUTTON));
+    gpio_error_dev = sysbus_create_simple("gpio-key", -1,
+                                          qdev_get_gpio_in(pl061_dev,
+                                                           GPIO_PIN_GENERIC_ERROR));
 
     qemu_fdt_add_subnode(fdt, "/gpio-keys");
     qemu_fdt_setprop_string(fdt, "/gpio-keys", "compatible", "gpio-keys");
@@ -2385,9 +2395,8 @@  static void machvirt_init(MachineState *machine)
 
     if (has_ged && aarch64 && firmware_loaded && virt_is_acpi_enabled(vms)) {
         vms->acpi_dev = create_acpi_ged(vms);
-    } else {
-        create_gpio_devices(vms, VIRT_GPIO, sysmem);
     }
+    create_gpio_devices(vms, VIRT_GPIO, sysmem);
 
     if (vms->secure && !vmc->no_secure_gpio) {
         create_gpio_devices(vms, VIRT_SECURE_GPIO, secure_sysmem);
@@ -3101,6 +3110,7 @@  static void virt_machine_class_init(ObjectClass *oc, void *data)
     mc->default_ram_id = "mach-virt.ram";
     mc->default_nic = "virtio-net-pci";
 
+    mc->generic_error_device_notify = virt_set_error;
     object_class_property_add(oc, "acpi", "OnOffAuto",
         virt_get_acpi, virt_set_acpi,
         NULL, NULL);
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index a4d937ed45ac..c9769d7d4d7f 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -49,6 +49,7 @@ 
 
 /* GPIO pins */
 #define GPIO_PIN_POWER_BUTTON  3
+#define GPIO_PIN_GENERIC_ERROR 6
 
 enum {
     VIRT_FLASH,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 48ff6d8b93f7..991f99138e57 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -308,6 +308,7 @@  struct MachineClass {
     int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx);
     ram_addr_t (*fixup_ram_size)(ram_addr_t size);
     uint64_t smbios_memory_device_size;
+    void (*generic_error_device_notify)(void);
 };
 
 /**