| 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
| ||
|
Date: Wed, 22 May 2024 10:59:45 -0700
From: Dave Jiang <dave.jiang@...el.com>
To: Smita Koralahalli <Smita.KoralahalliChannabasappa@....com>,
linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-cxl@...r.kernel.org
Cc: Ard Biesheuvel <ardb@...nel.org>,
Alison Schofield <alison.schofield@...el.com>,
Vishal Verma <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>,
Dan Williams <dan.j.williams@...el.com>,
Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Yazen Ghannam <yazen.ghannam@....com>, Bowman Terry <terry.bowman@....com>
Subject: Re: [PATCH 2/4] acpi/ghes, efi/cper: Recognize and process CXL
Protocol Errors.
On 5/22/24 8:08 AM, 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 Error Record and Cache Error
> Severity, Device ID, Device Serial number and CXL RAS capability struct in
> struct cxl_cper_prot_err. Include this struct as a member of struct
> cxl_cper_work_data.
>
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@....com>
> ---
> drivers/acpi/apei/ghes.c | 10 +++++
> drivers/firmware/efi/cper_cxl.c | 66 +++++++++++++++++++++++++++++++++
> include/linux/cxl-event.h | 26 +++++++++++++
> 3 files changed, 102 insertions(+)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 623cc0cb4a65..1a58032770ee 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -717,6 +717,14 @@ static void cxl_cper_post_event(enum cxl_event_type event_type,
> schedule_work(cxl_cper_work);
> }
>
> +static void cxl_cper_handle_prot_err(struct acpi_hest_generic_data *gdata)
> +{
> + struct cxl_cper_work_data wd;
> +
> + if (cxl_cper_handle_prot_err_info(gdata, &wd.p_err))
> + return;
> +}
> +
> int cxl_cper_register_work(struct work_struct *work)
> {
> if (cxl_cper_work)
> @@ -791,6 +799,8 @@ static bool ghes_do_proc(struct ghes *ghes,
> struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata);
>
> cxl_cper_post_event(CXL_CPER_EVENT_MEM_MODULE, rec);
> + } else if (guid_equal(sec_type, &CPER_SEC_CXL_PROT_ERR)) {
> + cxl_cper_handle_prot_err(gdata);
> } else {
> void *err = acpi_hest_get_payload(gdata);
>
> diff --git a/drivers/firmware/efi/cper_cxl.c b/drivers/firmware/efi/cper_cxl.c
> index 4fd8d783993e..03b9839f3b73 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)
> @@ -44,6 +45,17 @@ enum {
> USP, /* CXL Upstream Switch Port */
> };
>
> +static enum cxl_aer_err_type cper_severity_cxl_aer(int cper_severity)
> +{
> + switch (cper_severity) {
> + case CPER_SEV_RECOVERABLE:
> + case CPER_SEV_FATAL:
> + return CXL_AER_UNCORRECTABLE;
> + default:
> + return CXL_AER_CORRECTABLE;
> + }
> +}
> +
> void cper_print_prot_err(const char *pfx, const struct cper_sec_prot_err *prot_err)
> {
> if (prot_err->valid_bits & PROT_ERR_VALID_AGENT_TYPE)
> @@ -176,3 +188,57 @@ 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_prot_err *p_err)
> +{
> + struct cper_sec_prot_err *prot_err = acpi_hest_get_payload(gdata);
> + u8 *dvsec_start, *cap_start;
> +
> + if (!(prot_err->valid_bits & PROT_ERR_VALID_DEVICE_ID)) {
> + pr_err(FW_WARN "No Device ID\n");
> + return -EINVAL;
> + }
> +
> + /*
> + * The device ID or agent address is required for CXL RCD, CXL
> + * SLD, CXL LD, CXL Fabric Manager Managed LD, CXL Root Port,
> + * CXL Downstream Switch Port and CXL Upstream Switch Port.
> + */
> + if (prot_err->agent_type <= 0x7 && prot_err->agent_type != RCH_DP) {
Perhaps define an enum CXL_AGENT_TYPE_MAX instead of 0x7 magic number? Otherwise if a new type is introduced, it would break this code.
> + p_err->segment = prot_err->agent_addr.segment;
> + p_err->bus = prot_err->agent_addr.bus;
> + p_err->device = prot_err->agent_addr.device;
> + p_err->function = prot_err->agent_addr.function;
> + } else {
> + pr_err(FW_WARN "Invalid agent type\n");
> + return -EINVAL;
> + }
Up to you if you want to do this or not, but maybe:
if (prot_err->agent_type >= CXL_AGENT_TYPE_MAX || prot_err->agent_type == RCH_DP) {
pr_warn(...);
return -EINVAL;
}
p_err->segment = ...;
p_err->bus = ...;
...
Although perhaps a helper function cxl_cper_valid_agent_type() that checks invalid agent type by checking the valid_bits, the agent_type boundary, and if agent_type != RCH_DP?
> +
> + if (!(prot_err->valid_bits & PROT_ERR_VALID_ERROR_LOG)) {
> + pr_err(FW_WARN "Invalid Protocol Error log\n");
> + return -EINVAL;
> + }
> +
> + dvsec_start = (u8 *)(prot_err + 1);
> + cap_start = dvsec_start + prot_err->dvsec_len;
> + p_err->cxl_ras = *(struct cxl_ras_capability_regs *)cap_start;
> +
> + /*
> + * Set device serial number unconditionally.
> + *
> + * Print a warning message if it is not valid. The device serial
> + * number is required for CXL RCD, CXL SLD, CXL LD and CXL Fabric
> + * Manager Managed LD.
> + */
> + if (!(prot_err->valid_bits & PROT_ERR_VALID_SERIAL_NUMBER) ||
> + prot_err->agent_type > 0x4 || prot_err->agent_type == RCH_DP)
prot_err->agent_type > FM_LD? Although maybe it would be a clearer read if a helper function is defined to identify the agent types such as cxl_cper_prot_err_serial_needed() or cxl_cper_prot_agent_type_device() and with it a switch statement to explicitly identify all the agent types that require serial number. If a future device is defined, the > 0x4 logic may break.
DJ
> + pr_warn(FW_WARN "No Device Serial number\n");
> +
> + p_err->lower_dw = prot_err->dev_serial_num.lower_dw;
> + p_err->upper_dw = prot_err->dev_serial_num.upper_dw;
> +
> + p_err->severity = cper_severity_cxl_aer(gdata->error_severity);
> +
> + return 0;
> +}
> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> index f11e52ff565a..9c7b69e076a0 100644
> --- a/include/linux/cxl-event.h
> +++ b/include/linux/cxl-event.h
> @@ -165,11 +165,37 @@ struct cxl_ras_capability_regs {
> u32 header_log[16];
> };
>
> +enum cxl_aer_err_type {
> + CXL_AER_UNCORRECTABLE,
> + CXL_AER_CORRECTABLE,
> +};
> +
> +struct cxl_cper_prot_err {
> + struct cxl_ras_capability_regs cxl_ras;
> +
> + /* Device ID */
> + u8 function;
> + u8 device;
> + u8 bus;
> + u16 segment;
> +
> + /* Device Serial Number */
> + u32 lower_dw;
> + u32 upper_dw;
> +
> + int severity;
> +};
> +
> struct cxl_cper_work_data {
> enum cxl_event_type event_type;
> struct cxl_cper_event_rec rec;
> + struct cxl_cper_prot_err p_err;
> };
>
> +struct acpi_hest_generic_data;
> +int cxl_cper_handle_prot_err_info(struct acpi_hest_generic_data *gdata,
> + struct cxl_cper_prot_err *p_err);
> +
> #ifdef CONFIG_ACPI_APEI_GHES
> int cxl_cper_register_work(struct work_struct *work);
> int cxl_cper_unregister_work(struct work_struct *work);
Powered by blists - more mailing lists