On 12/24/24 5:11 AM, Jonathan Cameron wrote:
> On Wed, 4 Dec 2024 15:46:48 -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 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.
>
>
> 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);
>
