Re: [PATCH v4 2/2] cxl/edac: Mask DPA flags in the event record cache key

[Li Ming]

在 2026/11/2 20:32, Jongwon Lee 写道:
> The low bits of the DPA in a General Media or DRAM event record are
> flags: bit 0 is VOLATILE and bit 1 is NOT_REPAIRABLE. The trace events
> report the DPA with CXL_DPA_MASK applied, but cxl_store_rec_gen_media()
> and cxl_store_rec_dram() use the raw value as the xarray key.
>
> User space reads the DPA from the trace event and writes it to the
> mem_repair dpa attribute. If the record had any flag set, xa_load()
> misses it and the repair fails with -EINVAL. The raw key also lets a
> repair go through for a record the device marked NOT_REPAIRABLE, if the
> DPA is written with bit 1 set.
>
> Key the records by the masked DPA and skip records marked
> NOT_REPAIRABLE. cxl_find_rec_gen_media() now reads the record, so do
> the lookup under the xarray lock as cxl_find_rec_dram() does, and
> return a bool since its only caller tests the result.
>
> Fixes: 0b5ccb0de1e2 ("cxl/edac: Support for finding memory operation attributes from the current boot")
> Signed-off-by: Jongwon Lee <ai@xxxxxxxxx>
> ---
>   drivers/cxl/core/edac.c | 43 ++++++++++++++++++++++++++---------------
>   1 file changed, 27 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
> index c5c8dda20e..a5f198b561 100644
> --- a/drivers/cxl/core/edac.c
> +++ b/drivers/cxl/core/edac.c
> @@ -918,24 +918,37 @@ struct cxl_mem_repair_attrbs {
>   	enum cxl_mem_repair_type repair_type;
>   };
>   
> -static struct cxl_event_gen_media *
> -cxl_find_rec_gen_media(struct cxl_memdev *cxlmd,
> -		       struct cxl_mem_repair_attrbs *attrbs)
> +/*
> + * The low bits of the event record DPA are flags (VOLATILE, NOT_REPAIRABLE).
> + * Records are looked up by the DPA user space writes back, which comes from
> + * the trace event and has the flags masked off, so key them the same way.
> + */
> +static inline unsigned long cxl_rec_key(const struct cxl_event_media_hdr *hdr)
> +{
> +	return le64_to_cpu(hdr->phys_addr) & CXL_DPA_MASK;
> +}

This function actually returns the real DPA of the event record, I think 
the name should be cxl_rec_dpa()? xa_load() uses attrbs->dpa as the 
index, but xa_store() uses cxl_rec_key() as the index, that could make 
readers confused.


> +
> +static inline bool cxl_rec_not_repairable(const struct cxl_event_media_hdr *hdr)
> +{
> +	return le64_to_cpu(hdr->phys_addr) & CXL_DPA_NOT_REPAIRABLE;
> +}
> +
> +static bool cxl_find_rec_gen_media(struct cxl_memdev *cxlmd,
> +				   struct cxl_mem_repair_attrbs *attrbs)
>   {
>   	struct cxl_mem_err_rec *array_rec = cxlmd->err_rec_array;
>   	struct cxl_event_gen_media *rec;
> +	bool found;
>   
> -	if (!array_rec)
> -		return NULL;
> +	if (!array_rec || attrbs->repair_type != CXL_PPR)
> +		return false;
>   
> +	xa_lock(&array_rec->rec_gen_media);
>   	rec = xa_load(&array_rec->rec_gen_media, attrbs->dpa);
> -	if (!rec)
> -		return NULL;
> +	found = rec && !cxl_rec_not_repairable(&rec->media_hdr);
> +	xa_unlock(&array_rec->rec_gen_media);
>   
> -	if (attrbs->repair_type == CXL_PPR)
> -		return rec;
> -
> -	return NULL;
> +	return found;
>   }

I think the implementation of skipping unrepairable records should be 
moved into a separate patch, and this patch focuses on using real DPA as 
the index parameter of the xarray.

My first feeling is that we should not simply ignore unrepairable 
records without any logs. But I am not very familiar with this area, not 
sure if we should report this repairable bit in trace log or output a 
kernel log while finding an unrepairable record.

Let us see if maintainers have more comments on that.


Ming

>   
>   /* Caller holds the xarray lock. The record may be freed once it is dropped. */
> @@ -949,7 +962,7 @@ __cxl_find_rec_dram(struct xarray *rec_xarray,
>   	lockdep_assert_held(&rec_xarray->xa_lock);
>   
>   	rec = xa_load(rec_xarray, attrbs->dpa);
> -	if (!rec)
> +	if (!rec || cxl_rec_not_repairable(&rec->media_hdr))
>   		return NULL;
>   
>   	validity_flags = get_unaligned_le16(rec->media_hdr.validity_flags);
> @@ -1130,8 +1143,7 @@ int cxl_store_rec_gen_media(struct cxl_memdev *cxlmd, union cxl_event *evt)
>   		return -ENOMEM;
>   
>   	old_rec = xa_store(&array_rec->rec_gen_media,
> -			   le64_to_cpu(rec->media_hdr.phys_addr), rec,
> -			   GFP_KERNEL);
> +			   cxl_rec_key(&rec->media_hdr), rec, GFP_KERNEL);
>   	if (xa_is_err(old_rec)) {
>   		kfree(rec);
>   		return xa_err(old_rec);
> @@ -1160,8 +1172,7 @@ int cxl_store_rec_dram(struct cxl_memdev *cxlmd, union cxl_event *evt)
>   		return -ENOMEM;
>   
>   	old_rec = xa_store(&array_rec->rec_dram,
> -			   le64_to_cpu(rec->media_hdr.phys_addr), rec,
> -			   GFP_KERNEL);
> +			   cxl_rec_key(&rec->media_hdr), rec, GFP_KERNEL);
>   	if (xa_is_err(old_rec)) {
>   		kfree(rec);
>   		return xa_err(old_rec);