Re: [PATCH 0/4 v3] Multi-Region and Volatile Memory support for CXL Type-3 Devices

[Gregory Price <gregory.price@memverge.com>]

On Wed, Oct 26, 2022 at 08:13:24PM +0000, Adam Manzanares wrote:
> On Tue, Oct 25, 2022 at 08:47:33PM -0400, Gregory Price wrote:
> > Submitted as an extention to the multi-feature branch maintained
> > by Jonathan Cameron at:
> > https://urldefense.com/v3/__https://gitlab.com/jic23/qemu/-/tree/cxl-2022-10-24__;!!EwVzqGoTKBqv-0DWAJBm!RyiGL5B1XmQnVFwgxikKJeosPMKtoO1cTr61gIq8fwqfju8l4cbGZGwAEkKXIJB-Dbkfi_LNN2rGCbzMISz65cTxpAxI9pQ$   
> > 
> > 
> > Summary of Changes:
> > 1) E820 CFMW Bug fix.  
> > 2) Add CXL_CAPACITY_MULTIPLIER definition to replace magic numbers
> > 3) Multi-Region and Volatile Memory support for CXL Type-3 Devices
> > 4) CXL Type-3 SRAT Generation when NUMA node is attached to memdev
> > 
> > 
> > Regarding the E820 fix
> >   * This bugfix is required for memory regions to work on x86
> >   * input from Dan Williams and others suggest that E820 entry for
> >     the CFMW should not exist, as it is expected to be dynamically
> >     assigned at runtime.  If this entry exists, it instead blocks
> >     region creation by nature of the memory region being marked as
> >     reserved.
> 
> For CXL 2.0 it is my understanding that volatile capacity present at boot will
> be advertised by the firmware. In the absence of EFI I would assume this would
> be provided in the e820 map. 

The issue in this case is very explicitly that a double-mapping occurs
for the same region.  An E820 mapping for RESERVED is set *and* the
region driver allocates a CXL CFMW mapping.  As a result the region
drive straight up fails to allocate regions.

So in either case - at least from my view - the entry added as RESERVED
is just wrong.

This is separate from type-3 device SRAT entries and default mappings
for volatile regions.  For this situation, if you explicitly assign the
memdev backing a type-3 device to a numa node, then an SRAT area is
generated and an explicit e820 entry is generated and marked usable -
though I think there are likely issues with this kind of
double-referencing.

> 
> Is the region driver meant to cover volatile capacity present at boot? I was
> under the impression that it would be used for hot added volatile memory. It
> would be good to cover all of these assumptions for the e820 fix.

This region appears to cover hotplug memory behind the CFMW.  The
problem is that this e820 RESERVED mapping blocks the CFMW region from
being used at all.

Without this, you can't use a type-3 persistent region, even with
support, let alone a volatile region.  In attempting to use a persistent
region as volatile via ndctl and friends, I'm seeing further issues (it
cannot be assigned to a numa node successfully), but that's a separate
issue.

> 
> Lastly it is my understanding that the region driver does not have support for
> volatile memory. It would be great to add that functionality if we make this
> change in QEMU.
> 

Right now this is true, but it seems a bit of a chicken/egg scenario.
Nothing to test against vs no support.  Nudging this along such that we
can at least report an (unusable) hot-add volatile memory region would
provide someone working with the region driver something to poke and
prod at.

> > Regarding SRAT Generation for Type-3 Devices
> >   * Co-Developed by Davidlohr Bueso.  Built from his base patch and
> >     extended to work with both volatile and persistent regions.
> >   * This can be used to demonstrate static type-3 device mapping and
> >     testing numa-access to type-3 device memory regions.

Regarding "volatile memory present at boot" - there is still two ways
for that memory to be onlined: Statically (entered as an explicit e820
region after reading the SRAT), or Dynamically (hot-add by the region
driver).

This patch would at least allow an SRAT to be generated if you
explicitly add a NUMA node mapping to it.  Although I concede that I'm
not entirely sure what is "correct" here.

What this ends up looking like is mapping a memdev to both a numa node
and to a type-3 device.  Though that seems wrong.

After further testing it seems like creating a CPU-less, Memory-less
NUMA node with the intent of mapping volatile memory regions to it is
not supported (yet?).