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

[Alison Schofield <alison.schofield@intel.com>]

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));