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

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

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