mbox series

[RFC,v2,0/3] efi/cxl-cper: Report CPER CXL component events through trace events

Message ID 20230601-cxl-cper-v2-0-314d9c36ab02@intel.com
Headers show
Series efi/cxl-cper: Report CPER CXL component events through trace events | expand

Message

Ira Weiny Oct. 26, 2023, 6:21 p.m. UTC 
Series status/background
========================

This is another RFC version of processing the CXL CPER records through
the CXL trace mechanisms as Dan mentioned in [1].

I moved forward with eliminating the GUID to UUID conversion I mentioned
in the original RFC thread[2].  Instead a new event type is used once
the GUID or UUID's is used to decode the event.

This remains compile tested with only.

[1] https://lore.kernel.org/all/6528808cef2ba_780ef294c5@dwillia2-xfh.jf.intel.com.notmuch/
[2] https://lore.kernel.org/all/652f45e29915c_2bb07d2949b@iweiny-mobl.notmuch/

Cover letter
============

CXL Component Events, as defined by EFI 2.10 Section N.2.14, wrap a
mostly CXL event payload in an EFI Common Platform Error Record (CPER)
record.  If a device is configured for firmware first CXL event records
are not sent directly to the host.

The CXL sub-system uniquely has DPA to HPA translation information.  It
also already properly decodes the event record format.  Send the CXL
CPER records to the CXL sub-system for processing.

With CXL event logs the device interrupts the host with events.  In the
EFI case events are wrapped with device information which needs to be
matched with memdev devices the CXL driver is tracking.

A number of alternatives were considered to match the memdev with the
CPER record.  The most straight forward comparison is via serial number.

CPER records are identified with GUID's while CXL event logs contain
UUID's.  The UUID was previously printed for all events.  But the UUID
is redundant information which presents unnecessary complexity when
processing CPER data.  Remove the UUIDs from known events.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
Changes in RFC v2:
- iweiny: remove uuid from existing known event traces
- iweiny: pass an enum for the event type.
- Link to v1: https://lore.kernel.org/r/20230601-cxl-cper-v1-0-99ba43f8f770@intel.com

---
Ira Weiny (3):
      cxl/trace: Remove uuid from event trace known events
      firmware/efi: Process CXL Component Events
      cxl/memdev: Register for and process CPER events

 drivers/cxl/core/mbox.c         | 45 +++++++++++++++++++++------
 drivers/cxl/core/trace.h        | 10 +++---
 drivers/cxl/cxlmem.h            |  7 +++++
 drivers/cxl/pci.c               | 69 ++++++++++++++++++++++++++++++++++++++++-
 drivers/firmware/efi/cper.c     | 16 ++++++++++
 drivers/firmware/efi/cper_cxl.c | 40 ++++++++++++++++++++++++
 drivers/firmware/efi/cper_cxl.h | 29 +++++++++++++++++
 include/linux/efi.h             | 59 +++++++++++++++++++++++++++++++++++
 8 files changed, 259 insertions(+), 16 deletions(-)
---
base-commit: 1c8b86a3799f7e5be903c3f49fcdaee29fd385b5
change-id: 20230601-cxl-cper-26ffc839c6c6

Best regards,

Comments

Dan Williams Oct. 26, 2023, 9:02 p.m. UTC | #1 
Ira Weiny wrote:
> BIOS can configure memory devices as firmware first.  This will send CXL
> events to the firmware instead of the OS.  The firmware can then send
> these events to the OS via UEFI.
> 
> UEFI v2.10 section N.2.14 defines a Common Platform Error Record (CPER)
> format for CXL Component Events.  The format is mostly the same as the
> CXL Common Event Record Format.  The only difference is the UUID is
> passed via the Section Type as a GUID and not included as part of the
> record data.
> 
> Add EFI support to detect CXL CPER records and call a notifier chain
> with the record data blobs.
> 
> Note that the format of a GUID and UUID are not the same.  Therefore the
> Section Type GUID defines are duplicated from the CXL code.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> ---
> Changes from RFC v1
> [iweiny: use an enum for know record types and skip converting GUID to UUID]
> [iweiny: commit to the UUID not being part of the event record data]
> [iweiny: use defines for GUID definitions]
> ---
>  drivers/firmware/efi/cper.c     | 16 +++++++++++
>  drivers/firmware/efi/cper_cxl.c | 40 ++++++++++++++++++++++++++++
>  drivers/firmware/efi/cper_cxl.h | 29 ++++++++++++++++++++
>  include/linux/efi.h             | 59 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 144 insertions(+)
> 
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 35c37f667781..d6415c94d584 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -607,6 +607,22 @@ cper_estatus_print_section(const char *pfx, struct acpi_hest_generic_data *gdata
>  			cper_print_prot_err(newpfx, prot_err);
>  		else
>  			goto err_section_too_small;
> +	} else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA) ||
> +		   guid_equal(sec_type, &CPER_SEC_CXL_DRAM) ||
> +		   guid_equal(sec_type, &CPER_SEC_CXL_MEM_MODULE)) {
> +		struct cper_cxl_event_rec *rec = acpi_hest_get_payload(gdata);
> +
> +		printk("%ssection type: CXL Event\n", newpfx);

I would say that since this is going to be hanlded elsewhere the kernel
log can stay silent.

> +
> +		if (rec->hdr.length <= sizeof(rec->hdr))
> +			goto err_section_too_small;
> +
> +		if (rec->hdr.length > sizeof(*rec)) {
> +			pr_err(FW_WARN "error section length is too big\n");
> +			return;
> +		}
> +
> +		cper_post_cxl_event(newpfx, sec_type, rec);
>  	} else {
>  		const void *err = acpi_hest_get_payload(gdata);
>  
> diff --git a/drivers/firmware/efi/cper_cxl.c b/drivers/firmware/efi/cper_cxl.c
> index a55771b99a97..04234884898d 100644
> --- a/drivers/firmware/efi/cper_cxl.c
> +++ b/drivers/firmware/efi/cper_cxl.c
> @@ -187,3 +187,43 @@ void cper_print_prot_err(const char *pfx, const struct cper_sec_prot_err *prot_e
>  			       sizeof(cxl_ras->header_log), 0);
>  	}
>  }
> +
> +/* CXL CPER notifier chain */
> +static BLOCKING_NOTIFIER_HEAD(cxl_cper_chain_head);
> +
> +void cper_post_cxl_event(const char *pfx, guid_t *sec_type,
> +			 struct cper_cxl_event_rec *rec)
> +{
> +	struct cxl_cper_notifier_data nd = {
> +		.rec = rec,
> +	};
> +
> +	if (!(rec->hdr.validation_bits & CPER_CXL_COMP_EVENT_LOG_VALID)) {
> +		pr_err(FW_WARN "cxl event no Component Event Log present\n");
> +		return;
> +	}
> +
> +	if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA))
> +		nd.cper_event = CXL_CPER_EVENT_GEN_MEDIA;
> +	else if (guid_equal(sec_type, &CPER_SEC_CXL_DRAM))
> +		nd.cper_event = CXL_CPER_EVENT_DRAM;
> +	else if (guid_equal(sec_type, &CPER_SEC_CXL_MEM_MODULE))
> +		nd.cper_event = CXL_CPER_EVENT_MEM_MODULE;
> +
> +	if (blocking_notifier_call_chain(&cxl_cper_chain_head, 0, (void *)&nd)
> +			== NOTIFY_BAD)
> +		pr_err(FW_WARN "cxl event notifier chain failed\n");
> +}
> +
> +int register_cxl_cper_notifier(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_register(&cxl_cper_chain_head, nb);
> +}
> +EXPORT_SYMBOL(register_cxl_cper_notifier);

I think this should be EXPORT_SYMBOL_NS_GPL(..., CXL) since I can't
imagine a third-party driver use case for this.

> +
> +void unregister_cxl_cper_notifier(struct notifier_block *nb)
> +{
> +	blocking_notifier_chain_unregister(&cxl_cper_chain_head, nb);
> +}
> +EXPORT_SYMBOL(unregister_cxl_cper_notifier);
> +
> diff --git a/drivers/firmware/efi/cper_cxl.h b/drivers/firmware/efi/cper_cxl.h
> index 86bfcf7909ec..ca26126cd9b8 100644
> --- a/drivers/firmware/efi/cper_cxl.h
> +++ b/drivers/firmware/efi/cper_cxl.h
> @@ -10,11 +10,38 @@
>  #ifndef LINUX_CPER_CXL_H
>  #define LINUX_CPER_CXL_H
>  
> +#include <linux/efi.h>
> +
>  /* CXL Protocol Error Section */
>  #define CPER_SEC_CXL_PROT_ERR						\
>  	GUID_INIT(0x80B9EFB4, 0x52B5, 0x4DE3, 0xA7, 0x77, 0x68, 0x78,	\
>  		  0x4B, 0x77, 0x10, 0x48)
>  
> +/* CXL Event record UUIDs are used as the section type */
> +/*
> + * General Media Event Record
> + * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43
> + */
> +#define CPER_SEC_CXL_GEN_MEDIA						\
> +	GUID_INIT(0xfbcd0a77, 0xc260, 0x417f,				\
> +		  0x85, 0xa9, 0x08, 0x8b, 0x16, 0x21, 0xeb, 0xa6)
> +
> +/*
> + * DRAM Event Record
> + * CXL rev 3.0 section 8.2.9.2.1.2; Table 8-44
> + */
> +#define CPER_SEC_CXL_DRAM						\
> +	GUID_INIT(0x601dcbb3, 0x9c06, 0x4eab,				\
> +		  0xb8, 0xaf, 0x4e, 0x9b, 0xfb, 0x5c, 0x96, 0x24)
> +
> +/*
> + * Memory Module Event Record
> + * CXL rev 3.0 section 8.2.9.2.1.3; Table 8-45
> + */
> +#define CPER_SEC_CXL_MEM_MODULE						\
> +	GUID_INIT(0xfe927475, 0xdd59, 0x4339,				\
> +		  0xa5, 0x86, 0x79, 0xba, 0xb1, 0x13, 0xb7, 0x74)
> +
>  #pragma pack(1)
>  
>  /* Compute Express Link Protocol Error Section, UEFI v2.10 sec N.2.13 */
> @@ -62,5 +89,7 @@ struct cper_sec_prot_err {
>  #pragma pack()
>  
>  void cper_print_prot_err(const char *pfx, const struct cper_sec_prot_err *prot_err);
> +void cper_post_cxl_event(const char *pfx, guid_t *sec_type,
> +			 struct cper_cxl_event_rec *rec);
>  
>  #endif //__CPER_CXL_
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 80b21d1c6eaf..b5b8b46c8deb 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1355,4 +1355,63 @@ bool efi_config_table_is_usable(const efi_guid_t *guid, unsigned long table)
>  
>  umode_t efi_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n);
>  
> +/*
> + * Event log size adjusted for CPER
> + *
> + * Base table from CXL r3.0 Table 8-42: (30h + 50h)
> + * For lack of UUID: - 10h
> + *
> + * (30h + 50h) - 10h = 70h
> + */
> +#define CPER_CXL_COMP_EVENT_LOG_SIZE 0x70
> +#define CPER_CXL_DEVICE_ID_VALID		BIT(0)
> +#define CPER_CXL_DEVICE_SN_VALID		BIT(1)
> +#define CPER_CXL_COMP_EVENT_LOG_VALID		BIT(2)
> +struct cper_cxl_event_rec {
> +	struct {
> +		u32 length;
> +		u64 validation_bits;
> +		struct {
> +			u16 vendor_id;
> +			u16 device_id;
> +			u8 func_num;
> +			u8 device_num;
> +			u8 bus_num;
> +			u16 segment_num;
> +			u16 slot_num; /* bits 2:0 reserved */
> +			u8 reserved;
> +		} device_id;
> +		struct {
> +			u32 lower_dw;
> +			u32 upper_dw;
> +		} dev_serial_num;
> +	} hdr;
> +
> +	u8 comp_event_log[CPER_CXL_COMP_EVENT_LOG_SIZE];

Rather than define CPER_CXL_COMP_EVENT_LOG_SIZE I would prefer that CXL
and EFI share a common struct definition for these common fields.

That would also remove the need for BUILD_BUG_ON() since they literally
can not disagree on the size in that case.

> +};
> +#define CPER_CXL_REC_LEN(rec) (rec->hdr.length - sizeof(rec->hdr))
> +
> +enum cxl_cper_event {
> +	CXL_CPER_EVENT_GEN_MEDIA,
> +	CXL_CPER_EVENT_DRAM,
> +	CXL_CPER_EVENT_MEM_MODULE,
> +};

It follows from defining that common data structure above that this enum
would be a generic CXL namespace that drops "_CPER_". I.e. the CPER
notification handler and the native driver translate the event to this
common generic sub-structure that gets emitted.

> +
> +struct cxl_cper_notifier_data {
> +	enum cxl_cper_event cper_event;
> +	struct cper_cxl_event_rec *rec;
> +};
> +
> +#ifdef CONFIG_EFI
> +int register_cxl_cper_notifier(struct notifier_block *nb);
> +void unregister_cxl_cper_notifier(struct notifier_block *nb);
> +#else
> +static inline int register_cxl_cper_notifier(struct notifier_block *nb)
> +{
> +	return 0;
> +}
> +
> +static inline void unregister_cxl_cper_notifier(struct notifier_block *nb) { }
> +#endif
> +
>  #endif /* _LINUX_EFI_H */
> 
> -- 
> 2.41.0
>
Ira Weiny Oct. 31, 2023, 4:46 p.m. UTC | #2 
Dan Williams wrote:
> Ira Weiny wrote:

[snip]

> > 
> > diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> > index 35c37f667781..d6415c94d584 100644
> > --- a/drivers/firmware/efi/cper.c
> > +++ b/drivers/firmware/efi/cper.c
> > @@ -607,6 +607,22 @@ cper_estatus_print_section(const char *pfx, struct acpi_hest_generic_data *gdata
> >  			cper_print_prot_err(newpfx, prot_err);
> >  		else
> >  			goto err_section_too_small;
> > +	} else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA) ||
> > +		   guid_equal(sec_type, &CPER_SEC_CXL_DRAM) ||
> > +		   guid_equal(sec_type, &CPER_SEC_CXL_MEM_MODULE)) {
> > +		struct cper_cxl_event_rec *rec = acpi_hest_get_payload(gdata);
> > +
> > +		printk("%ssection type: CXL Event\n", newpfx);
> 
> I would say that since this is going to be hanlded elsewhere the kernel
> log can stay silent.

Yep, bad cargo cult.

Removed.

[snip]

> > +
> > +int register_cxl_cper_notifier(struct notifier_block *nb)
> > +{
> > +	return blocking_notifier_chain_register(&cxl_cper_chain_head, nb);
> > +}
> > +EXPORT_SYMBOL(register_cxl_cper_notifier);
> 
> I think this should be EXPORT_SYMBOL_NS_GPL(..., CXL) since I can't
> imagine a third-party driver use case for this.

Good point.  Done.

[snip]

> >  
> > +/*
> > + * Event log size adjusted for CPER
> > + *
> > + * Base table from CXL r3.0 Table 8-42: (30h + 50h)
> > + * For lack of UUID: - 10h
> > + *
> > + * (30h + 50h) - 10h = 70h
> > + */
> > +#define CPER_CXL_COMP_EVENT_LOG_SIZE 0x70
> > +#define CPER_CXL_DEVICE_ID_VALID		BIT(0)
> > +#define CPER_CXL_DEVICE_SN_VALID		BIT(1)
> > +#define CPER_CXL_COMP_EVENT_LOG_VALID		BIT(2)
> > +struct cper_cxl_event_rec {
> > +	struct {
> > +		u32 length;
> > +		u64 validation_bits;
> > +		struct {
> > +			u16 vendor_id;
> > +			u16 device_id;
> > +			u8 func_num;
> > +			u8 device_num;
> > +			u8 bus_num;
> > +			u16 segment_num;
> > +			u16 slot_num; /* bits 2:0 reserved */
> > +			u8 reserved;
> > +		} device_id;
> > +		struct {
> > +			u32 lower_dw;
> > +			u32 upper_dw;
> > +		} dev_serial_num;
> > +	} hdr;
> > +
> > +	u8 comp_event_log[CPER_CXL_COMP_EVENT_LOG_SIZE];
> 
> Rather than define CPER_CXL_COMP_EVENT_LOG_SIZE I would prefer that CXL
> and EFI share a common struct definition for these common fields.
> 
> That would also remove the need for BUILD_BUG_ON() since they literally
> can not disagree on the size in that case.

I don't know if we discussed this publicly or internally but I had
versions which lifted the CXL structs to the core.  But your opinion at
that time was it was not needed.

Looking at it again I might get away with the main event record struct
defined here.  But it is kind of odd to be in efi.h IMO.

> 
> > +};
> > +#define CPER_CXL_REC_LEN(rec) (rec->hdr.length - sizeof(rec->hdr))
> > +
> > +enum cxl_cper_event {
> > +	CXL_CPER_EVENT_GEN_MEDIA,
> > +	CXL_CPER_EVENT_DRAM,
> > +	CXL_CPER_EVENT_MEM_MODULE,
> > +};
> 
> It follows from defining that common data structure above that this enum
> would be a generic CXL namespace that drops "_CPER_". I.e. the CPER
> notification handler and the native driver translate the event to this
> common generic sub-structure that gets emitted.

Ok this would be along the lines of promoting the definitions to a common
header.  Again I kept things pretty separate because it seemed that was
the direction you wanted to go.

I don't particularly like the memcpy which Smita flagged either.  But I
think this will require reworking the trace code to take a 'non-uuid'
structure equal to the payload 'comp_event_log' above.

Based on these comments I'll add some header promotion of common record
structures and see how it works out with the modified trace code.

Do you have any opinions on the name of the core header?

Ira