Re: [PATCH v4 7/7] cxl/memdev: Register for and process CPER events

[Ira Weiny]

Dan Williams wrote:
> Smita Koralahalli wrote:
> > On 12/15/2023 3:26 PM, Ira Weiny wrote:
> > > If the firmware has configured CXL event support to be firmware first
> > > the OS can process those events through CPER records.  The CXL layer has
> > > unique DPA to HPA knowledge and standard event trace parsing in place.
> > > 
> > > CPER records contain Bus, Device, Function information which can be used
> > > to identify the PCI device which is sending the event.
> > > 
> > > Change pci driver registration to include registration for a CXL CPER
> > > notifier to process the events through the trace subsystem.
> > > 
> > > Define and use scoped based management to simplify the handling of the
> > > pci device object.
> > > 
> > > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> > > 
> > > ---
> > 
> > [snip]
> > 
> > 
> > > +	switch (event_type) {
> > > +	case CXL_CPER_EVENT_GEN_MEDIA:
> > > +		trace_cxl_general_media(cxlmd, type, &gen_media_event_uuid,
> > > +					&event->gen_media);
> > > +		break;
> > > +	case CXL_CPER_EVENT_DRAM:
> > > +		trace_cxl_dram(cxlmd, type, &dram_event_uuid, &event->dram);
> > > +		break;
> > > +	case CXL_CPER_EVENT_MEM_MODULE:
> > > +		trace_cxl_memory_module(cxlmd, type, &mem_mod_event_uuid,
> > > +					&event->mem_module);
> > > +		break;
> > > +	}
> > > +}
> > 
> > Is default case needed here?
> 
> Yeah, it looks like an uninitialized @type value can be passed through
> the stack here.

That was not my intention but yea.

Added a generic trace with a null UUID.

[snip]

> > > +#define CXL_EVENT_HDR_FLAGS_REC_SEVERITY GENMASK(1, 0)
> > > +static void cxl_cper_event_call(enum cxl_event_type ev_type,
> > > +				struct cxl_cper_event_rec *rec)
> > > +{
> > > +	struct cper_cxl_event_devid *device_id = &rec->hdr.device_id;
> > > +	struct pci_dev *pdev __free(pci_dev_put) = NULL;
> > > +	struct cxl_dev_state *cxlds = NULL;
> > > +	enum cxl_event_log_type log_type;
> > > +	unsigned int devfn;
> > > +	u32 hdr_flags;
> > > +
> > > +	devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
> > > +	pdev = pci_get_domain_bus_and_slot(device_id->segment_num,
> > > +					   device_id->bus_num, devfn);
> > > +	if (!pdev)
> > > +		return;
> > > +
> > > +	guard(device)(&pdev->dev);
> > > +	if (pdev->driver == &cxl_pci_driver)
> > > +		cxlds = pci_get_drvdata(pdev);
> > > +	if (!cxlds)
> > > +		return;
> > > +
> > > +	/* Fabricate a log type */
> > > +	hdr_flags = get_unaligned_le24(rec->event.generic.hdr.flags);
> > > +	log_type = FIELD_GET(CXL_EVENT_HDR_FLAGS_REC_SEVERITY, hdr_flags);
> > > +
> > > +	cxl_event_trace_record(cxlds->cxlmd, log_type, ev_type, &rec->event);
> > 
> > Currently, when I run this, I see two trace events printed. One from 
> > here, and another as a non_standard_event from ghes. I think both should 
> > be unified?

By the way, Smita,

Thanks for testing!  I really do appreciate it!

> > 
> > I remember Dan pointing out to me this when I sent decoding for protocol 
> > errors and its still pending on me for protocol errors.
> 
> Good point, so I think the responsibility to trace CXL events should
> belong to ghes_do_proc() and ghes_print_estatus() can just ignore CXL
> events.
> 
> Notice how ghes_proc() sometimes skips ghes_print_estatus(), but
> uncoditionally emits a trace event in ghes_do_proc()? To me that means
> that the cper_estatus_print() inside ghes_print_estatus() can just defer
> to the ghes code to do the hookup to the trace code.
> 
> For example, ras_userspace_consumers() was introduced to skip emitting
> events to the kernel log when the trace event might be handled. My
> assumption is that was for historical reasons, but since CXL events are
> new, just never emit them to the kernel log and always require the trace
> path.
> 
> I am open to other thoughts here, but it seems like ghes_do_proc() is
> where the callback needs to be triggered.

I see.

Ok.  I'll create a pre-patch which moves the protocol error first then
I'll put the events in the ghes_do_proc() well.


[snip]

> > > +	rc = cxl_cper_register_notifier(cxl_cper_event_call);
> 
> Quick aside as I am reading through this, the "notifier" name is
> misleading since this callback has nothing to do with the
> include/linux/notifier.h API.

Fair point.  I debated 'callback' vs 'notifier'.  I'll change it to
callback as I think that is equally correct and as you say does clarify
this is not a 'notifier'.

Ira