Re: [PATCH 3/4] cxl: Add extended linear cache address alias emission for cxl events

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 7 Jan 2025 12:41:46 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 12/24/24 5:11 AM, Jonathan Cameron wrote:
> > On Wed, 4 Dec 2024 15:46:48 -0700
> > Dave Jiang <dave.jiang@intel.com> wrote:
> >   
> >> Add the aliased address of extended linear cache when emitting event
> >> trace for DRAM and general media of CXL events.
> >>
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>  
> > I wonder if you want to future proof the hpa_alias for potential case
> > of there being more than 1?  hpa_alias0 or similar so that we can add
> > more as needed?  
> 
> Do you mean when emitting via the trace? Yeah I can change it to hpa_alias0 in the output string.
> 

err. Can't entirely remember what I meant but that seems sensible ;)

> 
> > 
> > 
> > Otherwise, looks like there is either a null point dereference or
> > overly paranoid existing code. I haven't checked which but changes
> > needed either way.  
> 
> I'll move it under the if (cxlr) and also add a check for cxlr in the helper function.
> 
> DJ
> 
> 
> > 
> > Jonathan
> >   
> >> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> >> index 5175138c4fb7..4017b7afa78a 100644
> >> --- a/drivers/cxl/core/mbox.c
> >> +++ b/drivers/cxl/core/mbox.c
> >> @@ -856,6 +856,24 @@ int cxl_enumerate_cmds(struct cxl_memdev_state *mds)
> >>  }
> >>  EXPORT_SYMBOL_NS_GPL(cxl_enumerate_cmds, CXL);
> >>  
> >> +static u64 cxlr_hpa_cache_alias(struct cxl_region *cxlr, u64 hpa)
> >> +{
> >> +	struct cxl_region_params *p = &cxlr->params;  
> > 
> > Bad idea to unconditionally dereference I think. See below.
> >   
> >> +	int nid;
> >> +
> >> +	if (!p->cache_size)
> >> +		return ~0ULL;
> >> +
> >> +	nid = cxl_region_nid(cxlr);
> >> +	if (nid == NUMA_NO_NODE)
> >> +		nid = 0;
> >> +
> >> +	if (hpa >= p->res->start + p->cache_size)
> >> +		return hpa - p->cache_size;
> >> +
> >> +	return hpa + p->cache_size;
> >> +}
> >> +
> >>  void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> >>  			    enum cxl_event_log_type type,
> >>  			    enum cxl_event_type event_type,
> >> @@ -871,7 +889,7 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> >>  	}
> >>  
> >>  	if (trace_cxl_general_media_enabled() || trace_cxl_dram_enabled()) {
> >> -		u64 dpa, hpa = ULLONG_MAX;
> >> +		u64 dpa, hpa = ULLONG_MAX, hpa_alias;
> >>  		struct cxl_region *cxlr;
> >>  
> >>  		/*
> >> @@ -887,11 +905,14 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> >>  		if (cxlr)
> >>  			hpa = cxl_dpa_to_hpa(cxlr, cxlmd, dpa);
> >>  
> >> +		hpa_alias = cxlr_hpa_cache_alias(cxlr, hpa);  
> > 
> > If there is no region, and hence no hpa does it make sense to call this?
> > Particularly as first thing done in this is to dereference cxlr.
> > 
> >   
> >> +
> >>  		if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
> >>  			trace_cxl_general_media(cxlmd, type, cxlr, hpa,
> >> -						&evt->gen_media);
> >> +						hpa_alias, &evt->gen_media);
> >>  		else if (event_type == CXL_CPER_EVENT_DRAM)
> >> -			trace_cxl_dram(cxlmd, type, cxlr, hpa, &evt->dram);
> >> +			trace_cxl_dram(cxlmd, type, cxlr, hpa, hpa_alias,
> >> +				       &evt->dram);
> >>  	}
> >>  }
> >>  EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL);  
> >   
> 
>