Re: [RFC PATCH 4/4] cxl/region: Introduce cxl_memdev_attach_region

[Dan Williams <djbw@kernel.org>]

[ add Lukas ]

Cheatham, Benjamin wrote:
> On 4/3/2026 4:00 PM, Dan Williams wrote:
> > To date, platform firmware maps accelerator memory and accelerator drivers
> > simply want an address range that they can map themselves. This typically
> > results in a single region being auto-assembled upon registration of a
> > memory device.  Use the @attach mechanism of devm_cxl_add_memdev()
> > parameter to retrieve that region while also adhering to CXL subsystem
> > locking and lifetime rules. As part of adhering to current object lifetime
> > rules, if the region or the CXL port topology is invalidated, the CXL core
> > arranges for the accelertor driver to be detached as well.
> 
> I'm probably missing something, but where does the core detach the
> accelerator driver?  I assume it's part of unregister_region(), but
> I'm not seeing any driver_detach() calls on the accelerator device.

It happens in detach_memdev().

> I don't think there's any demand for this at the moment, so it's more
> of a "food for thought" kind of thing, but why not allow for a
> fallback mode of operation instead of detaching the accelerator
> driver? First thing that comes to mind is an optional callback as part
> of struct cxl_memdev_attach that is called by the core during CXL
> removal. The assumption would be that an accelerator driver that
> provides the callback would clean up all of its CXL usage in the
> callback then continue in a PCIe-only mode of operation (hardware
> willing).  When the callback isn't provided, you get the behavior in
> this patch. Again, this isn't needed here but may be a nice
> middle-ground later on down the line.

I think that is possible, and also is not really a CXL specific feature. A
capability to move resources in active use by drivers has been proposed before
[1] (credit: Lukas).

[..]
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 11bc0b88b05f..090f52392b20 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1123,6 +1123,19 @@ static int cxl_rr_assign_decoder(struct cxl_port *port, struct cxl_region *cxlr,
> >  static void cxl_region_setup_flags(struct cxl_region *cxlr,
> >  				   struct cxl_decoder *cxld)
> >  {
> > +	if (is_endpoint_decoder(&cxld->dev)) {
> > +		struct cxl_endpoint_decoder *cxled = to_cxl_endpoint_decoder(&cxld->dev);
> > +		struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > +
> > +		/*
> > +		 * When a region's memdevs specify an @attach method the attach
> > +		 * provider is responsible for dispositioning the region for
> > +		 * both probe and userspace management
> > +		 */
> > +		if (cxlmd->attach)
> > +			set_bit(CXL_REGION_F_LOCK, &cxlr->flags);
> > +	}
> > +
> 
> It seems unfortunate that you set the region lock bit here and then
> immediately do a check to set the bit again.

There are 2 different cases to lock the region here, presence of attach, or
presence of hardware locked decoder.

> I can't think of a way to leverage cxld->flags for type 2 in an
> ergonomic way though, so I guess it's fine.

The flags are just caching hardware state, I do not want to have drivers
play games with the flags that dilute that "cached hardware value"
meaning.

[..]
> > +/*
> > + * The presence of an attach method indicates that the region is designated for
> > + * a purpose outside of CXL core memory expansion defaults.
> > + */
> > +static bool cxl_region_has_memdev_attach(struct cxl_region *cxlr)
> > +{
> 
> I think this should be renamed to something like "cxl_region_is_private()"; it
> better matches the intended use of the function while lessening the burden on
> a non-type 2 educated reader. Also has the added benefit of being one less
> change site if the mechanism for determining if a region belongs to an accelerator
> changes in the future.

Why does the CXL core naming need to cater to accelerator vs expansion
use case naming?  CXL core developer needs to understand all the use
cases and wants to make them unified as much as possible.
"cxl_region_is_private()" says nothing about what "private" means and
where to look next. It also muddies the namespace for when CXL regions
start becoming the backing store for "private" nodes. Is that a
"private" region?