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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <658226c9a49f3_277bd294fb@iweiny-mobl.notmuch>
Date: Tue, 19 Dec 2023 15:27:05 -0800
From: Ira Weiny <ira.weiny@...el.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>, Ira Weiny
	<ira.weiny@...el.com>
CC: Dan Williams <dan.j.williams@...el.com>, Smita Koralahalli
	<Smita.KoralahalliChannabasappa@....com>, Shiju Jose <shiju.jose@...wei.com>,
	Yazen Ghannam <yazen.ghannam@....com>, Davidlohr Bueso <dave@...olabs.net>,
	Dave Jiang <dave.jiang@...el.com>, Alison Schofield
	<alison.schofield@...el.com>, Vishal Verma <vishal.l.verma@...el.com>, "Ard
 Biesheuvel" <ardb@...nel.org>, <linux-efi@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>, Bjorn Helgaas
	<bhelgaas@...gle.com>
Subject: Re: [PATCH v4 7/7] cxl/memdev: Register for and process CPER events

Jonathan Cameron wrote:
> On Fri, 15 Dec 2023 15:26:33 -0800
> Ira Weiny <ira.weiny@...el.com> 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@...gle.com>
> > Signed-off-by: Ira Weiny <ira.weiny@...el.com>
> 
> I'd break out the pci guard stuff as a precursor patch.  That's likely
> to be used elsewhere so it would help for backporting for other users
> if it wasn't buried in a patch doing other stuff.

That is good.  I've done that.

> 
> Not to mention that has a different set of likely reviewers to the rest
> of this patch.

Yep.

> 
> More generally maybe we should just hardcode the UUID in the tracepoint
> definitions?  I think for everything other than the generic one we
> only ever call trace_cxl_memory_module(... &mem_mod_event_uuid..)
> etc.
> 
> It's a little ugly to match on the UUID to call a function where it
> hard coded, but less so than inserting the UUID like this does.
> Better I think to make it obvious that this isn't actually a variable
> (for the ones we understand).

I thought about that a bit.  But I built the tracing code with generic
macros which contained the UUID.  That complicated my efforts.

I've reworked it again and it took a bit of time but I got it to work.  It
was not that hard but there is a caveat in the generic macros, which I
made a note of.

[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;
> 
> This is handling two conditions. I'd find it more readable split like:
> 
> 	if (pdev->driver != &cxl_pci_driver)
> 		return;
> 
> 	cxlds = pci_get_drvdata(pdev);
> 	if (!cxlds)
> 		return;
> 
> and drop the = NULL above.

Done.

Ira