Re: [PATCH v5 4/4] cxl/core: Add region info to cxl_general_media and cxl_dram events

[Ira Weiny <ira.weiny@intel.com>]

Ira Weiny wrote:
> Alison Schofield wrote:
> > On Mon, Apr 29, 2024 at 07:19:02PM -0700, Ira Weiny wrote:
> > > alison.schofield@ wrote:
> > > > From: Alison Schofield <alison.schofield@intel.com>
> > > > 
> > > 
> > > [snip]
> > > 
> > > > diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> > > > index 03fa6d50d46f..5342755777cc 100644
> > > > --- a/include/linux/cxl-event.h
> > > > +++ b/include/linux/cxl-event.h
> > > > @@ -91,11 +91,21 @@ struct cxl_event_mem_module {
> > > >  	u8 reserved[0x3d];
> > > >  } __packed;
> > > >  
> > > > +/*
> > > > + * General Media or DRAM Event Common Fields
> > > > + * - provides common access to phys_addr
> > > > + */
> > > > +struct cxl_event_common {
> > > > +	struct cxl_event_record_hdr hdr;
> > > > +	__le64 phys_addr;
> > > > +} __packed;
> > > 
> > > Was this left over from a previous version?
> > > 
> > > I don't see it used.  Nor is it a defined event in the spec AFAIK.
> > > 
> > 
> > I introduced it in v4. It is for the 'convenience' of getting at that
> > common phys_addr field. You snipped the usage away. I'll paste it back 
> > here:
> > 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > 
> > @@ -842,14 +842,38 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> >  			    enum cxl_event_type event_type,
> >  			    const uuid_t *uuid, union cxl_event *evt)
> >  {
> > 
> > my snip...
> > 
> > +	if (trace_cxl_general_media_enabled() || trace_cxl_dram_enabled()) {
> > +		u64 dpa, hpa = ULLONG_MAX;
> > +		struct cxl_region *cxlr;
> > +
> > +		/*
> > +		 * These trace points are annotated with HPA and region
> > +		 * translations. Take topology mutation locks and lookup
> > +		 * { HPA, REGION } from { DPA, MEMDEV } in the event record.
> > +		 */
> > +		guard(rwsem_read)(&cxl_region_rwsem);
> > +		guard(rwsem_read)(&cxl_dpa_rwsem);
> > +
> > +		dpa = le64_to_cpu(evt->common.phys_addr) & CXL_DPA_MASK;
> > 
> > Used above. OK?
> 
> Ah sneaky.  It was not clear to me from your comment why this new 'event'
> existed.

Almost forgot.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>