On Fri, 10 Jan 2025 10:27:52 -0700
Dave Jiang <dave.jiang@xxxxxxxxx> wrote:
> On 1/10/25 9:32 AM, Jonathan Cameron wrote:
> > On Fri, 10 Jan 2025 08:17:46 -0700
> > Dave Jiang <dave.jiang@xxxxxxxxx> 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@xxxxxxxxx>
> > 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
>
> DJ
>
> >
> > Other than tidying that up this looks fine to me.
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >
> >> ---
> >> 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);
> >> }
> >> }
> >
> >
>
>
