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

[Alison Schofield <alison.schofield@intel.com>]

On Fri, Jan 17, 2025 at 10:28:32AM -0700, Dave Jiang 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>
> ---
> v3:
> - Drop unused region to nid function
> - Make sure hpa_alias defaults to ~0ULL. (Jonathan)
> ---
>  drivers/cxl/core/mbox.c  | 28 ++++++++++++++++++++++++----
>  drivers/cxl/core/trace.h | 24 ++++++++++++++++--------
>  2 files changed, 40 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 548564c770c0..f42c4c56dc43 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -856,6 +856,23 @@ 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;
> +
> +	if (!cxlr)
> +		return ~0ULL;
> +
> +	p = &cxlr->params;
> +	if (!p->cache_size)
> +		return ~0ULL;
> +
> +	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 +888,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 = ~0ULL;

A bit odd to use 2 different notations for same thing.
Prefer ULLONG_MAX here and in previous function.


>  		struct cxl_region *cxlr;
>  
>  		/*
> @@ -884,14 +901,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) {
>  			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);
>  	}
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, "CXL");
> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> index 8389a94adb1a..257f60f16e4c 100644
> --- a/drivers/cxl/core/trace.h
> +++ b/drivers/cxl/core/trace.h
> @@ -316,9 +316,10 @@ TRACE_EVENT(cxl_generic_event,
>  TRACE_EVENT(cxl_general_media,
>  
>  	TP_PROTO(const struct cxl_memdev *cxlmd, enum cxl_event_log_type log,
> -		 struct cxl_region *cxlr, u64 hpa, struct cxl_event_gen_media *rec),
> +		 struct cxl_region *cxlr, u64 hpa, u64 hpa_alias,
> +		 struct cxl_event_gen_media *rec),
>  
> -	TP_ARGS(cxlmd, log, cxlr, hpa, rec),
> +	TP_ARGS(cxlmd, log, cxlr, hpa, hpa_alias, rec),
>  
>  	TP_STRUCT__entry(
>  		CXL_EVT_TP_entry
> @@ -332,6 +333,7 @@ TRACE_EVENT(cxl_general_media,
>  		__array(u8, comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE)
>  		/* Following are out of order to pack trace record */
>  		__field(u64, hpa)
> +		__field(u64, hpa_alias)

I saw Jonathan's ask for hpa_alias to be hpa_alias0 or something?
That's done in the CXL_EVT_TP_printk output below. It needs to be
used here in the field name to be picked up in the trace event.
(same for cxl_dram event)

But...what's the deal with hpa_alias0. If we anticipate an array
of aliases, then maybe an array like is done for comp_id would
work. Expect that's overkill at the moment.


>  		__field_struct(uuid_t, region_uuid)
>  		__field(u16, validity_flags)
>  		__field(u8, rank)
> @@ -358,6 +360,7 @@ TRACE_EVENT(cxl_general_media,
>  			CXL_EVENT_GEN_MED_COMP_ID_SIZE);
>  		__entry->validity_flags = get_unaligned_le16(&rec->media_hdr.validity_flags);
>  		__entry->hpa = hpa;
> +		__entry->hpa_alias = hpa_alias;
>  		if (cxlr) {
>  			__assign_str(region_name);
>  			uuid_copy(&__entry->region_uuid, &cxlr->params.uuid);
> @@ -370,7 +373,7 @@ TRACE_EVENT(cxl_general_media,
>  	CXL_EVT_TP_printk("dpa=%llx dpa_flags='%s' " \
>  		"descriptor='%s' type='%s' transaction_type='%s' channel=%u rank=%u " \
>  		"device=%x comp_id=%s validity_flags='%s' " \
> -		"hpa=%llx region=%s region_uuid=%pUb",
> +		"hpa=%llx hpa_alias0=%llx region=%s region_uuid=%pUb",
>  		__entry->dpa, show_dpa_flags(__entry->dpa_flags),
>  		show_event_desc_flags(__entry->descriptor),
>  		show_gmer_mem_event_type(__entry->type),
> @@ -378,7 +381,8 @@ TRACE_EVENT(cxl_general_media,
>  		__entry->channel, __entry->rank, __entry->device,
>  		__print_hex(__entry->comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE),
>  		show_valid_flags(__entry->validity_flags),
> -		__entry->hpa, __get_str(region_name), &__entry->region_uuid
> +		__entry->hpa, __entry->hpa_alias, __get_str(region_name),
> +		&__entry->region_uuid
>  	)
>  );
>  
> @@ -424,9 +428,10 @@ TRACE_EVENT(cxl_general_media,
>  TRACE_EVENT(cxl_dram,
>  
>  	TP_PROTO(const struct cxl_memdev *cxlmd, enum cxl_event_log_type log,
> -		 struct cxl_region *cxlr, u64 hpa, struct cxl_event_dram *rec),
> +		 struct cxl_region *cxlr, u64 hpa, u64 hpa_alias,
> +		 struct cxl_event_dram *rec),
>  
> -	TP_ARGS(cxlmd, log, cxlr, hpa, rec),
> +	TP_ARGS(cxlmd, log, cxlr, hpa, hpa_alias, rec),
>  
>  	TP_STRUCT__entry(
>  		CXL_EVT_TP_entry
> @@ -442,6 +447,7 @@ TRACE_EVENT(cxl_dram,
>  		__field(u32, row)
>  		__array(u8, cor_mask, CXL_EVENT_DER_CORRECTION_MASK_SIZE)
>  		__field(u64, hpa)
> +		__field(u64, hpa_alias)
>  		__field_struct(uuid_t, region_uuid)
>  		__field(u8, rank)	/* Out of order to pack trace record */
>  		__field(u8, bank_group)	/* Out of order to pack trace record */
> @@ -472,6 +478,7 @@ TRACE_EVENT(cxl_dram,
>  		memcpy(__entry->cor_mask, &rec->correction_mask,
>  			CXL_EVENT_DER_CORRECTION_MASK_SIZE);
>  		__entry->hpa = hpa;
> +		__entry->hpa_alias = hpa_alias;
>  		if (cxlr) {
>  			__assign_str(region_name);
>  			uuid_copy(&__entry->region_uuid, &cxlr->params.uuid);
> @@ -485,7 +492,7 @@ TRACE_EVENT(cxl_dram,
>  		"transaction_type='%s' channel=%u rank=%u nibble_mask=%x " \
>  		"bank_group=%u bank=%u row=%u column=%u cor_mask=%s " \
>  		"validity_flags='%s' " \
> -		"hpa=%llx region=%s region_uuid=%pUb",
> +		"hpa=%llx hpa_alias0=%llx region=%s region_uuid=%pUb",
>  		__entry->dpa, show_dpa_flags(__entry->dpa_flags),
>  		show_event_desc_flags(__entry->descriptor),
>  		show_dram_mem_event_type(__entry->type),
> @@ -495,7 +502,8 @@ TRACE_EVENT(cxl_dram,
>  		__entry->row, __entry->column,
>  		__print_hex(__entry->cor_mask, CXL_EVENT_DER_CORRECTION_MASK_SIZE),
>  		show_dram_valid_flags(__entry->validity_flags),
> -		__entry->hpa, __get_str(region_name), &__entry->region_uuid
> +		__entry->hpa_alias, __entry->hpa, __get_str(region_name),

Needs swapping -  hpa then hpa_alias


> +		&__entry->region_uuid
>  	)
>  );
>  
> -- 
> 2.47.1
>