lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <6572740922eb6_269bd29445@dwillia2-mobl3.amr.corp.intel.com.notmuch> Date: Thu, 7 Dec 2023 17:40:25 -0800 From: Dan Williams <dan.j.williams@...el.com> To: Ira Weiny <ira.weiny@...el.com>, Dan Williams <dan.j.williams@...el.com>, Jonathan Cameron <jonathan.cameron@...wei.com>, Smita Koralahalli <Smita.KoralahalliChannabasappa@....com>, Shiju Jose <shiju.jose@...wei.com> CC: Yazen Ghannam <yazen.ghannam@....com>, Davidlohr Bueso <dave@...olabs.net>, Dave Jiang <dave.jiang@...el.com>, Alison Schofield <alison.schofield@...el.com>, Vishal Verma <vishal.l.verma@...el.com>, "Ard Biesheuvel" <ardb@...nel.org>, <linux-efi@...r.kernel.org>, <linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>, Ira Weiny <ira.weiny@...el.com> Subject: Re: [PATCH 5/6] firmware/efi: Process CXL Component Events 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 difference is a GUID is used in > the Section Type to identify the event type. > > Add EFI support to detect CXL CPER records and call a notifier chain > with the record data blobs to be processed by the CXL code. > > Signed-off-by: Ira Weiny <ira.weiny@...el.com> > > --- > Changes from RFC: > [Smita: use pragma packed for cper structures] > --- > drivers/firmware/efi/cper.c | 15 ++++++++++++ > drivers/firmware/efi/cper_cxl.c | 40 +++++++++++++++++++++++++++++++ > drivers/firmware/efi/cper_cxl.h | 29 ++++++++++++++++++++++ > include/linux/cxl-event.h | 53 +++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 137 insertions(+) > > diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c > index 35c37f667781..3d0b60144a07 100644 > --- a/drivers/firmware/efi/cper.c > +++ b/drivers/firmware/efi/cper.c > @@ -22,6 +22,7 @@ > #include <linux/aer.h> > #include <linux/printk.h> > #include <linux/bcd.h> > +#include <linux/cxl-event.h> > #include <acpi/ghes.h> > #include <ras/ras_event.h> > #include "cper_cxl.h" > @@ -607,6 +608,20 @@ 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); > + > + 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..bf642962a7ba 100644 > --- a/drivers/firmware/efi/cper_cxl.c > +++ b/drivers/firmware/efi/cper_cxl.c > @@ -8,6 +8,7 @@ > */ > > #include <linux/cper.h> > +#include <linux/cxl-event.h> > #include "cper_cxl.h" > > #define PROT_ERR_VALID_AGENT_TYPE BIT_ULL(0) > @@ -187,3 +188,42 @@ 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.event_type = CXL_CPER_EVENT_GEN_MEDIA; > + else if (guid_equal(sec_type, &CPER_SEC_CXL_DRAM)) > + nd.event_type = CXL_CPER_EVENT_DRAM; > + else if (guid_equal(sec_type, &CPER_SEC_CXL_MEM_MODULE)) > + nd.event_type = 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_NS_GPL(register_cxl_cper_notifier, CXL); > + > +void unregister_cxl_cper_notifier(struct notifier_block *nb) > +{ > + blocking_notifier_chain_unregister(&cxl_cper_chain_head, nb); > +} > +EXPORT_SYMBOL_NS_GPL(unregister_cxl_cper_notifier, CXL); So I am struggling with why is this a notifier chain vs something simpler and more explicit, something like: typedef (int)(*cxl_cper_event_fn)(struct cper_cxl_event_rec *rec) int register_cxl_cper(cxl_cper_event_fn func) { guard(rwsem_write)(cxl_cper_rwsem); if (cxl_cper_event) return -EBUSY; cxl_cper_event = func; return 0; } ...do the reverse on unregister and hold the rwsem for read while invoking to hold off unregistration while event processing is in flight. There are a couple properties of a blocking notifier chain that are unwanted: chaining, only the CXL subsystem cares about seeing these records, and loss of type-safety, no need to redirect through a (void *) payload compared to a direct call. Overall makes the implementation more explicit. > diff --git a/drivers/firmware/efi/cper_cxl.h b/drivers/firmware/efi/cper_cxl.h > index 86bfcf7909ec..aa3d36493586 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/cxl-event.h> > + > /* CXL Protocol Error Section */ > #define CPER_SEC_CXL_PROT_ERR \ > GUID_INIT(0x80B9EFB4, 0x52B5, 0x4DE3, 0xA7, 0x77, 0x68, 0x78, \ > 0x4B, 0x77, 0x10, 0x48) I like these defines, I notice that mbox.c uses "static const" defintions for something similar. Perhaps unify on the #define method? I think this define also wants a _GUID suffix to reduce potential confusion between the _UUID variant and the cxl_event_log_type definitions? > > +/* CXL Event record UUIDs are formated at GUIDs and reported in 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/cxl-event.h b/include/linux/cxl-event.h > index 18dab4d90dc8..114f8abb7152 100644 > --- a/include/linux/cxl-event.h > +++ b/include/linux/cxl-event.h > @@ -108,4 +108,57 @@ struct cxl_event_record_raw { > union cxl_event event; > } __packed; > > +enum cxl_event_type { > + CXL_CPER_EVENT_GEN_MEDIA, > + CXL_CPER_EVENT_DRAM, > + CXL_CPER_EVENT_MEM_MODULE, > +}; > + > +#pragma pack(1) > + > +#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 cper_cxl_event_devid { > + 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 cper_cxl_event_sn { > + u32 lower_dw; > + u32 upper_dw; > + } dev_serial_num; > + } hdr; > + > + union cxl_event event; > +}; > + > +struct cxl_cper_notifier_data { > + enum cxl_event_type event_type; > + struct cper_cxl_event_rec *rec; > +}; > + > +#pragma pack() > + > +#ifdef CONFIG_UEFI_CPER > +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_CXL_EVENT_H */ > > -- > 2.42.0 >
Powered by blists - more mailing lists