[PATCH v2] cxl/region: Allow out of order assembly of autodiscovered regions

[PATCH v2] cxl/region: Allow out of order assembly of autodiscovered regions

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Autodiscovered regions can fail to assemble if they are not discovered
in HPA decode order. The user will see failure messages like:

[] cxl region0: endpoint5: HPA order violation region1
[] cxl region0: endpoint5: failed to allocate region reference

The check that is causing the failure helps the CXL driver enforce
a CXL spec mandate that decoders be committed in HPA order. The
check is needless for autodiscovered regions since their decoders
are already programmed. Trying to enforce order in the assembly of
these regions is useless because they are assembled once all their
member endpoints arrive, and there is no guarantee on the order in
which endpoints are discovered during probe.

Keep the existing check, but for autodiscovered regions, allow the
out of order assembly after a sanity check that the lesser numbered
decoder has the lesser HPA starting address.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---

Changes since v1: 
- Get decoder via available struct cxled_endpoint_decoder. (Wonjae)
- Check F_AUTO in alloc_region_ref()
- Fold assignments into the declarations in auto_order_ok()
- Drop Tested-by tag due to changes
Link to v1: https://lore.kernel.org/linux-cxl/20240126045446.1750854-1-alison.schofield@intel.com/


 drivers/cxl/core/region.c | 47 ++++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 11 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 0f05692bfec3..28e8af1e54a2 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -753,8 +753,32 @@ static struct cxl_decoder *cxl_region_find_decoder(struct cxl_port *port,
 	return to_cxl_decoder(dev);
 }
 
-static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
-					       struct cxl_region *cxlr)
+static bool auto_order_ok(struct cxl_port *port, struct cxl_region *cxlr_iter,
+			  struct cxl_endpoint_decoder *cxled)
+{
+	struct cxl_region_ref *rr = cxl_rr_load(port, cxlr_iter);
+	struct cxl_decoder *cxld_iter = rr->decoder;
+	struct cxl_decoder *cxld = &cxled->cxld;
+
+	/*
+	 * Allow the out of order assembly of auto-discovered regions.
+	 * Per CXL Spec 3.1 8.2.4.20.12 software must commit decoders
+	 * in HPA order. Confirm that the decoder with the lesser HPA
+	 * starting address has the lesser id.
+	 */
+	dev_dbg(&cxld->dev, "check for HPA violation %s:%d < %s:%d\n",
+		dev_name(&cxld->dev), cxld->id,
+		dev_name(&cxld_iter->dev), cxld_iter->id);
+
+	if (cxld_iter->id > cxld->id)
+		return true;
+
+	return false;
+}
+
+static struct cxl_region_ref *
+alloc_region_ref(struct cxl_port *port, struct cxl_region *cxlr,
+		 struct cxl_endpoint_decoder *cxled)
 {
 	struct cxl_region_params *p = &cxlr->params;
 	struct cxl_region_ref *cxl_rr, *iter;
@@ -764,16 +788,17 @@ static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
 	xa_for_each(&port->regions, index, iter) {
 		struct cxl_region_params *ip = &iter->region->params;
 
-		if (!ip->res)
+		if (!ip->res || ip->res->start < p->res->start)
 			continue;
 
-		if (ip->res->start > p->res->start) {
-			dev_dbg(&cxlr->dev,
-				"%s: HPA order violation %s:%pr vs %pr\n",
-				dev_name(&port->dev),
-				dev_name(&iter->region->dev), ip->res, p->res);
-			return ERR_PTR(-EBUSY);
-		}
+		if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags) &&
+		    auto_order_ok(port, iter->region, cxled))
+			continue;
+
+		dev_dbg(&cxlr->dev, "%s: HPA order violation %s:%pr vs %pr\n",
+			dev_name(&port->dev),
+			dev_name(&iter->region->dev), ip->res, p->res);
+		return ERR_PTR(-EBUSY);
 	}
 
 	cxl_rr = kzalloc(sizeof(*cxl_rr), GFP_KERNEL);
@@ -953,7 +978,7 @@ static int cxl_port_attach_region(struct cxl_port *port,
 			nr_targets_inc = true;
 		}
 	} else {
-		cxl_rr = alloc_region_ref(port, cxlr);
+		cxl_rr = alloc_region_ref(port, cxlr, cxled);
 		if (IS_ERR(cxl_rr)) {
 			dev_dbg(&cxlr->dev,
 				"%s: failed to allocate region reference\n",

base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
-- 
2.37.3

Re: [PATCH v2] cxl/region: Allow out of order assembly of autodiscovered regions

[Dan Williams]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> Autodiscovered regions can fail to assemble if they are not discovered
> in HPA decode order. The user will see failure messages like:
> 
> [] cxl region0: endpoint5: HPA order violation region1
> [] cxl region0: endpoint5: failed to allocate region reference
> 
> The check that is causing the failure helps the CXL driver enforce
> a CXL spec mandate that decoders be committed in HPA order. The
> check is needless for autodiscovered regions since their decoders
> are already programmed. Trying to enforce order in the assembly of
> these regions is useless because they are assembled once all their
> member endpoints arrive, and there is no guarantee on the order in
> which endpoints are discovered during probe.
> 
> Keep the existing check, but for autodiscovered regions, allow the
> out of order assembly after a sanity check that the lesser numbered
> decoder has the lesser HPA starting address.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
> 
> Changes since v1: 
> - Get decoder via available struct cxled_endpoint_decoder. (Wonjae)
> - Check F_AUTO in alloc_region_ref()
> - Fold assignments into the declarations in auto_order_ok()
> - Drop Tested-by tag due to changes
> Link to v1: https://lore.kernel.org/linux-cxl/20240126045446.1750854-1-alison.schofield@intel.com/
> 
> 
>  drivers/cxl/core/region.c | 47 ++++++++++++++++++++++++++++++---------
>  1 file changed, 36 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 0f05692bfec3..28e8af1e54a2 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -753,8 +753,32 @@ static struct cxl_decoder *cxl_region_find_decoder(struct cxl_port *port,
>  	return to_cxl_decoder(dev);
>  }
>  
> -static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
> -					       struct cxl_region *cxlr)
> +static bool auto_order_ok(struct cxl_port *port, struct cxl_region *cxlr_iter,
> +			  struct cxl_endpoint_decoder *cxled)
> +{
> +	struct cxl_region_ref *rr = cxl_rr_load(port, cxlr_iter);
> +	struct cxl_decoder *cxld_iter = rr->decoder;
> +	struct cxl_decoder *cxld = &cxled->cxld;
> +
> +	/*
> +	 * Allow the out of order assembly of auto-discovered regions.
> +	 * Per CXL Spec 3.1 8.2.4.20.12 software must commit decoders
> +	 * in HPA order. Confirm that the decoder with the lesser HPA
> +	 * starting address has the lesser id.
> +	 */
> +	dev_dbg(&cxld->dev, "check for HPA violation %s:%d < %s:%d\n",
> +		dev_name(&cxld->dev), cxld->id,
> +		dev_name(&cxld_iter->dev), cxld_iter->id);
> +
> +	if (cxld_iter->id > cxld->id)

This only works if @port is an endpoint port. The order violations can
also happen within switch ports and in that case it is invalid to
compare a switch port decoder-id with an endpoint decoder id. So I think
this needs cxl_region_find_decoder(), and cxl_region_find_decoder()
needs to handle endpoint ports:

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index ce0e2d82bb2b..f1a9d1798d21 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -730,12 +730,17 @@ static int match_auto_decoder(struct device *dev, void *data)
        return 0;
 }
 
-static struct cxl_decoder *cxl_region_find_decoder(struct cxl_port *port,
-                                                  struct cxl_region *cxlr)
+static struct cxl_decoder *
+cxl_region_find_decoder(struct cxl_port *port,
+                       struct cxl_endpoint_decoder *cxled,
+                       struct cxl_region *cxlr)
 {
        struct device *dev;
        int id = 0;
 
+       if (port == cxled_to_port(cxled))
+               return &cxled->cxld;
+
        if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags))
                dev = device_find_child(&port->dev, &cxlr->params,
                                        match_auto_decoder);
@@ -853,10 +858,7 @@ static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
 {
        struct cxl_decoder *cxld;
 
-       if (port == cxled_to_port(cxled))
-               cxld = &cxled->cxld;
-       else
-               cxld = cxl_region_find_decoder(port, cxlr);
+       cxld = cxl_region_find_decoder(port, cxled, cxlr);
        if (!cxld) {
                dev_dbg(&cxlr->dev, "%s: no decoder available\n",
                        dev_name(&port->dev));

Re: [PATCH v2] cxl/region: Allow out of order assembly of autodiscovered regions

[Alison Schofield]

On Wed, Jan 31, 2024 at 12:56:11AM -0800, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Autodiscovered regions can fail to assemble if they are not discovered
> > in HPA decode order. The user will see failure messages like:
> > 
> > [] cxl region0: endpoint5: HPA order violation region1
> > [] cxl region0: endpoint5: failed to allocate region reference
> > 
> > The check that is causing the failure helps the CXL driver enforce
> > a CXL spec mandate that decoders be committed in HPA order. The
> > check is needless for autodiscovered regions since their decoders
> > are already programmed. Trying to enforce order in the assembly of
> > these regions is useless because they are assembled once all their
> > member endpoints arrive, and there is no guarantee on the order in
> > which endpoints are discovered during probe.
> > 
> > Keep the existing check, but for autodiscovered regions, allow the
> > out of order assembly after a sanity check that the lesser numbered
> > decoder has the lesser HPA starting address.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> > 
> > Changes since v1: 
> > - Get decoder via available struct cxled_endpoint_decoder. (Wonjae)
> > - Check F_AUTO in alloc_region_ref()
> > - Fold assignments into the declarations in auto_order_ok()
> > - Drop Tested-by tag due to changes
> > Link to v1: https://lore.kernel.org/linux-cxl/20240126045446.1750854-1-alison.schofield@intel.com/
> > 
> > 
> >  drivers/cxl/core/region.c | 47 ++++++++++++++++++++++++++++++---------
> >  1 file changed, 36 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 0f05692bfec3..28e8af1e54a2 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -753,8 +753,32 @@ static struct cxl_decoder *cxl_region_find_decoder(struct cxl_port *port,
> >  	return to_cxl_decoder(dev);
> >  }
> >  
> > -static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
> > -					       struct cxl_region *cxlr)
> > +static bool auto_order_ok(struct cxl_port *port, struct cxl_region *cxlr_iter,
> > +			  struct cxl_endpoint_decoder *cxled)
> > +{
> > +	struct cxl_region_ref *rr = cxl_rr_load(port, cxlr_iter);
> > +	struct cxl_decoder *cxld_iter = rr->decoder;
> > +	struct cxl_decoder *cxld = &cxled->cxld;
> > +
> > +	/*
> > +	 * Allow the out of order assembly of auto-discovered regions.
> > +	 * Per CXL Spec 3.1 8.2.4.20.12 software must commit decoders
> > +	 * in HPA order. Confirm that the decoder with the lesser HPA
> > +	 * starting address has the lesser id.
> > +	 */
> > +	dev_dbg(&cxld->dev, "check for HPA violation %s:%d < %s:%d\n",
> > +		dev_name(&cxld->dev), cxld->id,
> > +		dev_name(&cxld_iter->dev), cxld_iter->id);
> > +
> > +	if (cxld_iter->id > cxld->id)
> 
> This only works if @port is an endpoint port. The order violations can
> also happen within switch ports and in that case it is invalid to
> compare a switch port decoder-id with an endpoint decoder id. So I think
> this needs cxl_region_find_decoder(), and cxl_region_find_decoder()
> needs to handle endpoint ports:

Thanks! With your changes - it's looking right now:

bad compares:
[] cxl_core:auto_order_ok:769: cxl decoder8.0: check for HPA violation decoder8.0:0 < decoder8.1:1
[0 cxl_core:auto_order_ok:769: cxl decoder8.0: check for HPA violation decoder8.0:0 < decoder8.2:2
[] cxl_core:auto_order_ok:769: cxl decoder8.0: check for HPA violation decoder8.0:0 < decoder7.1:1
[] cxl_core:auto_order_ok:769: cxl decoder8.0: check for HPA violation decoder8.0:0 < decoder7.2:2
[] cxl_core:auto_order_ok:769: cxl decoder8.0: check for HPA violation decoder8.0:0 < decoder4.1:1
[] cxl_core:auto_order_ok:769: cxl decoder8.0: check for HPA violation decoder8.0:0 < decoder4.2:2

good compares:
[] cxl_core:auto_order_ok:773: cxl decoder8.0: check for HPA violation decoder8.0:0 < decoder8.1:1
[] cxl_core:auto_order_ok:773: cxl decoder8.0: check for HPA violation decoder8.0:0 < decoder8.2:2
[] cxl_core:auto_order_ok:773: cxl decoder7.0: check for HPA violation decoder7.0:0 < decoder7.1:1
[] cxl_core:auto_order_ok:773: cxl decoder7.0: check for HPA violation decoder7.0:0 < decoder7.2:2
[] cxl_core:auto_order_ok:773: cxl decoder4.0: check for HPA violation decoder4.0:0 < decoder4.1:1
[] cxl_core:auto_order_ok:773: cxl decoder4.0: check for HPA violation decoder4.0:0 < decoder4.2:2


> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index ce0e2d82bb2b..f1a9d1798d21 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -730,12 +730,17 @@ static int match_auto_decoder(struct device *dev, void *data)
>         return 0;
>  }
>  
> -static struct cxl_decoder *cxl_region_find_decoder(struct cxl_port *port,
> -                                                  struct cxl_region *cxlr)
> +static struct cxl_decoder *
> +cxl_region_find_decoder(struct cxl_port *port,
> +                       struct cxl_endpoint_decoder *cxled,
> +                       struct cxl_region *cxlr)
>  {
>         struct device *dev;
>         int id = 0;
>  
> +       if (port == cxled_to_port(cxled))
> +               return &cxled->cxld;
> +
>         if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags))
>                 dev = device_find_child(&port->dev, &cxlr->params,
>                                         match_auto_decoder);
> @@ -853,10 +858,7 @@ static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
>  {
>         struct cxl_decoder *cxld;
>  
> -       if (port == cxled_to_port(cxled))
> -               cxld = &cxled->cxld;
> -       else
> -               cxld = cxl_region_find_decoder(port, cxlr);
> +       cxld = cxl_region_find_decoder(port, cxled, cxlr);
>         if (!cxld) {
>                 dev_dbg(&cxlr->dev, "%s: no decoder available\n",
>                         dev_name(&port->dev));