[Qemu-devel] [RFC PATCH 0/6] hw/arm/virt: Introduce cpu topology support

[Qemu-devel] [RFC PATCH 0/6] hw/arm/virt: Introduce cpu topology support

[Andrew Jones]

This series provides support for booting mach-virt machines with
non-flat cpu topology, i.e. enabling the extended options of the
'-smp' command line parameter (sockets,cores,threads). Both DT and
ACPI description generators are added. We only apply the new feature
to 3.1 and later machine types, as the change is guest visible, even
when no command line change is made. This is because the basic
'-smp <N>' parameter makes the assumption that <N> refers to the
number of sockets, but when no topology description is provided,
Linux will use the MPIDR to guess. Neither the MPIDR exposed to
the guest when running with KVM nor TCG currently provides socket
information, leaving Linux to assume all processing elements are
cores in the same socket. For example, before this series '-smp 4'
would show up in the guest as

 CPU(s):                4
 On-line CPU(s) list:   0-3
 Thread(s) per core:    1
 Core(s) per socket:    4
 Socket(s):             1

and after it shows up as

 CPU(s):                4
 On-line CPU(s) list:   0-3
 Thread(s) per core:    1
 Core(s) per socket:    1
 Socket(s):             4

It's not expected that this should be a problem, but it's worth
considering. The only way to avoid the silent change is for QEMU to
provide boards a way to override the default '-smp' parsing function.
Otherwise, if a user wants to avoid a guest visible change, but still
use a 3.1 or later mach-virt machine type, then they must ensure the
command line specifies a single socket, e.g. '-smp sockets=1,cores=4'

Thanks,
drew


Andrew Jones (6):
  hw/arm/virt: Add virt-3.1 machine type
  device_tree: add qemu_fdt_add_path
  hw/arm/virt: DT: add cpu-map
  hw/arm/virt-acpi-build: distinguish possible and present cpus
  virt-acpi-build: add PPTT table
  hw/arm/virt: cpu topology: don't allow threads

 device_tree.c                | 24 +++++++++++++
 hw/acpi/aml-build.c          | 50 ++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c     | 25 ++++++++++---
 hw/arm/virt.c                | 69 +++++++++++++++++++++++++++++++++---
 include/hw/acpi/aml-build.h  |  2 ++
 include/hw/arm/virt.h        |  1 +
 include/sysemu/device_tree.h |  1 +
 7 files changed, 162 insertions(+), 10 deletions(-)

-- 
2.17.1

[Qemu-devel] [RFC PATCH 1/6] hw/arm/virt: Add virt-3.1 machine type

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 hw/arm/virt.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 281ddcdf6e26..880441275031 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1757,10 +1757,7 @@ static void machvirt_machine_init(void)
 }
 type_init(machvirt_machine_init);
 
-#define VIRT_COMPAT_2_12 \
-    HW_COMPAT_2_12
-
-static void virt_3_0_instance_init(Object *obj)
+static void virt_3_1_instance_init(Object *obj)
 {
     VirtMachineState *vms = VIRT_MACHINE(obj);
     VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
@@ -1830,10 +1827,24 @@ static void virt_3_0_instance_init(Object *obj)
     vms->irqmap = a15irqmap;
 }
 
+static void virt_machine_3_1_options(MachineClass *mc)
+{
+}
+DEFINE_VIRT_MACHINE_AS_LATEST(3, 1)
+
+static void virt_3_0_instance_init(Object *obj)
+{
+    virt_3_1_instance_init(obj);
+}
+
 static void virt_machine_3_0_options(MachineClass *mc)
 {
+    virt_machine_3_1_options(mc);
 }
-DEFINE_VIRT_MACHINE_AS_LATEST(3, 0)
+DEFINE_VIRT_MACHINE(3, 0)
+
+#define VIRT_COMPAT_2_12 \
+    HW_COMPAT_2_12
 
 static void virt_2_12_instance_init(Object *obj)
 {
-- 
2.17.1

Re: [Qemu-devel] [RFC PATCH 1/6] hw/arm/virt: Add virt-3.1 machine type

[Igor Mammedov]

On Wed,  4 Jul 2018 14:49:18 +0200
Andrew Jones <drjones@redhat.com> wrote:

> Signed-off-by: Andrew Jones <drjones@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/arm/virt.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 281ddcdf6e26..880441275031 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1757,10 +1757,7 @@ static void machvirt_machine_init(void)
>  }
>  type_init(machvirt_machine_init);
>  
> -#define VIRT_COMPAT_2_12 \
> -    HW_COMPAT_2_12
> -
> -static void virt_3_0_instance_init(Object *obj)
> +static void virt_3_1_instance_init(Object *obj)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(obj);
>      VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> @@ -1830,10 +1827,24 @@ static void virt_3_0_instance_init(Object *obj)
>      vms->irqmap = a15irqmap;
>  }
>  
> +static void virt_machine_3_1_options(MachineClass *mc)
> +{
> +}
> +DEFINE_VIRT_MACHINE_AS_LATEST(3, 1)
> +
> +static void virt_3_0_instance_init(Object *obj)
> +{
> +    virt_3_1_instance_init(obj);
> +}
> +
>  static void virt_machine_3_0_options(MachineClass *mc)
>  {
> +    virt_machine_3_1_options(mc);
>  }
> -DEFINE_VIRT_MACHINE_AS_LATEST(3, 0)
> +DEFINE_VIRT_MACHINE(3, 0)
> +
> +#define VIRT_COMPAT_2_12 \
> +    HW_COMPAT_2_12
>  
>  static void virt_2_12_instance_init(Object *obj)
>  {

Re: [Qemu-devel] [RFC PATCH 1/6] hw/arm/virt: Add virt-3.1 machine type

[Peter Maydell]

On 23 July 2018 at 13:22, Igor Mammedov <imammedo@redhat.com> wrote:
> On Wed,  4 Jul 2018 14:49:18 +0200
> Andrew Jones <drjones@redhat.com> wrote:
>
>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>

We're going to want a virt-3.1 machine anyway, so I'm
going to take just this patch from this series and apply it
to target-arm.next.

thanks
-- PMM

[Qemu-devel] [RFC PATCH 2/6] device_tree: add qemu_fdt_add_path

[Andrew Jones]

qemu_fdt_add_path works like qemu_fdt_add_subnode, except it
also recursively adds any missing parent nodes.

Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 device_tree.c                | 24 ++++++++++++++++++++++++
 include/sysemu/device_tree.h |  1 +
 2 files changed, 25 insertions(+)

diff --git a/device_tree.c b/device_tree.c
index 6d9c9726f66c..ad570a4dbe3a 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -520,6 +520,30 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
     return retval;
 }
 
+int qemu_fdt_add_path(void *fdt, const char *path)
+{
+    char *parent;
+    int offset;
+
+    offset = fdt_path_offset(fdt, path);
+    if (offset < 0 && offset != -FDT_ERR_NOTFOUND) {
+        error_report("%s Couldn't find node %s: %s", __func__, path,
+                     fdt_strerror(offset));
+        exit(1);
+    }
+
+    if (offset != -FDT_ERR_NOTFOUND) {
+        return offset;
+    }
+
+    parent = g_strdup(path);
+    strrchr(parent, '/')[0] = '\0';
+    qemu_fdt_add_path(fdt, parent);
+    g_free(parent);
+
+    return qemu_fdt_add_subnode(fdt, path);
+}
+
 void qemu_fdt_dumpdtb(void *fdt, int size)
 {
     const char *dumpdtb = qemu_opt_get(qemu_get_machine_opts(), "dumpdtb");
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index c16fd69bc0b1..d62fc873a3ea 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -101,6 +101,7 @@ uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
 uint32_t qemu_fdt_alloc_phandle(void *fdt);
 int qemu_fdt_nop_node(void *fdt, const char *node_path);
 int qemu_fdt_add_subnode(void *fdt, const char *name);
+int qemu_fdt_add_path(void *fdt, const char *path);
 
 #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
     do {                                                                      \
-- 
2.17.1

Re: [Qemu-devel] [RFC PATCH 2/6] device_tree: add qemu_fdt_add_path

[Igor Mammedov]

On Wed,  4 Jul 2018 14:49:19 +0200
Andrew Jones <drjones@redhat.com> wrote:

> qemu_fdt_add_path works like qemu_fdt_add_subnode, except it
> also recursively adds any missing parent nodes.

Probably add here why new helper is need?

> 
> Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  device_tree.c                | 24 ++++++++++++++++++++++++
>  include/sysemu/device_tree.h |  1 +
>  2 files changed, 25 insertions(+)
> 
> diff --git a/device_tree.c b/device_tree.c
> index 6d9c9726f66c..ad570a4dbe3a 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -520,6 +520,30 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
>      return retval;
>  }
>  
> +int qemu_fdt_add_path(void *fdt, const char *path)
> +{
> +    char *parent;
> +    int offset;
> +
> +    offset = fdt_path_offset(fdt, path);
> +    if (offset < 0 && offset != -FDT_ERR_NOTFOUND) {
> +        error_report("%s Couldn't find node %s: %s", __func__, path,
> +                     fdt_strerror(offset));
> +        exit(1);
> +    }
> +
> +    if (offset != -FDT_ERR_NOTFOUND) {
> +        return offset;
> +    }
> +
> +    parent = g_strdup(path);
> +    strrchr(parent, '/')[0] = '\0';
> +    qemu_fdt_add_path(fdt, parent);
can it be implemented without recursion?

> +    g_free(parent);
> +
> +    return qemu_fdt_add_subnode(fdt, path);
> +}
> +
>  void qemu_fdt_dumpdtb(void *fdt, int size)
>  {
>      const char *dumpdtb = qemu_opt_get(qemu_get_machine_opts(), "dumpdtb");
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index c16fd69bc0b1..d62fc873a3ea 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -101,6 +101,7 @@ uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
>  uint32_t qemu_fdt_alloc_phandle(void *fdt);
>  int qemu_fdt_nop_node(void *fdt, const char *node_path);
>  int qemu_fdt_add_subnode(void *fdt, const char *name);

/**
 * qemu_fdt_add_path: ....
 ...
*/

It would be nice to have a doc comment here

> +int qemu_fdt_add_path(void *fdt, const char *path);
>  
>  #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
>      do {                                                                      \

Re: [Qemu-devel] [RFC PATCH 2/6] device_tree: add qemu_fdt_add_path

[Andrew Jones]

On Mon, Jul 23, 2018 at 02:42:54PM +0200, Igor Mammedov wrote:
> On Wed,  4 Jul 2018 14:49:19 +0200
> Andrew Jones <drjones@redhat.com> wrote:
> 
> > qemu_fdt_add_path works like qemu_fdt_add_subnode, except it
> > also recursively adds any missing parent nodes.
> 
> Probably add here why new helper is need?

OK

> 
> > 
> > Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> > Cc: Alexander Graf <agraf@suse.de>
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  device_tree.c                | 24 ++++++++++++++++++++++++
> >  include/sysemu/device_tree.h |  1 +
> >  2 files changed, 25 insertions(+)
> > 
> > diff --git a/device_tree.c b/device_tree.c
> > index 6d9c9726f66c..ad570a4dbe3a 100644
> > --- a/device_tree.c
> > +++ b/device_tree.c
> > @@ -520,6 +520,30 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
> >      return retval;
> >  }
> >  
> > +int qemu_fdt_add_path(void *fdt, const char *path)
> > +{
> > +    char *parent;
> > +    int offset;
> > +
> > +    offset = fdt_path_offset(fdt, path);
> > +    if (offset < 0 && offset != -FDT_ERR_NOTFOUND) {
> > +        error_report("%s Couldn't find node %s: %s", __func__, path,
> > +                     fdt_strerror(offset));
> > +        exit(1);
> > +    }
> > +
> > +    if (offset != -FDT_ERR_NOTFOUND) {
> > +        return offset;
> > +    }
> > +
> > +    parent = g_strdup(path);
> > +    strrchr(parent, '/')[0] = '\0';
> > +    qemu_fdt_add_path(fdt, parent);
> can it be implemented without recursion?

It can, but it gets ugly quick, as we need to walk forward, instead of
backward, building the path node by node, instead of letting the recursion
do its magic. g_strsplit() and g_strjoinv() help, but it's still not very
nice. This file already uses recursion in read_fstree() and the recursion
of this function is bounded by the number of /'s in the input path. I'd
rather leave this function recursive, unless people feel strongly about
it. I should add a sanity check for the root '/' though.

> 
> > +    g_free(parent);
> > +
> > +    return qemu_fdt_add_subnode(fdt, path);
> > +}
> > +
> >  void qemu_fdt_dumpdtb(void *fdt, int size)
> >  {
> >      const char *dumpdtb = qemu_opt_get(qemu_get_machine_opts(), "dumpdtb");
> > diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> > index c16fd69bc0b1..d62fc873a3ea 100644
> > --- a/include/sysemu/device_tree.h
> > +++ b/include/sysemu/device_tree.h
> > @@ -101,6 +101,7 @@ uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
> >  uint32_t qemu_fdt_alloc_phandle(void *fdt);
> >  int qemu_fdt_nop_node(void *fdt, const char *node_path);
> >  int qemu_fdt_add_subnode(void *fdt, const char *name);
> 
> /**
>  * qemu_fdt_add_path: ....
>  ...
> */
> 
> It would be nice to have a doc comment here

OK

Thanks,
drew

> 
> > +int qemu_fdt_add_path(void *fdt, const char *path);
> >  
> >  #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
> >      do {                                                                      \
> 
> 

[Qemu-devel] [RFC PATCH 3/6] hw/arm/virt: DT: add cpu-map

[Andrew Jones]

Support devicetree CPU topology descriptions.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 hw/arm/virt.c         | 35 +++++++++++++++++++++++++++++++++++
 include/hw/arm/virt.h |  1 +
 2 files changed, 36 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 880441275031..6c5fecdd61df 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -40,6 +40,7 @@
 #include "hw/devices.h"
 #include "net/net.h"
 #include "sysemu/device_tree.h"
+#include "sysemu/cpus.h"
 #include "sysemu/numa.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
@@ -302,6 +303,7 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
     int cpu;
     int addr_cells = 1;
     const MachineState *ms = MACHINE(vms);
+    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
 
     /*
      * From Documentation/devicetree/bindings/arm/cpus.txt
@@ -358,8 +360,38 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
                 ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
         }
 
+        qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle",
+                              qemu_fdt_alloc_phandle(vms->fdt));
         g_free(nodename);
     }
+
+    if (!vmc->ignore_cpu_topology) {
+        /* From Documentation/devicetree/bindings/arm/topology.txt
+         */
+        qemu_fdt_add_subnode(vms->fdt, "/cpus/cpu-map");
+
+        for (cpu = vms->smp_cpus - 1; cpu >= 0; cpu--) {
+            char *cpu_path = g_strdup_printf("/cpus/cpu@%d", cpu);
+            char *map_path;
+
+            if (smp_threads > 1) {
+                map_path = g_strdup_printf(
+                               "/cpus/cpu-map/%s%d/%s%d/%s%d",
+                               "cluster", cpu / (smp_cores * smp_threads),
+                               "core", (cpu / smp_threads) % smp_cores,
+                               "thread", cpu % smp_threads);
+            } else {
+                map_path = g_strdup_printf(
+                               "/cpus/cpu-map/%s%d/%s%d",
+                               "cluster", cpu / smp_cores,
+                               "core", cpu % smp_cores);
+            }
+            qemu_fdt_add_path(vms->fdt, map_path);
+            qemu_fdt_setprop_phandle(vms->fdt, map_path, "cpu", cpu_path);
+            g_free(map_path);
+            g_free(cpu_path);
+        }
+    }
 }
 
 static void fdt_add_its_gic_node(VirtMachineState *vms)
@@ -1839,7 +1871,10 @@ static void virt_3_0_instance_init(Object *obj)
 
 static void virt_machine_3_0_options(MachineClass *mc)
 {
+    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
+
     virt_machine_3_1_options(mc);
+    vmc->ignore_cpu_topology = true;
 }
 DEFINE_VIRT_MACHINE(3, 0)
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 9a870ccb6a57..deb8bee72cda 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -94,6 +94,7 @@ typedef struct MemMapEntry {
 typedef struct {
     MachineClass parent;
     bool disallow_affinity_adjustment;
+    bool ignore_cpu_topology;
     bool no_its;
     bool no_pmu;
     bool claim_edge_triggered_timers;
-- 
2.17.1

Re: [Qemu-devel] [RFC PATCH 3/6] hw/arm/virt: DT: add cpu-map

[Igor Mammedov]

On Wed,  4 Jul 2018 14:49:20 +0200
Andrew Jones <drjones@redhat.com> wrote:

> Support devicetree CPU topology descriptions.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  hw/arm/virt.c         | 35 +++++++++++++++++++++++++++++++++++
>  include/hw/arm/virt.h |  1 +
>  2 files changed, 36 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 880441275031..6c5fecdd61df 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -40,6 +40,7 @@
>  #include "hw/devices.h"
>  #include "net/net.h"
>  #include "sysemu/device_tree.h"
> +#include "sysemu/cpus.h"
>  #include "sysemu/numa.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/kvm.h"
> @@ -302,6 +303,7 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
>      int cpu;
>      int addr_cells = 1;
>      const MachineState *ms = MACHINE(vms);
> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>  
>      /*
>       * From Documentation/devicetree/bindings/arm/cpus.txt
> @@ -358,8 +360,38 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
>                  ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
>          }
>  
> +        qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle",
> +                              qemu_fdt_alloc_phandle(vms->fdt));
it's not obvious how this hunk is related to patch
(also it affects ignore_cpu_topology = true which probably isn't intended)

>          g_free(nodename);
>      }
> +
> +    if (!vmc->ignore_cpu_topology) {
> +        /* From Documentation/devicetree/bindings/arm/topology.txt
> +         */
> +        qemu_fdt_add_subnode(vms->fdt, "/cpus/cpu-map");
> +
> +        for (cpu = vms->smp_cpus - 1; cpu >= 0; cpu--) {
I'd iterate over possible_cpus array instead

> +            char *cpu_path = g_strdup_printf("/cpus/cpu@%d", cpu);
> +            char *map_path;
> +
> +            if (smp_threads > 1) {
> +                map_path = g_strdup_printf(
> +                               "/cpus/cpu-map/%s%d/%s%d/%s%d",
> +                               "cluster", cpu / (smp_cores * smp_threads),
> +                               "core", (cpu / smp_threads) % smp_cores,
> +                               "thread", cpu % smp_threads);
> +            } else {
> +                map_path = g_strdup_printf(
> +                               "/cpus/cpu-map/%s%d/%s%d",
> +                               "cluster", cpu / smp_cores,
> +                               "core", cpu % smp_cores);
> +            }
not sure about direct calculation of numbers here,
do they relate in any way to ms->possible_cpus->cpus[].props.(socket|core|thread-id) ?


> +            qemu_fdt_add_path(vms->fdt, map_path);
> +            qemu_fdt_setprop_phandle(vms->fdt, map_path, "cpu", cpu_path);
> +            g_free(map_path);
> +            g_free(cpu_path);
> +        }
> +    }
>  }
>  
>  static void fdt_add_its_gic_node(VirtMachineState *vms)
> @@ -1839,7 +1871,10 @@ static void virt_3_0_instance_init(Object *obj)
>  
>  static void virt_machine_3_0_options(MachineClass *mc)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
>      virt_machine_3_1_options(mc);
> +    vmc->ignore_cpu_topology = true;
>  }
>  DEFINE_VIRT_MACHINE(3, 0)
>  
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 9a870ccb6a57..deb8bee72cda 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -94,6 +94,7 @@ typedef struct MemMapEntry {
>  typedef struct {
>      MachineClass parent;
>      bool disallow_affinity_adjustment;
> +    bool ignore_cpu_topology;
missing doc comment for the knob

>      bool no_its;
>      bool no_pmu;
>      bool claim_edge_triggered_timers;

[Qemu-devel] [RFC PATCH 4/6] hw/arm/virt-acpi-build: distinguish possible and present cpus

[Andrew Jones]

When building ACPI tables regarding CPUs we should always build
them for the number of possible CPUs, not the number of present
CPUs. We then ensure only the present CPUs are enabled.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 hw/arm/virt-acpi-build.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 6ea47e258832..1d1fc824da6f 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -49,14 +49,22 @@
 #define ARM_SPI_BASE 32
 #define ACPI_POWER_BUTTON_DEVICE "PWRB"
 
-static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
+static int possible_cpus(VirtMachineState *vms)
+{
+    return MACHINE_GET_CLASS(vms)->possible_cpu_arch_ids(MACHINE(vms))->len;
+}
+
+static void acpi_dsdt_add_cpus(Aml *scope, int possible, int present)
 {
     uint16_t i;
 
-    for (i = 0; i < smp_cpus; i++) {
+    for (i = 0; i < possible; i++) {
         Aml *dev = aml_device("C%.03X", i);
         aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007")));
         aml_append(dev, aml_name_decl("_UID", aml_int(i)));
+        if (i >= present) {
+            aml_append(dev, aml_name_decl("_STA", aml_int(0)));
+        }
         aml_append(scope, dev);
     }
 }
@@ -650,7 +658,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     gicd->base_address = cpu_to_le64(memmap[VIRT_GIC_DIST].base);
     gicd->version = vms->gic_version;
 
-    for (i = 0; i < vms->smp_cpus; i++) {
+    for (i = 0; i < possible_cpus(vms); i++) {
         AcpiMadtGenericCpuInterface *gicc = acpi_data_push(table_data,
                                                            sizeof(*gicc));
         ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
@@ -663,7 +671,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         gicc->cpu_interface_number = cpu_to_le32(i);
         gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
         gicc->uid = cpu_to_le32(i);
-        gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
+        if (i < vms->smp_cpus) {
+            gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
+        }
 
         if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
             gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
@@ -763,7 +773,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
      * the RTC ACPI device at all when using UEFI.
      */
     scope = aml_scope("\\_SB");
-    acpi_dsdt_add_cpus(scope, vms->smp_cpus);
+    acpi_dsdt_add_cpus(scope, possible_cpus(vms), vms->smp_cpus);
     acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
                        (irqmap[VIRT_UART] + ARM_SPI_BASE));
     acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
-- 
2.17.1

Re: [Qemu-devel] [RFC PATCH 4/6] hw/arm/virt-acpi-build: distinguish possible and present cpus

[Igor Mammedov]

On Wed,  4 Jul 2018 14:49:21 +0200
Andrew Jones <drjones@redhat.com> wrote:

> When building ACPI tables regarding CPUs we should always build
> them for the number of possible CPUs, not the number of present
> CPUs. We then ensure only the present CPUs are enabled.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 6ea47e258832..1d1fc824da6f 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -49,14 +49,22 @@
>  #define ARM_SPI_BASE 32
>  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>  
> -static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
> +static int possible_cpus(VirtMachineState *vms)
> +{
> +    return MACHINE_GET_CLASS(vms)->possible_cpu_arch_ids(MACHINE(vms))->len;
> +}
> +
> +static void acpi_dsdt_add_cpus(Aml *scope, int possible, int present)
>  {
>      uint16_t i;
>  
> -    for (i = 0; i < smp_cpus; i++) {
> +    for (i = 0; i < possible; i++) {
>          Aml *dev = aml_device("C%.03X", i);
>          aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007")));
>          aml_append(dev, aml_name_decl("_UID", aml_int(i)));
> +        if (i >= present) {

acpi_dsdt_add_cpus(Aml *scope, const CPUArchIdList *possible_cpus = mc->possible_cpu_arch_ids(machine)) {
      for (i = 0; i < possible_cpus->len; i++) {
          ...
          if (possible_cpus->cpus[i].cpu == NULL)
               aml_append(dev, aml_name_decl("_STA", aml_int(0)));
      }
}

would be better to be consistent with x86 variant (well I prefer using a single source for CPU enumeration whenever possible) and drop helper above.

> +            aml_append(dev, aml_name_decl("_STA", aml_int(0)));
> +        }
>          aml_append(scope, dev);
>      }
>  }
> @@ -650,7 +658,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      gicd->base_address = cpu_to_le64(memmap[VIRT_GIC_DIST].base);
>      gicd->version = vms->gic_version;
>  
> -    for (i = 0; i < vms->smp_cpus; i++) {
> +    for (i = 0; i < possible_cpus(vms); i++) {
ditto

>          AcpiMadtGenericCpuInterface *gicc = acpi_data_push(table_data,
>                                                             sizeof(*gicc));
>          ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
> @@ -663,7 +671,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          gicc->cpu_interface_number = cpu_to_le32(i);
>          gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
>          gicc->uid = cpu_to_le32(i);
> -        gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
> +        if (i < vms->smp_cpus) {
> +            gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
> +        }
>  
>          if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
>              gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
> @@ -763,7 +773,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>       * the RTC ACPI device at all when using UEFI.
>       */
>      scope = aml_scope("\\_SB");
> -    acpi_dsdt_add_cpus(scope, vms->smp_cpus);
> +    acpi_dsdt_add_cpus(scope, possible_cpus(vms), vms->smp_cpus);
>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);

[Qemu-devel] [RFC PATCH 5/6] virt-acpi-build: add PPTT table

[Andrew Jones]

The ACPI PPTT table supports topology descriptions for ACPI
guests. Note, while a DT boot Linux guest with a non-flat CPU
topology will see socket and core IDs being sequential integers
starting from zero, e.g. with -smp 4,sockets=2,cores=2,threads=1

a DT boot produces

 cpu:  0 package_id:  0 core_id:  0
 cpu:  1 package_id:  0 core_id:  1
 cpu:  2 package_id:  1 core_id:  0
 cpu:  3 package_id:  1 core_id:  1

an ACPI boot produces

 cpu:  0 package_id: 36 core_id:  0
 cpu:  1 package_id: 36 core_id:  1
 cpu:  2 package_id: 96 core_id:  2
 cpu:  3 package_id: 96 core_id:  3

This is due to several reasons:

 1) DT cpu nodes do not have an equivalent field to what the PPTT
    ACPI Processor ID must be, i.e. something equal to the MADT CPU
    UID or equal to the UID of an ACPI processor container. In both
    ACPI cases those are platform dependant IDs assigned by the
    vendor.

 2) While QEMU is the vendor for a guest, if the topology specifies
    SMT (> 1 thread), then, with ACPI, it is impossible to assign a
    core-id the same value as a package-id, thus it is not possible
    to have package-id=0 and core-id=0. This is because package and
    core containers must be in the same ACPI namespace and therefore
    must have unique UIDs.

 3) ACPI processor containers are not required for PPTT tables to
    be used and, due to the limitations of which IDs are selected
    described above in (2), they are not helpful for QEMU, so we
    don't build them with this patch. In the absence of them, Linux
    assigns its own unique IDs. The maintainers have chosen not to use
    counters from zero, but rather ACPI table offsets, which explains
    why the numbers are so much larger than with DT.

 4) When there is no SMT (threads=1) the core IDs for ACPI boot guests
    match the logical CPU IDs, because these IDs must be equal to the
    MADT CPU UID (as no processor containers are present), and QEMU
    uses the logical CPU ID for these MADT IDs.

Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 hw/acpi/aml-build.c         | 50 +++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    |  5 ++++
 include/hw/acpi/aml-build.h |  2 ++
 3 files changed, 57 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 1e43cd736de9..37e8f5182ae9 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -24,6 +24,7 @@
 #include "hw/acpi/aml-build.h"
 #include "qemu/bswap.h"
 #include "qemu/bitops.h"
+#include "sysemu/cpus.h"
 #include "sysemu/numa.h"
 
 static GArray *build_alloc_array(void)
@@ -1679,6 +1680,55 @@ void build_slit(GArray *table_data, BIOSLinker *linker)
                  table_data->len - slit_start, 1, NULL, NULL);
 }
 
+/*
+ * ACPI 6.2 Processor Properties Topology Table (PPTT)
+ */
+static void build_cpu_hierarchy(GArray *tbl, uint32_t flags,
+                                uint32_t parent, uint32_t id)
+{
+    build_append_byte(tbl, 0);  /* Type 0 - processor */
+    build_append_byte(tbl, 20); /* Length, no private resources */
+    build_append_int_noprefix(tbl, 0, 2);           /* Reserved */
+    build_append_int_noprefix(tbl, flags, 4);
+    build_append_int_noprefix(tbl, parent, 4);
+    build_append_int_noprefix(tbl, id, 4);
+    build_append_int_noprefix(tbl, 0, 4); /* Num private resources */
+}
+
+void build_pptt(GArray *table_data, BIOSLinker *linker, int possible_cpus)
+{
+    int pptt_start = table_data->len;
+    int uid = 0, cpus = 0, socket;
+
+    acpi_data_push(table_data, sizeof(AcpiTableHeader));
+
+    for (socket = 0; cpus < possible_cpus; socket++) {
+        uint32_t socket_offset = table_data->len - pptt_start;
+        int core;
+
+        build_cpu_hierarchy(table_data, 1, 0, socket);
+
+        for (core = 0; core < smp_cores; core++) {
+            uint32_t core_offset = table_data->len - pptt_start;
+            int thread;
+
+            if (smp_threads > 1) {
+                build_cpu_hierarchy(table_data, 0, socket_offset, core);
+                for (thread = 0; thread < smp_threads; thread++) {
+                    build_cpu_hierarchy(table_data, 2, core_offset, uid++);
+                }
+             } else {
+                build_cpu_hierarchy(table_data, 2, socket_offset, uid++);
+             }
+        }
+        cpus += smp_cores * smp_threads;
+    }
+
+    build_header(linker, table_data,
+                 (void *)(table_data->data + pptt_start), "PPTT",
+                 table_data->len - pptt_start, 1, NULL, NULL);
+}
+
 /* build rev1/rev3/rev5.1 FADT */
 void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
                 const char *oem_id, const char *oem_table_id)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 1d1fc824da6f..aa77e1f018d9 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -832,6 +832,11 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     acpi_add_table(table_offsets, tables_blob);
     build_madt(tables_blob, tables->linker, vms);
 
+    if (!vmc->ignore_cpu_topology) {
+        acpi_add_table(table_offsets, tables_blob);
+        build_pptt(tables_blob, tables->linker, possible_cpus(vms));
+    }
+
     acpi_add_table(table_offsets, tables_blob);
     build_gtdt(tables_blob, tables->linker, vms);
 
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 6c36903c0a5d..2b0fde6bd417 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -414,6 +414,8 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
 
 void build_slit(GArray *table_data, BIOSLinker *linker);
 
+void build_pptt(GArray *table_data, BIOSLinker *linker, int possible_cpus);
+
 void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
                 const char *oem_id, const char *oem_table_id);
 #endif
-- 
2.17.1

Re: [Qemu-devel] [RFC PATCH 5/6] virt-acpi-build: add PPTT table

[Andrew Jones]

On Wed, Jul 04, 2018 at 02:49:22PM +0200, Andrew Jones wrote:
> The ACPI PPTT table supports topology descriptions for ACPI
> guests. Note, while a DT boot Linux guest with a non-flat CPU
> topology will see socket and core IDs being sequential integers
> starting from zero, e.g. with -smp 4,sockets=2,cores=2,threads=1
> 
> a DT boot produces
> 
>  cpu:  0 package_id:  0 core_id:  0
>  cpu:  1 package_id:  0 core_id:  1
>  cpu:  2 package_id:  1 core_id:  0
>  cpu:  3 package_id:  1 core_id:  1
> 
> an ACPI boot produces
> 
>  cpu:  0 package_id: 36 core_id:  0
>  cpu:  1 package_id: 36 core_id:  1
>  cpu:  2 package_id: 96 core_id:  2
>  cpu:  3 package_id: 96 core_id:  3
> 
> This is due to several reasons:
> 
>  1) DT cpu nodes do not have an equivalent field to what the PPTT
>     ACPI Processor ID must be, i.e. something equal to the MADT CPU
>     UID or equal to the UID of an ACPI processor container. In both
>     ACPI cases those are platform dependant IDs assigned by the
>     vendor.
> 
>  2) While QEMU is the vendor for a guest, if the topology specifies
>     SMT (> 1 thread), then, with ACPI, it is impossible to assign a
>     core-id the same value as a package-id, thus it is not possible
>     to have package-id=0 and core-id=0. This is because package and
>     core containers must be in the same ACPI namespace and therefore
>     must have unique UIDs.
> 
>  3) ACPI processor containers are not required for PPTT tables to
>     be used and, due to the limitations of which IDs are selected
>     described above in (2), they are not helpful for QEMU, so we
>     don't build them with this patch. In the absence of them, Linux
>     assigns its own unique IDs. The maintainers have chosen not to use
>     counters from zero, but rather ACPI table offsets, which explains
>     why the numbers are so much larger than with DT.
> 
>  4) When there is no SMT (threads=1) the core IDs for ACPI boot guests
>     match the logical CPU IDs, because these IDs must be equal to the
>     MADT CPU UID (as no processor containers are present), and QEMU
>     uses the logical CPU ID for these MADT IDs.
> 
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Shannon Zhao <zhaoshenglong@huawei.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  hw/acpi/aml-build.c         | 50 +++++++++++++++++++++++++++++++++++++
>  hw/arm/virt-acpi-build.c    |  5 ++++
>  include/hw/acpi/aml-build.h |  2 ++
>  3 files changed, 57 insertions(+)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 1e43cd736de9..37e8f5182ae9 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -24,6 +24,7 @@
>  #include "hw/acpi/aml-build.h"
>  #include "qemu/bswap.h"
>  #include "qemu/bitops.h"
> +#include "sysemu/cpus.h"
>  #include "sysemu/numa.h"
>  
>  static GArray *build_alloc_array(void)
> @@ -1679,6 +1680,55 @@ void build_slit(GArray *table_data, BIOSLinker *linker)
>                   table_data->len - slit_start, 1, NULL, NULL);
>  }
>  
> +/*
> + * ACPI 6.2 Processor Properties Topology Table (PPTT)
> + */
> +static void build_cpu_hierarchy(GArray *tbl, uint32_t flags,
> +                                uint32_t parent, uint32_t id)
> +{
> +    build_append_byte(tbl, 0);  /* Type 0 - processor */
> +    build_append_byte(tbl, 20); /* Length, no private resources */
> +    build_append_int_noprefix(tbl, 0, 2);           /* Reserved */
> +    build_append_int_noprefix(tbl, flags, 4);
> +    build_append_int_noprefix(tbl, parent, 4);
> +    build_append_int_noprefix(tbl, id, 4);
> +    build_append_int_noprefix(tbl, 0, 4); /* Num private resources */
> +}
> +
> +void build_pptt(GArray *table_data, BIOSLinker *linker, int possible_cpus)
> +{
> +    int pptt_start = table_data->len;
> +    int uid = 0, cpus = 0, socket;
> +
> +    acpi_data_push(table_data, sizeof(AcpiTableHeader));
> +
> +    for (socket = 0; cpus < possible_cpus; socket++) {
> +        uint32_t socket_offset = table_data->len - pptt_start;
> +        int core;
> +
> +        build_cpu_hierarchy(table_data, 1, 0, socket);
> +
> +        for (core = 0; core < smp_cores; core++) {
> +            uint32_t core_offset = table_data->len - pptt_start;
> +            int thread;
> +
> +            if (smp_threads > 1) {
> +                build_cpu_hierarchy(table_data, 0, socket_offset, core);
> +                for (thread = 0; thread < smp_threads; thread++) {
> +                    build_cpu_hierarchy(table_data, 2, core_offset, uid++);
> +                }
> +             } else {
> +                build_cpu_hierarchy(table_data, 2, socket_offset, uid++);

It just occurred to me that the uid++'s above should be something like
uid[i++]'s, where uid[] is an argument to the function instead. Otherwise
this general aml builder function isn't written generally enough.

> +             }
> +        }
> +        cpus += smp_cores * smp_threads;
> +    }
> +
> +    build_header(linker, table_data,
> +                 (void *)(table_data->data + pptt_start), "PPTT",
> +                 table_data->len - pptt_start, 1, NULL, NULL);
> +}
> +
>  /* build rev1/rev3/rev5.1 FADT */
>  void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>                  const char *oem_id, const char *oem_table_id)
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 1d1fc824da6f..aa77e1f018d9 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -832,6 +832,11 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>      acpi_add_table(table_offsets, tables_blob);
>      build_madt(tables_blob, tables->linker, vms);
>  
> +    if (!vmc->ignore_cpu_topology) {
> +        acpi_add_table(table_offsets, tables_blob);
> +        build_pptt(tables_blob, tables->linker, possible_cpus(vms));
> +    }
> +
>      acpi_add_table(table_offsets, tables_blob);
>      build_gtdt(tables_blob, tables->linker, vms);
>  
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 6c36903c0a5d..2b0fde6bd417 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -414,6 +414,8 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>  
>  void build_slit(GArray *table_data, BIOSLinker *linker);
>  
> +void build_pptt(GArray *table_data, BIOSLinker *linker, int possible_cpus);
> +
>  void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>                  const char *oem_id, const char *oem_table_id);
>  #endif
> -- 
> 2.17.1
> 

Re: [Qemu-devel] [RFC PATCH 5/6] virt-acpi-build: add PPTT table

[Igor Mammedov]

On Wed,  4 Jul 2018 14:49:22 +0200
Andrew Jones <drjones@redhat.com> wrote:

> The ACPI PPTT table supports topology descriptions for ACPI
> guests. Note, while a DT boot Linux guest with a non-flat CPU
> topology will see socket and core IDs being sequential integers
> starting from zero, e.g. with -smp 4,sockets=2,cores=2,threads=1
> 
> a DT boot produces
> 
>  cpu:  0 package_id:  0 core_id:  0
>  cpu:  1 package_id:  0 core_id:  1
>  cpu:  2 package_id:  1 core_id:  0
>  cpu:  3 package_id:  1 core_id:  1
> 
> an ACPI boot produces
> 
>  cpu:  0 package_id: 36 core_id:  0
>  cpu:  1 package_id: 36 core_id:  1
>  cpu:  2 package_id: 96 core_id:  2
>  cpu:  3 package_id: 96 core_id:  3
> 
> This is due to several reasons:
> 
>  1) DT cpu nodes do not have an equivalent field to what the PPTT
>     ACPI Processor ID must be, i.e. something equal to the MADT CPU
>     UID or equal to the UID of an ACPI processor container. In both
>     ACPI cases those are platform dependant IDs assigned by the
>     vendor.
> 
>  2) While QEMU is the vendor for a guest, if the topology specifies
>     SMT (> 1 thread), then, with ACPI, it is impossible to assign a
>     core-id the same value as a package-id, thus it is not possible
>     to have package-id=0 and core-id=0. This is because package and
>     core containers must be in the same ACPI namespace and therefore
>     must have unique UIDs.
> 
>  3) ACPI processor containers are not required for PPTT tables to
>     be used and, due to the limitations of which IDs are selected
>     described above in (2), they are not helpful for QEMU, so we
>     don't build them with this patch. In the absence of them, Linux
>     assigns its own unique IDs. The maintainers have chosen not to use
>     counters from zero, but rather ACPI table offsets, which explains
>     why the numbers are so much larger than with DT.
> 
>  4) When there is no SMT (threads=1) the core IDs for ACPI boot guests
>     match the logical CPU IDs, because these IDs must be equal to the
>     MADT CPU UID (as no processor containers are present), and QEMU
>     uses the logical CPU ID for these MADT IDs.
> 
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Shannon Zhao <zhaoshenglong@huawei.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  hw/acpi/aml-build.c         | 50 +++++++++++++++++++++++++++++++++++++
>  hw/arm/virt-acpi-build.c    |  5 ++++
>  include/hw/acpi/aml-build.h |  2 ++
>  3 files changed, 57 insertions(+)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 1e43cd736de9..37e8f5182ae9 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -24,6 +24,7 @@
>  #include "hw/acpi/aml-build.h"
>  #include "qemu/bswap.h"
>  #include "qemu/bitops.h"
> +#include "sysemu/cpus.h"
>  #include "sysemu/numa.h"
>  
>  static GArray *build_alloc_array(void)
> @@ -1679,6 +1680,55 @@ void build_slit(GArray *table_data, BIOSLinker *linker)
>                   table_data->len - slit_start, 1, NULL, NULL);
>  }
>  
> +/*
> + * ACPI 6.2 Processor Properties Topology Table (PPTT)
ACPI 6.2: 5.2.29.1 Processor hierarchy node structure (Type 0)

> + */
> +static void build_cpu_hierarchy(GArray *tbl, uint32_t flags,
> +                                uint32_t parent, uint32_t id)
build_processor_hierarchy_node()

> +{
> +    build_append_byte(tbl, 0);  /* Type 0 - processor */
> +    build_append_byte(tbl, 20); /* Length, no private resources */
> +    build_append_int_noprefix(tbl, 0, 2);           /* Reserved */
> +    build_append_int_noprefix(tbl, flags, 4); 
just being pedantic, even for obvious fields add a comment that matches
field name in spec, like:

  build_append_int_noprefix(tbl, flags, 4); /* Parent */


> +    build_append_int_noprefix(tbl, parent, 4);
> +    build_append_int_noprefix(tbl, id, 4);
> +    build_append_int_noprefix(tbl, 0, 4); /* Num private resources */
put comment on new line if it doesn't fit into 80limit but use full
field name from spec

> +}
> +
> +void build_pptt(GArray *table_data, BIOSLinker *linker, int possible_cpus)
> +{
> +    int pptt_start = table_data->len;
> +    int uid = 0, cpus = 0, socket;
> +
> +    acpi_data_push(table_data, sizeof(AcpiTableHeader));
> +
> +    for (socket = 0; cpus < possible_cpus; socket++) {
probably should use possible_cpus->cpus[] here to iterate over
cpus and maybe socket_id... from there as well

> +        uint32_t socket_offset = table_data->len - pptt_start;
> +        int core;
> +
> +        build_cpu_hierarchy(table_data, 1, 0, socket);

build_cpu_hierarchy(table_data, ACPI_PROC_HEIRARHY_PACKAGE, 0 /* no parent */ , socket);
> +
> +        for (core = 0; core < smp_cores; core++) {
> +            uint32_t core_offset = table_data->len - pptt_start;
> +            int thread;
> +
> +            if (smp_threads > 1) {
> +                build_cpu_hierarchy(table_data, 0, socket_offset, core);
> +                for (thread = 0; thread < smp_threads; thread++) {
maybe set/use core_id and thread_id from possible_cpus instead of
making up ID numbers here?

> +                    build_cpu_hierarchy(table_data, 2, core_offset, uid++);
> +                }
> +             } else {
> +                build_cpu_hierarchy(table_data, 2, socket_offset, uid++);
> +             }
> +        }
> +        cpus += smp_cores * smp_threads;
> +    }
> +
> +    build_header(linker, table_data,
> +                 (void *)(table_data->data + pptt_start), "PPTT",
> +                 table_data->len - pptt_start, 1, NULL, NULL);
> +}
> +
>  /* build rev1/rev3/rev5.1 FADT */
>  void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>                  const char *oem_id, const char *oem_table_id)
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 1d1fc824da6f..aa77e1f018d9 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -832,6 +832,11 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>      acpi_add_table(table_offsets, tables_blob);
>      build_madt(tables_blob, tables->linker, vms);
>  
> +    if (!vmc->ignore_cpu_topology) {
> +        acpi_add_table(table_offsets, tables_blob);
> +        build_pptt(tables_blob, tables->linker, possible_cpus(vms));
> +    }
> +
>      acpi_add_table(table_offsets, tables_blob);
>      build_gtdt(tables_blob, tables->linker, vms);
>  
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 6c36903c0a5d..2b0fde6bd417 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -414,6 +414,8 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>  
>  void build_slit(GArray *table_data, BIOSLinker *linker);
>  
> +void build_pptt(GArray *table_data, BIOSLinker *linker, int possible_cpus);
> +
>  void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>                  const char *oem_id, const char *oem_table_id);
>  #endif

[Qemu-devel] [RFC PATCH 6/6] hw/arm/virt: cpu topology: don't allow threads

[Andrew Jones]

None of the cpu models supported by mach-virt support threads.
Furthermore when running with KVM and cpu=host, while the host
processor may support threads, we don't yet support telling KVM
that we want the guest to see that. So if the user tries to
select more than one thread for the cpu topology, just error-out.

We'll remove the restriction for KVM guests after adding support
to KVM and QEMU for userspace controlled VCPU MPIDRs.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 hw/arm/virt.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 6c5fecdd61df..8fc2751ab4bb 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1390,6 +1390,19 @@ static void machvirt_init(MachineState *machine)
         exit(1);
     }
 
+    if (!vmc->ignore_cpu_topology && smp_threads > 1) {
+        if (kvm_enabled() &&
+            strcmp(machine->cpu_type, ARM_CPU_TYPE_NAME("host")) == 0) {
+            error_report("mach-virt: KVM: user controlled MPIDR.MT not "
+                         "yet supported");
+        } else {
+            error_report("mach-virt: CPU type %s does not support SMT",
+                         machine->cpu_type);
+        }
+        error_report("mach-virt: smp_threads cannot be > 1");
+        exit(1);
+    }
+
     /* If we have an EL3 boot ROM then the assumption is that it will
      * implement PSCI itself, so disable QEMU's internal implementation
      * so it doesn't get in the way. Instead of starting secondary
-- 
2.17.1

RE: [Qemu-devel] [RFC PATCH 0/6] hw/arm/virt: Introduce cpu topology support

[Zengtao (B)]

Hi Andrew:

Any update for this patch series? I have met the same issue, and if the 
topology guessed by linux MPIDR conflicts with qemu specified numa, it
will failed to boot (sched domain initialization will fall into deadloop).

Thanks.

> -----Original Message-----
> From: Qemu-devel
> [mailto:qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.or
> g] On Behalf Of Andrew Jones
> Sent: Thursday, July 05, 2018 4:49 AM
> To: qemu-devel@nongnu.org; qemu-arm@nongnu.org
> Cc: wei@redhat.com; peter.maydell@linaro.org; eric.auger@redhat.com;
> imammedo@redhat.com
> Subject: [Qemu-devel] [RFC PATCH 0/6] hw/arm/virt: Introduce cpu
> topology support
> 
> This series provides support for booting mach-virt machines with
> non-flat cpu topology, i.e. enabling the extended options of the
> '-smp' command line parameter (sockets,cores,threads). Both DT and
> ACPI description generators are added. We only apply the new feature
> to 3.1 and later machine types, as the change is guest visible, even
> when no command line change is made. This is because the basic
> '-smp <N>' parameter makes the assumption that <N> refers to the
> number of sockets, but when no topology description is provided,
> Linux will use the MPIDR to guess. Neither the MPIDR exposed to
> the guest when running with KVM nor TCG currently provides socket
> information, leaving Linux to assume all processing elements are
> cores in the same socket. For example, before this series '-smp 4'
> would show up in the guest as
> 
>  CPU(s):                4
>  On-line CPU(s) list:   0-3
>  Thread(s) per core:    1
>  Core(s) per socket:    4
>  Socket(s):             1
> 
> and after it shows up as
> 
>  CPU(s):                4
>  On-line CPU(s) list:   0-3
>  Thread(s) per core:    1
>  Core(s) per socket:    1
>  Socket(s):             4
> 
> It's not expected that this should be a problem, but it's worth
> considering. The only way to avoid the silent change is for QEMU to
> provide boards a way to override the default '-smp' parsing function.
> Otherwise, if a user wants to avoid a guest visible change, but still
> use a 3.1 or later mach-virt machine type, then they must ensure the
> command line specifies a single socket, e.g. '-smp sockets=1,cores=4'
> 
> Thanks,
> drew
> 
> 
> Andrew Jones (6):
>   hw/arm/virt: Add virt-3.1 machine type
>   device_tree: add qemu_fdt_add_path
>   hw/arm/virt: DT: add cpu-map
>   hw/arm/virt-acpi-build: distinguish possible and present cpus
>   virt-acpi-build: add PPTT table
>   hw/arm/virt: cpu topology: don't allow threads
> 
>  device_tree.c                | 24 +++++++++++++
>  hw/acpi/aml-build.c          | 50 ++++++++++++++++++++++++++
>  hw/arm/virt-acpi-build.c     | 25 ++++++++++---
>  hw/arm/virt.c                | 69
> +++++++++++++++++++++++++++++++++---
>  include/hw/acpi/aml-build.h  |  2 ++
>  include/hw/arm/virt.h        |  1 +
>  include/sysemu/device_tree.h |  1 +
>  7 files changed, 162 insertions(+), 10 deletions(-)

Re: [Qemu-devel] [RFC PATCH 0/6] hw/arm/virt: Introduce cpu topology support

[Andrew Jones]

On Mon, Dec 09, 2019 at 02:14:09AM +0000, Zengtao (B) wrote:
> Hi Andrew:
> 
> Any update for this patch series? I have met the same issue, and if the 
> topology guessed by linux MPIDR conflicts with qemu specified numa, it
> will failed to boot (sched domain initialization will fall into deadloop).

Hi Zeng,

This has been on my TODO list a long time, but it keeps getting preempted.
We need to start by giving userspace control over the MPIDRs, including
when KVM is in use. The earliest I can return to this will be mid/late
January. If you'd like to jump in on it now, then feel free.

Thanks,
drew

> 
> Thanks.
> 
> > -----Original Message-----
> > From: Qemu-devel
> > [mailto:qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.or
> > g] On Behalf Of Andrew Jones
> > Sent: Thursday, July 05, 2018 4:49 AM
> > To: qemu-devel@nongnu.org; qemu-arm@nongnu.org
> > Cc: wei@redhat.com; peter.maydell@linaro.org; eric.auger@redhat.com;
> > imammedo@redhat.com
> > Subject: [Qemu-devel] [RFC PATCH 0/6] hw/arm/virt: Introduce cpu
> > topology support
> > 
> > This series provides support for booting mach-virt machines with
> > non-flat cpu topology, i.e. enabling the extended options of the
> > '-smp' command line parameter (sockets,cores,threads). Both DT and
> > ACPI description generators are added. We only apply the new feature
> > to 3.1 and later machine types, as the change is guest visible, even
> > when no command line change is made. This is because the basic
> > '-smp <N>' parameter makes the assumption that <N> refers to the
> > number of sockets, but when no topology description is provided,
> > Linux will use the MPIDR to guess. Neither the MPIDR exposed to
> > the guest when running with KVM nor TCG currently provides socket
> > information, leaving Linux to assume all processing elements are
> > cores in the same socket. For example, before this series '-smp 4'
> > would show up in the guest as
> > 
> >  CPU(s):                4
> >  On-line CPU(s) list:   0-3
> >  Thread(s) per core:    1
> >  Core(s) per socket:    4
> >  Socket(s):             1
> > 
> > and after it shows up as
> > 
> >  CPU(s):                4
> >  On-line CPU(s) list:   0-3
> >  Thread(s) per core:    1
> >  Core(s) per socket:    1
> >  Socket(s):             4
> > 
> > It's not expected that this should be a problem, but it's worth
> > considering. The only way to avoid the silent change is for QEMU to
> > provide boards a way to override the default '-smp' parsing function.
> > Otherwise, if a user wants to avoid a guest visible change, but still
> > use a 3.1 or later mach-virt machine type, then they must ensure the
> > command line specifies a single socket, e.g. '-smp sockets=1,cores=4'
> > 
> > Thanks,
> > drew
> > 
> > 
> > Andrew Jones (6):
> >   hw/arm/virt: Add virt-3.1 machine type
> >   device_tree: add qemu_fdt_add_path
> >   hw/arm/virt: DT: add cpu-map
> >   hw/arm/virt-acpi-build: distinguish possible and present cpus
> >   virt-acpi-build: add PPTT table
> >   hw/arm/virt: cpu topology: don't allow threads
> > 
> >  device_tree.c                | 24 +++++++++++++
> >  hw/acpi/aml-build.c          | 50 ++++++++++++++++++++++++++
> >  hw/arm/virt-acpi-build.c     | 25 ++++++++++---
> >  hw/arm/virt.c                | 69
> > +++++++++++++++++++++++++++++++++---
> >  include/hw/acpi/aml-build.h  |  2 ++
> >  include/hw/arm/virt.h        |  1 +
> >  include/sysemu/device_tree.h |  1 +
> >  7 files changed, 162 insertions(+), 10 deletions(-)

RE: [Qemu-devel] [RFC PATCH 0/6] hw/arm/virt: Introduce cpu topology support

[Zengtao (B)]

Hi Andrew:

Thanks for your reply.
It 's fine for me if you are still tracking the thread. And I can help to test
 if needed ^_^.

> -----Original Message-----
> From: Andrew Jones [mailto:drjones@redhat.com]
> Sent: Tuesday, December 10, 2019 6:13 PM
> To: Zengtao (B)
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; wei@redhat.com;
> peter.maydell@linaro.org; eric.auger@redhat.com;
> imammedo@redhat.com; xuwei (O); huangdaode
> Subject: Re: [Qemu-devel] [RFC PATCH 0/6] hw/arm/virt: Introduce cpu
> topology support
> 
> On Mon, Dec 09, 2019 at 02:14:09AM +0000, Zengtao (B) wrote:
> > Hi Andrew:
> >
> > Any update for this patch series? I have met the same issue, and if the
> > topology guessed by linux MPIDR conflicts with qemu specified numa, it
> > will failed to boot (sched domain initialization will fall into deadloop).
> 
> Hi Zeng,
> 
> This has been on my TODO list a long time, but it keeps getting preempted.
> We need to start by giving userspace control over the MPIDRs, including
> when KVM is in use. The earliest I can return to this will be mid/late
> January. If you'd like to jump in on it now, then feel free.
> 
> Thanks,
> drew
> 
> >
> > Thanks.
> >
> > > -----Original Message-----
> > > From: Qemu-devel
> > >
> [mailto:qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.or
> > > g] On Behalf Of Andrew Jones
> > > Sent: Thursday, July 05, 2018 4:49 AM
> > > To: qemu-devel@nongnu.org; qemu-arm@nongnu.org
> > > Cc: wei@redhat.com; peter.maydell@linaro.org;
> eric.auger@redhat.com;
> > > imammedo@redhat.com
> > > Subject: [Qemu-devel] [RFC PATCH 0/6] hw/arm/virt: Introduce cpu
> > > topology support
> > >
> > > This series provides support for booting mach-virt machines with
> > > non-flat cpu topology, i.e. enabling the extended options of the
> > > '-smp' command line parameter (sockets,cores,threads). Both DT and
> > > ACPI description generators are added. We only apply the new feature
> > > to 3.1 and later machine types, as the change is guest visible, even
> > > when no command line change is made. This is because the basic
> > > '-smp <N>' parameter makes the assumption that <N> refers to the
> > > number of sockets, but when no topology description is provided,
> > > Linux will use the MPIDR to guess. Neither the MPIDR exposed to
> > > the guest when running with KVM nor TCG currently provides socket
> > > information, leaving Linux to assume all processing elements are
> > > cores in the same socket. For example, before this series '-smp 4'
> > > would show up in the guest as
> > >
> > >  CPU(s):                4
> > >  On-line CPU(s) list:   0-3
> > >  Thread(s) per core:    1
> > >  Core(s) per socket:    4
> > >  Socket(s):             1
> > >
> > > and after it shows up as
> > >
> > >  CPU(s):                4
> > >  On-line CPU(s) list:   0-3
> > >  Thread(s) per core:    1
> > >  Core(s) per socket:    1
> > >  Socket(s):             4
> > >
> > > It's not expected that this should be a problem, but it's worth
> > > considering. The only way to avoid the silent change is for QEMU to
> > > provide boards a way to override the default '-smp' parsing function.
> > > Otherwise, if a user wants to avoid a guest visible change, but still
> > > use a 3.1 or later mach-virt machine type, then they must ensure the
> > > command line specifies a single socket, e.g. '-smp sockets=1,cores=4'
> > >
> > > Thanks,
> > > drew
> > >
> > >
> > > Andrew Jones (6):
> > >   hw/arm/virt: Add virt-3.1 machine type
> > >   device_tree: add qemu_fdt_add_path
> > >   hw/arm/virt: DT: add cpu-map
> > >   hw/arm/virt-acpi-build: distinguish possible and present cpus
> > >   virt-acpi-build: add PPTT table
> > >   hw/arm/virt: cpu topology: don't allow threads
> > >
> > >  device_tree.c                | 24 +++++++++++++
> > >  hw/acpi/aml-build.c          | 50 ++++++++++++++++++++++++++
> > >  hw/arm/virt-acpi-build.c     | 25 ++++++++++---
> > >  hw/arm/virt.c                | 69
> > > +++++++++++++++++++++++++++++++++---
> > >  include/hw/acpi/aml-build.h  |  2 ++
> > >  include/hw/arm/virt.h        |  1 +
> > >  include/sysemu/device_tree.h |  1 +
> > >  7 files changed, 162 insertions(+), 10 deletions(-)