On Thu, Sep 23, 2021 at 1:32 AM Gavin Shan <gshan@xxxxxxxxxx> wrote:
>
> Hi Rob and Ard,
>
> On 9/22/21 9:05 PM, Ard Biesheuvel wrote:
> > On Tue, 21 Sept 2021 at 21:45, Rob Herring <robh@xxxxxxxxxx> wrote:
> >> On Sun, Sep 5, 2021 at 11:16 PM Gavin Shan <gshan@xxxxxxxxxx> wrote:
> >>>
> >>> The empty memory nodes, where no memory resides in, are allowed.
> >>> For these empty memory nodes, the 'len' of 'reg' property is zero.
> >>> The NUMA node IDs are still valid and parsed, but memory may be
> >>> added to them through hotplug afterwards. Currently, QEMU fails
> >>> to boot when multiple empty memory nodes are specified. It's
> >>> caused by device-tree population failure and duplicated memory
> >>> node names.
> >
> > Those memory regions are known in advance, right? So wouldn't it be
> > better to use something like 'status = "disabled"' here?
> >
>
> Yes, these memory regions are known in advance. For the empty nodes,
> their 'len' property is zero and it's equal to disabled state.
>
> >>
> >> I still don't like the fake addresses. I can't really give suggestions
> >> on alternative ways to fix this with you just presenting a solution.
> >>
> >
> > Agreed. Please try to explain what the problem is, and why this is the
> > best way to solve it. Please include other solutions that were
> > considered and rejected if any exist.
> >
> >> What is the failure you see? Can we relax the kernel's expectations?
> >> What about UEFI boot as the memory nodes aren't used (or maybe they
> >> are for NUMA?) How does this work with ACPI?
> >>
> >
> > The EFI memory map only needs to describe the memory that was present
> > at boot. More memory can be represented as ACPI objects, including
> > coldplugged memory that is already present at boot. None of this
> > involves the memory nodes in DT.
> >
>
> I'm using the following command line to start a virtual machine (VM).
> There are 4 NUMA nodes specified, but the last two are empty. In QEMU,
> the device-tree nodes are populated to represent these 4 NUMA nodes.
> Unfortunately, QEMU fails to start because of the conflicting names
> for the empty node are found, as the following error message indicates.
>
> /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
> -accel kvm -machine virt,gic-version=host \
> -cpu host -smp 4,sockets=2,cores=2,threads=1 \
> -m 1024M,slots=16,maxmem=64G \
> -object memory-backend-ram,id=mem0,size=512M \
> -object memory-backend-ram,id=mem1,size=512M \
> -numa node,nodeid=0,cpus=0-1,memdev=mem0 \
> -numa node,nodeid=1,cpus=2-3,memdev=mem1 \
> -numa node,nodeid=2 \
> -numa node,nodeid=3 \
> :
> -device virtio-balloon-pci,id=balloon0,free-page-reporting=yes
> :
> :
> qemu-system-aarch64: FDT: Failed to create subnode /memory@80000000: FDT_ERR_EXISTS
>
> According to device-tree specification, the memory device-tree node's
> name is following the format 'memory@base-address'. For the empty
> NUMA nodes, their base addresses aren't determined. The device-tree
> specification doesn't indicate what 'base-address' should be assigned
> for the empty nodes. So I proposed this patch because I think the
> linux device-tree binding documentation is best place to get this
> documented.
Why even create the node?
What does IBM pSeries do here. AIUI, those platforms create/remove
nodes for hotplug. That's the reason CONFIG_OF_DYNAMIC existed
originally. Unfortunately, that's the extent of my knowledge on that.
> ACPI is different story. The NUMA nodes are represented by SRAT
> (System Resource Affinity Table). In the above example, there are
> 4 SRATs. We needn't assign names to the tables and we don't have
> the conflicting names as we do in device-tree case.
>
> By the way, QEMU currently prevents to expose SRATs for empty NUMA
> nodes. I need submit QEMU patch to break the limitation in future.
> With the limitation, the hot-added memory is always put into the
> last NUMA node and it's not exactly customer wants.
>
> >>> As device-tree specification indicates, the 'unit-address' of
> >>> these empty memory nodes, part of their names, are the equivalents
> >>> to 'base-address'. Unfortunately, I finds difficulty to get where
> >>> the assignment of 'base-address' is properly documented for these
> >>> empty memory nodes. So lets add a section for empty memory nodes
> >>> to cover this in NUMA binding document. The 'unit-address',
> >>> equivalent to 'base-address' in the 'reg' property of these empty
> >>> memory nodes is specified to be the summation of highest memory
> >>> address plus the NUMA node ID.
> >>>
> >>> Signed-off-by: Gavin Shan <gshan@xxxxxxxxxx>
> >>> Acked-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
> >>> ---
> >>> Documentation/devicetree/bindings/numa.txt | 60 +++++++++++++++++++++-
> >>> 1 file changed, 59 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/numa.txt b/Documentation/devicetree/bindings/numa.txt
> >>> index 21b35053ca5a..82f047bc8dd6 100644
> >>> --- a/Documentation/devicetree/bindings/numa.txt
> >>> +++ b/Documentation/devicetree/bindings/numa.txt
> >>> @@ -103,7 +103,65 @@ Example:
> >>> };
> >>>
> >>> ==============================================================================
> >>> -4 - Example dts
> >>> +4 - Empty memory nodes
> >>> +==============================================================================
> >>> +
> >>> +Empty memory nodes, which no memory resides in, are allowed. The 'length'
> >>> +field of the 'reg' property is zero. However, the 'base-address' is a
> >>> +dummy and invalid address, which is the summation of highest memory address
> >>> +plus the NUMA node ID. The NUMA node IDs and distance maps are still valid
> >>> +and memory may be added into them through hotplug afterwards.
> >>> +
> >>> +Example:
> >>> +
> >>> + memory@0 {
> >>> + device_type = "memory";
> >>> + reg = <0x0 0x0 0x0 0x80000000>;
> >>> + numa-node-id = <0>;
> >>> + };
> >>> +
> >>> + memory@80000000 {
> >>> + device_type = "memory";
> >>> + reg = <0x0 0x80000000 0x0 0x80000000>;
> >>> + numa-node-id = <1>;
> >>> + };
> >>> +
> >>> + /* Empty memory node */
> >>> + memory@100000002 {
> >>> + device_type = "memory";
> >>> + reg = <0x1 0x2 0x0 0x0>;
> >>> + numa-node-id = <2>;
> >>> + };
> >>> +
> >>> + /* Empty memory node */
> >>> + memory@100000003 {
> >>> + device_type = "memory";
> >>> + reg = <0x1 0x3 0x0 0x0>;
> >>> + numa-node-id = <3>;
> >>> + };
> >>
> >> Do you really need the memory nodes here or just some way to define
> >> numa node id's 2 and 3 as valid?
> >>
>
> It's the way to define NUMA node IDs are valid. Besides, the 'reg'
> property provides 'base-address', which is part of the device-tree
> node's name, as described in this patch.
The distance-matrix already lists all possible NUMA node IDs. That
should be enough information for the kernel. If not, fix the kernel.
Rob
