Re: [PATCH 02/26] cxl/core: Separate region mode from decoder mode

[Ira Weiny <ira.weiny@intel.com>]

Jonathan Cameron wrote:
> On Sun, 24 Mar 2024 16:18:05 -0700
> ira.weiny@intel.com wrote:

[snip]

> > 
> > ---
> > Changes for v1
> > <none>
> > ---
> >  drivers/cxl/core/region.c | 77 +++++++++++++++++++++++++++++++++++------------
> >  drivers/cxl/cxl.h         | 26 ++++++++++++++--
> >  2 files changed, 81 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 4c7fd2d5cccb..1723d17f121e 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> 
> 
> > @@ -2800,6 +2814,24 @@ static int match_region_by_range(struct device *dev, void *data)
> >  	return rc;
> >  }
> >  
> > +static enum cxl_region_mode
> > +cxl_decoder_to_region_mode(enum cxl_decoder_mode mode)
> > +{
> > +	switch (mode) {
> > +	case CXL_DECODER_NONE:
> > +		return CXL_REGION_NONE;
> > +	case CXL_DECODER_RAM:
> > +		return CXL_REGION_RAM;
> > +	case CXL_DECODER_PMEM:
> > +		return CXL_REGION_PMEM;
> > +	case CXL_DECODER_MIXED:
> > +	default:
> > +		return CXL_REGION_MIXED;
> > +	}
> > +
> 
> Dead code.

Fixed thanks.

> 
> > +	return CXL_REGION_MIXED;
> > +}
> > +
> >  /* Establish an empty region covering the given HPA range */
> >  static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> >  					   struct cxl_endpoint_decoder *cxled)
> > @@ -2808,12 +2840,17 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> >  	struct cxl_port *port = cxlrd_to_port(cxlrd);
> >  	struct range *hpa = &cxled->cxld.hpa_range;
> >  	struct cxl_region_params *p;
> > +	enum cxl_region_mode mode;
> >  	struct cxl_region *cxlr;
> >  	struct resource *res;
> >  	int rc;
> >  
> > +	if (cxled->mode == CXL_DECODER_DEAD)
> > +		return ERR_PTR(-EINVAL);
> 
> Not a bad thing necessarily, but why do we now need this and didn't before?

Ah.  Because in devm_cxl_add_region() the mode of CXL_DECODER_DEAD used to
return -EINVAL.

There is no logical equivalent to decoder dead in the region mode (regions
don't need it).  So this correctly flags the error based on the decoder
mode rather than introduce a mode for regions which does not make sense.

I'll update the commit message because that is hard to see.

> 
> > +
> > +	mode = cxl_decoder_to_region_mode(cxled->mode);
> >  	do {
> > -		cxlr = __create_region(cxlrd, cxled->mode,
> > +		cxlr = __create_region(cxlrd, mode,
> >  				       atomic_read(&cxlrd->region_id));
> >  	} while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY);
> 
> 
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index 003feebab79b..9a0cce1e6fca 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> 
> 
> >  /*
> >   * Track whether this decoder is reserved for region autodiscovery, or
> >   * free for userspace provisioning.
> > @@ -511,7 +532,8 @@ struct cxl_region_params {
> >   * struct cxl_region - CXL region
> >   * @dev: This region's device
> >   * @id: This region's id. Id is globally unique across all regions
> > - * @mode: Endpoint decoder allocation / access mode
> > + * @mode: Region mode which defines which endpoint decoder mode the region is
> mode or potentially modes?
> 
> If region is mixed, I guess that means endpoint could be pmem or ram in theory?
> Don't think anyone has implemented anything yet, but is the potential there?

Yes the potential is there.  The endpoint decoder is set to
CXL_DECODER_MIXED in __cxl_dpa_reserve().  But I am unclear how that will
ever be executed except if the BIOS sets up a decoder to span ram/pmem.
In this case the rest of the stack is not going to work and will complain
about mixed mode.

Ok Dan clued me in.  Check out cxl_dpa_alloc().  Spanning partitions is
not allowed.

So the comment is targeted toward the 'normal' case even though the region
could be created incorrectly via BIOS.

Ira