Re: [PATCH] cxl/region: Match auto-discovered region decoders by HPA range

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

On Fri, Aug 04, 2023 at 03:13:58PM -0700, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Today, when the region driver attaches a region to a port, it
> > selects the ports next available decoder to program.
> 
> A small nit: s/Today/Currently/
Got it.

> 
> > With the addition of auto-discovered regions, a port decoder has
> > already been programmed, so grabbing the next available decoder
> > can be a mismatch when there is more than one region using the
> > port. Match on the port HPA range for auto-discovered regions.
> 
> This patch looks correct to me, just a couple questions beloe.
> 
> It would be great if it was accompanied by a cxl_test scenario that
> tested the failing case, but barring that it would be good to have some
> logs from a scenario where people can notice if this fix applies to
> their failure.

Cxl_test:

The cxl_test regression test for this fix is to setup 2 regions for
auto-detection on the same port and load/reload enough times that
the HPA violation occurs when the fix is not in place. I'll see
about adding that in a v2 of this patch, and an ndctl PATCH for the
regression/unit test.

Dmesg logs:
The footprint of this failure is only visible with CXL DEBUG enabled:

[] cxl_core:alloc_region_ref:754: cxl region0: endpoint9: HPA order violation region0:[mem 0x14780000000-0x1478fffffff flags 0x200] vs [mem 0x880000000-0x185fffffff flags 0x200]
[] cxl_core:cxl_port_attach_region:972: cxl region0: endpoint9: failed to allocate region reference

When CXL DEBUG is not enabled, there is no failure message. The region
just never materializes. With this patch, I hope the HPA order violation
is only a dev_dbg() level message again.

Makes me wonder:

This case aside, the 'opportunistic' approach to region assembly doesn't
offer a place to make any summary statements about the auto discovery
results.

In cxl_endpoint_probe() we get here with CXL_DECODER_STATE_AUTO :

>>	/*
>>	 * Now that all endpoint decoders are successfully enumerated, try to
>>	 * assemble regions from committed decoders
>>	 */
>>	device_for_each_child(&port->dev, root, discover_region);

Do we need to do some expected/found work here?

> 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> 
> I think this wants a "Fixes" tag:
> 
> Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> 
Got it.

> > ---
> >  
> >  drivers/cxl/core/region.c | 31 ++++++++++++++++++++++++++++---
> >  1 file changed, 28 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index e115ba382e04..8bfec7a96975 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -717,13 +717,37 @@ static int match_free_decoder(struct device *dev, void *data)
> >  	return 0;
> >  }
> >  
> > +static int match_auto_decoder(struct device *dev, void *data)
> > +{
> > +	struct cxl_endpoint_decoder *cxled = data;
> > +	struct cxl_decoder *cxld;
> > +
> > +	if (!is_switch_decoder(dev))
> > +		return 0;
> 
> Is this check needed? Endpoint decoders should also match by range.
> Maybe make it explicit like:
> 
>     if (cxld == &cxled->cxld)
>     	return 0;
> 
> ...where it is obvious no further checks are needed, but I think that
> also goes away with the change proposal below:
> 
> 
> > +
> > +	cxld = to_cxl_decoder(dev);
> > +
> > +	if (!range_contains(&cxld->hpa_range, &cxled->cxld.hpa_range))
> > +		return 0;
> 
> Hmm, shouldn't it be identical and no bigger?
> 
> if (cxld->hpa_range != cxled->cxld.hpa_range)
> 
> > +
> > +	if (!cxld->region)
> > +		return 1;
> 
> Interesting, I am trying to think through the implications of failing
> here. That would only happen if the port had been setup previously with
> a different region for the same address range? How would that happen?
> 
> It feel like it should be:
> 
> 	if (cxld->region) {
> 		dev_WARN(...)
> 		return 0;
> 	}
> 
> 	return 1;
>

Understood. I picked up checks from the existing match_free_decoder() too
willy nilly.

> > +
> > +	return 0;
> > +}
> > +
> >  static struct cxl_decoder *cxl_region_find_decoder(struct cxl_port *port,
> > -						   struct cxl_region *cxlr)
> > +						   struct cxl_region *cxlr,
> > +						   struct cxl_endpoint_decoder *cxled)
> >  {
> >  	struct device *dev;
> >  	int id = 0;
> >  
> > -	dev = device_find_child(&port->dev, &id, match_free_decoder);
> > +	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags))
> > +		dev = device_find_child(&port->dev, cxled, match_auto_decoder);
> > +	else
> > +		dev = device_find_child(&port->dev, &id, match_free_decoder);
> > +
> >  	if (!dev)
> >  		return NULL;
> >  	/*
> > @@ -839,7 +863,8 @@ static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
> >  	if (port == cxled_to_port(cxled))
> >  		cxld = &cxled->cxld;
> >  	else
> > -		cxld = cxl_region_find_decoder(port, cxlr);
> > +		cxld = cxl_region_find_decoder(port, cxlr, cxled);
> 
> It looks like the cxled is only used to convey the range. Maybe just get
> that from cxlr->params->res and not add another parameter here? Of
> course that would then change the suggestions above where you can not
> compare 'struct range' instances directly.

Got it. Will compare the res->starts and res->ends directly and use
the cxlr->params.

Thanks Dan!