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

[Dan Williams <dan.j.williams@intel.com>]

Alison Schofield wrote:
> 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?

In the PMEM label case the CXL core could do that because it would have
an idea of what is supposed to be there. Otherwise, this code path can
not tell the difference between regions that were partially setup before
a kexec happened, or regions that were fully setup by the BIOS ahead of
time.

It might be worthwhile to report on System RAM that intersects a CXL
Window and expect a region to show up, but between BIOS quirks and bugs
it is difficult to make crisp "expected" statements here.