Re: [PATCH v5 1/2] cxl/hdm: Allow zero sized HDM decoders

["Dan Williams (nvidia)" <djbw@kernel.org>]

sashiko-bot@ wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] cxl_dpa_free() incorrectly bypasses teardown for zero-sized decoders, permanently locking hdm_end and breaking the LIFO DPA teardown sequence.
> 
> Pre-existing issues:
> - [High] Pre-existing bug in cxl_get_poison_unmapped: prematurely breaking out of the partition loop skips poison scanning for unmapped partitions.
> --
> 
> commit 66fbe5db5d8f3f27534b45af8c4ef71b88ab7f3e
> Author: Richard Cheng <icheng@nvidia.com>
> 
> cxl/hdm: Allow zero sized HDM decoders
> 
> Treat empty decoders as first class instead of special casing them, back
> them with a kmalloc'd resource, since the resource tree can't represent
> an empty range, and keep the skip and hdm_end accounting intact. Guard
> the paths an empty decoder can't serve, e.g. region attach, DPA free, and
> poison queries.
> 
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 0c80b76a5f9b4..ccbab2e21f060 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> [ ... ]
> > @@ -545,7 +569,7 @@ int cxl_dpa_free(struct cxl_endpoint_decoder *cxled)
> >  	struct device *dev = &cxled->cxld.dev;
> >  
> >  	guard(rwsem_write)(&cxl_rwsem.dpa);
> > -	if (!cxled->dpa_res)
> > +	if (cxled_empty(cxled))
> >  		return 0;
> 
> [Severity: High]
> Does this early return prevent decrementing port->hdm_end for zero-sized
> decoders?
> 
> If an unlocked zero-sized decoder is freed by userspace (for example, by
> writing 0 to dpa_size in sysfs), cxled_empty() returns true because
> resource_size(dpa_res) is 0. The function then returns 0 early, skipping
> devm_cxl_dpa_release().
> 
> Because hdm_end is decremented only inside __cxl_dpa_release(), bypassing it
> leaves port->hdm_end permanently elevated. Would subsequent attempts to free
> lower-indexed decoders fail the cxled->cxld.id != port->hdm_end check with
> -EBUSY, preventing further dynamic DPA reconfiguration?

This is blocked by the fact that there is no path to clear
CXL_DECODER_F_ENABLE due to empty decoders not participating in any
region. The permanent nature of empty decoders is ok for now.

Might be worth a comment to note that emtpy decoders are not manageable
by userspace.

> >  	if (cxled->cxld.region) {
> >  		dev_dbg(dev, "decoder assigned to: %s\n",
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index e50dc716d4e82..eeecb588821a4 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> [ ... ]
> > @@ -2953,9 +2960,17 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port)
> >  	};
> >  
> >  	rc = device_for_each_child(&port->dev, &ctx, poison_by_decoder);
> > -	if (rc == 1)
> > +	if (rc == 1) {
> > +		/*
> > +		 * No decoder with a sized DPA reservation was walked
> > +		 * (every committed decoder is zero-size): scan all
> > +		 * partitions in full.
> > +		 */
> > +		if (ctx.part < 0)
> > +			ctx.part = 0;
> >  		rc = cxl_get_poison_unmapped(to_cxl_memdev(port->uport_dev),
> >  					     &ctx);
> 
> [Severity: High]
> This is a pre-existing issue, but does cxl_get_poison_unmapped() prematurely
> break out of its partition loop when encountering a fully mapped partition?
> 
> Looking at cxl_get_poison_unmapped() in drivers/cxl/core/region.c:
> 
> for (int i = ctx->part; i < cxlds->nr_partitions; i++) {
>         ...
>         length = res->end - offset + 1;
>         if (!length)
>                 break;
>         rc = cxl_mem_get_poison(...);
> 
> If it processes a partition that is fully mapped, length becomes 0, and the
> break statement completely terminates the outer loop over the remaining
> partitions instead of moving to the next one. Should this be a continue
> instead of a break so that poison in subsequent unmapped partitions isn't
> silently ignored?

This looks accurate. On entry for a fully mapped partition ctx->part
will be initialized from the decoder for that partition. If there are
more unmapped partitions they will be skipped.