[3/4] acpi/ghes, efi/cper: Recognize and process CXL Protocol Errors.
Commit Message
UEFI v2.10 section N.2.13 defines a CPER record for CXL Protocol errors.
Add GHES support to detect CXL CPER Protocol record and cache error
severity, device_id, serial number and a pointer to CXL RAS capability
struct in struct cxl_cper_rec_data.
Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
---
drivers/acpi/apei/ghes.c | 15 ++++++++++
drivers/firmware/efi/cper_cxl.c | 52 +++++++++++++++++++++++++++++++++
include/linux/cxl-event.h | 6 ++++
3 files changed, 73 insertions(+)
Comments
Smita Koralahalli wrote:
> UEFI v2.10 section N.2.13 defines a CPER record for CXL Protocol errors.
>
> Add GHES support to detect CXL CPER Protocol record and cache error
> severity, device_id, serial number and a pointer to CXL RAS capability
> struct in struct cxl_cper_rec_data.
>
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> ---
> drivers/acpi/apei/ghes.c | 15 ++++++++++
> drivers/firmware/efi/cper_cxl.c | 52 +++++++++++++++++++++++++++++++++
> include/linux/cxl-event.h | 6 ++++
> 3 files changed, 73 insertions(+)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index d6a85fbc0a8b..6471584b2e79 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -714,6 +714,18 @@ static void cxl_cper_post_event(enum cxl_event_type event_type,
> cper_callback(event_type, &data);
> }
>
> +void cxl_cper_handle_prot_err(struct acpi_hest_generic_data *gdata)
> +{
> + struct cxl_cper_rec_data data;
> +
> + memset(&data, 0, sizeof(data));
Does this need to be done? Could cxl_cper_handle_prot_err_info() use
something like this initially?
*data = (struct cxl_cper_rec_data) {
.rec.hdr.dev_serial_num.lower_dw = prot_err->dev_serial_num.lower_dw;
.rec.hdr.dev_serial_num.upper_dw = prot_err->dev_serial_num.upper_dw;
};
The serial number is always set even if it is not valid.
> +
> + if (cxl_cper_handle_prot_err_info(gdata, &data))
> + return;
> +
> + data.severity = gdata->error_severity;
> +}
> +
> int cxl_cper_register_callback(cxl_cper_callback callback)
> {
> guard(rwsem_write)(&cxl_cper_rw_sem);
> @@ -768,6 +780,9 @@ static bool ghes_do_proc(struct ghes *ghes,
> else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
> queued = ghes_handle_arm_hw_error(gdata, sev);
> }
> + else if (guid_equal(sec_type, &CPER_SEC_CXL_PROT_ERR)) {
> + cxl_cper_handle_prot_err(gdata);
> + }
> else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) {
> struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata);
>
> diff --git a/drivers/firmware/efi/cper_cxl.c b/drivers/firmware/efi/cper_cxl.c
> index 4fd8d783993e..3bc0b9f28c9e 100644
> --- a/drivers/firmware/efi/cper_cxl.c
> +++ b/drivers/firmware/efi/cper_cxl.c
> @@ -8,6 +8,7 @@
> */
>
> #include <linux/cper.h>
> +#include <acpi/ghes.h>
> #include "cper_cxl.h"
>
> #define PROT_ERR_VALID_AGENT_TYPE BIT_ULL(0)
> @@ -176,3 +177,54 @@ void cper_print_prot_err(const char *pfx, const struct cper_sec_prot_err *prot_e
> sizeof(cxl_ras->header_log), 0);
> }
> }
> +
> +int cxl_cper_handle_prot_err_info(struct acpi_hest_generic_data *gdata,
> + struct cxl_cper_rec_data *data)
> +{
> + struct cper_sec_prot_err *prot_err = acpi_hest_get_payload(gdata);
> + struct cper_cxl_event_devid *device_id = &data->rec.hdr.device_id;
> + struct cper_cxl_event_sn *dev_serial_num = &data->rec.hdr.dev_serial_num;
> + size_t size = sizeof(*prot_err) + prot_err->dvsec_len;
> +
> + if (!(prot_err->valid_bits & PROT_ERR_VALID_ERROR_LOG)) {
> + pr_err(FW_WARN "Not a valid protocol error log\n");
> + return -EINVAL;
> + }
> +
> + if (!(prot_err->valid_bits & PROT_ERR_VALID_DEVICE_ID)) {
> + pr_err(FW_WARN "Not a valid Device ID\n");
> + return -EINVAL;
> + }
> +
> + /*
> + * The device ID or agent address is only valid for CXL 1.1 device,
> + * CXL 2.0 device, CXL 2.0 Logical device, CXL 2.0 Fabric Manager
> + * Managed Logical Device, CXL Root Port, CXL Downstream Switch
> + * Port, or CXL Upstream Switch Port.
> + */
> + if (prot_err->agent_type <= 0x7 && prot_err->agent_type != RCH_DP) {
> + device_id->segment_num = prot_err->agent_addr.segment;
> + device_id->bus_num = prot_err->agent_addr.bus;
> + device_id->device_num = prot_err->agent_addr.device;
> + device_id->func_num = prot_err->agent_addr.function;
> + } else {
> + pr_err(FW_WARN "Not a valid agent type\n");
> + return -EINVAL;
> + }
> +
> + /*
> + * The device serial number is only valid for CXL 1.1 device, CXL
> + * 2.0 device, CXL 2.0 Logical device, or CXL 2.0 Fabric Manager
> + * Managed Logical Device.
> + */
> + if (!(prot_err->valid_bits & PROT_ERR_VALID_SERIAL_NUMBER) ||
> + prot_err->agent_type > 0x4 || prot_err->agent_type == RCH_DP)
> + pr_warn(FW_WARN "No valid serial number present\n");
> +
> + dev_serial_num->lower_dw = prot_err->dev_serial_num.lower_dw;
> + dev_serial_num->upper_dw = prot_err->dev_serial_num.upper_dw;
> +
> + data->cxl_ras = (struct cxl_ras_capability_regs *)((long)prot_err + size);
I think this is ok now because the cxl trace code processes the data
before returning (in next patch). But I'm a bit leary about passing a
pointer to data controlled by the acpi sub-system. At some point the
event may get cached to be processed by another thread and it might be
better to copy the struct here.
> +
> + return 0;
> +}
> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> index 90d8390a73cb..7ba2dfd6619e 100644
> --- a/include/linux/cxl-event.h
> +++ b/include/linux/cxl-event.h
> @@ -154,11 +154,17 @@ struct cxl_ras_capability_regs {
>
> struct cxl_cper_rec_data {
> struct cxl_cper_event_rec rec;
> + struct cxl_ras_capability_regs *cxl_ras;
> + int severity;
NIT: When I saw this without any addition to event type I wondered if the
series would bisect. I see it does because the record is not sent until
the next patch. But I wonder if the 2 patches would be best reversed.
Also, should cxl_ras + severity be put in a protocol error sub-struct?
Does severity ever apply to event records?
Ira
> };
>
> typedef void (*cxl_cper_callback)(enum cxl_event_type type,
> struct cxl_cper_rec_data *data);
>
> +struct acpi_hest_generic_data;
> +int cxl_cper_handle_prot_err_info(struct acpi_hest_generic_data *gdata,
> + struct cxl_cper_rec_data *data);
> +
> #ifdef CONFIG_ACPI_APEI_GHES
> int cxl_cper_register_callback(cxl_cper_callback callback);
> int cxl_cper_unregister_callback(cxl_cper_callback callback);
> --
> 2.17.1
>
On 1/2/2024 9:58 AM, Ira Weiny wrote:
> Smita Koralahalli wrote:
>> UEFI v2.10 section N.2.13 defines a CPER record for CXL Protocol errors.
>>
>> Add GHES support to detect CXL CPER Protocol record and cache error
>> severity, device_id, serial number and a pointer to CXL RAS capability
>> struct in struct cxl_cper_rec_data.
>>
>> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
>> ---
>> drivers/acpi/apei/ghes.c | 15 ++++++++++
>> drivers/firmware/efi/cper_cxl.c | 52 +++++++++++++++++++++++++++++++++
>> include/linux/cxl-event.h | 6 ++++
>> 3 files changed, 73 insertions(+)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index d6a85fbc0a8b..6471584b2e79 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -714,6 +714,18 @@ static void cxl_cper_post_event(enum cxl_event_type event_type,
>> cper_callback(event_type, &data);
>> }
>>
>> +void cxl_cper_handle_prot_err(struct acpi_hest_generic_data *gdata)
>> +{
>> + struct cxl_cper_rec_data data;
>> +
>> + memset(&data, 0, sizeof(data));
>
> Does this need to be done? Could cxl_cper_handle_prot_err_info() use
> something like this initially?
>
> *data = (struct cxl_cper_rec_data) {
> .rec.hdr.dev_serial_num.lower_dw = prot_err->dev_serial_num.lower_dw;
> .rec.hdr.dev_serial_num.upper_dw = prot_err->dev_serial_num.upper_dw;
> };
>
> The serial number is always set even if it is not valid.
Ok will copy serial number at the beginning of
cxl_cper_handle_prot_err_info() and remove memset.
>
>> +
>> + if (cxl_cper_handle_prot_err_info(gdata, &data))
>> + return;
>> +
>> + data.severity = gdata->error_severity;
>> +}
>> +
>> int cxl_cper_register_callback(cxl_cper_callback callback)
>> {
>> guard(rwsem_write)(&cxl_cper_rw_sem);
>> @@ -768,6 +780,9 @@ static bool ghes_do_proc(struct ghes *ghes,
>> else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
>> queued = ghes_handle_arm_hw_error(gdata, sev);
>> }
>> + else if (guid_equal(sec_type, &CPER_SEC_CXL_PROT_ERR)) {
>> + cxl_cper_handle_prot_err(gdata);
>> + }
>> else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) {
>> struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata);
>>
>> diff --git a/drivers/firmware/efi/cper_cxl.c b/drivers/firmware/efi/cper_cxl.c
>> index 4fd8d783993e..3bc0b9f28c9e 100644
>> --- a/drivers/firmware/efi/cper_cxl.c
>> +++ b/drivers/firmware/efi/cper_cxl.c
>> @@ -8,6 +8,7 @@
>> */
>>
>> #include <linux/cper.h>
>> +#include <acpi/ghes.h>
>> #include "cper_cxl.h"
>>
>> #define PROT_ERR_VALID_AGENT_TYPE BIT_ULL(0)
>> @@ -176,3 +177,54 @@ void cper_print_prot_err(const char *pfx, const struct cper_sec_prot_err *prot_e
>> sizeof(cxl_ras->header_log), 0);
>> }
>> }
>> +
>> +int cxl_cper_handle_prot_err_info(struct acpi_hest_generic_data *gdata,
>> + struct cxl_cper_rec_data *data)
>> +{
>> + struct cper_sec_prot_err *prot_err = acpi_hest_get_payload(gdata);
>> + struct cper_cxl_event_devid *device_id = &data->rec.hdr.device_id;
>> + struct cper_cxl_event_sn *dev_serial_num = &data->rec.hdr.dev_serial_num;
>> + size_t size = sizeof(*prot_err) + prot_err->dvsec_len;
>> +
>> + if (!(prot_err->valid_bits & PROT_ERR_VALID_ERROR_LOG)) {
>> + pr_err(FW_WARN "Not a valid protocol error log\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (!(prot_err->valid_bits & PROT_ERR_VALID_DEVICE_ID)) {
>> + pr_err(FW_WARN "Not a valid Device ID\n");
>> + return -EINVAL;
>> + }
>> +
>> + /*
>> + * The device ID or agent address is only valid for CXL 1.1 device,
>> + * CXL 2.0 device, CXL 2.0 Logical device, CXL 2.0 Fabric Manager
>> + * Managed Logical Device, CXL Root Port, CXL Downstream Switch
>> + * Port, or CXL Upstream Switch Port.
>> + */
>> + if (prot_err->agent_type <= 0x7 && prot_err->agent_type != RCH_DP) {
>> + device_id->segment_num = prot_err->agent_addr.segment;
>> + device_id->bus_num = prot_err->agent_addr.bus;
>> + device_id->device_num = prot_err->agent_addr.device;
>> + device_id->func_num = prot_err->agent_addr.function;
>> + } else {
>> + pr_err(FW_WARN "Not a valid agent type\n");
>> + return -EINVAL;
>> + }
>> +
>> + /*
>> + * The device serial number is only valid for CXL 1.1 device, CXL
>> + * 2.0 device, CXL 2.0 Logical device, or CXL 2.0 Fabric Manager
>> + * Managed Logical Device.
>> + */
>> + if (!(prot_err->valid_bits & PROT_ERR_VALID_SERIAL_NUMBER) ||
>> + prot_err->agent_type > 0x4 || prot_err->agent_type == RCH_DP)
>> + pr_warn(FW_WARN "No valid serial number present\n");
>> +
>> + dev_serial_num->lower_dw = prot_err->dev_serial_num.lower_dw;
>> + dev_serial_num->upper_dw = prot_err->dev_serial_num.upper_dw;
>> +
>> + data->cxl_ras = (struct cxl_ras_capability_regs *)((long)prot_err + size);
>
> I think this is ok now because the cxl trace code processes the data
> before returning (in next patch). But I'm a bit leary about passing a
> pointer to data controlled by the acpi sub-system. At some point the
> event may get cached to be processed by another thread and it might be
> better to copy the struct here.
Ok will make the changes. Rewrote the struct in patch 1.
>
>> +
>> + return 0;
>> +}
>> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
>> index 90d8390a73cb..7ba2dfd6619e 100644
>> --- a/include/linux/cxl-event.h
>> +++ b/include/linux/cxl-event.h
>> @@ -154,11 +154,17 @@ struct cxl_ras_capability_regs {
>>
>> struct cxl_cper_rec_data {
>> struct cxl_cper_event_rec rec;
>> + struct cxl_ras_capability_regs *cxl_ras;
>> + int severity;
>
> NIT: When I saw this without any addition to event type I wondered if the
> series would bisect. I see it does because the record is not sent until
> the next patch. But I wonder if the 2 patches would be best reversed.
You mean to say patch 4 to be 3 and 3 to be 4?
And since I haven't used severity yet, fix it by declaring severity to
where it is used..
>
> Also, should cxl_ras + severity be put in a protocol error sub-struct?
> Does severity ever apply to event records?
No, not in any of the component event records. Only place is in "Event
Record Flags" in Common Event Record Format (Table 8-42).
Addressed in patch 1 to make a sub-struct.
Thanks,
Smita
>
> Ira
>
>> };
>>
>> typedef void (*cxl_cper_callback)(enum cxl_event_type type,
>> struct cxl_cper_rec_data *data);
>>
>> +struct acpi_hest_generic_data;
>> +int cxl_cper_handle_prot_err_info(struct acpi_hest_generic_data *gdata,
>> + struct cxl_cper_rec_data *data);
>> +
>> #ifdef CONFIG_ACPI_APEI_GHES
>> int cxl_cper_register_callback(cxl_cper_callback callback);
>> int cxl_cper_unregister_callback(cxl_cper_callback callback);
>> --
>> 2.17.1
>>
>
>
Smita Koralahalli wrote:
> On 1/2/2024 9:58 AM, Ira Weiny wrote:
> > Smita Koralahalli wrote:
[snip]
> >> +
> >> + return 0;
> >> +}
> >> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> >> index 90d8390a73cb..7ba2dfd6619e 100644
> >> --- a/include/linux/cxl-event.h
> >> +++ b/include/linux/cxl-event.h
> >> @@ -154,11 +154,17 @@ struct cxl_ras_capability_regs {
> >>
> >> struct cxl_cper_rec_data {
> >> struct cxl_cper_event_rec rec;
> >> + struct cxl_ras_capability_regs *cxl_ras;
> >> + int severity;
> >
> > NIT: When I saw this without any addition to event type I wondered if the
> > series would bisect. I see it does because the record is not sent until
> > the next patch. But I wonder if the 2 patches would be best reversed.
>
> You mean to say patch 4 to be 3 and 3 to be 4?
>
> And since I haven't used severity yet, fix it by declaring severity to
> where it is used..
Yes. What you have is not technically wrong but it threw me in review.
>
> >
> > Also, should cxl_ras + severity be put in a protocol error sub-struct?
> > Does severity ever apply to event records?
>
> No, not in any of the component event records. Only place is in "Event
> Record Flags" in Common Event Record Format (Table 8-42).
>
> Addressed in patch 1 to make a sub-struct.
Thanks!
Ira
@@ -714,6 +714,18 @@ static void cxl_cper_post_event(enum cxl_event_type event_type,
cper_callback(event_type, &data);
}
+void cxl_cper_handle_prot_err(struct acpi_hest_generic_data *gdata)
+{
+ struct cxl_cper_rec_data data;
+
+ memset(&data, 0, sizeof(data));
+
+ if (cxl_cper_handle_prot_err_info(gdata, &data))
+ return;
+
+ data.severity = gdata->error_severity;
+}
+
int cxl_cper_register_callback(cxl_cper_callback callback)
{
guard(rwsem_write)(&cxl_cper_rw_sem);
@@ -768,6 +780,9 @@ static bool ghes_do_proc(struct ghes *ghes,
else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
queued = ghes_handle_arm_hw_error(gdata, sev);
}
+ else if (guid_equal(sec_type, &CPER_SEC_CXL_PROT_ERR)) {
+ cxl_cper_handle_prot_err(gdata);
+ }
else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) {
struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata);
@@ -8,6 +8,7 @@
*/
#include <linux/cper.h>
+#include <acpi/ghes.h>
#include "cper_cxl.h"
#define PROT_ERR_VALID_AGENT_TYPE BIT_ULL(0)
@@ -176,3 +177,54 @@ void cper_print_prot_err(const char *pfx, const struct cper_sec_prot_err *prot_e
sizeof(cxl_ras->header_log), 0);
}
}
+
+int cxl_cper_handle_prot_err_info(struct acpi_hest_generic_data *gdata,
+ struct cxl_cper_rec_data *data)
+{
+ struct cper_sec_prot_err *prot_err = acpi_hest_get_payload(gdata);
+ struct cper_cxl_event_devid *device_id = &data->rec.hdr.device_id;
+ struct cper_cxl_event_sn *dev_serial_num = &data->rec.hdr.dev_serial_num;
+ size_t size = sizeof(*prot_err) + prot_err->dvsec_len;
+
+ if (!(prot_err->valid_bits & PROT_ERR_VALID_ERROR_LOG)) {
+ pr_err(FW_WARN "Not a valid protocol error log\n");
+ return -EINVAL;
+ }
+
+ if (!(prot_err->valid_bits & PROT_ERR_VALID_DEVICE_ID)) {
+ pr_err(FW_WARN "Not a valid Device ID\n");
+ return -EINVAL;
+ }
+
+ /*
+ * The device ID or agent address is only valid for CXL 1.1 device,
+ * CXL 2.0 device, CXL 2.0 Logical device, CXL 2.0 Fabric Manager
+ * Managed Logical Device, CXL Root Port, CXL Downstream Switch
+ * Port, or CXL Upstream Switch Port.
+ */
+ if (prot_err->agent_type <= 0x7 && prot_err->agent_type != RCH_DP) {
+ device_id->segment_num = prot_err->agent_addr.segment;
+ device_id->bus_num = prot_err->agent_addr.bus;
+ device_id->device_num = prot_err->agent_addr.device;
+ device_id->func_num = prot_err->agent_addr.function;
+ } else {
+ pr_err(FW_WARN "Not a valid agent type\n");
+ return -EINVAL;
+ }
+
+ /*
+ * The device serial number is only valid for CXL 1.1 device, CXL
+ * 2.0 device, CXL 2.0 Logical device, or CXL 2.0 Fabric Manager
+ * Managed Logical Device.
+ */
+ if (!(prot_err->valid_bits & PROT_ERR_VALID_SERIAL_NUMBER) ||
+ prot_err->agent_type > 0x4 || prot_err->agent_type == RCH_DP)
+ pr_warn(FW_WARN "No valid serial number present\n");
+
+ dev_serial_num->lower_dw = prot_err->dev_serial_num.lower_dw;
+ dev_serial_num->upper_dw = prot_err->dev_serial_num.upper_dw;
+
+ data->cxl_ras = (struct cxl_ras_capability_regs *)((long)prot_err + size);
+
+ return 0;
+}
@@ -154,11 +154,17 @@ struct cxl_ras_capability_regs {
struct cxl_cper_rec_data {
struct cxl_cper_event_rec rec;
+ struct cxl_ras_capability_regs *cxl_ras;
+ int severity;
};
typedef void (*cxl_cper_callback)(enum cxl_event_type type,
struct cxl_cper_rec_data *data);
+struct acpi_hest_generic_data;
+int cxl_cper_handle_prot_err_info(struct acpi_hest_generic_data *gdata,
+ struct cxl_cper_rec_data *data);
+
#ifdef CONFIG_ACPI_APEI_GHES
int cxl_cper_register_callback(cxl_cper_callback callback);
int cxl_cper_unregister_callback(cxl_cper_callback callback);