Re: [PATCH 6/6] ACPI: NUMA: Add a node and memblk for each CFMWS not in SRAT

From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Rafael J Wysocki <rafael.j.wysocki@intel.com>,
	<linux-cxl@vger.kernel.org>,
	Alison Schofield <alison.schofield@intel.com>,
	Linux ACPI <linux-acpi@vger.kernel.org>
Subject: Re: [PATCH 6/6] ACPI: NUMA: Add a node and memblk for each CFMWS not in SRAT
Date: Thu, 18 Nov 2021 18:18:00 +0000	[thread overview]
Message-ID: <20211118181800.00005b91@Huawei.com> (raw)
In-Reply-To: <CAPcyv4hbYSk1p=_Wtvw-U6TPaAaxusb=cBYvzQDuFzT=5yaOeQ@mail.gmail.com>

On Thu, 18 Nov 2021 10:10:18 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> On Thu, Nov 18, 2021 at 9:54 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Thu, 18 Nov 2021 09:14:07 -0800
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >  
> > > On Thu, Nov 18, 2021 at 5:12 AM Jonathan Cameron
> > > <Jonathan.Cameron@huawei.com> wrote:  
> > > >
> > > > On Fri, 29 Oct 2021 12:51:59 -0700
> > > > Dan Williams <dan.j.williams@intel.com> wrote:
> > > >  
> > > > > From: Alison Schofield <alison.schofield@intel.com>
> > > > >
> > > > > During NUMA init, CXL memory defined in the SRAT Memory Affinity
> > > > > subtable may be assigned to a NUMA node. Since there is no
> > > > > requirement that the SRAT be comprehensive for CXL memory another
> > > > > mechanism is needed to assign NUMA nodes to CXL memory not identified
> > > > > in the SRAT.
> > > > >
> > > > > Use the CXL Fixed Memory Window Structure (CFMWS) of the ACPI CXL
> > > > > Early Discovery Table (CEDT) to find all CXL memory ranges.
> > > > > Create a NUMA node for each CFMWS that is not already assigned to
> > > > > a NUMA node. Add a memblk attaching its host physical address
> > > > > range to the node.
> > > > >
> > > > > Note that these ranges may not actually map any memory at boot time.
> > > > > They may describe persistent capacity or may be present to enable
> > > > > hot-plug.
> > > > >
> > > > > Consumers can use phys_to_target_node() to discover the NUMA node.
> > > > >
> > > > > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>  
> > > > Hi,
> > > >
> > > > I was just discussing this with one of our firmware / ACPI experts and he asked
> > > > an interesting question.   If you want to have CFMWS regions correspond to
> > > > new NUMA nodes, why not put them in SRAT as hotpluggable memory, but have none
> > > > present in the memory map (whichever route you use to get that)?
> > > > We do this for normal memory hotplug as (via the other discussion on qemu virtio-mem
> > > > nodes) apparently does qemu.
> > > >
> > > > https://lore.kernel.org/all/655c65af-fd7a-8007-37b3-a56c60a0ec5b@redhat.com/
> > > >
> > > > This doesn't solve the question of whether we have enough nodes, but it's
> > > > not worse than if we use CFMWS regions and fits within existing ACPI spec.
> > > >
> > > > The only reason I can immediately think of to not do this, is that it might be
> > > > a pain to later change over to dynamic numa node allocation in a fashion that
> > > > then ignores SRAT entries.  Probably a solvable problem.  
> > >
> > > Interesting, yes, that works for expanding the NUMA node number space.
> > > However, if you populate SRAT what do you put in the corresponding
> > > HMAT entries? In the case of dynamic CXL regions the driver is going
> > > to generate the equivalent of the corresponding HMAT data based on
> > > what devices it decides to place in that range. I actually do not know
> > > what happens with HMAT today for memory hotplug, but I suspect there
> > > are less degrees of freedom of what might populate those ranges than
> > > what CXL allows, and there is a chance to pre-populate the HMAT for
> > > future hotplug.  
> >
> > So... There are two answers to that question as I understand it.
> >
> > 1) What Linux does is nothing.  You get whatever was in HMAT to start with.
> >    Worth noting that HMAT doesn't need to be in any sense 'complete' so it
> >    is possible there was nothing there with a target in this NUMA node.
> >
> > 2) What ACPI intends to happen if anyone implements it.  There is an event
> > notification for this..
> >
> > "Heterogeneous Memory Attributes Update. Dynamic reconfiguration of
> > the system may cause existing latency, bandwidth or memory side caching
> > attribute to change. The platform software issues the Heterogeneous
> > Memory Attributes Update notification to a point on a device tree to
> > indicate to OSPM that it needs to invoke the _HMA objects associated
> > with the Heterogeneous Memory Attributes on the device tree starting
> > from the point notified."
> >
> > So call an AML method in DSDT for which ever device has a notification event.
> > A similar dance is actually implemented for NFIT updates and Linux
> > actually implements that handling for that one.  
> 
> Oh, yes, I know I helped implement it, and I think it is objectively a
> terrible interface because you have no idea what changed or is allowed
> to change. _HMA is slightly less problematic because it can't invent
> new ranges, but I have low expectations that the BIOS would be able to
> coordinate properly with the OS that has done the dynamic
> re-configuration of the CXL topology. The OS could tell the BIOS what
> to put in the HMAT, but might as well skip that step and just have the
> OS populate its parsed copy of the HMAT data. I like your "just don't
> publish HMAT for entries for these ranges" observation better.

I absolutely aggree and wouldn't suggest using _HMA for CXL devices.
The OS can do a better job (I'm sure the firmware folks will disagree)
Nothing says we have to take any notice even if there is such an update :)

It's more reasonable for DIMM hotplug where some level of firmware is heavily
involved already or possibly for virtualization if it didn't know the right
answer at boot for some reason.  Then a hotplug controller / management
controller can fill in the details.

Jonathan