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

Ira Weiny
2024-04-02T23:25:06Z

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