linux-kernel - Re: [PATCH v5 9/9] cxl/pci: 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 for Android: free password hash cracker in your pocket

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

Message-ID: <659dddca99517_5cee29433@dwillia2-xfh.jf.intel.com.notmuch>
Date: Tue, 9 Jan 2024 15:59:06 -0800
From: Dan Williams <dan.j.williams@...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>
Subject: Re: [PATCH v5 9/9] cxl/pci: Register for and process CPER events

Jonathan Cameron wrote:
> On Wed, 20 Dec 2023 16:17:36 -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 the PCI driver registration to include registration of a CXL
> > CPER callback to process events through the trace subsystem.
> > 
> > Use new scoped based management to simplify the handling of the PCI
> > device object.
> > 
> > NOTE this patch depends on Dan's addition of a device guard[1].
> > 
> > [1] https://lore.kernel.org/all/170250854466.1522182.17555361077409628655.stgit@dwillia2-xfh.jf.intel.com/
> > 
> One trivial comment inline.
> The guard change Dan suggests makes sense.  Otherwise I'm fine with this.
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> 
> I'll bolt in the other stuff I need to test it from QEMU this week.
> Did the protocol error first, but these are easy to add now I have
> that working,
> 
> Jonathan
> > ---
> > Changes for v5:
> > [Smita/djbw: trace a generic UUID if the type is unknown]
> > [Jonathan: clean up pci and device state error handling]
> > [iweiny: consolidate the trace function]
> > ---
> >  drivers/cxl/core/mbox.c   | 49 ++++++++++++++++++++++++++++-----------
> >  drivers/cxl/cxlmem.h      |  4 ++++
> >  drivers/cxl/pci.c         | 58 ++++++++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/cxl-event.h |  1 +
> >  4 files changed, 98 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 06957696247b..b801faaccd45 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -836,21 +836,44 @@ int cxl_enumerate_cmds(struct cxl_memdev_state *mds)
> >  }
> >  EXPORT_SYMBOL_NS_GPL(cxl_enumerate_cmds, CXL);
> >  
> > -static void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> > -				   enum cxl_event_log_type type,
> > -				   struct cxl_event_record_raw *record)
> > +void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> > +			    enum cxl_event_log_type type,
> > +			    enum cxl_event_type event_type,
> > +			    const uuid_t *uuid, union cxl_event *evt)
> >  {
> > -	union cxl_event *evt = &record->event;
> > -	uuid_t *id = &record->id;
> > -
> > -	if (uuid_equal(id, &CXL_EVENT_GEN_MEDIA_UUID))
> > +	switch (event_type) {
> > +	case CXL_CPER_EVENT_GEN_MEDIA:
> >  		trace_cxl_general_media(cxlmd, type, &evt->gen_media);
> > -	else if (uuid_equal(id, &CXL_EVENT_DRAM_UUID))
> > +		break;
> 
> Might as well return directly and save a reviewer having to check if anything else happens
> after the switch

Might as well keep it as an "if () else" tree as that's equally clear
and more compact.

That immeidiately then opens the concern of why the upper level
__cxl_event_trace_record() is calling a lower level function without the
prefix. That can be swapped later to meet common expectations, but it
feels like gymnastics to parse all the uuids *and* still pass the uuid
to the cxl_event_trace_record() helper. Yes, I see how it happens, just
not totally comfortable with the result, but not enough to hold up the
series.