[Qemu-devel] [PATCH for 2.13 v2 0/2] target/ppc: Support adding memory to initially memory-less NUMA nodes

[Qemu-devel] [PATCH for 2.13 v2 0/2] target/ppc: Support adding memory to initially memory-less NUMA nodes

[Serhii Popovych]

Now PowerPC Linux kernel supports hot-add to NUMA nodes not populated
initially with memory we can enable such support in qemu. This requires
two changes:

  o Add device tree property "ibm,max-associativity-domains" to let
    guest kernel chance to find max possible NUMA node

  o Revert  commit b556854bd852 ("spapr: Don't allow memory hotplug to
    memory less nodes") to remove check for hot-add to memory-less node.

See description messges for individual changes for more details.

v2:
  - Reorder patches in series according to description above.
  - Add extra coment to revert noticing return to previous behaviour for
    guests without support for hot-add to empty node.
  - Drop max_cpus from topology in property due to vcpu id discontiguous
    allocations. Thanks to David Gibson for extra explanation.
  - Rebase to current state of master branch.

Serhii Popovych (2):
  spapr: Add ibm,max-associativity-domains property
  Revert "spapr: Don't allow memory hotplug to memory less nodes"

 hw/ppc/spapr.c | 33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH for 2.13 v2 1/2] spapr: Add ibm, max-associativity-domains property

[Serhii Popovych]

Now recent kernels (i.e. since linux-stable commit a346137e9142
("powerpc/numa: Use ibm,max-associativity-domains to discover possible nodes")
support this property to mark initially memory-less NUMA nodes as "possible"
to allow further memory hot-add to them.

Advertise this property for pSeries machines to let guest kernels detect
maximum supported node configuration and benefit from kernel side change
when hot-add memory to specific, possibly empty before, NUMA node.

Signed-off-by: Serhii Popovych <spopovyc@redhat.com>
---
 hw/ppc/spapr.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 2c0be8c..3f61785 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -909,6 +909,14 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr, void *fdt)
         0, cpu_to_be32(SPAPR_MEMORY_BLOCK_SIZE),
         cpu_to_be32(max_cpus / smp_threads),
     };
+    uint32_t maxdomains[] = {
+        cpu_to_be32(5),
+        cpu_to_be32(0),
+        cpu_to_be32(0),
+        cpu_to_be32(0),
+        cpu_to_be32(nb_numa_nodes - 1),
+        cpu_to_be32(0),
+    };
 
     _FDT(rtas = fdt_add_subnode(fdt, 0, "rtas"));
 
@@ -945,6 +953,9 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr, void *fdt)
     _FDT(fdt_setprop(fdt, rtas, "ibm,associativity-reference-points",
                      refpoints, sizeof(refpoints)));
 
+    _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains",
+                     maxdomains, sizeof(maxdomains)));
+
     _FDT(fdt_setprop_cell(fdt, rtas, "rtas-error-log-max",
                           RTAS_ERROR_LOG_MAX));
     _FDT(fdt_setprop_cell(fdt, rtas, "rtas-event-scan-rate",
-- 
1.8.3.1

[Qemu-devel] [PATCH for 2.13 v2 2/2] Revert "spapr: Don't allow memory hotplug to memory less nodes"

[Serhii Popovych]

This reverts commit b556854bd8524c26b8be98ab1bfdf0826831e793.

Leave change @node type from uint32_t to to int from reverted commit
because node < 0 is always false.

Note that implementing capability or some trick to detect if guest
kernel does not support hot-add to memory: this returns previous
behavour where memory added to first non-empty node.

Signed-off-by: Serhii Popovych <spopovyc@redhat.com>
---
 hw/ppc/spapr.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3f61785..cd7a347 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3488,28 +3488,6 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
             return;
         }
 
-        /*
-         * Currently PowerPC kernel doesn't allow hot-adding memory to
-         * memory-less node, but instead will silently add the memory
-         * to the first node that has some memory. This causes two
-         * unexpected behaviours for the user.
-         *
-         * - Memory gets hotplugged to a different node than what the user
-         *   specified.
-         * - Since pc-dimm subsystem in QEMU still thinks that memory belongs
-         *   to memory-less node, a reboot will set things accordingly
-         *   and the previously hotplugged memory now ends in the right node.
-         *   This appears as if some memory moved from one node to another.
-         *
-         * So until kernel starts supporting memory hotplug to memory-less
-         * nodes, just prevent such attempts upfront in QEMU.
-         */
-        if (nb_numa_nodes && !numa_info[node].node_mem) {
-            error_setg(errp, "Can't hotplug memory to memory-less node %d",
-                       node);
-            return;
-        }
-
         spapr_memory_plug(hotplug_dev, dev, node, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         spapr_core_plug(hotplug_dev, dev, errp);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for 2.13 v2 1/2] spapr: Add ibm, max-associativity-domains property

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2420 bytes --]

On Tue, Apr 10, 2018 at 02:12:25PM -0400, Serhii Popovych wrote:
> Now recent kernels (i.e. since linux-stable commit a346137e9142
> ("powerpc/numa: Use ibm,max-associativity-domains to discover possible nodes")
> support this property to mark initially memory-less NUMA nodes as "possible"
> to allow further memory hot-add to them.
> 
> Advertise this property for pSeries machines to let guest kernels detect
> maximum supported node configuration and benefit from kernel side change
> when hot-add memory to specific, possibly empty before, NUMA node.
> 
> Signed-off-by: Serhii Popovych <spopovyc@redhat.com>
> ---
>  hw/ppc/spapr.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 2c0be8c..3f61785 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -909,6 +909,14 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr, void *fdt)
>          0, cpu_to_be32(SPAPR_MEMORY_BLOCK_SIZE),
>          cpu_to_be32(max_cpus / smp_threads),
>      };
> +    uint32_t maxdomains[] = {
> +        cpu_to_be32(5),
> +        cpu_to_be32(0),
> +        cpu_to_be32(0),
> +        cpu_to_be32(0),
> +        cpu_to_be32(nb_numa_nodes - 1),
> +        cpu_to_be32(0),
> +    };

Ah.. close, but not quite right.  This is saying that there's exactly
one node at the bottom (cpu) level, which isn't what we want.  Instead
of setting it to 0, we want to completely drop that layer, keeping it
unspecified.

To do that you need to change the first cell from 5 to 4 (since only 4
levels will be listed) and drop the last cell entirely.

>      _FDT(rtas = fdt_add_subnode(fdt, 0, "rtas"));
>  
> @@ -945,6 +953,9 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr, void *fdt)
>      _FDT(fdt_setprop(fdt, rtas, "ibm,associativity-reference-points",
>                       refpoints, sizeof(refpoints)));
>  
> +    _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains",
> +                     maxdomains, sizeof(maxdomains)));
> +
>      _FDT(fdt_setprop_cell(fdt, rtas, "rtas-error-log-max",
>                            RTAS_ERROR_LOG_MAX));
>      _FDT(fdt_setprop_cell(fdt, rtas, "rtas-event-scan-rate",

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH for 2.13 v2 1/2] spapr: Add ibm, max-associativity-domains property

[Greg Kurz]

On Wed, 11 Apr 2018 10:03:48 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Apr 10, 2018 at 02:12:25PM -0400, Serhii Popovych wrote:
> > Now recent kernels (i.e. since linux-stable commit a346137e9142
> > ("powerpc/numa: Use ibm,max-associativity-domains to discover possible nodes")
> > support this property to mark initially memory-less NUMA nodes as "possible"
> > to allow further memory hot-add to them.
> > 
> > Advertise this property for pSeries machines to let guest kernels detect
> > maximum supported node configuration and benefit from kernel side change
> > when hot-add memory to specific, possibly empty before, NUMA node.
> > 
> > Signed-off-by: Serhii Popovych <spopovyc@redhat.com>
> > ---
> >  hw/ppc/spapr.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 2c0be8c..3f61785 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -909,6 +909,14 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr, void *fdt)
> >          0, cpu_to_be32(SPAPR_MEMORY_BLOCK_SIZE),
> >          cpu_to_be32(max_cpus / smp_threads),
> >      };
> > +    uint32_t maxdomains[] = {
> > +        cpu_to_be32(5),
> > +        cpu_to_be32(0),
> > +        cpu_to_be32(0),
> > +        cpu_to_be32(0),
> > +        cpu_to_be32(nb_numa_nodes - 1),
> > +        cpu_to_be32(0),
> > +    };  
> 
> Ah.. close, but not quite right.  This is saying that there's exactly
> one node at the bottom (cpu) level, which isn't what we want.  Instead
> of setting it to 0, we want to completely drop that layer, keeping it
> unspecified.
> 
> To do that you need to change the first cell from 5 to 4 (since only 4
> levels will be listed) and drop the last cell entirely.
> 

Alternatively, if we don't want to do any assumptions on the guest
expectations, it is possible to pass the right value in the 6th cell:

	cpu_to_be32(spapr_vcpu_id(spapr, max_cpus - 1))

> >      _FDT(rtas = fdt_add_subnode(fdt, 0, "rtas"));
> >  
> > @@ -945,6 +953,9 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr, void *fdt)
> >      _FDT(fdt_setprop(fdt, rtas, "ibm,associativity-reference-points",
> >                       refpoints, sizeof(refpoints)));
> >  
> > +    _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains",
> > +                     maxdomains, sizeof(maxdomains)));
> > +
> >      _FDT(fdt_setprop_cell(fdt, rtas, "rtas-error-log-max",
> >                            RTAS_ERROR_LOG_MAX));
> >      _FDT(fdt_setprop_cell(fdt, rtas, "rtas-event-scan-rate",  
>