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

Mauro Carvalho Chehab
2024-07-31T05:22:04Z

Em Tue, 30 Jul 2024 16:11:42 +0800
Zhao Liu <zhao1.liu@intel.com> 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 <mchehab+huawei@kernel.org>
> > 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 <Jonathan.Cameron@huawei.com>
> >
> > 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/1272350481-27951-8-git-send-email-ying.huang@intel.com/
> >
> > [mchehab: use a define for the generic event pin number and do some cleanups]
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> > 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