Re: "invalid agent type: 1" in acpi/ghes, cper: Recognize and cache CXL Protocol errors

[Koralahalli Channabasappa, Smita]

Hi Marc,

On 7/22/2025 12:24 PM, Marc Herbert wrote:
> Hi Smita,
>
>    The code below triggers the error "invalid agent type: 1" in Intel
> validation (internal issue 15018133056)
>
> It's not clear to anyone we asked why you did not include RCH_DP in
> the `switch (prot_err->agent_type)` in cxl_cper_post_prot_err() below.
>
> I can see how RCH_DP is special in cxl_cper_PRINT_prot_err() and I can
> even understand (despite my near-zero CPER knowledge) some of the
> special cases there. But in cxl_cper_post_prot_err() here, it's not
> clear why RCH_DP would be rejected. Could this be an oversight? If not,
> a comment with a short explanation would not hurt.
>
> Marc
>
> PS: the newer cxl_cper_post_prot_err() code is longer and does
> something with `wd`. That's irrelevant for this test case since the
> function errors and returns earlier anyway.

You're right. RCH_DP was excluded because it doesn’t report a valid SBDF 
in the CPER record. Instead, it provides only the RCRB base address.

I haven't thoroughly investigated whether SBDF can be reliably derived 
from the RCRB base. There might be a platform-specific mechanism for 
that, but at the time, it seemed non-trivial to implement. Introducing 
additional infrastructure solely to support RCH_DP felt like it was 
adding more complexity.

I agree that a brief comment explaining this rationale would help. I'm 
okay if you plan to include a fixup for this along with the one for the 
device serial number.

Thanks
Smita

> On 2025-01-23 00:44, Smita Koralahalli wrote:
> > Add support in GHES to detect and process CXL CPER Protocol errors, as
> > defined in UEFI v2.10, section N.2.13.
> >
> > Define struct cxl_cper_prot_err_work_data to cache CXL protocol error
> > information, including RAS capabilities and severity, for further
> > handling.
> >
> > These cached CXL CPER records will later be processed by workqueues
> > within the CXL subsystem.
> >
> > Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@xxxxxxx>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> > Reviewed-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> > ---
> >   drivers/acpi/apei/ghes.c | 54 ++++++++++++++++++++++++++++++++++++++++
> >   include/cxl/event.h      |  6 +++++
> >   2 files changed, 60 insertions(+)
> >
> > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> > index b72772494655..4d725d988c43 100644
> > --- a/drivers/acpi/apei/ghes.c
> > +++ b/drivers/acpi/apei/ghes.c
> > @@ -674,6 +674,56 @@ static void ghes_defer_non_standard_event(struct acpi_hest_generic_data *gdata,
> >   	schedule_work(&entry->work);
> >   }
> >   
> > +static void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
> > +				   int severity)
> > +{
> > +#ifdef CONFIG_ACPI_APEI_PCIEAER
> > +	struct cxl_cper_prot_err_work_data wd;
> > +	u8 *dvsec_start, *cap_start;
> > +
> > +	if (!(prot_err->valid_bits & PROT_ERR_VALID_AGENT_ADDRESS)) {
> > +		pr_err_ratelimited("CXL CPER invalid agent type\n");
> > +		return;
> > +	}
> > +
> > +	if (!(prot_err->valid_bits & PROT_ERR_VALID_ERROR_LOG)) {
> > +		pr_err_ratelimited("CXL CPER invalid protocol error log\n");
> > +		return;
> > +	}
> > +
> > +	if (prot_err->err_len != sizeof(struct cxl_ras_capability_regs)) {
> > +		pr_err_ratelimited("CXL CPER invalid RAS Cap size (%u)\n",
> > +				   prot_err->err_len);
> > +		return;
> > +	}
> > +
> > +	if (!(prot_err->valid_bits & PROT_ERR_VALID_SERIAL_NUMBER))
> > +		pr_warn(FW_WARN "CXL CPER no device serial number\n");
> > +
> > +	switch (prot_err->agent_type) {
> > +	case RCD:
> > +	case DEVICE:
> > +	case LD:
> > +	case FMLD:
> > +	case RP:
> > +	case DSP:
> > +	case USP:
> > +		memcpy(&wd.prot_err, prot_err, sizeof(wd.prot_err));
> > +
> > +		dvsec_start = (u8 *)(prot_err + 1);
> > +		cap_start = dvsec_start + prot_err->dvsec_len;
> > +
> > +		memcpy(&wd.ras_cap, cap_start, sizeof(wd.ras_cap));
> > +		wd.severity = cper_severity_to_aer(severity);
> > +		break;
> > +	default:
> > +		pr_err_ratelimited("CXL CPER invalid agent type: %d\n",
> > +				   prot_err->agent_type);
> > +		return;
> > +	}
> > +#endif
> > +}
> > +
> >   /* Room for 8 entries for each of the 4 event log queues */
> >   #define CXL_CPER_FIFO_DEPTH 32
> >   DEFINE_KFIFO(cxl_cper_fifo, struct cxl_cper_work_data, CXL_CPER_FIFO_DEPTH);
> > @@ -777,6 +827,10 @@ 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, sync);
> > +		} else if (guid_equal(sec_type, &CPER_SEC_CXL_PROT_ERR)) {
> > +			struct cxl_cper_sec_prot_err *prot_err = acpi_hest_get_payload(gdata);
> > +
> > +			cxl_cper_post_prot_err(prot_err, gdata->error_severity);
> >   		} 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/include/cxl/event.h b/include/cxl/event.h
> > index 66d85fc87701..ee1c3dec62fa 100644
> > --- a/include/cxl/event.h
> > +++ b/include/cxl/event.h
> > @@ -232,6 +232,12 @@ struct cxl_ras_capability_regs {
> >   	u32 header_log[16];
> >   };
> >   
> > +struct cxl_cper_prot_err_work_data {
> > +	struct cxl_cper_sec_prot_err prot_err;
> > +	struct cxl_ras_capability_regs ras_cap;
> > +	int severity;
> > +};
> > +
> >   #ifdef CONFIG_ACPI_APEI_GHES
> >   int cxl_cper_register_work(struct work_struct *work);
> >   int cxl_cper_unregister_work(struct work_struct *work);