| 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