2023-10-13 06:55:38

by Ira Weiny

[permalink] [raw]
Subject: [PATCH RFC 0/2] efi/cxl-cper: Report CPER CXL component events through trace events

I know that Smita has taken the initiative[1] on this but I had a
skeleton of using notifiers to allow the CXL code to process the CPER
records via the standard tracing code like Dan mentioned.[2]

So here is a slightly polished version of that code. This is compile
tested with only. Smita, feel free to use this any way you see fit.

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 these CPER event
records can be processed instead of reading the CXL Event logs directly
from the device.

A number of alternatives were considered to match the memdev with the
CPER record. For now a simple comparison with the serial number is used
to match a CPER record with a specific device. Other fields in the CPER
record could be used as well.

[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/

Signed-off-by: Ira Weiny <[email protected]>
---
Ira Weiny (2):
firmware/efi: Process CXL Component Events
cxl/memdev: Register for and process CPER events

drivers/cxl/core/mbox.c | 7 +++--
drivers/cxl/cxlmem.h | 5 +++
drivers/cxl/pci.c | 70 ++++++++++++++++++++++++++++++++++++++++-
drivers/firmware/efi/cper.c | 16 ++++++++++
drivers/firmware/efi/cper_cxl.c | 39 +++++++++++++++++++++++
drivers/firmware/efi/cper_cxl.h | 29 +++++++++++++++++
include/linux/efi.h | 49 +++++++++++++++++++++++++++++
7 files changed, 211 insertions(+), 4 deletions(-)
---
base-commit: 1c8b86a3799f7e5be903c3f49fcdaee29fd385b5
change-id: 20230601-cxl-cper-26ffc839c6c6

Best regards,
--
Ira Weiny <[email protected]>


2023-10-13 06:56:20

by Ira Weiny

[permalink] [raw]
Subject: [PATCH RFC 1/2] firmware/efi: Process CXL Component Events

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 must be converted to a UUID prior to being used by the
CXL code. Make that conversion and send it along to those registered.

Not-Yet-Signed-off-by: Ira Weiny <[email protected]>

---
RFC comments:
I'm still not sure if the 'CXL Component Event Log' portion of the CPER
record includes the UUID from the CXL Common Event Record Format.

The 2.10 version of the UEFI spec says:

"For the CXL Component Event Log: Refer to the Common Event Record field
(Offset 16) of the Events Record Format for each CXL component."

This implies that the first 16 bytes (the UUID) is not included. It
would be nice if the UUID were included there as a copy of what would
normally be in the CXL event log. If so the Section Type GUID could be
used solely to match for a CXL record and ignored in the CXL code.

For now convert the GUID to UUID and pass to the notifier users.

Smita, Another idea I had was to add your cper_print_comp_event()
function to the dump here to capture that CPER specific data in the EFI
log.
---
drivers/firmware/efi/cper.c | 16 ++++++++++++++
drivers/firmware/efi/cper_cxl.c | 39 ++++++++++++++++++++++++++++++++
drivers/firmware/efi/cper_cxl.h | 29 ++++++++++++++++++++++++
include/linux/efi.h | 49 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 133 insertions(+)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 35c37f667781..af2c59f5e21d 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, &gen_media_event_guid) ||
+ guid_equal(sec_type, &dram_event_guid) ||
+ guid_equal(sec_type, &mem_mod_event_guid)) {
+ struct cper_cxl_event_rec *rec = acpi_hest_get_payload(gdata);
+
+ printk("%ssection type: CXL Event\n", newpfx);
+
+ 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..9b1a64ed542e 100644
--- a/drivers/firmware/efi/cper_cxl.c
+++ b/drivers/firmware/efi/cper_cxl.c
@@ -187,3 +187,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 *guid, struct cper_cxl_event_rec *rec)
+{
+ struct cxl_cper_notifier_data nd = {
+ .rec = rec,
+ };
+ char guid_str[UUID_STRING_LEN + 1]; /* + trailing NULL */
+
+ if (!(rec->hdr.validation_bits & CPER_CXL_COMP_EVENT_LOG_VALID)) {
+ pr_err(FW_WARN "cxl event no Compoent Event Log present\n");
+ return;
+ }
+
+ snprintf(guid_str, UUID_STRING_LEN + 1, "%pU", guid);
+ if (uuid_parse(guid_str, &nd.uuid))
+ pr_err(FW_WARN "cxl event uuid conversion failed\n");
+
+ pr_info("%s cxl event guid %pU, uuid %pU\n", pfx, guid, &nd.uuid);
+
+ 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);
+
+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..47fc83dad8d2 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
+ */
+static const guid_t gen_media_event_guid =
+ 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
+ */
+static const guid_t dram_event_guid =
+ 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
+ */
+static const guid_t mem_mod_event_guid =
+ 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 *guid,
+ struct cper_cxl_event_rec *rec);

#endif //__CPER_CXL_
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 80b21d1c6eaf..eb03962b2547 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1355,4 +1355,53 @@ 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);

+/*
+ * CXL r3.0 Table 8-42
+ * 30h + 50h - 10h (For lack of UUID)
+ */
+#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];
+};
+#define CPER_CXL_REC_LEN(rec) (rec->hdr.length - sizeof(rec->hdr))
+
+struct cxl_cper_notifier_data {
+ uuid_t uuid;
+ 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

2023-10-13 06:56:29

by Ira Weiny

[permalink] [raw]
Subject: [PATCH RFC 2/2] cxl/memdev: Register for and process CPER events

If the firmware has configured CXL event support to be firmware first
the OS can process those events through CPER records.

Detect firmware first configuration and register a notifier callback to
process catch records for this memdev. Process those records destined
for this memdev through the normal trace mechanism.

Not-Yet-Signed-off-by: Ira Weiny <[email protected]>

---
RFC comments:
The matching of the CPER event to the MDS is a bit hacky right now and
could probably be much more robust. But the general approach seems
sound. Simply register a notifier for each device and when that device
finds a record for itself call the normal trace mechanisms.
---
drivers/cxl/core/mbox.c | 7 ++---
drivers/cxl/cxlmem.h | 5 ++++
drivers/cxl/pci.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++-
3 files changed, 78 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 4df4f614f490..3a8ce7801e04 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -860,9 +860,9 @@ static const uuid_t mem_mod_event_uuid =
UUID_INIT(0xfe927475, 0xdd59, 0x4339,
0xa5, 0x86, 0x79, 0xba, 0xb1, 0x13, 0xb7, 0x74);

-static void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
- enum cxl_event_log_type type,
- struct cxl_event_record_raw *record)
+void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
+ enum cxl_event_log_type type,
+ struct cxl_event_record_raw *record)
{
uuid_t *id = &record->hdr.id;

@@ -885,6 +885,7 @@ static void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
trace_cxl_generic_event(cxlmd, type, record);
}
}
+EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL);

static int cxl_clear_event_record(struct cxl_memdev_state *mds,
enum cxl_event_log_type log,
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 706f8a6d1ef4..2b4210c291b9 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -477,6 +477,8 @@ struct cxl_memdev_state {
struct cxl_security_state security;
struct cxl_fw_state fw;

+ struct notifier_block cxl_cper_nb;
+
struct rcuwait mbox_wait;
int (*mbox_send)(struct cxl_memdev_state *mds,
struct cxl_mbox_cmd *cmd);
@@ -863,6 +865,9 @@ void set_exclusive_cxl_commands(struct cxl_memdev_state *mds,
void clear_exclusive_cxl_commands(struct cxl_memdev_state *mds,
unsigned long *cmds);
void cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status);
+void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
+ enum cxl_event_log_type type,
+ struct cxl_event_record_raw *record);
int cxl_set_timestamp(struct cxl_memdev_state *mds);
int cxl_poison_state_init(struct cxl_memdev_state *mds);
int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 44a21ab7add5..19922e32c098 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/* Copyright(c) 2020 Intel Corporation. All rights reserved. */
+#include <asm-generic/unaligned.h>
#include <linux/io-64-nonatomic-lo-hi.h>
#include <linux/moduleparam.h>
#include <linux/module.h>
@@ -10,6 +11,7 @@
#include <linux/pci.h>
#include <linux/aer.h>
#include <linux/io.h>
+#include <linux/efi.h>
#include "cxlmem.h"
#include "cxlpci.h"
#include "cxl.h"
@@ -748,6 +750,70 @@ static bool cxl_event_int_is_fw(u8 setting)
return mode == CXL_INT_FW;
}

+#define CXL_EVENT_HDR_FLAGS_REC_SEVERITY GENMASK(1, 0)
+int cxl_cper_event(struct notifier_block *nb, unsigned long action, void *data)
+{
+ struct cxl_cper_notifier_data *nd = data;
+ struct cxl_event_record_raw record;
+ enum cxl_event_log_type log_type;
+ struct cxl_memdev_state *mds;
+ u32 hdr_flags;
+
+ mds = container_of(nb, struct cxl_memdev_state, cxl_cper_nb);
+
+ /* Need serial number for device identification */
+ if (!(nd->rec->hdr.validation_bits & CPER_CXL_DEVICE_SN_VALID))
+ return NOTIFY_DONE;
+
+ /* FIXME endianess and bytes of serial number need verification */
+ /* FIXME Should other values be checked? */
+ if (memcmp(&mds->cxlds.serial, &nd->rec->hdr.dev_serial_num,
+ sizeof(mds->cxlds.serial)))
+ return NOTIFY_DONE;
+
+ /*
+ * UEFI v2.10 defines N.2.14 defines the CXL CPER record as not
+ * including the uuid field from the CXL record.
+ *
+ * Build the record from the UUID passed.
+ */
+ record = (struct cxl_event_record_raw) {
+ .hdr.id = nd->uuid,
+ };
+ memcpy(&record.hdr.length, &nd->rec->comp_event_log,
+ CPER_CXL_REC_LEN(nd->rec));
+
+ /* ensure record can always handle the full CPER provided data */
+ BUILD_BUG_ON(sizeof(record) <
+ (CPER_CXL_COMP_EVENT_LOG_SIZE + sizeof(record.hdr.id)));
+
+ hdr_flags = get_unaligned_le24(record.hdr.flags);
+ log_type = FIELD_GET(CXL_EVENT_HDR_FLAGS_REC_SEVERITY, hdr_flags);
+
+ cxl_event_trace_record(mds->cxlds.cxlmd, log_type, &record);
+
+ return NOTIFY_OK;
+}
+
+static void cxl_unregister_cper_events(void *_mds)
+{
+ struct cxl_memdev_state *mds = _mds;
+
+ unregister_cxl_cper_notifier(&mds->cxl_cper_nb);
+}
+
+static void register_cper_events(struct cxl_memdev_state *mds)
+{
+ mds->cxl_cper_nb.notifier_call = cxl_cper_event;
+
+ if (register_cxl_cper_notifier(&mds->cxl_cper_nb)) {
+ dev_err(mds->cxlds.dev, "CPER registration failed\n");
+ return;
+ }
+
+ devm_add_action_or_reset(mds->cxlds.dev, cxl_unregister_cper_events, mds);
+}
+
static int cxl_event_config(struct pci_host_bridge *host_bridge,
struct cxl_memdev_state *mds)
{
@@ -758,8 +824,10 @@ static int cxl_event_config(struct pci_host_bridge *host_bridge,
* When BIOS maintains CXL error reporting control, it will process
* event records. Only one agent can do so.
*/
- if (!host_bridge->native_cxl_error)
+ if (!host_bridge->native_cxl_error) {
+ register_cper_events(mds);
return 0;
+ }

rc = cxl_mem_alloc_event_buf(mds);
if (rc)

--
2.41.0

2023-10-18 02:38:32

by Ira Weiny

[permalink] [raw]
Subject: Re: [PATCH RFC 1/2] 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 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 must be converted to a UUID prior to being used by the
> CXL code. Make that conversion and send it along to those registered.
>
> Not-Yet-Signed-off-by: Ira Weiny <[email protected]>
>
> ---
> RFC comments:
> I'm still not sure if the 'CXL Component Event Log' portion of the CPER
> record includes the UUID from the CXL Common Event Record Format.
>
> The 2.10 version of the UEFI spec says:
>
> "For the CXL Component Event Log: Refer to the Common Event Record field
> (Offset 16) of the Events Record Format for each CXL component."
>
> This implies that the first 16 bytes (the UUID) is not included. It
> would be nice if the UUID were included there as a copy of what would
> normally be in the CXL event log. If so the Section Type GUID could be
> used solely to match for a CXL record and ignored in the CXL code.
>
> For now convert the GUID to UUID and pass to the notifier users.
>
> Smita, Another idea I had was to add your cper_print_comp_event()
> function to the dump here to capture that CPER specific data in the EFI
> log.
> ---
> drivers/firmware/efi/cper.c | 16 ++++++++++++++
> drivers/firmware/efi/cper_cxl.c | 39 ++++++++++++++++++++++++++++++++
> drivers/firmware/efi/cper_cxl.h | 29 ++++++++++++++++++++++++
> include/linux/efi.h | 49 +++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 133 insertions(+)
>
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 35c37f667781..af2c59f5e21d 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, &gen_media_event_guid) ||
> + guid_equal(sec_type, &dram_event_guid) ||
> + guid_equal(sec_type, &mem_mod_event_guid)) {
> + struct cper_cxl_event_rec *rec = acpi_hest_get_payload(gdata);
> +

Smita,

Dan and I were discussing the processing of these GUIDs vs the UUIDs in the
trace prints offline.

Here we could separate out the comparisons and use a token for the events
rather than passing the guid through to be converted into a uuid...

> + printk("%ssection type: CXL Event\n", newpfx);
> +
> + 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..9b1a64ed542e 100644
> --- a/drivers/firmware/efi/cper_cxl.c
> +++ b/drivers/firmware/efi/cper_cxl.c
> @@ -187,3 +187,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 *guid, struct cper_cxl_event_rec *rec)
> +{
> + struct cxl_cper_notifier_data nd = {
> + .rec = rec,
> + };
> + char guid_str[UUID_STRING_LEN + 1]; /* + trailing NULL */
> +
> + if (!(rec->hdr.validation_bits & CPER_CXL_COMP_EVENT_LOG_VALID)) {
> + pr_err(FW_WARN "cxl event no Compoent Event Log present\n");
> + return;
> + }
> +
> + snprintf(guid_str, UUID_STRING_LEN + 1, "%pU", guid);
> + if (uuid_parse(guid_str, &nd.uuid))
> + pr_err(FW_WARN "cxl event uuid conversion failed\n");

> +
> + pr_info("%s cxl event guid %pU, uuid %pU\n", pfx, guid, &nd.uuid);
> +
> + if (blocking_notifier_call_chain(&cxl_cper_chain_head, 0, (void *)&nd)
> + == NOTIFY_BAD)
> + pr_err(FW_WARN "cxl event notifier chain failed\n");
> +}
> +

2023-10-18 02:42:16

by Ira Weiny

[permalink] [raw]
Subject: Re: [PATCH RFC 1/2] firmware/efi: Process CXL Component Events

Ira Weiny wrote:
>

[snip]

> ---
> RFC comments:
> I'm still not sure if the 'CXL Component Event Log' portion of the CPER
> record includes the UUID from the CXL Common Event Record Format.
>
> The 2.10 version of the UEFI spec says:
>
> "For the CXL Component Event Log: Refer to the Common Event Record field
> (Offset 16) of the Events Record Format for each CXL component."
>
> This implies that the first 16 bytes (the UUID) is not included. It
> would be nice if the UUID were included there as a copy of what would
> normally be in the CXL event log. If so the Section Type GUID could be
> used solely to match for a CXL record and ignored in the CXL code.
>
> For now convert the GUID to UUID and pass to the notifier users.

Smita,

Dan and I were discussing the processing of these GUIDs vs the UUIDs offline.

>
> Smita, Another idea I had was to add your cper_print_comp_event()
> function to the dump here to capture that CPER specific data in the EFI
> log.
> ---
> drivers/firmware/efi/cper.c | 16 ++++++++++++++
> drivers/firmware/efi/cper_cxl.c | 39 ++++++++++++++++++++++++++++++++
> drivers/firmware/efi/cper_cxl.h | 29 ++++++++++++++++++++++++
> include/linux/efi.h | 49 +++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 133 insertions(+)
>
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 35c37f667781..af2c59f5e21d 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, &gen_media_event_guid) ||
> + guid_equal(sec_type, &dram_event_guid) ||
> + guid_equal(sec_type, &mem_mod_event_guid)) {
> + struct cper_cxl_event_rec *rec = acpi_hest_get_payload(gdata);
> +

Here we could separate out the comparisons and use a token for the event types
rather than passing the guid through to be converted into a uuid...

> + printk("%ssection type: CXL Event\n", newpfx);
> +
> + 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..9b1a64ed542e 100644
> --- a/drivers/firmware/efi/cper_cxl.c
> +++ b/drivers/firmware/efi/cper_cxl.c
> @@ -187,3 +187,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 *guid, struct cper_cxl_event_rec *rec)
> +{
> + struct cxl_cper_notifier_data nd = {
> + .rec = rec,
> + };
> + char guid_str[UUID_STRING_LEN + 1]; /* + trailing NULL */
> +
> + if (!(rec->hdr.validation_bits & CPER_CXL_COMP_EVENT_LOG_VALID)) {
> + pr_err(FW_WARN "cxl event no Compoent Event Log present\n");
> + return;
> + }
> +
> + snprintf(guid_str, UUID_STRING_LEN + 1, "%pU", guid);
> + if (uuid_parse(guid_str, &nd.uuid))
> + pr_err(FW_WARN "cxl event uuid conversion failed\n");

Thus this conversion is unnecessary.

> +
> + pr_info("%s cxl event guid %pU, uuid %pU\n", pfx, guid, &nd.uuid);
> +
> + if (blocking_notifier_call_chain(&cxl_cper_chain_head, 0, (void *)&nd)
> + == NOTIFY_BAD)
> + pr_err(FW_WARN "cxl event notifier chain failed\n");
> +}
> +

But this will mean that the trace events will have no uuid printed (probably
not a big deal as the trace point itself has that information).

(This would change the 2nd patch as it would no longer be getting a uuid.)

Thoughts?

Ira


[snip]

2023-10-18 02:43:55

by Ira Weiny

[permalink] [raw]
Subject: Re: [PATCH RFC 1/2] firmware/efi: Process CXL Component Events

Ira Weiny wrote:
> Ira Weiny wrote:

[snip]

> >
> > diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> > index 35c37f667781..af2c59f5e21d 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, &gen_media_event_guid) ||
> > + guid_equal(sec_type, &dram_event_guid) ||
> > + guid_equal(sec_type, &mem_mod_event_guid)) {
> > + struct cper_cxl_event_rec *rec = acpi_hest_get_payload(gdata);
> > +
>
> Smita,
>
> Dan and I were discussing the processing of these GUIDs vs the UUIDs in the
> trace prints offline.
>
> Here we could separate out the comparisons and use a token for the events
> rather than passing the guid through to be converted into a uuid...
>

Sorry I hit send too soon... Please see my other post.

Ira

[snip]