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

[Dave Jiang <dave.jiang@intel.com>]

On 1/13/25 5:53 AM, Jonathan Cameron wrote:
> On Fri, 10 Jan 2025 10:27:52 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> On 1/10/25 9:32 AM, Jonathan Cameron wrote:
>>> On Fri, 10 Jan 2025 08:17:46 -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 think the value set when there isn't a value differs between no
>>> cache and no region. That seems an odd bit of ABI.  Maybe go with ~0ULL in
>>> both cases?  
>>
>> Not sure I follow. Currently both cases do return ~0ULL.
> 
> See inline.  I don't think they do... In the no region case
> hpa is ~0ULL, hpa_alias is 0

Ah I see what you are saying now. Thanks! I'll fix that. 

DJ

> 
> 
>>
>> DJ
>>
>>>
>>> Other than tidying that up this looks fine to me.
>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>   
>>>> ---
>>>> v2:
>>>> - Emit hpa_alias0 instead of hpa_alias. (Jonathan)
>>>> - Check valid cxlr before dereference. (Jonathan)
>>>> ---
>>>>  drivers/cxl/core/core.h   |  5 +++++
>>>>  drivers/cxl/core/mbox.c   | 33 +++++++++++++++++++++++++++++----
>>>>  drivers/cxl/core/region.c | 12 ++++++++++++
>>>>  drivers/cxl/core/trace.h  | 24 ++++++++++++++++--------
>>>>  4 files changed, 62 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
>>>> index 0fb779b612d1..afbefc72c8fa 100644
>>>> --- a/drivers/cxl/core/core.h
>>>> +++ b/drivers/cxl/core/core.h
>>>> @@ -30,8 +30,13 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port);
>>>>  struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa);
>>>>  u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
>>>>  		   u64 dpa);
>>>> +int cxl_region_nid(struct cxl_region *cxlr);
>>>>  
>>>>  #else
>>>> +static inline int cxl_region_nid(struct cxl_region *cxlr)
>>>> +{
>>>> +	return NUMA_NO_NODE;
>>>> +}
>>>>  static inline u64 cxl_dpa_to_hpa(struct cxl_region *cxlr,
>>>>  				 const struct cxl_memdev *cxlmd, u64 dpa)
>>>>  {
>>>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
>>>> index 548564c770c0..d7999260f004 100644
>>>> --- a/drivers/cxl/core/mbox.c
>>>> +++ b/drivers/cxl/core/mbox.c
>>>> @@ -856,6 +856,28 @@ 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;
>>>> +	int nid;
>>>> +
>>>> +	if (!cxlr)
>>>> +		return ~0ULL;  
>>>   
>>>> +	p = &cxlr->params;
>>>> +	if (!p->cache_size)
>>>> +		return ~0ULL;  
>>>
>>> So no cache is all 1s. .. See below.
>>>   
>>>> +
>>>> +	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 +893,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 = 0;  
>>>
>>> If I read this right if there is a region but no alias then the hpa_alias is
>>> ~0ULL but if there no region it is 0.  Seems a tiny bit inconsistent.
> 
> Here you set hpa_alias to 0.
> 
>>>   
>>>>  		struct cxl_region *cxlr;
>>>>  
>>>>  		/*
>>>> @@ -884,14 +906,17 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
>>>>  
>>>>  		dpa = le64_to_cpu(evt->media_hdr.phys_addr) & CXL_DPA_MASK;
>>>>  		cxlr = cxl_dpa_to_region(cxlmd, dpa);
>>>> -		if (cxlr)
>>>> +		if (cxlr) {
> 
> If no region, nothing changes hpa_alias.
> 
>>>>  			hpa = cxl_dpa_to_hpa(cxlr, cxlmd, dpa);
>>>> +			hpa_alias = cxlr_hpa_cache_alias(cxlr, hpa);
>>>> +		}
>>>>  
>>>>  		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);
>>>>  	}
>>>>  }  
>>>
>>>   
>>
>>
>