[PATCH v2 0/4] Add DPA->HPA translation to dram & general_media

[PATCH v2 0/4] Add DPA->HPA translation to dram & general_media

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Changes in v2:
- Fix !CONFIG_CXL_REGION build error in cxl/core.h stub (lkp@intel.com)
- Remove 'to dpa' usage when DPA is already adjusted in TP_fast_assign
- Use a common macro for extracting dpa from event record (Ira)
- Return "" instead of NULL for region name in stub (Jonathan)
- Replace store_region_info MACRO w static inline func (Dan)
- Set uuid to uuid_null when no region info available (Ira)
- Remove useless macro wrapping cxl_to_region_name() (Ira)
- Remove extraneous diff in Patch 4's TP_printk (Jonathan)
- Update commit msg patch 1 & 2, s/stubs/stub (Jonathan)
Link to v1: https://lore.kernel.org/cover.1711598777.git.alison.schofield@intel.com/

An update to the cxl_events unit test is in review here:
https://lore.kernel.org/20240328043727.2186722-1-alison.schofield@intel.com/


Begin Cover Letter:
Add HPA translations to CXL events: cxl_dram and cxl_general_media

Patches 1 & 2:
Before adding the new support, do some housekeeping and move related
helpers to the region driver because there is no looking up region
related info without CONFIG_CXL_REGION.

Patch 3:
The new functionality is introduced - cxl_dram & cxl_general_media
events will lookup and log the DPA->HPA translation along with the
region name and region uuid.

Patch 4:
Apply the new lookup helpers to the existing cxl_poison event, so it
can be the same, share in the new goodness, and also tidy up its
implementation.

An update to the cxl_event unit test is posted separately.

Alison Schofield (4):
  cxl/region: Move cxl_dpa_to_region() work to the region driver
  cxl/region: Move cxl_trace_hpa() work to the region driver
  cxl/core: Add region info to cxl_general_media and cxl_dram events
  cxl/core: Remove cxlr dependency from cxl_poison trace events

 drivers/cxl/core/core.h   |  20 +++++
 drivers/cxl/core/mbox.c   |  22 ++++--
 drivers/cxl/core/memdev.c |  52 +------------
 drivers/cxl/core/region.c | 151 +++++++++++++++++++++++++++++++++++++-
 drivers/cxl/core/trace.c  |  91 -----------------------
 drivers/cxl/core/trace.h  |  81 ++++++++++++++------
 drivers/cxl/cxlmem.h      |   3 +-
 7 files changed, 245 insertions(+), 175 deletions(-)


base-commit: 4cece764965020c22cff7665b18a012006359095
-- 
2.37.3

[PATCH v2 1/4] cxl/region: Move cxl_dpa_to_region() work to the region driver

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

This helper belongs in the region driver as it is only useful
with CONFIG_CXL_REGION. Add a stub in core.h for when the region
driver is not built.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/cxl/core/core.h   |  7 +++++++
 drivers/cxl/core/memdev.c | 44 ---------------------------------------
 drivers/cxl/core/region.c | 44 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 44 deletions(-)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index bc5a95665aa0..87008505f8a9 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -27,7 +27,14 @@ void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled);
 int cxl_region_init(void);
 void cxl_region_exit(void);
 int cxl_get_poison_by_endpoint(struct cxl_port *port);
+struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa);
+
 #else
+static inline
+struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa)
+{
+	return NULL;
+}
 static inline int cxl_get_poison_by_endpoint(struct cxl_port *port)
 {
 	return 0;
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index d4e259f3a7e9..0277726afd04 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -251,50 +251,6 @@ int cxl_trigger_poison_list(struct cxl_memdev *cxlmd)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_trigger_poison_list, CXL);
 
-struct cxl_dpa_to_region_context {
-	struct cxl_region *cxlr;
-	u64 dpa;
-};
-
-static int __cxl_dpa_to_region(struct device *dev, void *arg)
-{
-	struct cxl_dpa_to_region_context *ctx = arg;
-	struct cxl_endpoint_decoder *cxled;
-	u64 dpa = ctx->dpa;
-
-	if (!is_endpoint_decoder(dev))
-		return 0;
-
-	cxled = to_cxl_endpoint_decoder(dev);
-	if (!cxled->dpa_res || !resource_size(cxled->dpa_res))
-		return 0;
-
-	if (dpa > cxled->dpa_res->end || dpa < cxled->dpa_res->start)
-		return 0;
-
-	dev_dbg(dev, "dpa:0x%llx mapped in region:%s\n", dpa,
-		dev_name(&cxled->cxld.region->dev));
-
-	ctx->cxlr = cxled->cxld.region;
-
-	return 1;
-}
-
-static struct cxl_region *cxl_dpa_to_region(struct cxl_memdev *cxlmd, u64 dpa)
-{
-	struct cxl_dpa_to_region_context ctx;
-	struct cxl_port *port;
-
-	ctx = (struct cxl_dpa_to_region_context) {
-		.dpa = dpa,
-	};
-	port = cxlmd->endpoint;
-	if (port && is_cxl_endpoint(port) && cxl_num_decoders_committed(port))
-		device_for_each_child(&port->dev, &ctx, __cxl_dpa_to_region);
-
-	return ctx.cxlr;
-}
-
 static int cxl_validate_poison_dpa(struct cxl_memdev *cxlmd, u64 dpa)
 {
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 5c186e0a39b9..4b227659e3f8 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2679,6 +2679,50 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port)
 	return rc;
 }
 
+struct cxl_dpa_to_region_context {
+	struct cxl_region *cxlr;
+	u64 dpa;
+};
+
+static int __cxl_dpa_to_region(struct device *dev, void *arg)
+{
+	struct cxl_dpa_to_region_context *ctx = arg;
+	struct cxl_endpoint_decoder *cxled;
+	u64 dpa = ctx->dpa;
+
+	if (!is_endpoint_decoder(dev))
+		return 0;
+
+	cxled = to_cxl_endpoint_decoder(dev);
+	if (!cxled->dpa_res || !resource_size(cxled->dpa_res))
+		return 0;
+
+	if (dpa > cxled->dpa_res->end || dpa < cxled->dpa_res->start)
+		return 0;
+
+	dev_dbg(dev, "dpa:0x%llx mapped in region:%s\n", dpa,
+		dev_name(&cxled->cxld.region->dev));
+
+	ctx->cxlr = cxled->cxld.region;
+
+	return 1;
+}
+
+struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa)
+{
+	struct cxl_dpa_to_region_context ctx;
+	struct cxl_port *port;
+
+	ctx = (struct cxl_dpa_to_region_context) {
+		.dpa = dpa,
+	};
+	port = cxlmd->endpoint;
+	if (port && is_cxl_endpoint(port) && cxl_num_decoders_committed(port))
+		device_for_each_child(&port->dev, &ctx, __cxl_dpa_to_region);
+
+	return ctx.cxlr;
+}
+
 static struct lock_class_key cxl_pmem_region_key;
 
 static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
-- 
2.37.3

[PATCH v2 2/4] cxl/region: Move cxl_trace_hpa() work to the region driver

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

This work belongs in the region driver as it is only useful with
CONFIG_CXL_REGION. Add a stub in core.h for when the region driver
is not built.

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

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/core.h   |  7 +++
 drivers/cxl/core/region.c | 91 +++++++++++++++++++++++++++++++++++++++
 drivers/cxl/core/trace.c  | 91 ---------------------------------------
 drivers/cxl/core/trace.h  |  2 -
 4 files changed, 98 insertions(+), 93 deletions(-)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 87008505f8a9..625394486459 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -28,8 +28,15 @@ int cxl_region_init(void);
 void cxl_region_exit(void);
 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_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
+		  u64 dpa);
 
 #else
+static inline u64
+cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd, u64 dpa)
+{
+	return ULLONG_MAX;
+}
 static inline
 struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa)
 {
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 4b227659e3f8..45eb9c560fd6 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2723,6 +2723,97 @@ struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa)
 	return ctx.cxlr;
 }
 
+static bool cxl_is_hpa_in_range(u64 hpa, struct cxl_region *cxlr, int pos)
+{
+	struct cxl_region_params *p = &cxlr->params;
+	int gran = p->interleave_granularity;
+	int ways = p->interleave_ways;
+	u64 offset;
+
+	/* Is the hpa within this region at all */
+	if (hpa < p->res->start || hpa > p->res->end) {
+		dev_dbg(&cxlr->dev,
+			"Addr trans fail: hpa 0x%llx not in region\n", hpa);
+		return false;
+	}
+
+	/* Is the hpa in an expected chunk for its pos(-ition) */
+	offset = hpa - p->res->start;
+	offset = do_div(offset, gran * ways);
+	if ((offset >= pos * gran) && (offset < (pos + 1) * gran))
+		return true;
+
+	dev_dbg(&cxlr->dev,
+		"Addr trans fail: hpa 0x%llx not in expected chunk\n", hpa);
+
+	return false;
+}
+
+static u64 cxl_dpa_to_hpa(u64 dpa,  struct cxl_region *cxlr,
+			  struct cxl_endpoint_decoder *cxled)
+{
+	u64 dpa_offset, hpa_offset, bits_upper, mask_upper, hpa;
+	struct cxl_region_params *p = &cxlr->params;
+	int pos = cxled->pos;
+	u16 eig = 0;
+	u8 eiw = 0;
+
+	ways_to_eiw(p->interleave_ways, &eiw);
+	granularity_to_eig(p->interleave_granularity, &eig);
+
+	/*
+	 * The device position in the region interleave set was removed
+	 * from the offset at HPA->DPA translation. To reconstruct the
+	 * HPA, place the 'pos' in the offset.
+	 *
+	 * The placement of 'pos' in the HPA is determined by interleave
+	 * ways and granularity and is defined in the CXL Spec 3.0 Section
+	 * 8.2.4.19.13 Implementation Note: Device Decode Logic
+	 */
+
+	/* Remove the dpa base */
+	dpa_offset = dpa - cxl_dpa_resource_start(cxled);
+
+	mask_upper = GENMASK_ULL(51, eig + 8);
+
+	if (eiw < 8) {
+		hpa_offset = (dpa_offset & mask_upper) << eiw;
+		hpa_offset |= pos << (eig + 8);
+	} else {
+		bits_upper = (dpa_offset & mask_upper) >> (eig + 8);
+		bits_upper = bits_upper * 3;
+		hpa_offset = ((bits_upper << (eiw - 8)) + pos) << (eig + 8);
+	}
+
+	/* The lower bits remain unchanged */
+	hpa_offset |= dpa_offset & GENMASK_ULL(eig + 7, 0);
+
+	/* Apply the hpa_offset to the region base address */
+	hpa = hpa_offset + p->res->start;
+
+	if (!cxl_is_hpa_in_range(hpa, cxlr, cxled->pos))
+		return ULLONG_MAX;
+
+	return hpa;
+}
+
+u64 cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
+		  u64 dpa)
+{
+	struct cxl_region_params *p = &cxlr->params;
+	struct cxl_endpoint_decoder *cxled = NULL;
+
+	for (int i = 0; i <  p->nr_targets; i++) {
+		cxled = p->targets[i];
+		if (cxlmd == cxled_to_memdev(cxled))
+			break;
+	}
+	if (!cxled || cxlmd != cxled_to_memdev(cxled))
+		return ULLONG_MAX;
+
+	return cxl_dpa_to_hpa(dpa, cxlr, cxled);
+}
+
 static struct lock_class_key cxl_pmem_region_key;
 
 static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
diff --git a/drivers/cxl/core/trace.c b/drivers/cxl/core/trace.c
index d0403dc3c8ab..7f2a9dd0d0e3 100644
--- a/drivers/cxl/core/trace.c
+++ b/drivers/cxl/core/trace.c
@@ -6,94 +6,3 @@
 
 #define CREATE_TRACE_POINTS
 #include "trace.h"
-
-static bool cxl_is_hpa_in_range(u64 hpa, struct cxl_region *cxlr, int pos)
-{
-	struct cxl_region_params *p = &cxlr->params;
-	int gran = p->interleave_granularity;
-	int ways = p->interleave_ways;
-	u64 offset;
-
-	/* Is the hpa within this region at all */
-	if (hpa < p->res->start || hpa > p->res->end) {
-		dev_dbg(&cxlr->dev,
-			"Addr trans fail: hpa 0x%llx not in region\n", hpa);
-		return false;
-	}
-
-	/* Is the hpa in an expected chunk for its pos(-ition) */
-	offset = hpa - p->res->start;
-	offset = do_div(offset, gran * ways);
-	if ((offset >= pos * gran) && (offset < (pos + 1) * gran))
-		return true;
-
-	dev_dbg(&cxlr->dev,
-		"Addr trans fail: hpa 0x%llx not in expected chunk\n", hpa);
-
-	return false;
-}
-
-static u64 cxl_dpa_to_hpa(u64 dpa,  struct cxl_region *cxlr,
-			  struct cxl_endpoint_decoder *cxled)
-{
-	u64 dpa_offset, hpa_offset, bits_upper, mask_upper, hpa;
-	struct cxl_region_params *p = &cxlr->params;
-	int pos = cxled->pos;
-	u16 eig = 0;
-	u8 eiw = 0;
-
-	ways_to_eiw(p->interleave_ways, &eiw);
-	granularity_to_eig(p->interleave_granularity, &eig);
-
-	/*
-	 * The device position in the region interleave set was removed
-	 * from the offset at HPA->DPA translation. To reconstruct the
-	 * HPA, place the 'pos' in the offset.
-	 *
-	 * The placement of 'pos' in the HPA is determined by interleave
-	 * ways and granularity and is defined in the CXL Spec 3.0 Section
-	 * 8.2.4.19.13 Implementation Note: Device Decode Logic
-	 */
-
-	/* Remove the dpa base */
-	dpa_offset = dpa - cxl_dpa_resource_start(cxled);
-
-	mask_upper = GENMASK_ULL(51, eig + 8);
-
-	if (eiw < 8) {
-		hpa_offset = (dpa_offset & mask_upper) << eiw;
-		hpa_offset |= pos << (eig + 8);
-	} else {
-		bits_upper = (dpa_offset & mask_upper) >> (eig + 8);
-		bits_upper = bits_upper * 3;
-		hpa_offset = ((bits_upper << (eiw - 8)) + pos) << (eig + 8);
-	}
-
-	/* The lower bits remain unchanged */
-	hpa_offset |= dpa_offset & GENMASK_ULL(eig + 7, 0);
-
-	/* Apply the hpa_offset to the region base address */
-	hpa = hpa_offset + p->res->start;
-
-	if (!cxl_is_hpa_in_range(hpa, cxlr, cxled->pos))
-		return ULLONG_MAX;
-
-	return hpa;
-}
-
-u64 cxl_trace_hpa(struct cxl_region *cxlr, struct cxl_memdev *cxlmd,
-		  u64 dpa)
-{
-	struct cxl_region_params *p = &cxlr->params;
-	struct cxl_endpoint_decoder *cxled = NULL;
-
-	for (int i = 0; i <  p->nr_targets; i++) {
-		cxled = p->targets[i];
-		if (cxlmd == cxled_to_memdev(cxled))
-			break;
-	}
-	if (!cxled || cxlmd != cxled_to_memdev(cxled))
-		return ULLONG_MAX;
-
-	return cxl_dpa_to_hpa(dpa, cxlr, cxled);
-}
diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
index e5f13260fc52..161bdb5734b0 100644
--- a/drivers/cxl/core/trace.h
+++ b/drivers/cxl/core/trace.h
@@ -642,8 +642,6 @@ TRACE_EVENT(cxl_memory_module,
 #define cxl_poison_overflow(flags, time)				\
 	(flags & CXL_POISON_FLAG_OVERFLOW ? le64_to_cpu(time) : 0)
 
-u64 cxl_trace_hpa(struct cxl_region *cxlr, struct cxl_memdev *memdev, u64 dpa);
-
 TRACE_EVENT(cxl_poison,
 
 	TP_PROTO(struct cxl_memdev *cxlmd, struct cxl_region *cxlr,
-- 
2.37.3

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

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

User space may need to know which region, if any, maps the DPAs
(device physical addresses) reported in a cxl_general_media or
cxl_dram event. Since the mapping can change, the kernel provides
this information at the time the event occurs. This informs user
space that at event <timestamp> this <region> mapped this <DPA>
to this <HPA>.

Add the same region info that is included in the cxl_poison trace
event: the DPA->HPA translation, region name, and region uuid.
Introduce and use new helpers that lookup that region info using
the struct cxl_memdev and a DPA.

The new fields are inserted in the trace event and no existing
fields are modified. If the DPA is not mapped, user will see:
hpa=ULLONG_MAX, region="", and uuid=0

This work must be protected by dpa_rwsem & region_rwsem since
it is looking up region mappings.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/cxl/core/core.h   |  6 +++++
 drivers/cxl/core/mbox.c   | 17 ++++++++++---
 drivers/cxl/core/region.c |  8 ++++++
 drivers/cxl/core/trace.h  | 52 +++++++++++++++++++++++++++++++++++----
 4 files changed, 74 insertions(+), 9 deletions(-)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 625394486459..2fd8d9797f36 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -30,8 +30,14 @@ 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_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
 		  u64 dpa);
+const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa);
 
 #else
+static inline
+const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa)
+{
+	return "";
+}
 static inline u64
 cxl_trace_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 9adda4795eb7..3c1c37d5fcb0 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -842,14 +842,23 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
 			    enum cxl_event_type event_type,
 			    const uuid_t *uuid, union cxl_event *evt)
 {
+	if (event_type == CXL_CPER_EVENT_MEM_MODULE) {
+		trace_cxl_memory_module(cxlmd, type, &evt->mem_module);
+		return;
+	}
+	if (event_type == CXL_CPER_EVENT_GENERIC) {
+		trace_cxl_generic_event(cxlmd, type, uuid, &evt->generic);
+		return;
+	}
+
+	/* Protect trace events that do DPA->HPA translations */
+	guard(rwsem_read)(&cxl_region_rwsem);
+	guard(rwsem_read)(&cxl_dpa_rwsem);
+
 	if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
 		trace_cxl_general_media(cxlmd, type, &evt->gen_media);
 	else if (event_type == CXL_CPER_EVENT_DRAM)
 		trace_cxl_dram(cxlmd, type, &evt->dram);
-	else if (event_type == CXL_CPER_EVENT_MEM_MODULE)
-		trace_cxl_memory_module(cxlmd, type, &evt->mem_module);
-	else
-		trace_cxl_generic_event(cxlmd, type, uuid, &evt->generic);
 }
 EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL);
 
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 45eb9c560fd6..a5b1eaee1e58 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2723,6 +2723,14 @@ struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa)
 	return ctx.cxlr;
 }
 
+const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa)
+{
+	struct cxl_region *cxlr = cxl_dpa_to_region(cxlmd, dpa);
+
+	/* trace __string() assignment requires "", not NULL */
+	return cxlr ? dev_name(&cxlr->dev) : "";
+}
+
 static bool cxl_is_hpa_in_range(u64 hpa, struct cxl_region *cxlr, int pos)
 {
 	struct cxl_region_params *p = &cxlr->params;
diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
index 161bdb5734b0..2e24364b2b8d 100644
--- a/drivers/cxl/core/trace.h
+++ b/drivers/cxl/core/trace.h
@@ -14,6 +14,28 @@
 #include <cxlmem.h>
 #include "core.h"
 
+#ifndef __CXL_EVENTS_DECLARE_ONCE_ONLY
+#define __CXL_EVENTS_DECLARE_ONCE_ONLY
+static inline
+void store_region_info(const struct cxl_memdev *cxlmd, u64 dpa, uuid_t *uuid,
+		       u64 *hpa)
+{
+	struct cxl_region *cxlr;
+
+	cxlr = cxl_dpa_to_region(cxlmd, dpa);
+	if (cxlr) {
+		uuid_copy(uuid, &cxlr->params.uuid);
+		*hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
+	} else {
+		uuid_copy(uuid, &uuid_null);
+		*hpa = ULLONG_MAX;
+	}
+}
+#endif /* __CXL_EVENTS_DECLARE_ONCE_ONLY */
+
+#define rec_pa_to_dpa(record)						\
+	(le64_to_cpu(rec->phys_addr) & CXL_DPA_MASK)
+
 #define CXL_RAS_UC_CACHE_DATA_PARITY	BIT(0)
 #define CXL_RAS_UC_CACHE_ADDR_PARITY	BIT(1)
 #define CXL_RAS_UC_CACHE_BE_PARITY	BIT(2)
@@ -330,10 +352,14 @@ TRACE_EVENT(cxl_general_media,
 		__field(u8, channel)
 		__field(u32, device)
 		__array(u8, comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE)
-		__field(u16, validity_flags)
 		/* Following are out of order to pack trace record */
+		__field(u64, hpa)
+		__field_struct(uuid_t, region_uuid)
+		__field(u16, validity_flags)
 		__field(u8, rank)
 		__field(u8, dpa_flags)
+		__string(region_name,
+			 cxl_trace_to_region_name(cxlmd, rec_pa_to_dpa(record)))
 	),
 
 	TP_fast_assign(
@@ -354,18 +380,24 @@ TRACE_EVENT(cxl_general_media,
 		memcpy(__entry->comp_id, &rec->component_id,
 			CXL_EVENT_GEN_MED_COMP_ID_SIZE);
 		__entry->validity_flags = get_unaligned_le16(&rec->validity_flags);
+		__assign_str(region_name,
+			     cxl_trace_to_region_name(cxlmd, __entry->dpa));
+		store_region_info(cxlmd, __entry->dpa, &__entry->region_uuid,
+				  &__entry->hpa);
 	),
 
 	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'",
+		"device=%x comp_id=%s validity_flags='%s' " \
+		"hpa=%llx region=%s region_uuid=%pUb",
 		__entry->dpa, show_dpa_flags(__entry->dpa_flags),
 		show_event_desc_flags(__entry->descriptor),
 		show_mem_event_type(__entry->type),
 		show_trans_type(__entry->transaction_type),
 		__entry->channel, __entry->rank, __entry->device,
 		__print_hex(__entry->comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE),
-		show_valid_flags(__entry->validity_flags)
+		show_valid_flags(__entry->validity_flags),
+		__entry->hpa, __get_str(region_name), &__entry->region_uuid
 	)
 );
 
@@ -417,10 +449,14 @@ TRACE_EVENT(cxl_dram,
 		__field(u32, nibble_mask)
 		__field(u32, row)
 		__array(u8, cor_mask, CXL_EVENT_DER_CORRECTION_MASK_SIZE)
+		__field(u64, hpa)
+		__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 */
 		__field(u8, bank)	/* Out of order to pack trace record */
 		__field(u8, dpa_flags)	/* Out of order to pack trace record */
+		__string(region_name,
+			 cxl_trace_to_region_name(cxlmd, rec_pa_to_dpa(record)))
 	),
 
 	TP_fast_assign(
@@ -444,12 +480,17 @@ TRACE_EVENT(cxl_dram,
 		__entry->column = get_unaligned_le16(rec->column);
 		memcpy(__entry->cor_mask, &rec->correction_mask,
 			CXL_EVENT_DER_CORRECTION_MASK_SIZE);
+		__assign_str(region_name,
+			     cxl_trace_to_region_name(cxlmd, __entry->dpa));
+		store_region_info(cxlmd, __entry->dpa, &__entry->region_uuid,
+				  &__entry->hpa);
 	),
 
 	CXL_EVT_TP_printk("dpa=%llx dpa_flags='%s' descriptor='%s' type='%s' " \
 		"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'",
+		"validity_flags='%s' " \
+		"hpa=%llx region=%s region_uuid=%pUb",
 		__entry->dpa, show_dpa_flags(__entry->dpa_flags),
 		show_event_desc_flags(__entry->descriptor),
 		show_mem_event_type(__entry->type),
@@ -458,7 +499,8 @@ TRACE_EVENT(cxl_dram,
 		__entry->bank_group, __entry->bank,
 		__entry->row, __entry->column,
 		__print_hex(__entry->cor_mask, CXL_EVENT_DER_CORRECTION_MASK_SIZE),
-		show_dram_valid_flags(__entry->validity_flags)
+		show_dram_valid_flags(__entry->validity_flags),
+		__entry->hpa, __get_str(region_name), &__entry->region_uuid
 	)
 );
 
-- 
2.37.3

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

[Ira Weiny]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 

[snip]

> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> index 161bdb5734b0..2e24364b2b8d 100644
> --- a/drivers/cxl/core/trace.h
> +++ b/drivers/cxl/core/trace.h
> @@ -14,6 +14,28 @@
>  #include <cxlmem.h>
>  #include "core.h"
>  
> +#ifndef __CXL_EVENTS_DECLARE_ONCE_ONLY
> +#define __CXL_EVENTS_DECLARE_ONCE_ONLY
> +static inline
> +void store_region_info(const struct cxl_memdev *cxlmd, u64 dpa, uuid_t *uuid,
> +		       u64 *hpa)
> +{
> +	struct cxl_region *cxlr;
> +
> +	cxlr = cxl_dpa_to_region(cxlmd, dpa);

This would normally be:  (somewhat nitty though...)

	if (!cxlr) {
		uuid_copy(uuid, &uuid_null);
		*hpa = ULLONG_MAX;
		return;
	}

	uuid_copy(uuid, &cxlr->params.uuid);
	*hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
}

But in this context I don't think it is critical.

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


[snip]

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

[Alison Schofield]

On Mon, Apr 22, 2024 at 09:23:32PM -0700, Ira Weiny wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> 
> [snip]
> 
> > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> > index 161bdb5734b0..2e24364b2b8d 100644
> > --- a/drivers/cxl/core/trace.h
> > +++ b/drivers/cxl/core/trace.h
> > @@ -14,6 +14,28 @@
> >  #include <cxlmem.h>
> >  #include "core.h"
> >  
> > +#ifndef __CXL_EVENTS_DECLARE_ONCE_ONLY
> > +#define __CXL_EVENTS_DECLARE_ONCE_ONLY
> > +static inline
> > +void store_region_info(const struct cxl_memdev *cxlmd, u64 dpa, uuid_t *uuid,
> > +		       u64 *hpa)
> > +{
> > +	struct cxl_region *cxlr;
> > +
> > +	cxlr = cxl_dpa_to_region(cxlmd, dpa);
> 
> This would normally be:  (somewhat nitty though...)
> 
> 	if (!cxlr) {
> 		uuid_copy(uuid, &uuid_null);
> 		*hpa = ULLONG_MAX;
> 		return;
> 	}
> 
> 	uuid_copy(uuid, &cxlr->params.uuid);
> 	*hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
> }
> 
> But in this context I don't think it is critical.

Thanks for pointing out. When I switch to inline I didn't
'see' that. I'd like to fix it up. Let me see what else 
comes in.

--Alison

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

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

[Dan Williams]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> User space may need to know which region, if any, maps the DPAs
> (device physical addresses) reported in a cxl_general_media or
> cxl_dram event. Since the mapping can change, the kernel provides
> this information at the time the event occurs. This informs user
> space that at event <timestamp> this <region> mapped this <DPA>
> to this <HPA>.
> 
> Add the same region info that is included in the cxl_poison trace
> event: the DPA->HPA translation, region name, and region uuid.
> Introduce and use new helpers that lookup that region info using
> the struct cxl_memdev and a DPA.
> 
> The new fields are inserted in the trace event and no existing
> fields are modified. If the DPA is not mapped, user will see:
> hpa=ULLONG_MAX, region="", and uuid=0
> 
> This work must be protected by dpa_rwsem & region_rwsem since
> it is looking up region mappings.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/cxl/core/core.h   |  6 +++++
>  drivers/cxl/core/mbox.c   | 17 ++++++++++---
>  drivers/cxl/core/region.c |  8 ++++++
>  drivers/cxl/core/trace.h  | 52 +++++++++++++++++++++++++++++++++++----
>  4 files changed, 74 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 625394486459..2fd8d9797f36 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -30,8 +30,14 @@ 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_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
>  		  u64 dpa);
> +const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa);
>  
>  #else
> +static inline
> +const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa)
> +{
> +	return "";
> +}
>  static inline u64
>  cxl_trace_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 9adda4795eb7..3c1c37d5fcb0 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -842,14 +842,23 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
>  			    enum cxl_event_type event_type,
>  			    const uuid_t *uuid, union cxl_event *evt)
>  {
> +	if (event_type == CXL_CPER_EVENT_MEM_MODULE) {
> +		trace_cxl_memory_module(cxlmd, type, &evt->mem_module);
> +		return;
> +	}
> +	if (event_type == CXL_CPER_EVENT_GENERIC) {
> +		trace_cxl_generic_event(cxlmd, type, uuid, &evt->generic);
> +		return;
> +	}
> +
> +	/* Protect trace events that do DPA->HPA translations */
> +	guard(rwsem_read)(&cxl_region_rwsem);
> +	guard(rwsem_read)(&cxl_dpa_rwsem);
> +
>  	if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
>  		trace_cxl_general_media(cxlmd, type, &evt->gen_media);
>  	else if (event_type == CXL_CPER_EVENT_DRAM)
>  		trace_cxl_dram(cxlmd, type, &evt->dram);
> -	else if (event_type == CXL_CPER_EVENT_MEM_MODULE)
> -		trace_cxl_memory_module(cxlmd, type, &evt->mem_module);
> -	else
> -		trace_cxl_generic_event(cxlmd, type, uuid, &evt->generic);
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL);
>  
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 45eb9c560fd6..a5b1eaee1e58 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2723,6 +2723,14 @@ struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa)
>  	return ctx.cxlr;
>  }
>  
> +const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa)
> +{
> +	struct cxl_region *cxlr = cxl_dpa_to_region(cxlmd, dpa);
> +
> +	/* trace __string() assignment requires "", not NULL */
> +	return cxlr ? dev_name(&cxlr->dev) : "";
> +}
> +
>  static bool cxl_is_hpa_in_range(u64 hpa, struct cxl_region *cxlr, int pos)
>  {
>  	struct cxl_region_params *p = &cxlr->params;
> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> index 161bdb5734b0..2e24364b2b8d 100644
> --- a/drivers/cxl/core/trace.h
> +++ b/drivers/cxl/core/trace.h
> @@ -14,6 +14,28 @@
>  #include <cxlmem.h>
>  #include "core.h"
>  
> +#ifndef __CXL_EVENTS_DECLARE_ONCE_ONLY
> +#define __CXL_EVENTS_DECLARE_ONCE_ONLY
> +static inline
> +void store_region_info(const struct cxl_memdev *cxlmd, u64 dpa, uuid_t *uuid,
> +		       u64 *hpa)
> +{
> +	struct cxl_region *cxlr;
> +
> +	cxlr = cxl_dpa_to_region(cxlmd, dpa);
> +	if (cxlr) {
> +		uuid_copy(uuid, &cxlr->params.uuid);
> +		*hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
> +	} else {
> +		uuid_copy(uuid, &uuid_null);
> +		*hpa = ULLONG_MAX;
> +	}
> +}
> +#endif /* __CXL_EVENTS_DECLARE_ONCE_ONLY */

This ifdef usage looks awkward...

> +
> +#define rec_pa_to_dpa(record)						\
> +	(le64_to_cpu(rec->phys_addr) & CXL_DPA_MASK)
> +
>  #define CXL_RAS_UC_CACHE_DATA_PARITY	BIT(0)
>  #define CXL_RAS_UC_CACHE_ADDR_PARITY	BIT(1)
>  #define CXL_RAS_UC_CACHE_BE_PARITY	BIT(2)
> @@ -330,10 +352,14 @@ TRACE_EVENT(cxl_general_media,
>  		__field(u8, channel)
>  		__field(u32, device)
>  		__array(u8, comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE)
> -		__field(u16, validity_flags)
>  		/* Following are out of order to pack trace record */
> +		__field(u64, hpa)
> +		__field_struct(uuid_t, region_uuid)
> +		__field(u16, validity_flags)
>  		__field(u8, rank)
>  		__field(u8, dpa_flags)
> +		__string(region_name,
> +			 cxl_trace_to_region_name(cxlmd, rec_pa_to_dpa(record)))

...and this looks complicated.

A bit too much dynamic resolution happening within the trace function
for my taste. Just do the region lookup in cxl_event_trace_record() and
pass it in. That also makes the rwsem usage more apparent rather than
digging through trace to find the dependency.

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

[Alison Schofield]

On Tue, Apr 23, 2024 at 10:17:44PM -0700, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > User space may need to know which region, if any, maps the DPAs
> > (device physical addresses) reported in a cxl_general_media or
> > cxl_dram event. Since the mapping can change, the kernel provides
> > this information at the time the event occurs. This informs user
> > space that at event <timestamp> this <region> mapped this <DPA>
> > to this <HPA>.
> > 
> > Add the same region info that is included in the cxl_poison trace
> > event: the DPA->HPA translation, region name, and region uuid.
> > Introduce and use new helpers that lookup that region info using
> > the struct cxl_memdev and a DPA.
> > 
> > The new fields are inserted in the trace event and no existing
> > fields are modified. If the DPA is not mapped, user will see:
> > hpa=ULLONG_MAX, region="", and uuid=0
> > 
> > This work must be protected by dpa_rwsem & region_rwsem since
> > it is looking up region mappings.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  drivers/cxl/core/core.h   |  6 +++++
> >  drivers/cxl/core/mbox.c   | 17 ++++++++++---
> >  drivers/cxl/core/region.c |  8 ++++++
> >  drivers/cxl/core/trace.h  | 52 +++++++++++++++++++++++++++++++++++----
> >  4 files changed, 74 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> > index 625394486459..2fd8d9797f36 100644
> > --- a/drivers/cxl/core/core.h
> > +++ b/drivers/cxl/core/core.h
> > @@ -30,8 +30,14 @@ 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_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> >  		  u64 dpa);
> > +const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa);
> >  
> >  #else
> > +static inline
> > +const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa)
> > +{
> > +	return "";
> > +}
> >  static inline u64
> >  cxl_trace_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 9adda4795eb7..3c1c37d5fcb0 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -842,14 +842,23 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> >  			    enum cxl_event_type event_type,
> >  			    const uuid_t *uuid, union cxl_event *evt)
> >  {
> > +	if (event_type == CXL_CPER_EVENT_MEM_MODULE) {
> > +		trace_cxl_memory_module(cxlmd, type, &evt->mem_module);
> > +		return;
> > +	}
> > +	if (event_type == CXL_CPER_EVENT_GENERIC) {
> > +		trace_cxl_generic_event(cxlmd, type, uuid, &evt->generic);
> > +		return;
> > +	}
> > +
> > +	/* Protect trace events that do DPA->HPA translations */
> > +	guard(rwsem_read)(&cxl_region_rwsem);
> > +	guard(rwsem_read)(&cxl_dpa_rwsem);
> > +
> >  	if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
> >  		trace_cxl_general_media(cxlmd, type, &evt->gen_media);
> >  	else if (event_type == CXL_CPER_EVENT_DRAM)
> >  		trace_cxl_dram(cxlmd, type, &evt->dram);
> > -	else if (event_type == CXL_CPER_EVENT_MEM_MODULE)
> > -		trace_cxl_memory_module(cxlmd, type, &evt->mem_module);
> > -	else
> > -		trace_cxl_generic_event(cxlmd, type, uuid, &evt->generic);
> >  }
> >  EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL);
> >  
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 45eb9c560fd6..a5b1eaee1e58 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -2723,6 +2723,14 @@ struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa)
> >  	return ctx.cxlr;
> >  }
> >  
> > +const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa)
> > +{
> > +	struct cxl_region *cxlr = cxl_dpa_to_region(cxlmd, dpa);
> > +
> > +	/* trace __string() assignment requires "", not NULL */
> > +	return cxlr ? dev_name(&cxlr->dev) : "";
> > +}
> > +
> >  static bool cxl_is_hpa_in_range(u64 hpa, struct cxl_region *cxlr, int pos)
> >  {
> >  	struct cxl_region_params *p = &cxlr->params;
> > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> > index 161bdb5734b0..2e24364b2b8d 100644
> > --- a/drivers/cxl/core/trace.h
> > +++ b/drivers/cxl/core/trace.h
> > @@ -14,6 +14,28 @@
> >  #include <cxlmem.h>
> >  #include "core.h"
> >  
> > +#ifndef __CXL_EVENTS_DECLARE_ONCE_ONLY
> > +#define __CXL_EVENTS_DECLARE_ONCE_ONLY
> > +static inline
> > +void store_region_info(const struct cxl_memdev *cxlmd, u64 dpa, uuid_t *uuid,
> > +		       u64 *hpa)
> > +{
> > +	struct cxl_region *cxlr;
> > +
> > +	cxlr = cxl_dpa_to_region(cxlmd, dpa);
> > +	if (cxlr) {
> > +		uuid_copy(uuid, &cxlr->params.uuid);
> > +		*hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
> > +	} else {
> > +		uuid_copy(uuid, &uuid_null);
> > +		*hpa = ULLONG_MAX;
> > +	}
> > +}
> > +#endif /* __CXL_EVENTS_DECLARE_ONCE_ONLY */
> 
> This ifdef usage looks awkward...

I only mimic'd others that defined static inline funcs in trace header files.
I originally thought it would be protected from this define that already
wraps this header file content, but it doesn't.

#if !defined(_CXL_EVENTS_H) || defined(TRACE_HEADER_MULTI_READ)
#define _CXL_EVENTS_H

> 
> > +
> > +#define rec_pa_to_dpa(record)						\
> > +	(le64_to_cpu(rec->phys_addr) & CXL_DPA_MASK)
> > +
> >  #define CXL_RAS_UC_CACHE_DATA_PARITY	BIT(0)
> >  #define CXL_RAS_UC_CACHE_ADDR_PARITY	BIT(1)
> >  #define CXL_RAS_UC_CACHE_BE_PARITY	BIT(2)
> > @@ -330,10 +352,14 @@ TRACE_EVENT(cxl_general_media,
> >  		__field(u8, channel)
> >  		__field(u32, device)
> >  		__array(u8, comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE)
> > -		__field(u16, validity_flags)
> >  		/* Following are out of order to pack trace record */
> > +		__field(u64, hpa)
> > +		__field_struct(uuid_t, region_uuid)
> > +		__field(u16, validity_flags)
> >  		__field(u8, rank)
> >  		__field(u8, dpa_flags)
> > +		__string(region_name,
> > +			 cxl_trace_to_region_name(cxlmd, rec_pa_to_dpa(record)))
> 
> ...and this looks complicated.
> 
> A bit too much dynamic resolution happening within the trace function
> for my taste. Just do the region lookup in cxl_event_trace_record() and
> pass it in. That also makes the rwsem usage more apparent rather than
> digging through trace to find the dependency.

When these cxl_general_media,dram,poison trace events were originally
created once of the goals was to push work down *here* so that if the
trace events were not enable, no needless work is done.

When you suggest doing the region lookup before calling the trace handler,
I'm thinking something like this where the region lookup work still gets
skipped if tracing is not enabled:

	if (trace_cxl_general_media_enabled()) {
		cxlr = lookup_region(cxlmd, record);
		trace_cxl_general_media(...., cxlr);

-- Alison

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

[Dan Williams]

Alison Schofield wrote:
> On Tue, Apr 23, 2024 at 10:17:44PM -0700, Dan Williams wrote:
> > alison.schofield@ wrote:
> > > From: Alison Schofield <alison.schofield@intel.com>
> > > 
> > > User space may need to know which region, if any, maps the DPAs
> > > (device physical addresses) reported in a cxl_general_media or
> > > cxl_dram event. Since the mapping can change, the kernel provides
> > > this information at the time the event occurs. This informs user
> > > space that at event <timestamp> this <region> mapped this <DPA>
> > > to this <HPA>.
> > > 
> > > Add the same region info that is included in the cxl_poison trace
> > > event: the DPA->HPA translation, region name, and region uuid.
> > > Introduce and use new helpers that lookup that region info using
> > > the struct cxl_memdev and a DPA.
> > > 
> > > The new fields are inserted in the trace event and no existing
> > > fields are modified. If the DPA is not mapped, user will see:
> > > hpa=ULLONG_MAX, region="", and uuid=0
> > > 
> > > This work must be protected by dpa_rwsem & region_rwsem since
> > > it is looking up region mappings.
> > > 
> > > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
[..]
> > > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> > > index 161bdb5734b0..2e24364b2b8d 100644
> > > --- a/drivers/cxl/core/trace.h
> > > +++ b/drivers/cxl/core/trace.h
> > > @@ -14,6 +14,28 @@
> > >  #include <cxlmem.h>
> > >  #include "core.h"
> > >  
> > > +#ifndef __CXL_EVENTS_DECLARE_ONCE_ONLY
> > > +#define __CXL_EVENTS_DECLARE_ONCE_ONLY
> > > +static inline
> > > +void store_region_info(const struct cxl_memdev *cxlmd, u64 dpa, uuid_t *uuid,
> > > +		       u64 *hpa)
> > > +{
> > > +	struct cxl_region *cxlr;
> > > +
> > > +	cxlr = cxl_dpa_to_region(cxlmd, dpa);
> > > +	if (cxlr) {
> > > +		uuid_copy(uuid, &cxlr->params.uuid);
> > > +		*hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
> > > +	} else {
> > > +		uuid_copy(uuid, &uuid_null);
> > > +		*hpa = ULLONG_MAX;
> > > +	}
> > > +}
> > > +#endif /* __CXL_EVENTS_DECLARE_ONCE_ONLY */
> > 
> > This ifdef usage looks awkward...
> 
> I only mimic'd others that defined static inline funcs in trace header files.

That's fine, but that awkward trace hack can be skipped altogether if an
@region argument is passed to the tracepoints.

> I originally thought it would be protected from this define that already
> wraps this header file content, but it doesn't.
> 
> #if !defined(_CXL_EVENTS_H) || defined(TRACE_HEADER_MULTI_READ)
> #define _CXL_EVENTS_H

That's what TRACE_HEADER_MULTI_READ allows which is needed for the
tricky way that tracepoints are built.

> > > +
> > > +#define rec_pa_to_dpa(record)						\
> > > +	(le64_to_cpu(rec->phys_addr) & CXL_DPA_MASK)
> > > +
> > >  #define CXL_RAS_UC_CACHE_DATA_PARITY	BIT(0)
> > >  #define CXL_RAS_UC_CACHE_ADDR_PARITY	BIT(1)
> > >  #define CXL_RAS_UC_CACHE_BE_PARITY	BIT(2)
> > > @@ -330,10 +352,14 @@ TRACE_EVENT(cxl_general_media,
> > >  		__field(u8, channel)
> > >  		__field(u32, device)
> > >  		__array(u8, comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE)
> > > -		__field(u16, validity_flags)
> > >  		/* Following are out of order to pack trace record */
> > > +		__field(u64, hpa)
> > > +		__field_struct(uuid_t, region_uuid)
> > > +		__field(u16, validity_flags)
> > >  		__field(u8, rank)
> > >  		__field(u8, dpa_flags)
> > > +		__string(region_name,
> > > +			 cxl_trace_to_region_name(cxlmd, rec_pa_to_dpa(record)))
> > 
> > ...and this looks complicated.
> > 
> > A bit too much dynamic resolution happening within the trace function
> > for my taste. Just do the region lookup in cxl_event_trace_record() and
> > pass it in. That also makes the rwsem usage more apparent rather than
> > digging through trace to find the dependency.
> 
> When these cxl_general_media,dram,poison trace events were originally
> created once of the goals was to push work down *here* so that if the
> trace events were not enable, no needless work is done.

The moment a code path needs to hold a lock, (2 locks in this case!),
over a tracepoint it has already incurred significant overhead. So if
the name of the game is "skip doing unnecessary work when the tracepoint
is disabled", then just check if the tracepoint is disabled, something
like:

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 60a51ea3ff25..31f44101582e 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -851,14 +851,23 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
                return;
        }
 
-       /* Protect trace events that do DPA->HPA translations */
-       guard(rwsem_read)(&cxl_region_rwsem);
-       guard(rwsem_read)(&cxl_dpa_rwsem);
-
-       if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
-               trace_cxl_general_media(cxlmd, type, &evt->gen_media);
-       else if (event_type == CXL_CPER_EVENT_DRAM)
-               trace_cxl_dram(cxlmd, type, &evt->dram);
+       if (trace_cxl_general_media_enabled() || trace_cxl_dram_enabled()) {
+               /*
+                * These trace points are annotated with HPA and corresponding
+                * region translation. 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);
+
+               region = ...
+               hpa = ...
+
+               if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
+                       trace_cxl_general_media(cxlmd, type, region, hpa, &evt->gen_media);
+               else if (event_type == CXL_CPER_EVENT_DRAM)
+                       trace_cxl_dram(cxlmd, type, region, hpa, &evt->dram);
+       }
 }
 EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL);
 
> When you suggest doing the region lookup before calling the trace handler,
> I'm thinking something like this where the region lookup work still gets
> skipped if tracing is not enabled:
> 
> 	if (trace_cxl_general_media_enabled()) {
> 		cxlr = lookup_region(cxlmd, record);
> 		trace_cxl_general_media(...., cxlr);
> 

I am chuckling because I wrote all of the above diatribe after finishing
that "no needless work is done sentence". After building the example,
compile testing a version of it, and pasting it into the mail *then* I
read this next paragraph.

So yes, we came to the same conclusion. Please use trace_*_enabled()
which makes it clear what the locks are protecting. Some
lockdep_assert_held() usage in the lookup helpers would not hurt either.

[PATCH v2 4/4] cxl/core: Remove cxlr dependency from cxl_poison trace events

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

cxl_poison trace events pass a pointer to a struct cxl_region
when poison is discovered in a mapped endpoint. This made for
easy look up of region name, uuid, and was useful in starting
the dpa->hpa translation.

Another set of trace helpers is now available that eliminates
the need to pass on that cxlr. (It was passed along a lot!)

In addition to tidying up the cxl_poison calling path, shifting
to the new helpers also means all CXL trace events will share
the same code in that regard.

Switch from a uuid array to the field_struct type uuid_t to
align on one uuid format in all CXL trace events. That is useful
when sharing region uuid lookup helpers.

No externally visible naming changes are made to cxl_poison trace
events.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/cxl/core/mbox.c   |  5 ++---
 drivers/cxl/core/memdev.c |  8 ++++----
 drivers/cxl/core/region.c |  8 ++++----
 drivers/cxl/core/trace.h  | 27 +++++++++++----------------
 drivers/cxl/cxlmem.h      |  3 +--
 5 files changed, 22 insertions(+), 29 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 3c1c37d5fcb0..60a51ea3ff25 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1299,8 +1299,7 @@ int cxl_set_timestamp(struct cxl_memdev_state *mds)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_set_timestamp, CXL);
 
-int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
-		       struct cxl_region *cxlr)
+int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len)
 {
 	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
 	struct cxl_mbox_poison_out *po;
@@ -1332,7 +1331,7 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
 			break;
 
 		for (int i = 0; i < le16_to_cpu(po->count); i++)
-			trace_cxl_poison(cxlmd, cxlr, &po->record[i],
+			trace_cxl_poison(cxlmd, &po->record[i],
 					 po->flags, po->overflow_ts,
 					 CXL_POISON_TRACE_LIST);
 
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 0277726afd04..1a0b596da7b6 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -200,14 +200,14 @@ static int cxl_get_poison_by_memdev(struct cxl_memdev *cxlmd)
 	if (resource_size(&cxlds->pmem_res)) {
 		offset = cxlds->pmem_res.start;
 		length = resource_size(&cxlds->pmem_res);
-		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
+		rc = cxl_mem_get_poison(cxlmd, offset, length);
 		if (rc)
 			return rc;
 	}
 	if (resource_size(&cxlds->ram_res)) {
 		offset = cxlds->ram_res.start;
 		length = resource_size(&cxlds->ram_res);
-		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
+		rc = cxl_mem_get_poison(cxlmd, offset, length);
 		/*
 		 * Invalid Physical Address is not an error for
 		 * volatile addresses. Device support is optional.
@@ -321,7 +321,7 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
 		.address = cpu_to_le64(dpa),
 		.length = cpu_to_le32(1),
 	};
-	trace_cxl_poison(cxlmd, cxlr, &record, 0, 0, CXL_POISON_TRACE_INJECT);
+	trace_cxl_poison(cxlmd, &record, 0, 0, CXL_POISON_TRACE_INJECT);
 out:
 	up_read(&cxl_dpa_rwsem);
 	up_read(&cxl_region_rwsem);
@@ -385,7 +385,7 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
 		.address = cpu_to_le64(dpa),
 		.length = cpu_to_le32(1),
 	};
-	trace_cxl_poison(cxlmd, cxlr, &record, 0, 0, CXL_POISON_TRACE_CLEAR);
+	trace_cxl_poison(cxlmd, &record, 0, 0, CXL_POISON_TRACE_CLEAR);
 out:
 	up_read(&cxl_dpa_rwsem);
 	up_read(&cxl_region_rwsem);
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index a5b1eaee1e58..8cd057fc212c 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2585,7 +2585,7 @@ static int cxl_get_poison_unmapped(struct cxl_memdev *cxlmd,
 	if (ctx->mode == CXL_DECODER_RAM) {
 		offset = ctx->offset;
 		length = resource_size(&cxlds->ram_res) - offset;
-		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
+		rc = cxl_mem_get_poison(cxlmd, offset, length);
 		if (rc == -EFAULT)
 			rc = 0;
 		if (rc)
@@ -2603,7 +2603,7 @@ static int cxl_get_poison_unmapped(struct cxl_memdev *cxlmd,
 		return 0;
 	}
 
-	return cxl_mem_get_poison(cxlmd, offset, length, NULL);
+	return cxl_mem_get_poison(cxlmd, offset, length);
 }
 
 static int poison_by_decoder(struct device *dev, void *arg)
@@ -2637,7 +2637,7 @@ static int poison_by_decoder(struct device *dev, void *arg)
 	if (cxled->skip) {
 		offset = cxled->dpa_res->start - cxled->skip;
 		length = cxled->skip;
-		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
+		rc = cxl_mem_get_poison(cxlmd, offset, length);
 		if (rc == -EFAULT && cxled->mode == CXL_DECODER_RAM)
 			rc = 0;
 		if (rc)
@@ -2646,7 +2646,7 @@ static int poison_by_decoder(struct device *dev, void *arg)
 
 	offset = cxled->dpa_res->start;
 	length = cxled->dpa_res->end - offset + 1;
-	rc = cxl_mem_get_poison(cxlmd, offset, length, cxled->cxld.region);
+	rc = cxl_mem_get_poison(cxlmd, offset, length);
 	if (rc == -EFAULT && cxled->mode == CXL_DECODER_RAM)
 		rc = 0;
 	if (rc)
diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
index 2e24364b2b8d..1cfdd1558e6c 100644
--- a/drivers/cxl/core/trace.h
+++ b/drivers/cxl/core/trace.h
@@ -686,23 +686,24 @@ TRACE_EVENT(cxl_memory_module,
 
 TRACE_EVENT(cxl_poison,
 
-	TP_PROTO(struct cxl_memdev *cxlmd, struct cxl_region *cxlr,
+	TP_PROTO(const struct cxl_memdev *cxlmd,
 		 const struct cxl_poison_record *record, u8 flags,
 		 __le64 overflow_ts, enum cxl_poison_trace_type trace_type),
 
-	TP_ARGS(cxlmd, cxlr, record, flags, overflow_ts, trace_type),
+	TP_ARGS(cxlmd, record, flags, overflow_ts, trace_type),
 
 	TP_STRUCT__entry(
 		__string(memdev, dev_name(&cxlmd->dev))
 		__string(host, dev_name(cxlmd->dev.parent))
 		__field(u64, serial)
 		__field(u8, trace_type)
-		__string(region, cxlr ? dev_name(&cxlr->dev) : "")
+		__string(region,
+			 cxl_trace_to_region_name(cxlmd, cxl_poison_record_dpa(record)))
 		__field(u64, overflow_ts)
 		__field(u64, hpa)
 		__field(u64, dpa)
 		__field(u32, dpa_length)
-		__array(char, uuid, 16)
+		__field_struct(uuid_t, uuid)
 		__field(u8, source)
 		__field(u8, flags)
 	    ),
@@ -717,27 +718,21 @@ TRACE_EVENT(cxl_poison,
 		__entry->source = cxl_poison_record_source(record);
 		__entry->trace_type = trace_type;
 		__entry->flags = flags;
-		if (cxlr) {
-			__assign_str(region, dev_name(&cxlr->dev));
-			memcpy(__entry->uuid, &cxlr->params.uuid, 16);
-			__entry->hpa = cxl_trace_hpa(cxlr, cxlmd,
-						     __entry->dpa);
-		} else {
-			__assign_str(region, "");
-			memset(__entry->uuid, 0, 16);
-			__entry->hpa = ULLONG_MAX;
-		}
+		__assign_str(region,
+			     cxl_trace_to_region_name(cxlmd, __entry->dpa));
+		store_region_info(cxlmd, __entry->dpa, &__entry->uuid,
+				  &__entry->hpa);
 	    ),
 
 	TP_printk("memdev=%s host=%s serial=%lld trace_type=%s region=%s "  \
-		"region_uuid=%pU hpa=0x%llx dpa=0x%llx dpa_length=0x%x "    \
+		"region_uuid=%pUb hpa=0x%llx dpa=0x%llx dpa_length=0x%x "   \
 		"source=%s flags=%s overflow_time=%llu",
 		__get_str(memdev),
 		__get_str(host),
 		__entry->serial,
 		show_poison_trace_type(__entry->trace_type),
 		__get_str(region),
-		__entry->uuid,
+		&__entry->uuid,
 		__entry->hpa,
 		__entry->dpa,
 		__entry->dpa_length,
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 20fb3b35e89e..a733b31b7799 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -828,8 +828,7 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
 			    const uuid_t *uuid, union cxl_event *evt);
 int cxl_set_timestamp(struct cxl_memdev_state *mds);
 int cxl_poison_state_init(struct cxl_memdev_state *mds);
-int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
-		       struct cxl_region *cxlr);
+int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len);
 int cxl_trigger_poison_list(struct cxl_memdev *cxlmd);
 int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa);
 int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa);
-- 
2.37.3

Re: [PATCH v2 4/4] cxl/core: Remove cxlr dependency from cxl_poison trace events

[Dan Williams]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> cxl_poison trace events pass a pointer to a struct cxl_region
> when poison is discovered in a mapped endpoint. This made for
> easy look up of region name, uuid, and was useful in starting
> the dpa->hpa translation.
> 
> Another set of trace helpers is now available that eliminates
> the need to pass on that cxlr. (It was passed along a lot!)
> 
> In addition to tidying up the cxl_poison calling path, shifting
> to the new helpers also means all CXL trace events will share
> the same code in that regard.
> 
> Switch from a uuid array to the field_struct type uuid_t to
> align on one uuid format in all CXL trace events. That is useful
> when sharing region uuid lookup helpers.
> 
> No externally visible naming changes are made to cxl_poison trace
> events.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/cxl/core/mbox.c   |  5 ++---
>  drivers/cxl/core/memdev.c |  8 ++++----
>  drivers/cxl/core/region.c |  8 ++++----
>  drivers/cxl/core/trace.h  | 27 +++++++++++----------------
>  drivers/cxl/cxlmem.h      |  3 +--
>  5 files changed, 22 insertions(+), 29 deletions(-)

A lot of the thrash here is about removing cxlr as an argument, and per
the comment on patch3 I think it is cleaner to go the other way and
always pass it in.

Re: [PATCH v2 4/4] cxl/core: Remove cxlr dependency from cxl_poison trace events

[Alison Schofield]

On Tue, Apr 23, 2024 at 10:33:05PM -0700, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > cxl_poison trace events pass a pointer to a struct cxl_region
> > when poison is discovered in a mapped endpoint. This made for
> > easy look up of region name, uuid, and was useful in starting
> > the dpa->hpa translation.
> > 
> > Another set of trace helpers is now available that eliminates
> > the need to pass on that cxlr. (It was passed along a lot!)
> > 
> > In addition to tidying up the cxl_poison calling path, shifting
> > to the new helpers also means all CXL trace events will share
> > the same code in that regard.
> > 
> > Switch from a uuid array to the field_struct type uuid_t to
> > align on one uuid format in all CXL trace events. That is useful
> > when sharing region uuid lookup helpers.
> > 
> > No externally visible naming changes are made to cxl_poison trace
> > events.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  drivers/cxl/core/mbox.c   |  5 ++---
> >  drivers/cxl/core/memdev.c |  8 ++++----
> >  drivers/cxl/core/region.c |  8 ++++----
> >  drivers/cxl/core/trace.h  | 27 +++++++++++----------------
> >  drivers/cxl/cxlmem.h      |  3 +--
> >  5 files changed, 22 insertions(+), 29 deletions(-)
> 
> A lot of the thrash here is about removing cxlr as an argument, and per
> the comment on patch3 I think it is cleaner to go the other way and
> always pass it in.

I still think we can mimic what is done for general_media and dram
trace events. ie. lookup the region only if trace_cxl_poison_enabled().
as you suggested in previous patch.

I can see why you call it thrash. I'll counter that with the fact that
it is a fairly easy to read diff, and the fact that removing the passed
along cxlr param, is pretty much 'tested' when it compiles.

-- Alison