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

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

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.

Ira

> 
> +		cxlr = cxl_dpa_to_region(cxlmd, dpa);
> +		if (cxlr)
> +			hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
> +
> +		if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
> +			trace_cxl_general_media(cxlmd, type, cxlr, hpa,
> +						&evt->gen_media);
> +		else if (event_type == CXL_CPER_EVENT_DRAM)
> +			trace_cxl_dram(cxlmd, type, cxlr, hpa, &evt->dram);
> +	}
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL);
> 
> 
> 
> 
> > The rest looks good.
> > 
> > Ira
> >