Re: [PATCH v2 2/4] cxl/acpi: Restore XOR'd position bits during address translation

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

On Wed, May 29, 2024 at 08:55:30PM -0700, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > When a CXL region is created in a CXL Window (CFMWS) that uses XOR
> > interleave arithmetic XOR maps are applied during the HPA->DPA
> > translation. The XOR function changes the interleave selector
> > bit (aka position bit) in the HPA thereby varying which host bridge
> > services an HPA. The purpose is to minimize hot spots thereby
> > improving performance.
> 
> I think it is important to either detail how "interleave hot spots"
> arise, or just omit that description since it is ultimately not relevant
> to Linux.  I.e. the "why XOR" question is irrelevant compared to "what
> happens to a end user with a kernel that does not comprehend XOR
> interleave maths".

I think I understand. Long ago we decided to add XOR Math support to
the CXL Driver. The point now is to do it correctly not rejustify it's
existence.

I'll chop 'The purpose...'

> 
> > When a device reports a DPA in events such as poison, general_media,
> > and dram, the driver translates that DPA back to an HPA. Presently,
> > the CXL driver translation only considers the modulo position and
> > will report the wrong HPA for XOR configured CFMWS's.
> > 
> > Add a helper function that restores the XOR'd bits during DPA->HPA
> > address translation. Plumb a root decoder callback to the new helper
> > when XOR interleave arithmetic is in use. For MODULO arithmetic, just
> > let the callback be NULL - as in no extra work required.
> 
> Perhaps add a handful of sentences about the testing for this patch to
> make sure the maths are now correct compared to what was there before?
> 

There was nothing there before, but I think I know what you mean.
More on that below...

> > 
> > Fixes: 28a3ae4ff66c ("cxl/trace: Add an HPA to cxl_poison trace events")
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---

snip

> > +
> > +	for (int i = 0; i < cximsd->nr_maps; i++) {
> > +		if (!cximsd->xormaps[i])
> > +			continue;
> > +		pos = __ffs(cximsd->xormaps[i]);
> > +		val = (hweight64(hpa & cximsd->xormaps[i]) & 1);
> > +		hpa = (hpa & ~(1ULL << pos)) | (val << pos);
> > +	}
> 
> This looks correct to me, but so did the original broken implementation.
> I would feel better about a self test either integrated into
> tools/testing/cxl/, or documented / referenced in the CXL Device Driver
> Writer's Guide that gives confidence that "This is the way".

Let me see about adding a unit test to the cxl suite that exercises the
translation path using data from the sample XOR translation tables that are
currently being added to the CXL Driver Writer's Guide. That would allow the
test to verify against a known set of DPA->HPA offsets for a given XOR region
config. 

-- Alison