Re: [PATCH v8 03/13] acpi/ghes: Add support for GED error device

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

Em Mon, 19 Aug 2024 13:43:04 +0200
Igor Mammedov <imammedo@redhat.com> escreveu:

> On Fri, 16 Aug 2024 09:37:35 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > As a GED error device is now defined, add another type
> > of notification.
> > 
> > Add error notification to GHES v2 using
> >a GED error device GED triggered via interrupt.  
>  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This is hard to parse, perhaps update so it would be
> more clear what does what
> 
> > 
> > [mchehab: do some cleanups at ACPI_HEST_SRC_ID_* checks and
> >  rename HEST event to better identify GED interrupt OSPM]
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > ---  
> 
> in addition to change log in cover letter,
> I'd suggest to keep per patch change log as well (after ---),
> it helps reviewer to notice intended changes.
> 
> 
> [...]
> > +    case ACPI_HEST_SRC_ID_GED:
> > +        build_ghes_hw_error_notification(table_data, ACPI_GHES_NOTIFY_GPIO);  
> While GPIO works for arm, it's not the case for other machines.
> I recall a suggestion to use ACPI_GHES_NOTIFY_EXTERNAL instead of GPIO one,
> but that got lost somewhere...

True, but the same also applies to SEA, which is ARMv8+. After having
everything in place, I confined the source ID into this code inside
ghes.c:

	enum AcpiHestSourceId {
	    ACPI_HEST_SRC_ID_SEA,
	    ACPI_HEST_SRC_ID_GED,

	    /* Shall be the last one */
	    ACPI_HEST_SRC_ID_COUNT
	} AcpiHestSourceId;

	static bool ghes_notify_to_source_id(enum AcpiGhesNotifyType notify,
	                                     enum AcpiHestSourceId *source_id)
	{
	    switch (notify) {
	    case ACPI_GHES_NOTIFY_SEA:             /* ARMv8 */
	        *source_id = ACPI_HEST_SRC_ID_SEA;
	        return false;
	    case ACPI_GHES_NOTIFY_GPIO:
	        *source_id = ACPI_HEST_SRC_ID_GED;
	        return false;
	    default:
	        /* Unsupported notification types */
	        return true;
	    }
	}

The only place where the source ID number is used is at
ghes_notify_to_source_id() - still we use ACPI_HEST_SRC_ID_COUNT on other
places to initialize and fill in the HEST table and its error source 
structures.

On other words, the source ID field is filled from the notification types as
defined at include/hw/acpi/ghes.h:

    ACPI_GHES_NOTIFY_POLLED = 0,
    ACPI_GHES_NOTIFY_EXTERNAL = 1,
    ACPI_GHES_NOTIFY_LOCAL = 2,
    ACPI_GHES_NOTIFY_SCI = 3,
    ACPI_GHES_NOTIFY_NMI = 4,
    ACPI_GHES_NOTIFY_CMCI = 5,
    ACPI_GHES_NOTIFY_MCE = 6,
    ACPI_GHES_NOTIFY_GPIO = 7,
    ACPI_GHES_NOTIFY_SEA = 8,
    ACPI_GHES_NOTIFY_SEI = 9,
    ACPI_GHES_NOTIFY_GSIV = 10,
    ACPI_GHES_NOTIFY_SDEI = 11,

(please notice that ACPI already defines "EXTERNAL" as being something 
else)

Now, if we want to add support for x86, we could either add some ifdefs
inside ghes.c, e. g. something like:

	enum AcpiHestSourceId {
	#ifdef TARGET_ARM
	    ACPI_HEST_SRC_ID_SEA,
	    ACPI_HEST_SRC_ID_GED,
	#endif
	#ifdef TARGET_I386
	   ACPI_HEST_SRC_ID_MCE,
        #endif

	    /* Shall be the last one */
	    ACPI_HEST_SRC_ID_COUNT
	} AcpiHestSourceId;

and something similar at ghes_notify_to_source_id():
	static bool ghes_notify_to_source_id(enum AcpiGhesNotifyType notify,
	                                     enum AcpiHestSourceId *source_id)
	{
	    switch (notify) {
	#ifdef TARGET_ARM
	    case ACPI_GHES_NOTIFY_SEA:             /* ARMv8 */
	        *source_id = ACPI_HEST_SRC_ID_SEA;
	        return false;
	    case ACPI_GHES_NOTIFY_GPIO:
	        *source_id = ACPI_HEST_SRC_ID_GED;
	        return false;
	#endif
	#ifdef TARGET_I386
	    case ACPI_GHES_NOTIFY_MCE:
	        *source_id = ACPI_HEST_SRC_ID_MCE;
	        return false;
	#endif
	    default:
	        /* Unsupported notification types */
	        return true;
	    }
	}

An alternative would be to move source id/notification code out, placing
them at hw/arm, hw/i386, but a more complex binding logic will be needed.

If we're willing to do something like that, I would prefer to not do such
redesign now. Better to do such change when we'll be ready to add some 
notification support that works on x86 (MCE? SCI? NMI?).

Regards,
Mauro