[PATCH v1 0/9] s390x: CPU Topology

[PATCH v1 0/9] s390x: CPU Topology

[Pierre Morel]

Hi,

This series is a first part of the implementation of CPU topology
for S390.

====================
A short introduction
====================

CPU Topology is described in the S390 POP with essentially the description
of two instructions:

PTF Perform Topology function used to poll for topology change
    and used to set the polarization but this part is not part of this item.

STSI Store System Information and the SYSIB 15.1.x providing the Topology
    configuration.

S390 Topology is a 6 levels hierarchical topology with up to 5 level
     of containers. The last topology level, specifying the CPU cores.
    
     To get the information on the topology, S390 provides the STSI
     instruction, which stores a structures providing the list of the
     containers used in the Machine topology: the SYSIB.
     A selector within the STSI instruction allow to chose how many topology
     levels will be provide in the SYSIB.

     Using the Topology List Entries (TLE) provided inside the SYSIB we
     the Linux kernel is able to compute the information about the cache
     distance between two cores and can use this information to take
     scheduling decisions.

     We first provide a simple topology for the case QEMU does not
     use NUMA by extending the -smp argument and then we provide more
     fine grain topology in the case QEMU uses the -numa arguments.

Note:
-----
     Z15 reports 3 levels of containers, drawers, book, sockets as
     Container-TLEs above the core description inside CPU-TLEs.

The Topology can be seen at several places inside zLinux:
    - sysfs: /sys/devices/system/cpu/cpuX/topology
    - procfs: /proc/sysinfo and /proc/cpuinfo
    - lscpu -e : gives toplogy information

The different Topology levels have names:
    - Node - Drawer - Book - sockets or physical package - core

Threads:
    Multithreading, is not part of the topology as described by the
    SYSIB 15.1.x

The interest of the guest to know the CPU topology is obviously to be
able to optimise the load balancing and the migration of threads.
KVM will have the same interest concerning vCPUs scheduling and cache
optimisation.


====================
The design
====================

1) To be ready for hotplug, I chose an Object oriented design
of the topology containers:
- A node is a bridge on the SYSBUS and defines a "node bus"
- A drawer is hotplug on the "node bus"
- A book on the "drawer bus"
- A socket on the "book bus"
- And the CPU Topology List Entry (CPU-TLE)sits on the socket bus.
These objects will be enhanced with the cache information when
NUMA is implemented.

This also allows for easy retrieval when building the different SYSIB
for Store Topology System Information (STSI)

2) Perform Topology Function (PTF) instruction is made available to the
guest with a new KVM capability and intercepted in QEMU, allowing the
guest to pool for topology changes.


======================
Current implementation
======================

* qemu command line is extended with the new topology levels.
  Here a comparison with X86

for X86:
-smp [cpus=]n[,cores=cores][,threads=threads][,dies=dies][,sockets=sockets][,maxcpus=maxcpus]

old S390:
-smp [cpus=]n[,sockets=sockets][,cores=cores][,maxcpus=maxcpus]

new S390:
-smp [cpus=]n[,drawers=drawers][,books=books][,sockets=sockets][,cores=cores][,maxcpus=maxcpus]


Example:
--------

Here we want to use all cores on book 3, on socket-id 2 and the cores 0 and 1:

/usr/local/bin/qemu-system-s390x \
        -machine s390-ccw-virtio,accel=kvm \
        -enable-kvm \
        -m 10G \
        \
        -smp 15,drawers=5,books=2,sockets=2,cores=12,maxcpus=240 \
        \
        -object memory-backend-ram,id=mem0,size=2G  \
        -object memory-backend-ram,id=mem1,size=2G  \
        -object memory-backend-ram,id=mem2,size=2G  \
        -object memory-backend-ram,id=mem3,size=2G  \
        -object memory-backend-ram,id=mem4,size=2G  \
        \
        -numa node,nodeid=0,memdev=mem0\
        -numa node,nodeid=1,memdev=mem1\
        -numa node,nodeid=2,memdev=mem2\
        -numa node,nodeid=3,memdev=mem3\
        -numa node,nodeid=4,memdev=mem4\
        \
     -numa cpu,node-id=1,core-id=0 \
     -numa cpu,node-id=4,core-id=1 \
     -numa cpu,node-id=2,socket-id=2 \
     -numa cpu,node-id=3,book-id=3 \
        \
        -netdev tap,id=hn0,queues=1 \
        \
        -device virtio-net-ccw,netdev=hn0,mq=on \
        -kernel /boot/vmlinuz-${YOUR_KERNEL} \
        -initrd ${YOUR_ROOTFS} \
        -append "loglevel=8 selinux=0 root=/dev/ram earlyprintk=1" \
        -nographic

=====================
Features and TBD list
=====================

- using a default memory device

- There is a warning about all CPU should be described in NUMA config and that
  this would be obsolete in the future, I will need help to know how to 
  handle this.

- There is no direct match between IDs shown by:
  - lscpu (unrelated numbered list),
  - used by -numa cpu (numbered list related to topology)
  - SYSIB 15.1.x (topology ID)

  in the example above socket-id=6 is shown in SYSIB 15.1.x as
  socket ID 0 of Book ID 3 since a Book has 2 sockets and appear
  on lscpu as socket 2.

- The CPU number, left column of lscpu, is used to reference a CPU
  by Linux tools
  While the CPU address is used by QEMU for hotplug.

- To plug a new CPU inside the topology one can simply use the CPU
  address like in:

  (qemu) device_add host-s390x-cpu,core-id=8
CPU NODE DRAWER BOOK SOCKET CORE L1d:L1i:L2d:L2i ONLINE CONFIGURED POLARIZATION ADDRESS
  0    0      0    0      0    0 0:0:0:0            yes yes        horizontal   0
  1    0      0    1      1    1 1:1:1:1            yes yes        horizontal   24
  2    0      0    1      1    2 2:2:2:2            yes yes        horizontal   25
....
 36    0      1    3      4   36 36:36:36:36        yes yes        horizontal   94
 37    0      1    3      4   37 37:37:37:37        yes yes        horizontal   95
 38    -      -    -      -    - :::                 no yes        horizontal   8

  # chcpu -e 38


- Documentation will come with the next iteration


Regards,
Pierre

Pierre Morel (9):
  s390x: smp: s390x dedicated smp parsing
  s390x: toplogy: adding drawers and books to smp parsing
  s390x: cpu topology: CPU topology objects and structures
  s390x: Topology list entries and SYSIB 15.x.x
  s390x: topology: implementating Store Topology System Information
  s390x: kvm: topology: interception of PTF instruction
  s390x: SCLP: reporting the maximum nested topology entries
  s390x: numa: define drawers and books for NUMA
  s390x: numa: implement NUMA for S390x

 hw/core/machine.c                  |  18 +
 hw/s390x/cpu-topology.c            | 595 +++++++++++++++++++++++++++++
 hw/s390x/meson.build               |   1 +
 hw/s390x/s390-virtio-ccw.c         | 107 +++++-
 hw/s390x/sclp.c                    |   1 +
 include/hw/s390x/cpu-topology.h    |  99 +++++
 include/hw/s390x/s390-virtio-ccw.h |   3 +
 include/hw/s390x/sclp.h            |   4 +-
 qapi/machine.json                  |   2 +
 softmmu/vl.c                       |   6 +
 target/s390x/cpu.c                 |   4 +
 target/s390x/cpu.h                 |  45 +++
 target/s390x/kvm/kvm.c             | 273 +++++++++++++
 13 files changed, 1150 insertions(+), 8 deletions(-)
 create mode 100644 hw/s390x/cpu-topology.c
 create mode 100644 include/hw/s390x/cpu-topology.h

-- 
2.25.1

[PATCH v1 1/9] s390x: smp: s390x dedicated smp parsing

[Pierre Morel]

We need a s390x dedicated SMP parsing to handle s390x specificities.

In this patch we only handle threads, cores and sockets for
s390x:
- do not support threads, we always have 1 single thread per core
- the sockets are filled one after the other with the cores

Both these handlings are different from the standard smp_parse
functionement and reflect the CPU topology in the simple case
where all CPU belong to the same book.

Topology levels above sockets, i.e. books, drawers, are not
considered at this stage and will be introduced in a later patch.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 42 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index e4b18aef49..899d3a4137 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -582,6 +582,47 @@ static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
     return newsz;
 }
 
+/*
+ * In S390CCW machine we do not support threads for now,
+ * only sockets and cores.
+ */
+static void s390_smp_parse(MachineState *ms, QemuOpts *opts)
+{
+    unsigned cpus    = qemu_opt_get_number(opts, "cpus", 1);
+    unsigned sockets = qemu_opt_get_number(opts, "sockets", 1);
+    unsigned cores   = qemu_opt_get_number(opts, "cores", 1);
+
+    if (opts) {
+        if (cpus == 0 || sockets == 0 || cores == 0) {
+            error_report("cpu topology: "
+                         "sockets (%u), cores (%u) or number of CPU(%u) "
+                         "can not be zero", sockets, cores, cpus);
+            exit(1);
+        }
+    }
+
+    ms->smp.max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
+    if (ms->smp.max_cpus < cpus) {
+        error_report("maxcpus must be equal to or greater than smp");
+        exit(1);
+    }
+
+    if (!qemu_opt_find(opts, "cores")) {
+        cores = ms->smp.max_cpus / sockets;
+    }
+
+    if (sockets * cores != ms->smp.max_cpus) {
+        error_report("Invalid CPU topology: sockets (%u) * cores (%u) "
+                     "!= maxcpus (%u)", sockets, cores, ms->smp.max_cpus);
+        exit(1);
+    }
+
+    ms->smp.threads = 1;
+    ms->smp.cpus = cpus;
+    ms->smp.cores = cores;
+    ms->smp.sockets = sockets;
+}
+
 static void ccw_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -612,6 +653,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     hc->unplug_request = s390_machine_device_unplug_request;
     nc->nmi_monitor_handler = s390_nmi;
     mc->default_ram_id = "s390.ram";
+    mc->smp_parse = s390_smp_parse;
 }
 
 static inline bool machine_get_aes_key_wrap(Object *obj, Error **errp)
-- 
2.25.1

[PATCH v1 2/9] s390x: toplogy: adding drawers and books to smp parsing

[Pierre Morel]

Drawers and Books are levels 4 and 3 of the S390 CPU
topology.
We allow the user to define these levels and we will
store the values inside the S390CcwMachineState.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c         | 22 +++++++++++++++-------
 include/hw/s390x/s390-virtio-ccw.h |  2 ++
 qapi/machine.json                  |  2 ++
 softmmu/vl.c                       |  6 ++++++
 4 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 899d3a4137..42d9be7333 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -591,12 +591,17 @@ static void s390_smp_parse(MachineState *ms, QemuOpts *opts)
     unsigned cpus    = qemu_opt_get_number(opts, "cpus", 1);
     unsigned sockets = qemu_opt_get_number(opts, "sockets", 1);
     unsigned cores   = qemu_opt_get_number(opts, "cores", 1);
+    unsigned drawers = qemu_opt_get_number(opts, "drawers", 1);
+    unsigned books   = qemu_opt_get_number(opts, "books", 1);
+    S390CcwMachineState *s390ms = S390_CCW_MACHINE(ms);
 
     if (opts) {
-        if (cpus == 0 || sockets == 0 || cores == 0) {
+        if (cpus == 0 || drawers == 0 || books == 0 || sockets == 0 ||
+            cores == 0) {
             error_report("cpu topology: "
-                         "sockets (%u), cores (%u) or number of CPU(%u) "
-                         "can not be zero", sockets, cores, cpus);
+                         "drawers (%u) books (%u) sockets (%u), cores (%u) "
+                         "or number of CPU(%u) can not be zero", drawers,
+                         books, sockets, cores, cpus);
             exit(1);
         }
     }
@@ -608,12 +613,13 @@ static void s390_smp_parse(MachineState *ms, QemuOpts *opts)
     }
 
     if (!qemu_opt_find(opts, "cores")) {
-        cores = ms->smp.max_cpus / sockets;
+        cores = ms->smp.max_cpus / (drawers * books * sockets);
     }
 
-    if (sockets * cores != ms->smp.max_cpus) {
-        error_report("Invalid CPU topology: sockets (%u) * cores (%u) "
-                     "!= maxcpus (%u)", sockets, cores, ms->smp.max_cpus);
+    if (drawers * books * sockets * cores != ms->smp.max_cpus) {
+        error_report("Invalid CPU topology: drawers (%u) books (%u) "
+                     "sockets (%u) * cores (%u) != maxcpus (%u)",
+                     drawers, books, sockets, cores, ms->smp.max_cpus);
         exit(1);
     }
 
@@ -621,6 +627,8 @@ static void s390_smp_parse(MachineState *ms, QemuOpts *opts)
     ms->smp.cpus = cpus;
     ms->smp.cores = cores;
     ms->smp.sockets = sockets;
+    s390ms->drawers = drawers;
+    s390ms->books = books;
 }
 
 static void ccw_machine_class_init(ObjectClass *oc, void *data)
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 3331990e02..fb3c3a50ce 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -28,6 +28,8 @@ struct S390CcwMachineState {
     bool dea_key_wrap;
     bool pv;
     uint8_t loadparm[8];
+    int drawers;
+    int books;
 };
 
 struct S390CcwMachineClass {
diff --git a/qapi/machine.json b/qapi/machine.json
index c3210ee1fb..98aff804c6 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -883,6 +883,8 @@
   'data': { '*node-id': 'int',
             '*socket-id': 'int',
             '*die-id': 'int',
+            '*drawer-id': 'int',
+            '*book-id': 'int',
             '*core-id': 'int',
             '*thread-id': 'int'
   }
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 4df1496101..fc73107b91 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -699,6 +699,12 @@ static QemuOptsList qemu_smp_opts = {
         }, {
             .name = "dies",
             .type = QEMU_OPT_NUMBER,
+        }, {
+            .name = "books",
+            .type = QEMU_OPT_NUMBER,
+        }, {
+            .name = "drawers",
+            .type = QEMU_OPT_NUMBER,
         }, {
             .name = "cores",
             .type = QEMU_OPT_NUMBER,
-- 
2.25.1

[PATCH v1 3/9] s390x: cpu topology: CPU topology objects and structures

[Pierre Morel]

We use new objects to have a dynamic administration of the CPU topology.
The highier level object is the S390 Node. In a first implementation
we will have only a single S390 node.
The node is built as a SYSBUS bridge during the CPU initialisation.

Every object under this single node will be build dynamically
immediately after a CPU has be realized if it is needed.
The CPU will fill the sockets once after the other, according to the
number of core per socket defined during the smp parsing, socket will fill
each book according to the book per sockect value and book each drawer,
always starting from the lowest ID to the greatest one.

Each CPU inside a socket will be represented by a bit in a 64bit
unsigned long. Set on plug and clear on unplug of a CPU.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/s390x/cpu-topology.c         | 576 ++++++++++++++++++++++++++++++++
 hw/s390x/meson.build            |   1 +
 hw/s390x/s390-virtio-ccw.c      |   4 +
 include/hw/s390x/cpu-topology.h |  91 +++++
 4 files changed, 672 insertions(+)
 create mode 100644 hw/s390x/cpu-topology.c
 create mode 100644 include/hw/s390x/cpu-topology.h

diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
new file mode 100644
index 0000000000..1224137f56
--- /dev/null
+++ b/hw/s390x/cpu-topology.c
@@ -0,0 +1,576 @@
+/*
+ * CPU Topology
+ *
+ * Copyright 2021 IBM Corp.
+ * Author(s): Pierre Morel <pmorel@linux.ibm.com>
+
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "hw/sysbus.h"
+#include "hw/s390x/cpu-topology.h"
+#include "hw/qdev-properties.h"
+#include "hw/boards.h"
+#include "qemu/typedefs.h"
+#include "hw/s390x/s390-virtio-ccw.h"
+
+static S390TopologyCores *s390_create_cores(S390TopologySocket *socket,
+                                            int origin)
+{
+    DeviceState *dev;
+    S390TopologyCores *cores;
+    const MachineState *ms = MACHINE(qdev_get_machine());
+
+    if (socket->bus->num_children >= ms->smp.cores) {
+        return NULL;
+    }
+
+    dev = qdev_new(TYPE_S390_TOPOLOGY_CORES);
+    qdev_realize_and_unref(dev, socket->bus, &error_fatal);
+
+    cores = S390_TOPOLOGY_CORES(dev);
+    cores->origin = origin;
+    socket->cnt += 1;
+
+    return cores;
+}
+
+static S390TopologySocket *s390_create_socket(S390TopologyBook *book, int id)
+{
+    DeviceState *dev;
+    S390TopologySocket *socket;
+    const MachineState *ms = MACHINE(qdev_get_machine());
+
+    if (book->bus->num_children >= ms->smp.sockets) {
+        return NULL;
+    }
+
+    dev = qdev_new(TYPE_S390_TOPOLOGY_SOCKET);
+    qdev_realize_and_unref(dev, book->bus, &error_fatal);
+
+    socket = S390_TOPOLOGY_SOCKET(dev);
+    socket->socket_id = id;
+    book->cnt++;
+
+    return socket;
+}
+
+static S390TopologyBook *s390_create_book(S390TopologyDrawer *drawer, int id)
+{
+    S390TopologyBook *book;
+    DeviceState *dev;
+    const MachineState *ms = MACHINE(qdev_get_machine());
+    S390CcwMachineState *s390ms = S390_CCW_MACHINE(ms);
+
+    if (drawer->bus->num_children >= s390ms->books) {
+        return NULL;
+    }
+
+    dev = qdev_new(TYPE_S390_TOPOLOGY_BOOK);
+    qdev_realize_and_unref(dev, drawer->bus, &error_fatal);
+
+    book = S390_TOPOLOGY_BOOK(dev);
+    book->book_id = id;
+    drawer->cnt++;
+
+    return book;
+}
+
+static S390TopologyDrawer *s390_create_drawer(S390TopologyNode *node, int id)
+{
+    S390TopologyDrawer *drawer;
+    DeviceState *dev;
+    const MachineState *ms = MACHINE(qdev_get_machine());
+    S390CcwMachineState *s390ms = S390_CCW_MACHINE(ms);
+
+    if (node->bus->num_children >= s390ms->drawers) {
+        return NULL;
+    }
+
+    dev = qdev_new(TYPE_S390_TOPOLOGY_DRAWER);
+    qdev_realize_and_unref(dev, node->bus, &error_fatal);
+
+    drawer = S390_TOPOLOGY_DRAWER(dev);
+    drawer->drawer_id = id;
+    node->cnt++;
+
+    return drawer;
+}
+
+static S390TopologyCores *s390_get_cores(S390TopologySocket *socket, int origin)
+{
+    S390TopologyCores *cores;
+    BusChild *kid;
+
+    QTAILQ_FOREACH(kid, &socket->bus->children, sibling) {
+        cores = S390_TOPOLOGY_CORES(kid->child);
+        if (cores->origin == origin) {
+            return cores;
+        }
+    }
+    return s390_create_cores(socket, origin);
+}
+
+static S390TopologySocket *s390_get_socket(S390TopologyBook *book,
+                                           int socket_id)
+{
+    S390TopologySocket *socket;
+    BusChild *kid;
+
+    QTAILQ_FOREACH(kid, &book->bus->children, sibling) {
+        socket = S390_TOPOLOGY_SOCKET(kid->child);
+        if (socket->socket_id == socket_id) {
+            return socket;
+        }
+    }
+    return s390_create_socket(book, socket_id);
+}
+
+static S390TopologyBook *s390_get_book(S390TopologyDrawer *drawer, int book_id)
+{
+    S390TopologyBook *book;
+    BusChild *kid;
+
+    QTAILQ_FOREACH(kid, &drawer->bus->children, sibling) {
+        book = S390_TOPOLOGY_BOOK(kid->child);
+        if (book->book_id == book_id) {
+            return book;
+        }
+    }
+    return s390_create_book(drawer, book_id);
+}
+
+static S390TopologyDrawer *s390_get_drawer(S390TopologyNode *node,
+                                           int drawer_id)
+{
+    S390TopologyDrawer *drawer;
+    BusChild *kid;
+
+    QTAILQ_FOREACH(kid, &node->bus->children, sibling) {
+        drawer = S390_TOPOLOGY_DRAWER(kid->child);
+        if (drawer->drawer_id == drawer_id) {
+            return drawer;
+        }
+    }
+    return s390_create_drawer(node, drawer_id);
+}
+
+/*
+ * s390_topology_new_cpu:
+ * @core_id: the core ID is machine wide
+ *
+ * We have a single node returned by s390_get_topology(),
+ * then we build the hierarchy on demand.
+ * Note that we do not destroy the hierarchy on error creating
+ * an entry in the topology, we just keept it empty.
+ * We do not need to worry about not finding a topology level
+ * entry this would have been catched during smp parsing.
+ */
+void s390_topology_new_cpu(int core_id)
+{
+    const MachineState *ms = MACHINE(qdev_get_machine());
+    S390CcwMachineState *s390ms = S390_CCW_MACHINE(ms);
+    S390TopologyDrawer *drawer;
+    S390TopologyBook *book;
+    S390TopologySocket *socket;
+    S390TopologyCores *cores;
+    S390TopologyNode *node;
+    int cores_per_drawer, cores_per_book;
+    int drawer_idx, book_idx, sock_idx;
+    int origin, bit;
+
+    cores_per_drawer = ms->smp.max_cpus / s390ms->drawers;
+    cores_per_book = cores_per_drawer / s390ms->books;
+
+    node = s390_get_topology();
+
+    drawer_idx = core_id / cores_per_drawer;
+    drawer = s390_get_drawer(node, drawer_idx);
+
+    book_idx = (core_id % cores_per_drawer) / cores_per_book;
+    book = s390_get_book(drawer, book_idx);
+
+    sock_idx = (core_id % cores_per_book) / ms->smp.cores;
+    socket = s390_get_socket(book, sock_idx);
+
+    /*
+     * We assert that all CPU are identical IFL, shared and
+     * horizontal topology, the only reason to have several
+     * S390TopologyCores is to have more than 64 CPUs.
+     */
+    origin = 64 * (core_id / 64);
+
+    cores = s390_get_cores(socket, origin);
+
+    bit = 63 - (core_id - origin);
+    set_bit(bit, &cores->mask);
+    cores->origin = origin;
+}
+
+/*
+ * Setting the first topology: 1 node, 1 drawer, 1 book, 1 socket
+ * This is enough for 64 cores if the topology is flat (single socket)
+ */
+void s390_topology_setup(MachineState *ms)
+{
+    DeviceState *dev;
+
+    /* Create NODE bridge device */
+    dev = qdev_new(TYPE_S390_TOPOLOGY_NODE);
+    object_property_add_child(qdev_get_machine(),
+                              TYPE_S390_TOPOLOGY_NODE, OBJECT(dev));
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+}
+
+S390TopologyNode *s390_get_topology(void)
+{
+    static S390TopologyNode *node;
+
+    if (!node) {
+        node = S390_TOPOLOGY_NODE(
+            object_resolve_path(TYPE_S390_TOPOLOGY_NODE, NULL));
+        assert(node != NULL);
+    }
+
+    return node;
+}
+
+/* --- CORES Definitions --- */
+
+static Property s390_topology_cores_properties[] = {
+    DEFINE_PROP_BOOL("dedicated", S390TopologyCores, dedicated, false),
+    DEFINE_PROP_UINT8("polarity", S390TopologyCores, polarity,
+                      S390_TOPOLOGY_POLARITY_H),
+    DEFINE_PROP_UINT8("cputype", S390TopologyCores, cputype,
+                      S390_TOPOLOGY_CPU_TYPE),
+    DEFINE_PROP_UINT16("origin", S390TopologyCores, origin, 0),
+    DEFINE_PROP_UINT64("mask", S390TopologyCores, mask, 0),
+    DEFINE_PROP_UINT8("id", S390TopologyCores, id, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void cpu_cores_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
+
+    device_class_set_props(dc, s390_topology_cores_properties);
+    hc->unplug = qdev_simple_device_unplug_cb;
+    dc->bus_type = TYPE_S390_TOPOLOGY_SOCKET_BUS;
+    dc->desc = "topology cpu entry";
+}
+
+static const TypeInfo cpu_cores_info = {
+    .name          = TYPE_S390_TOPOLOGY_CORES,
+    .parent        = TYPE_DEVICE,
+    .instance_size = sizeof(S390TopologyCores),
+    .class_init    = cpu_cores_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
+};
+
+/* --- SOCKETS Definitions --- */
+static Property s390_topology_socket_properties[] = {
+    DEFINE_PROP_UINT8("socket_id", S390TopologySocket, socket_id, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static char *socket_bus_get_dev_path(DeviceState *dev)
+{
+    S390TopologySocket *socket = S390_TOPOLOGY_SOCKET(dev);
+    DeviceState *book = dev->parent_bus->parent;
+    char *id = qdev_get_dev_path(book);
+    char *ret;
+
+    if (id) {
+        ret = g_strdup_printf("%s:%02d", id, socket->socket_id);
+        g_free(id);
+    } else {
+        ret = g_malloc(6);
+        snprintf(ret, 6, "_:%02d", socket->socket_id);
+    }
+
+    return ret;
+}
+
+static void socket_bus_class_init(ObjectClass *oc, void *data)
+{
+    BusClass *k = BUS_CLASS(oc);
+
+    k->get_dev_path = socket_bus_get_dev_path;
+    k->max_dev = S390_MAX_SOCKETS;
+}
+
+static const TypeInfo socket_bus_info = {
+    .name = TYPE_S390_TOPOLOGY_SOCKET_BUS,
+    .parent = TYPE_BUS,
+    .instance_size = 0,
+    .class_init = socket_bus_class_init,
+};
+
+static void s390_socket_device_realize(DeviceState *dev, Error **errp)
+{
+    S390TopologySocket *socket = S390_TOPOLOGY_SOCKET(dev);
+    BusState *bus;
+
+    bus = qbus_create(TYPE_S390_TOPOLOGY_SOCKET_BUS, dev,
+                      TYPE_S390_TOPOLOGY_SOCKET_BUS);
+    qbus_set_hotplug_handler(bus, OBJECT(dev));
+    socket->bus = bus;
+}
+
+static void socket_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
+
+    hc->unplug = qdev_simple_device_unplug_cb;
+    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+    dc->bus_type = TYPE_S390_TOPOLOGY_BOOK_BUS;
+    dc->realize = s390_socket_device_realize;
+    device_class_set_props(dc, s390_topology_socket_properties);
+    dc->desc = "topology socket";
+}
+
+static const TypeInfo socket_info = {
+    .name          = TYPE_S390_TOPOLOGY_SOCKET,
+    .parent        = TYPE_DEVICE,
+    .instance_size = sizeof(S390TopologySocket),
+    .class_init    = socket_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
+};
+
+/* --- BOOKS Definitions --- */
+static Property s390_topology_book_properties[] = {
+    DEFINE_PROP_UINT8("book_id", S390TopologyBook, book_id, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static char *book_bus_get_dev_path(DeviceState *dev)
+{
+    S390TopologyBook *book = S390_TOPOLOGY_BOOK(dev);
+    DeviceState *drawer = dev->parent_bus->parent;
+    char *id = qdev_get_dev_path(drawer);
+    char *ret;
+
+    if (id) {
+        ret = g_strdup_printf("%s:%02d", id, book->book_id);
+        g_free(id);
+    } else {
+        ret = g_malloc(6);
+        snprintf(ret, 6, "_:%02d", book->book_id);
+    }
+
+    return ret;
+}
+
+static void book_bus_class_init(ObjectClass *oc, void *data)
+{
+    BusClass *k = BUS_CLASS(oc);
+
+    k->get_dev_path = book_bus_get_dev_path;
+    k->max_dev = S390_MAX_BOOKS;
+}
+
+static const TypeInfo book_bus_info = {
+    .name = TYPE_S390_TOPOLOGY_BOOK_BUS,
+    .parent = TYPE_BUS,
+    .instance_size = 0,
+    .class_init = book_bus_class_init,
+};
+
+static void s390_book_device_realize(DeviceState *dev, Error **errp)
+{
+    S390TopologyBook *book = S390_TOPOLOGY_BOOK(dev);
+    BusState *bus;
+
+    bus = qbus_create(TYPE_S390_TOPOLOGY_BOOK_BUS, dev,
+                      TYPE_S390_TOPOLOGY_BOOK_BUS);
+    qbus_set_hotplug_handler(bus, OBJECT(dev));
+    book->bus = bus;
+}
+
+static void book_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
+
+    hc->unplug = qdev_simple_device_unplug_cb;
+    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+    dc->bus_type = TYPE_S390_TOPOLOGY_DRAWER_BUS;
+    dc->realize = s390_book_device_realize;
+    device_class_set_props(dc, s390_topology_book_properties);
+    dc->desc = "topology book";
+}
+
+static const TypeInfo book_info = {
+    .name          = TYPE_S390_TOPOLOGY_BOOK,
+    .parent        = TYPE_DEVICE,
+    .instance_size = sizeof(S390TopologyBook),
+    .class_init    = book_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
+};
+
+/* --- DRAWER Definitions --- */
+static Property s390_topology_drawer_properties[] = {
+    DEFINE_PROP_UINT8("drawer_id", S390TopologyDrawer, drawer_id, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static char *drawer_bus_get_dev_path(DeviceState *dev)
+{
+    S390TopologyDrawer *drawer = S390_TOPOLOGY_DRAWER(dev);
+    DeviceState *node = dev->parent_bus->parent;
+    char *id = qdev_get_dev_path(node);
+    char *ret;
+
+    if (id) {
+        ret = g_strdup_printf("%s:%02d", id, drawer->drawer_id);
+        g_free(id);
+    } else {
+        ret = g_malloc(6);
+        snprintf(ret, 6, "_:%02d", drawer->drawer_id);
+    }
+
+    return ret;
+}
+
+static void drawer_bus_class_init(ObjectClass *oc, void *data)
+{
+    BusClass *k = BUS_CLASS(oc);
+
+    k->get_dev_path = drawer_bus_get_dev_path;
+    k->max_dev = S390_MAX_DRAWERS;
+}
+
+static const TypeInfo drawer_bus_info = {
+    .name = TYPE_S390_TOPOLOGY_DRAWER_BUS,
+    .parent = TYPE_BUS,
+    .instance_size = 0,
+    .class_init = drawer_bus_class_init,
+};
+
+static void s390_drawer_device_realize(DeviceState *dev, Error **errp)
+{
+    S390TopologyDrawer *drawer = S390_TOPOLOGY_DRAWER(dev);
+    BusState *bus;
+
+    bus = qbus_create(TYPE_S390_TOPOLOGY_DRAWER_BUS, dev,
+                      TYPE_S390_TOPOLOGY_DRAWER_BUS);
+    qbus_set_hotplug_handler(bus, OBJECT(dev));
+    drawer->bus = bus;
+}
+
+static void drawer_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
+
+    hc->unplug = qdev_simple_device_unplug_cb;
+    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+    dc->bus_type = TYPE_S390_TOPOLOGY_NODE_BUS;
+    dc->realize = s390_drawer_device_realize;
+    device_class_set_props(dc, s390_topology_drawer_properties);
+    dc->desc = "topology drawer";
+}
+
+static const TypeInfo drawer_info = {
+    .name          = TYPE_S390_TOPOLOGY_DRAWER,
+    .parent        = TYPE_DEVICE,
+    .instance_size = sizeof(S390TopologyDrawer),
+    .class_init    = drawer_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
+};
+
+/* --- NODE Definitions --- */
+
+/*
+ * Nodes are the first level of CPU topology we support
+ * only one NODE for the moment.
+ */
+static char *node_bus_get_dev_path(DeviceState *dev)
+{
+    return g_strdup_printf("00");
+}
+
+static void node_bus_class_init(ObjectClass *oc, void *data)
+{
+    BusClass *k = BUS_CLASS(oc);
+
+    k->get_dev_path = node_bus_get_dev_path;
+    k->max_dev = S390_MAX_NODES;
+}
+
+static const TypeInfo node_bus_info = {
+    .name = TYPE_S390_TOPOLOGY_NODE_BUS,
+    .parent = TYPE_BUS,
+    .instance_size = 0,
+    .class_init = node_bus_class_init,
+};
+
+static void s390_node_device_realize(DeviceState *dev, Error **errp)
+{
+    S390TopologyNode *node = S390_TOPOLOGY_NODE(dev);
+    BusState *bus;
+
+    /* Create NODE bus on NODE bridge device */
+    bus = qbus_create(TYPE_S390_TOPOLOGY_NODE_BUS, dev,
+                      TYPE_S390_TOPOLOGY_NODE_BUS);
+    node->bus = bus;
+
+    /* Enable hotplugging */
+    qbus_set_hotplug_handler(bus, OBJECT(dev));
+}
+
+static void node_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
+
+    hc->unplug = qdev_simple_device_unplug_cb;
+    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+    dc->realize = s390_node_device_realize;
+    dc->desc = "topology node";
+}
+
+static const TypeInfo node_info = {
+    .name          = TYPE_S390_TOPOLOGY_NODE,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(S390TopologyNode),
+    .class_init    = node_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
+};
+
+static void topology_register(void)
+{
+    type_register_static(&cpu_cores_info);
+    type_register_static(&socket_bus_info);
+    type_register_static(&socket_info);
+    type_register_static(&book_bus_info);
+    type_register_static(&book_info);
+    type_register_static(&drawer_bus_info);
+    type_register_static(&drawer_info);
+    type_register_static(&node_bus_info);
+    type_register_static(&node_info);
+}
+
+type_init(topology_register);
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index 28484256ec..74678861cf 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -2,6 +2,7 @@ s390x_ss = ss.source_set()
 s390x_ss.add(files(
   'ap-bridge.c',
   'ap-device.c',
+  'cpu-topology.c',
   'ccw-device.c',
   'css-bridge.c',
   'css.c',
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 42d9be7333..3708ad3c46 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -42,6 +42,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/s390x/pv.h"
 #include "migration/blocker.h"
+#include "hw/s390x/cpu-topology.h"
 
 static Error *pv_mig_blocker;
 
@@ -88,6 +89,7 @@ static void s390_init_cpus(MachineState *machine)
     /* initialize possible_cpus */
     mc->possible_cpu_arch_ids(machine);
 
+    s390_topology_setup(machine);
     for (i = 0; i < machine->smp.cpus; i++) {
         s390x_new_cpu(machine->cpu_type, i, &error_fatal);
     }
@@ -305,6 +307,8 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev,
     g_assert(!ms->possible_cpus->cpus[cpu->env.core_id].cpu);
     ms->possible_cpus->cpus[cpu->env.core_id].cpu = OBJECT(dev);
 
+    s390_topology_new_cpu(cpu->env.core_id);
+
     if (dev->hotplugged) {
         raise_irq_cpu_hotplug();
     }
diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
new file mode 100644
index 0000000000..64424cb457
--- /dev/null
+++ b/include/hw/s390x/cpu-topology.h
@@ -0,0 +1,91 @@
+/*
+ * CPU Topology
+ *
+ * Copyright 2021 IBM Corp.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#ifndef HW_S390X_CPU_TOPOLOGY_H
+#define HW_S390X_CPU_TOPOLOGY_H
+
+#include "hw/qdev-core.h"
+#include "qom/object.h"
+
+#define S390_TOPOLOGY_CPU_TYPE    0x03
+
+#define S390_TOPOLOGY_POLARITY_H  0x00
+#define S390_TOPOLOGY_POLARITY_VL 0x01
+#define S390_TOPOLOGY_POLARITY_VM 0x02
+#define S390_TOPOLOGY_POLARITY_VH 0x03
+
+#define TYPE_S390_TOPOLOGY_CORES "topology cores"
+struct S390TopologyCores {
+    DeviceState parent_obj;
+    uint8_t id;
+    bool dedicated;
+    uint8_t polarity;
+    uint8_t cputype;
+    uint16_t origin;
+    uint64_t mask;
+    int cnt;
+};
+typedef struct S390TopologyCores S390TopologyCores;
+OBJECT_DECLARE_SIMPLE_TYPE(S390TopologyCores, S390_TOPOLOGY_CORES)
+
+#define TYPE_S390_TOPOLOGY_SOCKET "topology socket"
+#define TYPE_S390_TOPOLOGY_SOCKET_BUS "socket-bus"
+struct S390TopologySocket {
+    DeviceState parent_obj;
+    BusState *bus;
+    uint8_t socket_id;
+    int cnt;
+};
+typedef struct S390TopologySocket S390TopologySocket;
+OBJECT_DECLARE_SIMPLE_TYPE(S390TopologySocket, S390_TOPOLOGY_SOCKET)
+#define S390_MAX_SOCKETS 8
+
+#define TYPE_S390_TOPOLOGY_BOOK "topology book"
+#define TYPE_S390_TOPOLOGY_BOOK_BUS "book-bus"
+struct S390TopologyBook {
+    DeviceState parent_obj;
+    BusState *bus;
+    uint8_t book_id;
+    int cnt;
+};
+typedef struct S390TopologyBook S390TopologyBook;
+OBJECT_DECLARE_SIMPLE_TYPE(S390TopologyBook, S390_TOPOLOGY_BOOK)
+#define S390_MAX_BOOKS 8
+
+#define TYPE_S390_TOPOLOGY_DRAWER "topology drawer"
+#define TYPE_S390_TOPOLOGY_DRAWER_BUS "drawer-bus"
+struct S390TopologyDrawer {
+    DeviceState parent_obj;
+    BusState *bus;
+    uint8_t drawer_id;
+    int cnt;
+};
+typedef struct S390TopologyDrawer S390TopologyDrawer;
+OBJECT_DECLARE_SIMPLE_TYPE(S390TopologyDrawer, S390_TOPOLOGY_DRAWER)
+#define S390_MAX_DRAWERS 5
+
+#define TYPE_S390_TOPOLOGY_NODE "topology node"
+#define TYPE_S390_TOPOLOGY_NODE_BUS "node-bus"
+struct S390TopologyNode {
+    SysBusDevice parent_obj;
+    BusState *bus;
+    uint8_t node_id;
+    int cnt;
+};
+typedef struct S390TopologyNode S390TopologyNode;
+OBJECT_DECLARE_SIMPLE_TYPE(S390TopologyNode, S390_TOPOLOGY_NODE)
+#define S390_MAX_NODES 1
+
+S390TopologyNode *s390_init_topology(void);
+
+S390TopologyNode *s390_get_topology(void);
+void s390_topology_setup(MachineState *ms);
+void s390_topology_new_cpu(int core_id);
+
+#endif
-- 
2.25.1

[PATCH v1 4/9] s390x: Topology list entries and SYSIB 15.x.x

[Pierre Morel]

We define the CPU type Topology List Entry and the
Container type Topology List Entry to implement SYSIB 15.1.x

This patch will be squatched with the next patch.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 target/s390x/cpu.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index b26ae8fff2..d573ba205e 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -564,6 +564,50 @@ typedef union SysIB {
 } SysIB;
 QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
 
+/* CPU type Topology List Entry */
+typedef struct SysIBTl_cpu {
+        uint8_t nl;
+        uint8_t reserved0[3];
+        uint8_t reserved1:5;
+        uint8_t dedicated:1;
+        uint8_t polarity:2;
+        uint8_t type;
+        uint16_t origin;
+        uint64_t mask;
+} QEMU_PACKED SysIBTl_cpu;
+
+/* Container type Topology List Entry */
+typedef struct SysIBTl_container {
+        uint8_t nl;
+        uint8_t reserved[6];
+        uint8_t id;
+} QEMU_PACKED SysIBTl_container;
+
+/* Generic Topology List Entry */
+typedef union SysIBTl_entry {
+        uint8_t nl;
+        SysIBTl_container container;
+        SysIBTl_cpu cpu;
+} QEMU_PACKED SysIBTl_entry;
+
+#define TOPOLOGY_NR_MAG  6
+#define TOPOLOGY_NR_MAG6 0
+#define TOPOLOGY_NR_MAG5 1
+#define TOPOLOGY_NR_MAG4 2
+#define TOPOLOGY_NR_MAG3 3
+#define TOPOLOGY_NR_MAG2 4
+#define TOPOLOGY_NR_MAG1 5
+/* Configuration topology */
+typedef struct SysIB_151x {
+    uint8_t  res0[2];
+    uint16_t length;
+    uint8_t  mag[TOPOLOGY_NR_MAG];
+    uint8_t  res1;
+    uint8_t  mnest;
+    uint32_t res2;
+    SysIBTl_entry tle[0];
+} QEMU_PACKED SysIB_151x;
+
 /* MMU defines */
 #define ASCE_ORIGIN           (~0xfffULL) /* segment table origin             */
 #define ASCE_SUBSPACE         0x200       /* subspace group control           */
-- 
2.25.1

[PATCH v1 5/9] s390x: topology: implementating Store Topology System Information

[Pierre Morel]

The handling of STSI is enhenced with the interception of the
function code 15 for storing CPU topology.

Using the objects built during the pluging of CPU, we build the
SYSIB 15_1_x structures.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 target/s390x/kvm/kvm.c | 222 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 222 insertions(+)

diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 5b1fdb55c4..d78261c089 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -52,6 +52,7 @@
 #include "hw/s390x/s390-virtio-ccw.h"
 #include "hw/s390x/s390-virtio-hcall.h"
 #include "hw/s390x/pv.h"
+#include "hw/s390x/cpu-topology.h"
 
 #ifndef DEBUG_KVM
 #define DEBUG_KVM  0
@@ -1893,6 +1894,223 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64 addr, uint8_t ar)
     }
 }
 
+static int stsi_15_container(void *p, int nl, int id)
+{
+    SysIBTl_container *tle = (SysIBTl_container *)p;
+
+    tle->nl = nl;
+    tle->id = id;
+
+    return sizeof(*tle);
+}
+
+static int stsi_15_cpus(void *p, S390TopologyCores *cd)
+{
+    SysIBTl_cpu *tle = (SysIBTl_cpu *)p;
+
+    tle->nl = 0;
+    tle->dedicated = cd->dedicated;
+    tle->polarity = cd->polarity;
+    tle->type = cd->cputype;
+    tle->origin = cd->origin;
+    tle->mask = cd->mask;
+
+    return sizeof(*tle);
+}
+
+static int set_socket(const MachineState *ms, void *p,
+                      S390TopologySocket *socket, int level, int off)
+{
+    BusChild *kid;
+    int l, len = 0;
+
+    len += stsi_15_container(p, 1, off * ms->smp.sockets + socket->socket_id);
+    p += len;
+
+    QTAILQ_FOREACH_REVERSE(kid, &socket->bus->children, sibling) {
+        l = stsi_15_cpus(p, S390_TOPOLOGY_CORES(kid->child));
+        p += l;
+        len += l;
+    }
+    return len;
+}
+
+static int set_book(const MachineState *ms, void *p,
+                    S390TopologyBook *book, int level, int off)
+{
+    S390CcwMachineState *s390ms = S390_CCW_MACHINE(ms);
+    BusChild *kid;
+    int l, len = 0;
+    int offset = 0;
+
+    if (level >= 3) {
+        len += stsi_15_container(p, 2, off * s390ms->books + book->book_id);
+        p += len;
+    } else {
+        offset = off * s390ms->books + book->book_id;
+    }
+
+    QTAILQ_FOREACH_REVERSE(kid, &book->bus->children, sibling) {
+        l = set_socket(ms, p, S390_TOPOLOGY_SOCKET(kid->child), level,
+                       offset);
+        p += l;
+        len += l;
+    }
+
+    return len;
+}
+
+static int set_drawer(const MachineState *ms, void *p,
+                      S390TopologyDrawer *drawer, int level)
+{
+    BusChild *kid;
+    int l, len = 0;
+    int offset = 0;
+
+    if (level >= 4) {
+        len += stsi_15_container(p, 3, drawer->drawer_id);
+        p += len;
+    } else {
+        offset = drawer->drawer_id;
+    }
+
+    QTAILQ_FOREACH_REVERSE(kid, &drawer->bus->children, sibling) {
+        l = set_book(ms, p, S390_TOPOLOGY_BOOK(kid->child), level, offset);
+        p += l;
+        len += l;
+    }
+
+    return len;
+}
+
+static void insert_stsi_15_1_2(const MachineState *ms, void *p)
+{
+    S390CcwMachineState *s390ms = S390_CCW_MACHINE(ms);
+    S390TopologyDrawer *drawer;
+    S390TopologyNode *node;
+    SysIB_151x *sysib;
+    BusChild *kid;
+    int level = 2;
+    int len, l, nb_sockets;
+
+    sysib = (SysIB_151x *)p;
+    sysib->mnest = level;
+    nb_sockets = ms->smp.sockets * s390ms->books * s390ms->drawers;
+    sysib->mag[TOPOLOGY_NR_MAG2] = nb_sockets;
+    sysib->mag[TOPOLOGY_NR_MAG1] = ms->smp.cores;
+
+    node = s390_get_topology();
+    len = sizeof(SysIB_151x);
+    p += len;
+
+    QTAILQ_FOREACH_REVERSE(kid, &node->bus->children, sibling) {
+        drawer = S390_TOPOLOGY_DRAWER(kid->child);
+        l = set_drawer(ms, p, drawer, level);
+        p += l;
+        len += l;
+    }
+
+    sysib->length = len;
+}
+
+static void insert_stsi_15_1_3(const MachineState *ms, void *p)
+{
+    S390CcwMachineState *s390ms = S390_CCW_MACHINE(ms);
+    S390TopologyDrawer *drawer;
+    S390TopologyNode *node;
+    SysIB_151x *sysib;
+    BusChild *kid;
+    int level = 3;
+    int len, l;
+
+    sysib = (SysIB_151x *)p;
+    sysib->mnest = level;
+    sysib->mag[TOPOLOGY_NR_MAG3] = s390ms->books * s390ms->drawers;
+    sysib->mag[TOPOLOGY_NR_MAG2] = ms->smp.sockets;
+    sysib->mag[TOPOLOGY_NR_MAG1] = ms->smp.cores;
+
+    node = s390_get_topology();
+    len = sizeof(SysIB_151x);
+    p += len;
+
+    QTAILQ_FOREACH_REVERSE(kid, &node->bus->children, sibling) {
+        drawer = S390_TOPOLOGY_DRAWER(kid->child);
+        l = set_drawer(ms, p, drawer, level);
+        p += l;
+        len += l;
+    }
+
+    sysib->length = len;
+}
+
+static void insert_stsi_15_1_4(const MachineState *ms, void *p)
+{
+    S390CcwMachineState *s390ms = S390_CCW_MACHINE(ms);
+    S390TopologyDrawer *drawer;
+    S390TopologyNode *node;
+    SysIB_151x *sysib;
+    BusChild *kid;
+    int level = 4;
+    int len, l;
+
+    sysib = (SysIB_151x *)p;
+    sysib->mnest = level;
+    sysib->mag[TOPOLOGY_NR_MAG4] = s390ms->drawers;
+    sysib->mag[TOPOLOGY_NR_MAG3] = s390ms->books;
+    sysib->mag[TOPOLOGY_NR_MAG2] = ms->smp.sockets;
+    sysib->mag[TOPOLOGY_NR_MAG1] = ms->smp.cores;
+
+    node = s390_get_topology();
+    len = sizeof(SysIB_151x);
+    p += len;
+
+    QTAILQ_FOREACH_REVERSE(kid, &node->bus->children, sibling) {
+        drawer = S390_TOPOLOGY_DRAWER(kid->child);
+        l = set_drawer(ms, p, drawer, level);
+        p += l;
+        len += l;
+    }
+
+    sysib->length = len;
+}
+
+#define SCLP_READ_SCP_INFO_MNEST 2
+static void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
+{
+    const MachineState *machine = MACHINE(qdev_get_machine());
+    void *p;
+
+    if (sel2 > SCLP_READ_SCP_INFO_MNEST) {
+        setcc(cpu, 3);
+        return;
+    }
+
+    p = g_malloc0(4096);
+
+    switch (sel2) {
+    case 2:
+        insert_stsi_15_1_2(machine, p);
+        break;
+    case 3:
+        insert_stsi_15_1_3(machine, p);
+        break;
+    case 4:
+        insert_stsi_15_1_4(machine, p);
+        break;
+    default:
+        setcc(cpu, 3);
+        return;
+    }
+
+    if (s390_is_pv()) {
+        s390_cpu_pv_mem_write(cpu, 0, p, 4096);
+    } else {
+        s390_cpu_virt_mem_write(cpu, addr, ar, p, 4096);
+    }
+    setcc(cpu, 0);
+    g_free(p);
+}
+
 static int handle_stsi(S390CPU *cpu)
 {
     CPUState *cs = CPU(cpu);
@@ -1906,6 +2124,10 @@ static int handle_stsi(S390CPU *cpu)
         /* Only sysib 3.2.2 needs post-handling for now. */
         insert_stsi_3_2_2(cpu, run->s390_stsi.addr, run->s390_stsi.ar);
         return 0;
+    case 15:
+        insert_stsi_15_1_x(cpu, run->s390_stsi.sel2, run->s390_stsi.addr,
+                           run->s390_stsi.ar);
+        return 0;
     default:
         return 0;
     }
-- 
2.25.1

[PATCH v1 6/9] s390x: kvm: topology: interception of PTF instruction

[Pierre Morel]

Interception of the PTF instruction depending on the new
KVM_CAP_S390_CPU_TOPOLOGY KVM extension.

A global value is used to remember if a Topology change occured since
the last interception of a PTF instruction with function code 0.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/s390x/cpu-topology.c            | 19 +++++++++++
 include/hw/s390x/cpu-topology.h    |  8 +++++
 include/hw/s390x/s390-virtio-ccw.h |  1 +
 target/s390x/cpu.c                 |  4 +++
 target/s390x/cpu.h                 |  1 +
 target/s390x/kvm/kvm.c             | 52 ++++++++++++++++++++++++++++++
 6 files changed, 85 insertions(+)

diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index 1224137f56..5e1cac9529 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -19,12 +19,25 @@
 #include "qemu/typedefs.h"
 #include "hw/s390x/s390-virtio-ccw.h"
 
+int s390_topology_changed(void)
+{
+    const MachineState *ms = MACHINE(qdev_get_machine());
+    S390CcwMachineState *s390ms = S390_CCW_MACHINE(ms);
+
+    if (s390ms->topology_changed) {
+        s390ms->topology_changed = 0;
+        return 1;
+    }
+    return 0;
+}
+
 static S390TopologyCores *s390_create_cores(S390TopologySocket *socket,
                                             int origin)
 {
     DeviceState *dev;
     S390TopologyCores *cores;
     const MachineState *ms = MACHINE(qdev_get_machine());
+    S390CcwMachineState *s390ms = S390_CCW_MACHINE(ms);
 
     if (socket->bus->num_children >= ms->smp.cores) {
         return NULL;
@@ -36,6 +49,7 @@ static S390TopologyCores *s390_create_cores(S390TopologySocket *socket,
     cores = S390_TOPOLOGY_CORES(dev);
     cores->origin = origin;
     socket->cnt += 1;
+    s390ms->topology_changed = 1;
 
     return cores;
 }
@@ -45,6 +59,7 @@ static S390TopologySocket *s390_create_socket(S390TopologyBook *book, int id)
     DeviceState *dev;
     S390TopologySocket *socket;
     const MachineState *ms = MACHINE(qdev_get_machine());
+    S390CcwMachineState *s390ms = S390_CCW_MACHINE(ms);
 
     if (book->bus->num_children >= ms->smp.sockets) {
         return NULL;
@@ -56,6 +71,7 @@ static S390TopologySocket *s390_create_socket(S390TopologyBook *book, int id)
     socket = S390_TOPOLOGY_SOCKET(dev);
     socket->socket_id = id;
     book->cnt++;
+    s390ms->topology_changed = 1;
 
     return socket;
 }
@@ -77,6 +93,7 @@ static S390TopologyBook *s390_create_book(S390TopologyDrawer *drawer, int id)
     book = S390_TOPOLOGY_BOOK(dev);
     book->book_id = id;
     drawer->cnt++;
+    s390ms->topology_changed = 1;
 
     return book;
 }
@@ -98,6 +115,7 @@ static S390TopologyDrawer *s390_create_drawer(S390TopologyNode *node, int id)
     drawer = S390_TOPOLOGY_DRAWER(dev);
     drawer->drawer_id = id;
     node->cnt++;
+    s390ms->topology_changed = 1;
 
     return drawer;
 }
@@ -210,6 +228,7 @@ void s390_topology_new_cpu(int core_id)
     bit = 63 - (core_id - origin);
     set_bit(bit, &cores->mask);
     cores->origin = origin;
+    s390ms->topology_changed = 1;
 }
 
 /*
diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
index 64424cb457..549a3e9a19 100644
--- a/include/hw/s390x/cpu-topology.h
+++ b/include/hw/s390x/cpu-topology.h
@@ -12,6 +12,7 @@
 
 #include "hw/qdev-core.h"
 #include "qom/object.h"
+#include "include/hw/sysbus.h"
 
 #define S390_TOPOLOGY_CPU_TYPE    0x03
 
@@ -88,4 +89,11 @@ S390TopologyNode *s390_get_topology(void);
 void s390_topology_setup(MachineState *ms);
 void s390_topology_new_cpu(int core_id);
 
+#define S390_PTF_REASON_NONE (0x00 << 8)
+#define S390_PTF_REASON_DONE (0x01 << 8)
+#define S390_PTF_REASON_BUSY (0x02 << 8)
+extern int s390_topology_changed(void);
+
+#define S390_TOPO_FC_MASK 0xffUL
+
 #endif
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index fb3c3a50ce..a091468c79 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -30,6 +30,7 @@ struct S390CcwMachineState {
     uint8_t loadparm[8];
     int drawers;
     int books;
+    int topology_changed;
 };
 
 struct S390CcwMachineClass {
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 7b7b05f1d3..ac7b161190 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -35,6 +35,7 @@
 #include "fpu/softfloat-helpers.h"
 #include "disas/capstone.h"
 #include "sysemu/tcg.h"
+#include "hw/s390x/cpu-topology.h"
 
 #define CR0_RESET       0xE0UL
 #define CR14_RESET      0xC2000000UL;
@@ -154,6 +155,9 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
 
         env->pfault_token = -1UL;
         env->bpbc = false;
+#if !defined(CONFIG_USER_ONLY)
+        s390_topology_changed();
+#endif
         break;
     default:
         g_assert_not_reached();
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index d573ba205e..4eacd06c59 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -901,4 +901,5 @@ typedef S390CPU ArchCPU;
 
 #include "exec/cpu-all.h"
 
+int s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra);
 #endif
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index d78261c089..7c3594d793 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -98,6 +98,7 @@
 
 #define PRIV_B9_EQBS                    0x9c
 #define PRIV_B9_CLP                     0xa0
+#define PRIV_B9_PTF                     0xa2
 #define PRIV_B9_PCISTG                  0xd0
 #define PRIV_B9_PCILG                   0xd2
 #define PRIV_B9_RPCIT                   0xd3
@@ -1453,6 +1454,54 @@ static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
     }
 }
 
+int s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra)
+{
+    CPUS390XState *env = &cpu->env;
+    uint64_t reg = env->regs[r1];
+    uint8_t fc = reg & S390_TOPO_FC_MASK;
+
+    if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
+        s390_program_interrupt(env, PGM_OPERAND, ra);
+        return 0;
+    }
+
+    if (env->psw.mask & PSW_MASK_PSTATE) {
+        s390_program_interrupt(env, PGM_PRIVILEGED, ra);
+        return 0;
+    }
+
+    if (reg & ~S390_TOPO_FC_MASK) {
+        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+        return 0;
+    }
+
+    switch (fc) {
+    case 0:    /* Horizontal polarization is already set */
+        env->regs[r1] = S390_PTF_REASON_DONE;
+        return 2;
+    case 1:    /* Vertical polarization is not supported */
+        env->regs[r1] = S390_PTF_REASON_NONE;
+        return 2;
+    case 2:    /* Report if a topology change report is pending */
+        return s390_topology_changed();
+    default:
+        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+        break;
+    }
+
+    return 0;
+}
+
+static int kvm_handle_ptf(S390CPU *cpu, struct kvm_run *run)
+{
+    uint8_t r1 = (run->s390_sieic.ipb >> 20) & 0x0f;
+    uint8_t ret;
+
+    ret = s390_handle_ptf(cpu, r1, RA_IGNORED);
+    setcc(cpu, ret);
+    return 0;
+}
+
 static int handle_b9(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
 {
     int r = 0;
@@ -1470,6 +1519,9 @@ static int handle_b9(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
     case PRIV_B9_RPCIT:
         r = kvm_rpcit_service_call(cpu, run);
         break;
+    case PRIV_B9_PTF:
+        r = kvm_handle_ptf(cpu, run);
+        break;
     case PRIV_B9_EQBS:
         /* just inject exception */
         r = -1;
-- 
2.25.1

[PATCH v1 7/9] s390x: SCLP: reporting the maximum nested topology entries

[Pierre Morel]

The maximum nested topology entries is used by the guest to know
how many nested topology are available on the machine.

As we now implemented drawers and books above sockets and core
we can set the maximum nested topology reported by SCLP to 4.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/s390x/sclp.c         | 1 +
 include/hw/s390x/sclp.h | 4 +++-
 target/s390x/kvm/kvm.c  | 1 -
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index edb6e3ea01..f5fe067ffb 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -125,6 +125,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 
     /* CPU information */
     prepare_cpu_entries(machine, entries_start, &cpu_count);
+    read_info->stsi_parm = SCLP_READ_SCP_INFO_MNEST;
     read_info->entries_cpu = cpu_to_be16(cpu_count);
     read_info->offset_cpu = cpu_to_be16(offset_cpu);
     read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index d3ade40a5a..6ec1185f30 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -116,7 +116,9 @@ typedef struct ReadInfo {
     SCCBHeader h;
     uint16_t rnmax;
     uint8_t rnsize;
-    uint8_t  _reserved1[16 - 11];       /* 11-15 */
+    uint8_t  _reserved1[15 - 11];       /* 11-15 */
+#define SCLP_READ_SCP_INFO_MNEST 4
+    uint8_t  stsi_parm;
     uint16_t entries_cpu;               /* 16-17 */
     uint16_t offset_cpu;                /* 18-19 */
     uint8_t  _reserved2[24 - 20];       /* 20-23 */
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 7c3594d793..bc8b392e69 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -2126,7 +2126,6 @@ static void insert_stsi_15_1_4(const MachineState *ms, void *p)
     sysib->length = len;
 }
 
-#define SCLP_READ_SCP_INFO_MNEST 2
 static void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
 {
     const MachineState *machine = MACHINE(qdev_get_machine());
-- 
2.25.1

[PATCH v1 8/9] s390x: numa: define drawers and books for NUMA

[Pierre Morel]

S390 uses 5 levels of CPU topology, we implement the four lower levels:
drawers, books, sockets and cores.

Until now drawers and books were not defined, this patch add the
definition for drawers and books to the machine.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/core/machine.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 6f59fb0b7f..a193ffcd3b 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -692,6 +692,16 @@ void machine_set_cpu_numa_node(MachineState *machine,
             return;
         }
 
+        if (props->has_drawer_id && !slot->props.has_drawer_id) {
+            error_setg(errp, "drawer-id is not supported");
+            return;
+        }
+
+        if (props->has_book_id && !slot->props.has_book_id) {
+            error_setg(errp, "book-id is not supported");
+            return;
+        }
+
         /* skip slots with explicit mismatch */
         if (props->has_thread_id && props->thread_id != slot->props.thread_id) {
                 continue;
@@ -705,6 +715,14 @@ void machine_set_cpu_numa_node(MachineState *machine,
                 continue;
         }
 
+        if (props->has_drawer_id && props->drawer_id != slot->props.drawer_id) {
+                continue;
+        }
+
+        if (props->has_book_id && props->book_id != slot->props.book_id) {
+                continue;
+        }
+
         if (props->has_socket_id && props->socket_id != slot->props.socket_id) {
                 continue;
         }
-- 
2.25.1

[PATCH v1 9/9] s390x: numa: implement NUMA for S390x

[Pierre Morel]

We add the possibility to define the CPU topology to QEMU S390x.

This allows the user chose which CPU in the topology is active.

A NUMA node is considered to be a socket and chosing the NUMA node
leads to chose the specific socket in a book inside a drawer.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 53 +++++++++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 7 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 3708ad3c46..0fd938fe3f 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -84,14 +84,37 @@ out:
 static void s390_init_cpus(MachineState *machine)
 {
     MachineClass *mc = MACHINE_GET_CLASS(machine);
-    int i;
+    const CPUArchId *slot;
+    int i, n = 1;
 
     /* initialize possible_cpus */
     mc->possible_cpu_arch_ids(machine);
 
     s390_topology_setup(machine);
-    for (i = 0; i < machine->smp.cpus; i++) {
+
+    /* Create CPU0 */
+    s390x_new_cpu(machine->cpu_type, 0, &error_fatal);
+
+    /* For NUMA configuration create defined nodes */
+    if (machine->numa_state->num_nodes) {
+        for (i = 1; i < machine->smp.max_cpus; i++) {
+            slot = &machine->possible_cpus->cpus[i];
+            if (slot->props.node_id) {
+                s390x_new_cpu(machine->cpu_type, i, &error_fatal);
+                n++;
+            }
+        }
+    }
+
+    /* create all remaining CPUs */
+    for (i = 1; n < machine->smp.cpus && i < machine->smp.max_cpus; i++) {
+        slot = &machine->possible_cpus->cpus[i];
+        /* For NUMA configuration skip defined nodes */
+        if (machine->numa_state->num_nodes && slot->props.node_id) {
+            continue;
+        }
         s390x_new_cpu(machine->cpu_type, i, &error_fatal);
+        n++;
     }
 }
 
@@ -530,6 +553,7 @@ static const CPUArchIdList *s390_possible_cpu_arch_ids(MachineState *ms)
 {
     int i;
     unsigned int max_cpus = ms->smp.max_cpus;
+    S390CcwMachineState *s390ms = S390_CCW_MACHINE(ms);
 
     if (ms->possible_cpus) {
         g_assert(ms->possible_cpus && ms->possible_cpus->len == max_cpus);
@@ -540,11 +564,20 @@ static const CPUArchIdList *s390_possible_cpu_arch_ids(MachineState *ms)
                                   sizeof(CPUArchId) * max_cpus);
     ms->possible_cpus->len = max_cpus;
     for (i = 0; i < ms->possible_cpus->len; i++) {
-        ms->possible_cpus->cpus[i].type = ms->cpu_type;
-        ms->possible_cpus->cpus[i].vcpus_count = 1;
-        ms->possible_cpus->cpus[i].arch_id = i;
-        ms->possible_cpus->cpus[i].props.has_core_id = true;
-        ms->possible_cpus->cpus[i].props.core_id = i;
+        CPUArchId *slot = &ms->possible_cpus->cpus[i];
+
+        slot->type = ms->cpu_type;
+        slot->vcpus_count = 1;
+        slot->arch_id = i;
+
+        slot->props.core_id = i;
+        slot->props.has_core_id = true;
+        slot->props.socket_id = i / ms->smp.cores;
+        slot->props.has_socket_id = true;
+        slot->props.book_id = slot->props.socket_id / ms->smp.sockets;
+        slot->props.has_book_id = true;
+        slot->props.drawer_id = slot->props.book_id / s390ms->books;
+        slot->props.has_drawer_id = true;
     }
 
     return ms->possible_cpus;
@@ -635,6 +668,11 @@ static void s390_smp_parse(MachineState *ms, QemuOpts *opts)
     s390ms->books = books;
 }
 
+static int64_t s390x_get_default_cpu_node_id(const MachineState *ms, int idx)
+{
+    return idx / ms->smp.cores;
+}
+
 static void ccw_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -666,6 +704,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     nc->nmi_monitor_handler = s390_nmi;
     mc->default_ram_id = "s390.ram";
     mc->smp_parse = s390_smp_parse;
+    mc->get_default_cpu_node_id = s390x_get_default_cpu_node_id;
 }
 
 static inline bool machine_get_aes_key_wrap(Object *obj, Error **errp)
-- 
2.25.1

Re: [PATCH v1 2/9] s390x: toplogy: adding drawers and books to smp parsing

[Markus Armbruster]

Pierre Morel <pmorel@linux.ibm.com> writes:

> Drawers and Books are levels 4 and 3 of the S390 CPU
> topology.
> We allow the user to define these levels and we will
> store the values inside the S390CcwMachineState.

Double-checking: are these members specific to S390?

>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---

[...]

> diff --git a/qapi/machine.json b/qapi/machine.json
> index c3210ee1fb..98aff804c6 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -883,6 +883,8 @@
   ##
   # @CpuInstanceProperties:
   #
   # List of properties to be used for hotplugging a CPU instance,
   # it should be passed by management with device_add command when
   # a CPU is being hotplugged.
   #
   # @node-id: NUMA node ID the CPU belongs to
   # @socket-id: socket number within node/board the CPU belongs to

Missing: documentation for your new members.

   # @die-id: die number within node/board the CPU belongs to (Since 4.1)
   # @core-id: core number within die the CPU belongs to
   # @thread-id: thread number within core the CPU belongs to
   #
   # Note: currently there are 5 properties that could be present
   #       but management should be prepared to pass through other
   #       properties with device_add command to allow for future
   #       interface extension. This also requires the filed names to be kept in
   #       sync with the properties passed to -device/device_add.
   #
   # Since: 2.7
   ##
   { 'struct': 'CpuInstanceProperties',
>    'data': { '*node-id': 'int',
>              '*socket-id': 'int',
>              '*die-id': 'int',
> +            '*drawer-id': 'int',
> +            '*book-id': 'int',
>              '*core-id': 'int',
>              '*thread-id': 'int'
>    }

[...]

Re: [PATCH v1 2/9] s390x: toplogy: adding drawers and books to smp parsing

[Pierre Morel]

On 7/15/21 8:16 AM, Markus Armbruster wrote:
> Pierre Morel <pmorel@linux.ibm.com> writes:
> 
>> Drawers and Books are levels 4 and 3 of the S390 CPU
>> topology.
>> We allow the user to define these levels and we will
>> store the values inside the S390CcwMachineState.
> 
> Double-checking: are these members specific to S390?

Yes AFAIK

> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
> 
> [...]
> 
>> diff --git a/qapi/machine.json b/qapi/machine.json
>> index c3210ee1fb..98aff804c6 100644
>> --- a/qapi/machine.json
>> +++ b/qapi/machine.json
>> @@ -883,6 +883,8 @@
>     ##
>     # @CpuInstanceProperties:
>     #
>     # List of properties to be used for hotplugging a CPU instance,
>     # it should be passed by management with device_add command when
>     # a CPU is being hotplugged.
>     #
>     # @node-id: NUMA node ID the CPU belongs to
>     # @socket-id: socket number within node/board the CPU belongs to
> 
> Missing: documentation for your new members.

Oh yes, right forgot these, thanks.

> 
>     # @die-id: die number within node/board the CPU belongs to (Since 4.1)
>     # @core-id: core number within die the CPU belongs to
>     # @thread-id: thread number within core the CPU belongs to
>     #
>     # Note: currently there are 5 properties that could be present
>     #       but management should be prepared to pass through other
>     #       properties with device_add command to allow for future
>     #       interface extension. This also requires the filed names to be kept in
>     #       sync with the properties passed to -device/device_add.
>     #
>     # Since: 2.7
>     ##
>     { 'struct': 'CpuInstanceProperties',
>>     'data': { '*node-id': 'int',
>>               '*socket-id': 'int',
>>               '*die-id': 'int',
>> +            '*drawer-id': 'int',
>> +            '*book-id': 'int',
>>               '*core-id': 'int',
>>               '*thread-id': 'int'
>>     }
> 
> [...]
> 
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 2/9] s390x: toplogy: adding drawers and books to smp parsing

[Markus Armbruster]

Pierre Morel <pmorel@linux.ibm.com> writes:

> On 7/15/21 8:16 AM, Markus Armbruster wrote:
>> Pierre Morel <pmorel@linux.ibm.com> writes:
>> 
>>> Drawers and Books are levels 4 and 3 of the S390 CPU
>>> topology.
>>> We allow the user to define these levels and we will
>>> store the values inside the S390CcwMachineState.
>> 
>> Double-checking: are these members specific to S390?
>
> Yes AFAIK

Makes me wonder whether they should be conditional on TARGET_S390X.

What happens when you specify them for another target?  Silently
ignored, or error?

Re: [PATCH v1 1/9] s390x: smp: s390x dedicated smp parsing

[Cornelia Huck]

On Wed, Jul 14 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:

> We need a s390x dedicated SMP parsing to handle s390x specificities.
>
> In this patch we only handle threads, cores and sockets for
> s390x:
> - do not support threads, we always have 1 single thread per core
> - the sockets are filled one after the other with the cores
>
> Both these handlings are different from the standard smp_parse
> functionement and reflect the CPU topology in the simple case
> where all CPU belong to the same book.
>
> Topology levels above sockets, i.e. books, drawers, are not
> considered at this stage and will be introduced in a later patch.
>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 42 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index e4b18aef49..899d3a4137 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -582,6 +582,47 @@ static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
>      return newsz;
>  }
>  
> +/*
> + * In S390CCW machine we do not support threads for now,
> + * only sockets and cores.
> + */
> +static void s390_smp_parse(MachineState *ms, QemuOpts *opts)

It seems you based this on an older version of the code? The current
signature of this function since 1e63fe685804 ("machine: pass QAPI
struct to mc->smp_parse") is

void (*smp_parse)(MachineState *ms, SMPConfiguration *config, Error **errp);

That affects your parsing, and also lets you get rid of the ugly exit(1)
statements.

> +{
> +    unsigned cpus    = qemu_opt_get_number(opts, "cpus", 1);
> +    unsigned sockets = qemu_opt_get_number(opts, "sockets", 1);
> +    unsigned cores   = qemu_opt_get_number(opts, "cores", 1);
> +
> +    if (opts) {
> +        if (cpus == 0 || sockets == 0 || cores == 0) {

This behaviour looks different from what we do for other targets: if you
specify the value as 0, a value is calculated from the other values;
here, you error out. It's probably not a good idea to differ.

> +            error_report("cpu topology: "
> +                         "sockets (%u), cores (%u) or number of CPU(%u) "
> +                         "can not be zero", sockets, cores, cpus);
> +            exit(1);
> +        }
> +    }
> +

Re: [PATCH v1 2/9] s390x: toplogy: adding drawers and books to smp parsing

[Cornelia Huck]

On Thu, Jul 15 2021, Markus Armbruster <armbru@redhat.com> wrote:

> Pierre Morel <pmorel@linux.ibm.com> writes:
>
>> On 7/15/21 8:16 AM, Markus Armbruster wrote:
>>> Pierre Morel <pmorel@linux.ibm.com> writes:
>>> 
>>>> Drawers and Books are levels 4 and 3 of the S390 CPU
>>>> topology.
>>>> We allow the user to define these levels and we will
>>>> store the values inside the S390CcwMachineState.
>>> 
>>> Double-checking: are these members specific to S390?
>>
>> Yes AFAIK
>
> Makes me wonder whether they should be conditional on TARGET_S390X.
>
> What happens when you specify them for another target?  Silently
> ignored, or error?

I'm wondering whether we should include them in the base machine state
and treat them as we treat 'dies' (i.e. the standard parser errors out
if they are set, and only the s390x parser supports them.)

[I remember that we had a discussion ages ago about which parameters
should be included in the cpu topology, but I cannot recall the details :/]

Re: [PATCH v1 1/9] s390x: smp: s390x dedicated smp parsing

[Daniel P. Berrangé]

On Fri, Jul 16, 2021 at 10:54:08AM +0200, Cornelia Huck wrote:
> On Wed, Jul 14 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
> > We need a s390x dedicated SMP parsing to handle s390x specificities.
> >
> > In this patch we only handle threads, cores and sockets for
> > s390x:
> > - do not support threads, we always have 1 single thread per core
> > - the sockets are filled one after the other with the cores
> >
> > Both these handlings are different from the standard smp_parse
> > functionement and reflect the CPU topology in the simple case
> > where all CPU belong to the same book.
> >
> > Topology levels above sockets, i.e. books, drawers, are not
> > considered at this stage and will be introduced in a later patch.
> >
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >  hw/s390x/s390-virtio-ccw.c | 42 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >
> > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > index e4b18aef49..899d3a4137 100644
> > --- a/hw/s390x/s390-virtio-ccw.c
> > +++ b/hw/s390x/s390-virtio-ccw.c
> > @@ -582,6 +582,47 @@ static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
> >      return newsz;
> >  }
> >  
> > +/*
> > + * In S390CCW machine we do not support threads for now,
> > + * only sockets and cores.
> > + */
> > +static void s390_smp_parse(MachineState *ms, QemuOpts *opts)
> 
> It seems you based this on an older version of the code? The current
> signature of this function since 1e63fe685804 ("machine: pass QAPI
> struct to mc->smp_parse") is
> 
> void (*smp_parse)(MachineState *ms, SMPConfiguration *config, Error **errp);
> 
> That affects your parsing, and also lets you get rid of the ugly exit(1)
> statements.
> 
> > +{
> > +    unsigned cpus    = qemu_opt_get_number(opts, "cpus", 1);
> > +    unsigned sockets = qemu_opt_get_number(opts, "sockets", 1);
> > +    unsigned cores   = qemu_opt_get_number(opts, "cores", 1);
> > +
> > +    if (opts) {
> > +        if (cpus == 0 || sockets == 0 || cores == 0) {
> 
> This behaviour looks different from what we do for other targets: if you
> specify the value as 0, a value is calculated from the other values;
> here, you error out. It's probably not a good idea to differ.

I increasingly worry that we're making a mistake by going down the
route of having custom smp_parse implementations per target, as this
is showing signs of inconsistent behaviour and error reportings. I
think the differences / restrictions have granularity at a different
level that is being tested in many cases too.

Whether threads != 1 is valid will likely vary depending on what
CPU model is chosen, rather than what architecture is chosen.
The same is true for dies != 1. We're not really checking this
closely even in x86 - for example I can request nonsense such
as a 25 year old i486 CPU model with hyperthreading and multiple
dies

  qemu-system-x86_64 -cpu 486 -smp 16,cores=4,dies=2,threads=2

In this patch, there is no error reporting if the user specifies
dies != 1 or threads != 1 - it just silently ignores the request
which is not good.

Some machine types may have constraints on CPU sockets.

This can of course all be handled by custom smp_parse impls, but
this is ultimately going to lead to alot of duplicated and
inconsistent logic I fear.

I wonder if we would be better off having machine class callback
that can report topology constraints for the current configuration,
along lines of

     smp_constraints(MachineState *ms,
                     int *max_sockets,
                     int *max_dies,
                     int *max_cores,
                     int *max_threads)

And then have only a single smp_parse impl that takes into account
these constraints, to report errors / fill in missing fields / etc ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 2/9] s390x: toplogy: adding drawers and books to smp parsing

[Daniel P. Berrangé]

On Fri, Jul 16, 2021 at 11:10:04AM +0200, Cornelia Huck wrote:
> On Thu, Jul 15 2021, Markus Armbruster <armbru@redhat.com> wrote:
> 
> > Pierre Morel <pmorel@linux.ibm.com> writes:
> >
> >> On 7/15/21 8:16 AM, Markus Armbruster wrote:
> >>> Pierre Morel <pmorel@linux.ibm.com> writes:
> >>> 
> >>>> Drawers and Books are levels 4 and 3 of the S390 CPU
> >>>> topology.
> >>>> We allow the user to define these levels and we will
> >>>> store the values inside the S390CcwMachineState.
> >>> 
> >>> Double-checking: are these members specific to S390?
> >>
> >> Yes AFAIK
> >
> > Makes me wonder whether they should be conditional on TARGET_S390X.
> >
> > What happens when you specify them for another target?  Silently
> > ignored, or error?
> 
> I'm wondering whether we should include them in the base machine state
> and treat them as we treat 'dies' (i.e. the standard parser errors out
> if they are set, and only the s390x parser supports them.)

To repeat what i just wrote in my reply to patch 1, I think we ought to
think  about a different approach to handling the usage constraints,
which doesn't require full re-implementation of the smp_parse method
each time.  There should be a way for each target to report topology
constraints, such the the single smp_parse method can do the right
thing, especially wrt error reporting for unsupported values.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 6/9] s390x: kvm: topology: interception of PTF instruction

[Cornelia Huck]

On Wed, Jul 14 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:

> Interception of the PTF instruction depending on the new
> KVM_CAP_S390_CPU_TOPOLOGY KVM extension.

Wasn't that the capability that you dropped?

Is PTF supposed to be always intercepting? If that isn't configurable,
wouldn't older QEMUs generate exceptions for it? I'm a bit confused.

>
> A global value is used to remember if a Topology change occured since
> the last interception of a PTF instruction with function code 0.
>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  hw/s390x/cpu-topology.c            | 19 +++++++++++
>  include/hw/s390x/cpu-topology.h    |  8 +++++
>  include/hw/s390x/s390-virtio-ccw.h |  1 +
>  target/s390x/cpu.c                 |  4 +++
>  target/s390x/cpu.h                 |  1 +
>  target/s390x/kvm/kvm.c             | 52 ++++++++++++++++++++++++++++++
>  6 files changed, 85 insertions(+)

Re: [PATCH v1 2/9] s390x: toplogy: adding drawers and books to smp parsing

[Daniel P. Berrangé]

On Thu, Jul 15, 2021 at 08:16:32AM +0200, Markus Armbruster wrote:
> Pierre Morel <pmorel@linux.ibm.com> writes:
> 
> > Drawers and Books are levels 4 and 3 of the S390 CPU
> > topology.
> > We allow the user to define these levels and we will
> > store the values inside the S390CcwMachineState.
> 
> Double-checking: are these members specific to S390?
> 
> >
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> 
> [...]
> 
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index c3210ee1fb..98aff804c6 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -883,6 +883,8 @@
>    ##
>    # @CpuInstanceProperties:
>    #
>    # List of properties to be used for hotplugging a CPU instance,
>    # it should be passed by management with device_add command when
>    # a CPU is being hotplugged.
>    #
>    # @node-id: NUMA node ID the CPU belongs to
>    # @socket-id: socket number within node/board the CPU belongs to
> 
> Missing: documentation for your new members.

It is also missing in qemu-options.hx which covers -smp

To quote the lscpu manpage, it seems drawer/book fit inbetween
NUMA node and socket level:

       CPU    The logical CPU number of a CPU as used by the Linux kernel.

       CORE   The logical core number.  A core can contain several CPUs.

       SOCKET The logical socket number.  A socket can contain several cores.

       BOOK   The logical book number.  A book can contain several sockets.

       DRAWER The logical drawer number.  A drawer can contain several books.

       NODE   The logical NUMA node number.  A node can contain several drawers.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 7/9] s390x: SCLP: reporting the maximum nested topology entries

[Cornelia Huck]

On Wed, Jul 14 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:

> The maximum nested topology entries is used by the guest to know
> how many nested topology are available on the machine.
>
> As we now implemented drawers and books above sockets and core
> we can set the maximum nested topology reported by SCLP to 4.

Does that work with tcg as well? (Have not yet really looked at the
patches above.)

>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  hw/s390x/sclp.c         | 1 +
>  include/hw/s390x/sclp.h | 4 +++-
>  target/s390x/kvm/kvm.c  | 1 -
>  3 files changed, 4 insertions(+), 2 deletions(-)

Re: [PATCH v1 2/9] s390x: toplogy: adding drawers and books to smp parsing

[Cornelia Huck]

On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Fri, Jul 16, 2021 at 11:10:04AM +0200, Cornelia Huck wrote:
>> On Thu, Jul 15 2021, Markus Armbruster <armbru@redhat.com> wrote:
>> 
>> > Pierre Morel <pmorel@linux.ibm.com> writes:
>> >
>> >> On 7/15/21 8:16 AM, Markus Armbruster wrote:
>> >>> Pierre Morel <pmorel@linux.ibm.com> writes:
>> >>> 
>> >>>> Drawers and Books are levels 4 and 3 of the S390 CPU
>> >>>> topology.
>> >>>> We allow the user to define these levels and we will
>> >>>> store the values inside the S390CcwMachineState.
>> >>> 
>> >>> Double-checking: are these members specific to S390?
>> >>
>> >> Yes AFAIK
>> >
>> > Makes me wonder whether they should be conditional on TARGET_S390X.
>> >
>> > What happens when you specify them for another target?  Silently
>> > ignored, or error?
>> 
>> I'm wondering whether we should include them in the base machine state
>> and treat them as we treat 'dies' (i.e. the standard parser errors out
>> if they are set, and only the s390x parser supports them.)
>
> To repeat what i just wrote in my reply to patch 1, I think we ought to
> think  about a different approach to handling the usage constraints,
> which doesn't require full re-implementation of the smp_parse method
> each time.  There should be a way for each target to report topology
> constraints, such the the single smp_parse method can do the right
> thing, especially wrt error reporting for unsupported values.

That would mean that all possible fields would need to go into common
code, right?

I'm wondering whether there are more architecture/cpu specific values
lurking in the corner, it would get unwieldy if we need to go beyond the
existing fields and drawers/books.

Re: [PATCH v1 1/9] s390x: smp: s390x dedicated smp parsing

[Pierre Morel]

On 7/16/21 10:54 AM, Cornelia Huck wrote:
> On Wed, Jul 14 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> We need a s390x dedicated SMP parsing to handle s390x specificities.
>>
>> In this patch we only handle threads, cores and sockets for
>> s390x:
>> - do not support threads, we always have 1 single thread per core
>> - the sockets are filled one after the other with the cores
>>
>> Both these handlings are different from the standard smp_parse
>> functionement and reflect the CPU topology in the simple case
>> where all CPU belong to the same book.
>>
>> Topology levels above sockets, i.e. books, drawers, are not
>> considered at this stage and will be introduced in a later patch.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/s390-virtio-ccw.c | 42 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 42 insertions(+)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index e4b18aef49..899d3a4137 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -582,6 +582,47 @@ static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
>>       return newsz;
>>   }
>>   
>> +/*
>> + * In S390CCW machine we do not support threads for now,
>> + * only sockets and cores.
>> + */
>> +static void s390_smp_parse(MachineState *ms, QemuOpts *opts)
> 
> It seems you based this on an older version of the code? The current
> signature of this function since 1e63fe685804 ("machine: pass QAPI
> struct to mc->smp_parse") is
> 
> void (*smp_parse)(MachineState *ms, SMPConfiguration *config, Error **errp);
> 
> That affects your parsing, and also lets you get rid of the ugly exit(1)
> statements.

hum, yes, thanks

> 
>> +{
>> +    unsigned cpus    = qemu_opt_get_number(opts, "cpus", 1);
>> +    unsigned sockets = qemu_opt_get_number(opts, "sockets", 1);
>> +    unsigned cores   = qemu_opt_get_number(opts, "cores", 1);
>> +
>> +    if (opts) {
>> +        if (cpus == 0 || sockets == 0 || cores == 0) {
> 
> This behaviour looks different from what we do for other targets: if you
> specify the value as 0, a value is calculated from the other values;
> here, you error out. It's probably not a good idea to differ.

right, thanks

> 
>> +            error_report("cpu topology: "
>> +                         "sockets (%u), cores (%u) or number of CPU(%u) "
>> +                         "can not be zero", sockets, cores, cpus);
>> +            exit(1);
>> +        }
>> +    }
>> +
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 2/9] s390x: toplogy: adding drawers and books to smp parsing

[Daniel P. Berrangé]

On Fri, Jul 16, 2021 at 12:44:49PM +0200, Cornelia Huck wrote:
> On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> > On Fri, Jul 16, 2021 at 11:10:04AM +0200, Cornelia Huck wrote:
> >> On Thu, Jul 15 2021, Markus Armbruster <armbru@redhat.com> wrote:
> >> 
> >> > Pierre Morel <pmorel@linux.ibm.com> writes:
> >> >
> >> >> On 7/15/21 8:16 AM, Markus Armbruster wrote:
> >> >>> Pierre Morel <pmorel@linux.ibm.com> writes:
> >> >>> 
> >> >>>> Drawers and Books are levels 4 and 3 of the S390 CPU
> >> >>>> topology.
> >> >>>> We allow the user to define these levels and we will
> >> >>>> store the values inside the S390CcwMachineState.
> >> >>> 
> >> >>> Double-checking: are these members specific to S390?
> >> >>
> >> >> Yes AFAIK
> >> >
> >> > Makes me wonder whether they should be conditional on TARGET_S390X.
> >> >
> >> > What happens when you specify them for another target?  Silently
> >> > ignored, or error?
> >> 
> >> I'm wondering whether we should include them in the base machine state
> >> and treat them as we treat 'dies' (i.e. the standard parser errors out
> >> if they are set, and only the s390x parser supports them.)
> >
> > To repeat what i just wrote in my reply to patch 1, I think we ought to
> > think  about a different approach to handling the usage constraints,
> > which doesn't require full re-implementation of the smp_parse method
> > each time.  There should be a way for each target to report topology
> > constraints, such the the single smp_parse method can do the right
> > thing, especially wrt error reporting for unsupported values.
> 
> That would mean that all possible fields would need to go into common
> code, right?

Yes, that is an implication of what i'm suggesting.

> I'm wondering whether there are more architecture/cpu specific values
> lurking in the corner, it would get unwieldy if we need to go beyond the
> existing fields and drawers/books.

Is the book/drawer thing architecture specific, or is it machine
type / CPU specific. ie do /all/ the s390x machine types / CPUS
QEMU support the book/drawer concept, or only a subset.

If only a subset, then restricting it per target on QAPI doesn't
fully solve the root problem, and we instead are better focusing
on accurate runtime error reporting.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 1/9] s390x: smp: s390x dedicated smp parsing

[Pierre Morel]

On 7/16/21 11:14 AM, Daniel P. Berrangé wrote:
> On Fri, Jul 16, 2021 at 10:54:08AM +0200, Cornelia Huck wrote:
>> On Wed, Jul 14 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>> We need a s390x dedicated SMP parsing to handle s390x specificities.
>>>
>>> In this patch we only handle threads, cores and sockets for
>>> s390x:
>>> - do not support threads, we always have 1 single thread per core
>>> - the sockets are filled one after the other with the cores
>>>
>>> Both these handlings are different from the standard smp_parse
>>> functionement and reflect the CPU topology in the simple case
>>> where all CPU belong to the same book.
>>>
>>> Topology levels above sockets, i.e. books, drawers, are not
>>> considered at this stage and will be introduced in a later patch.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   hw/s390x/s390-virtio-ccw.c | 42 ++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 42 insertions(+)
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index e4b18aef49..899d3a4137 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -582,6 +582,47 @@ static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
>>>       return newsz;
>>>   }
>>>   
>>> +/*
>>> + * In S390CCW machine we do not support threads for now,
>>> + * only sockets and cores.
>>> + */
>>> +static void s390_smp_parse(MachineState *ms, QemuOpts *opts)
>>
>> It seems you based this on an older version of the code? The current
>> signature of this function since 1e63fe685804 ("machine: pass QAPI
>> struct to mc->smp_parse") is
>>
>> void (*smp_parse)(MachineState *ms, SMPConfiguration *config, Error **errp);
>>
>> That affects your parsing, and also lets you get rid of the ugly exit(1)
>> statements.
>>
>>> +{
>>> +    unsigned cpus    = qemu_opt_get_number(opts, "cpus", 1);
>>> +    unsigned sockets = qemu_opt_get_number(opts, "sockets", 1);
>>> +    unsigned cores   = qemu_opt_get_number(opts, "cores", 1);
>>> +
>>> +    if (opts) {
>>> +        if (cpus == 0 || sockets == 0 || cores == 0) {
>>
>> This behaviour looks different from what we do for other targets: if you
>> specify the value as 0, a value is calculated from the other values;
>> here, you error out. It's probably not a good idea to differ.
> 
> I increasingly worry that we're making a mistake by going down the
> route of having custom smp_parse implementations per target, as this
> is showing signs of inconsistent behaviour and error reportings. I
> think the differences / restrictions have granularity at a different
> level that is being tested in many cases too.
> 
> Whether threads != 1 is valid will likely vary depending on what
> CPU model is chosen, rather than what architecture is chosen.
> The same is true for dies != 1. We're not really checking this
> closely even in x86 - for example I can request nonsense such
> as a 25 year old i486 CPU model with hyperthreading and multiple
> dies
> 
>    qemu-system-x86_64 -cpu 486 -smp 16,cores=4,dies=2,threads=2
> 
> In this patch, there is no error reporting if the user specifies
> dies != 1 or threads != 1 - it just silently ignores the request
> which is not good.

yes, I should change this

> 
> Some machine types may have constraints on CPU sockets.
> 
> This can of course all be handled by custom smp_parse impls, but
> this is ultimately going to lead to alot of duplicated and
> inconsistent logic I fear.
> 
> I wonder if we would be better off having machine class callback
> that can report topology constraints for the current configuration,
> along lines of
> 
>       smp_constraints(MachineState *ms,
>                       int *max_sockets,
>                       int *max_dies,
>                       int *max_cores,
>                       int *max_threads)


I find the idee good, but what about making it really machine agnostic 
by removing names and using a generic

     smp_constraints(MachineState *ms,
             int *nb_levels,
             int *levels[]
             );

Level can be replaced by another name like container.
The machine could also provide the level/container names according to 
its internal documentation.

Regards,
Pierre


> 
> And then have only a single smp_parse impl that takes into account
> these constraints, to report errors / fill in missing fields / etc ?
> 
> Regards,
> Daniel
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 2/9] s390x: toplogy: adding drawers and books to smp parsing

[Pierre Morel]

On 7/16/21 11:23 AM, Daniel P. Berrangé wrote:
> On Thu, Jul 15, 2021 at 08:16:32AM +0200, Markus Armbruster wrote:
>> Pierre Morel <pmorel@linux.ibm.com> writes:
>>
>>> Drawers and Books are levels 4 and 3 of the S390 CPU
>>> topology.
>>> We allow the user to define these levels and we will
>>> store the values inside the S390CcwMachineState.
>>
>> Double-checking: are these members specific to S390?
>>
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>
>> [...]
>>
>>> diff --git a/qapi/machine.json b/qapi/machine.json
>>> index c3210ee1fb..98aff804c6 100644
>>> --- a/qapi/machine.json
>>> +++ b/qapi/machine.json
>>> @@ -883,6 +883,8 @@
>>     ##
>>     # @CpuInstanceProperties:
>>     #
>>     # List of properties to be used for hotplugging a CPU instance,
>>     # it should be passed by management with device_add command when
>>     # a CPU is being hotplugged.
>>     #
>>     # @node-id: NUMA node ID the CPU belongs to
>>     # @socket-id: socket number within node/board the CPU belongs to
>>
>> Missing: documentation for your new members.
> 
> It is also missing in qemu-options.hx which covers -smp
> 
> To quote the lscpu manpage, it seems drawer/book fit inbetween
> NUMA node and socket level:
> 
>         CPU    The logical CPU number of a CPU as used by the Linux kernel.
> 
>         CORE   The logical core number.  A core can contain several CPUs.
> 
>         SOCKET The logical socket number.  A socket can contain several cores.
> 
>         BOOK   The logical book number.  A book can contain several sockets.
> 
>         DRAWER The logical drawer number.  A drawer can contain several books.
> 
>         NODE   The logical NUMA node number.  A node can contain several drawers.
> 
> Regards,
> Daniel
> 

Yes, seems I missed a fundamental change.
Will adapt this too.

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 7/9] s390x: SCLP: reporting the maximum nested topology entries

[Pierre Morel]

On 7/16/21 11:24 AM, Cornelia Huck wrote:
> On Wed, Jul 14 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> The maximum nested topology entries is used by the guest to know
>> how many nested topology are available on the machine.
>>
>> As we now implemented drawers and books above sockets and core
>> we can set the maximum nested topology reported by SCLP to 4.
> 
> Does that work with tcg as well? (Have not yet really looked at the
> patches above.)

I must make more tests on this.

regards,
Pierre

> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/sclp.c         | 1 +
>>   include/hw/s390x/sclp.h | 4 +++-
>>   target/s390x/kvm/kvm.c  | 1 -
>>   3 files changed, 4 insertions(+), 2 deletions(-)
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 6/9] s390x: kvm: topology: interception of PTF instruction

[Pierre Morel]

On 7/16/21 11:22 AM, Cornelia Huck wrote:
> On Wed, Jul 14 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> Interception of the PTF instruction depending on the new
>> KVM_CAP_S390_CPU_TOPOLOGY KVM extension.
> 
> Wasn't that the capability that you dropped?

yes,

> 
> Is PTF supposed to be always intercepting? If that isn't configurable,
> wouldn't older QEMUs generate exceptions for it? I'm a bit confused.

Yes, PTF generated an OPERATION exception on old QEMU, but was not used 
by the guest if it has not the topology facility 11.

So just as for STSI, I think we need the KVM_CAP_S390_CPU_TOPOLOGY I 
dropped because otherwise, now that the kernel will advertise facility 
11, the guest will use it and it will get the exception that it should 
not get.

Regards,
Pierre

> 
>>
>> A global value is used to remember if a Topology change occured since
>> the last interception of a PTF instruction with function code 0.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/cpu-topology.c            | 19 +++++++++++
>>   include/hw/s390x/cpu-topology.h    |  8 +++++
>>   include/hw/s390x/s390-virtio-ccw.h |  1 +
>>   target/s390x/cpu.c                 |  4 +++
>>   target/s390x/cpu.h                 |  1 +
>>   target/s390x/kvm/kvm.c             | 52 ++++++++++++++++++++++++++++++
>>   6 files changed, 85 insertions(+)
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 6/9] s390x: kvm: topology: interception of PTF instruction

[Cornelia Huck]

On Fri, Jul 16 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 7/16/21 11:22 AM, Cornelia Huck wrote:
>> On Wed, Jul 14 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:
>> 
>>> Interception of the PTF instruction depending on the new
>>> KVM_CAP_S390_CPU_TOPOLOGY KVM extension.
>> 
>> Wasn't that the capability that you dropped?
>
> yes,
>
>> 
>> Is PTF supposed to be always intercepting? If that isn't configurable,
>> wouldn't older QEMUs generate exceptions for it? I'm a bit confused.
>
> Yes, PTF generated an OPERATION exception on old QEMU, but was not used 
> by the guest if it has not the topology facility 11.
>
> So just as for STSI, I think we need the KVM_CAP_S390_CPU_TOPOLOGY I 
> dropped because otherwise, now that the kernel will advertise facility 
> 11, the guest will use it and it will get the exception that it should 
> not get.

Ok, makes sense.

Re: [PATCH v1 1/9] s390x: smp: s390x dedicated smp parsing

[Cornelia Huck]

(restored cc:s)

On Fri, Jul 16 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 7/16/21 11:14 AM, Daniel P. Berrangé wrote:
>> I increasingly worry that we're making a mistake by going down the
>> route of having custom smp_parse implementations per target, as this
>> is showing signs of inconsistent behaviour and error reportings. I
>> think the differences / restrictions have granularity at a different
>> level that is being tested in many cases too.
>> 
>> Whether threads != 1 is valid will likely vary depending on what
>> CPU model is chosen, rather than what architecture is chosen.
>> The same is true for dies != 1. We're not really checking this
>> closely even in x86 - for example I can request nonsense such
>> as a 25 year old i486 CPU model with hyperthreading and multiple
>> dies
>> 
>>    qemu-system-x86_64 -cpu 486 -smp 16,cores=4,dies=2,threads=2

Now that's what I'd call an upgrade :)

>> 
>> In this patch, there is no error reporting if the user specifies
>> dies != 1 or threads != 1 - it just silently ignores the request
>> which is not good.
>
> yes, I should change this
>
>> 
>> Some machine types may have constraints on CPU sockets.
>> 
>> This can of course all be handled by custom smp_parse impls, but
>> this is ultimately going to lead to alot of duplicated and
>> inconsistent logic I fear.
>> 
>> I wonder if we would be better off having machine class callback
>> that can report topology constraints for the current configuration,
>> along lines ofsmp_constraints(MachineState *ms,
>> 
>>       smp_constraints(MachineState *ms,
>>                       int *max_sockets,
>>                       int *max_dies,
>>                       int *max_cores,
>>                       int *max_threads)
>
> I find the idee good, but what about making it really machine agnostic 
> by removing names and using a generic
>
> 	smp_constraints(MachineState *ms,
> 			int *nb_levels,
> 			int *levels[]
> 			);
>
> Level can be replaced by another name like container.
> The machine could also provide the level/container names according to 
> its internal documentation.

In theory, this could give us more flexibility; however, wouldn't
that still mean that the core needs to have some knowledge of the
individual levels? We also have the command line parsing to consider,
and that one uses concrete names (which may or may not make sense,
depending on what machine you are trying to configure), and we'd still
have to map these to 'levels'.

>
> Regards,
> Pierre
>
>
>
>> 
>> And then have only a single smp_parse impl that takes into account
>> these constraints, to report errors / fill in missing fields / etc ?
>> 
>> Regards,
>> Daniel
>> 
>
> -- 
> Pierre Morel
> IBM Lab Boeblingen

Re: [PATCH v1 2/9] s390x: toplogy: adding drawers and books to smp parsing

[Cornelia Huck]

On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Fri, Jul 16, 2021 at 12:44:49PM +0200, Cornelia Huck wrote:
>> On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote:
>> 
>> > On Fri, Jul 16, 2021 at 11:10:04AM +0200, Cornelia Huck wrote:
>> >> On Thu, Jul 15 2021, Markus Armbruster <armbru@redhat.com> wrote:
>> >> 
>> >> > Pierre Morel <pmorel@linux.ibm.com> writes:
>> >> >
>> >> >> On 7/15/21 8:16 AM, Markus Armbruster wrote:
>> >> >>> Pierre Morel <pmorel@linux.ibm.com> writes:
>> >> >>> 
>> >> >>>> Drawers and Books are levels 4 and 3 of the S390 CPU
>> >> >>>> topology.
>> >> >>>> We allow the user to define these levels and we will
>> >> >>>> store the values inside the S390CcwMachineState.
>> >> >>> 
>> >> >>> Double-checking: are these members specific to S390?
>> >> >>
>> >> >> Yes AFAIK
>> >> >
>> >> > Makes me wonder whether they should be conditional on TARGET_S390X.
>> >> >
>> >> > What happens when you specify them for another target?  Silently
>> >> > ignored, or error?
>> >> 
>> >> I'm wondering whether we should include them in the base machine state
>> >> and treat them as we treat 'dies' (i.e. the standard parser errors out
>> >> if they are set, and only the s390x parser supports them.)
>> >
>> > To repeat what i just wrote in my reply to patch 1, I think we ought to
>> > think  about a different approach to handling the usage constraints,
>> > which doesn't require full re-implementation of the smp_parse method
>> > each time.  There should be a way for each target to report topology
>> > constraints, such the the single smp_parse method can do the right
>> > thing, especially wrt error reporting for unsupported values.
>> 
>> That would mean that all possible fields would need to go into common
>> code, right?
>
> Yes, that is an implication of what i'm suggesting.
>
>> I'm wondering whether there are more architecture/cpu specific values
>> lurking in the corner, it would get unwieldy if we need to go beyond the
>> existing fields and drawers/books.
>
> Is the book/drawer thing architecture specific, or is it machine
> type / CPU specific. ie do /all/ the s390x machine types / CPUS
> QEMU support the book/drawer concept, or only a subset.

Should not be by machine type, but might be by cpu model (e.g. older
hardware lacking the needed support for exposing this to the guest.) IBM
folks, please correct me if I'm wrong.

> If only a subset, then restricting it per target on QAPI doesn't
> fully solve the root problem, and we instead are better focusing
> on accurate runtime error reporting.

Nod. Runtime error reporting should also be more flexible.

Re: [PATCH v1 1/9] s390x: smp: s390x dedicated smp parsing

[Daniel P. Berrangé]

On Mon, Jul 19, 2021 at 05:43:29PM +0200, Cornelia Huck wrote:
> (restored cc:s)
> 
> On Fri, Jul 16 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
> > On 7/16/21 11:14 AM, Daniel P. Berrangé wrote:
> >> I increasingly worry that we're making a mistake by going down the
> >> route of having custom smp_parse implementations per target, as this
> >> is showing signs of inconsistent behaviour and error reportings. I
> >> think the differences / restrictions have granularity at a different
> >> level that is being tested in many cases too.
> >> 
> >> Whether threads != 1 is valid will likely vary depending on what
> >> CPU model is chosen, rather than what architecture is chosen.
> >> The same is true for dies != 1. We're not really checking this
> >> closely even in x86 - for example I can request nonsense such
> >> as a 25 year old i486 CPU model with hyperthreading and multiple
> >> dies
> >> 
> >>    qemu-system-x86_64 -cpu 486 -smp 16,cores=4,dies=2,threads=2
> 
> Now that's what I'd call an upgrade :)
> 
> >> 
> >> In this patch, there is no error reporting if the user specifies
> >> dies != 1 or threads != 1 - it just silently ignores the request
> >> which is not good.
> >
> > yes, I should change this
> >
> >> 
> >> Some machine types may have constraints on CPU sockets.
> >> 
> >> This can of course all be handled by custom smp_parse impls, but
> >> this is ultimately going to lead to alot of duplicated and
> >> inconsistent logic I fear.
> >> 
> >> I wonder if we would be better off having machine class callback
> >> that can report topology constraints for the current configuration,
> >> along lines ofsmp_constraints(MachineState *ms,
> >> 
> >>       smp_constraints(MachineState *ms,
> >>                       int *max_sockets,
> >>                       int *max_dies,
> >>                       int *max_cores,
> >>                       int *max_threads)
> >
> > I find the idee good, but what about making it really machine agnostic 
> > by removing names and using a generic
> >
> > 	smp_constraints(MachineState *ms,
> > 			int *nb_levels,
> > 			int *levels[]
> > 			);
> >
> > Level can be replaced by another name like container.
> > The machine could also provide the level/container names according to 
> > its internal documentation.
> 
> In theory, this could give us more flexibility; however, wouldn't
> that still mean that the core needs to have some knowledge of the
> individual levels? We also have the command line parsing to consider,
> and that one uses concrete names (which may or may not make sense,
> depending on what machine you are trying to configure), and we'd still
> have to map these to 'levels'.

Yeah, we need to deal with names in several places, so I don't think
abstracting it in one place is desirable, as it introduces the need
to convert between the two and potentially obscures the semantics.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 1/9] s390x: smp: s390x dedicated smp parsing

[Pierre Morel]

On 7/19/21 5:52 PM, Daniel P. Berrangé wrote:
> On Mon, Jul 19, 2021 at 05:43:29PM +0200, Cornelia Huck wrote:
>> (restored cc:s)
>>
>> On Fri, Jul 16 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>> On 7/16/21 11:14 AM, Daniel P. Berrangé wrote:
>>>> I increasingly worry that we're making a mistake by going down the
>>>> route of having custom smp_parse implementations per target, as this
>>>> is showing signs of inconsistent behaviour and error reportings. I
>>>> think the differences / restrictions have granularity at a different
>>>> level that is being tested in many cases too.
>>>>
>>>> Whether threads != 1 is valid will likely vary depending on what
>>>> CPU model is chosen, rather than what architecture is chosen.
>>>> The same is true for dies != 1. We're not really checking this
>>>> closely even in x86 - for example I can request nonsense such
>>>> as a 25 year old i486 CPU model with hyperthreading and multiple
>>>> dies
>>>>
>>>>     qemu-system-x86_64 -cpu 486 -smp 16,cores=4,dies=2,threads=2
>>
>> Now that's what I'd call an upgrade :)
>>
>>>>
>>>> In this patch, there is no error reporting if the user specifies
>>>> dies != 1 or threads != 1 - it just silently ignores the request
>>>> which is not good.
>>>
>>> yes, I should change this
>>>
>>>>
>>>> Some machine types may have constraints on CPU sockets.
>>>>
>>>> This can of course all be handled by custom smp_parse impls, but
>>>> this is ultimately going to lead to alot of duplicated and
>>>> inconsistent logic I fear.
>>>>
>>>> I wonder if we would be better off having machine class callback
>>>> that can report topology constraints for the current configuration,
>>>> along lines ofsmp_constraints(MachineState *ms,
>>>>
>>>>        smp_constraints(MachineState *ms,
>>>>                        int *max_sockets,
>>>>                        int *max_dies,
>>>>                        int *max_cores,
>>>>                        int *max_threads)
>>>
>>> I find the idee good, but what about making it really machine agnostic
>>> by removing names and using a generic
>>>
>>> 	smp_constraints(MachineState *ms,
>>> 			int *nb_levels,
>>> 			int *levels[]
>>> 			);
>>>
>>> Level can be replaced by another name like container.
>>> The machine could also provide the level/container names according to
>>> its internal documentation.
>>
>> In theory, this could give us more flexibility; however, wouldn't
>> that still mean that the core needs to have some knowledge of the
>> individual levels? We also have the command line parsing to consider,
>> and that one uses concrete names (which may or may not make sense,
>> depending on what machine you are trying to configure), and we'd still
>> have to map these to 'levels'.
> 
> Yeah, we need to deal with names in several places, so I don't think
> abstracting it in one place is desirable, as it introduces the need
> to convert between the two and potentially obscures the semantics.
> 

Converting with names looks possible to me, every architecture can 
export a topology_name array or structure indicating names and other 
informations like the maximum possible count of entries at this level.

We have now the SMPConfiguration, couldn't we use it for this?

Regards,
Pierre

> 
> Regards,
> Daniel
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 2/9] s390x: toplogy: adding drawers and books to smp parsing

[Pierre Morel]

On 7/19/21 5:50 PM, Cornelia Huck wrote:
> On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
>> On Fri, Jul 16, 2021 at 12:44:49PM +0200, Cornelia Huck wrote:
>>> On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>
>>>> On Fri, Jul 16, 2021 at 11:10:04AM +0200, Cornelia Huck wrote:
>>>>> On Thu, Jul 15 2021, Markus Armbruster <armbru@redhat.com> wrote:
>>>>>
>>>>>> Pierre Morel <pmorel@linux.ibm.com> writes:
>>>>>>
>>>>>>> On 7/15/21 8:16 AM, Markus Armbruster wrote:
>>>>>>>> Pierre Morel <pmorel@linux.ibm.com> writes:
>>>>>>>>
>>>>>>>>> Drawers and Books are levels 4 and 3 of the S390 CPU
>>>>>>>>> topology.
>>>>>>>>> We allow the user to define these levels and we will
>>>>>>>>> store the values inside the S390CcwMachineState.
>>>>>>>>
>>>>>>>> Double-checking: are these members specific to S390?
>>>>>>>
>>>>>>> Yes AFAIK
>>>>>>
>>>>>> Makes me wonder whether they should be conditional on TARGET_S390X.
>>>>>>
>>>>>> What happens when you specify them for another target?  Silently
>>>>>> ignored, or error?
>>>>>
>>>>> I'm wondering whether we should include them in the base machine state
>>>>> and treat them as we treat 'dies' (i.e. the standard parser errors out
>>>>> if they are set, and only the s390x parser supports them.)
>>>>
>>>> To repeat what i just wrote in my reply to patch 1, I think we ought to
>>>> think  about a different approach to handling the usage constraints,
>>>> which doesn't require full re-implementation of the smp_parse method
>>>> each time.  There should be a way for each target to report topology
>>>> constraints, such the the single smp_parse method can do the right
>>>> thing, especially wrt error reporting for unsupported values.
>>>
>>> That would mean that all possible fields would need to go into common
>>> code, right?
>>
>> Yes, that is an implication of what i'm suggesting.
>>
>>> I'm wondering whether there are more architecture/cpu specific values
>>> lurking in the corner, it would get unwieldy if we need to go beyond the
>>> existing fields and drawers/books.
>>
>> Is the book/drawer thing architecture specific, or is it machine
>> type / CPU specific. ie do /all/ the s390x machine types / CPUS
>> QEMU support the book/drawer concept, or only a subset.
> 
> Should not be by machine type, but might be by cpu model (e.g. older
> hardware lacking the needed support for exposing this to the guest.) IBM
> folks, please correct me if I'm wrong.


Looks correct to me this is an information indicated by a facility 
introduced with Z10 if I do not make an error.

Regards,
Pierre

> 
>> If only a subset, then restricting it per target on QAPI doesn't
>> fully solve the root problem, and we instead are better focusing
>> on accurate runtime error reporting.
> 
> Nod. Runtime error reporting should also be more flexible.
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 2/9] s390x: toplogy: adding drawers and books to smp parsing

[Cornelia Huck]

On Tue, Jul 20 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 7/19/21 5:50 PM, Cornelia Huck wrote:
>> On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>> Is the book/drawer thing architecture specific, or is it machine
>>> type / CPU specific. ie do /all/ the s390x machine types / CPUS
>>> QEMU support the book/drawer concept, or only a subset.
>> 
>> Should not be by machine type, but might be by cpu model (e.g. older
>> hardware lacking the needed support for exposing this to the guest.) IBM
>> folks, please correct me if I'm wrong.
>
>
> Looks correct to me this is an information indicated by a facility 
> introduced with Z10 if I do not make an error.

Hm. Would that become a problem if we made availability of parameters
dependent upon a value in the machine (see the other thread I cc:ed you
on?)

Re: [PATCH v1 1/9] s390x: smp: s390x dedicated smp parsing

[Pierre Morel]

On 7/20/21 9:37 AM, Pierre Morel wrote:
> 
> 
> On 7/19/21 5:52 PM, Daniel P. Berrangé wrote:
>> On Mon, Jul 19, 2021 at 05:43:29PM +0200, Cornelia Huck wrote:
>>> (restored cc:s)
>>>
>>> On Fri, Jul 16 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>
>>>> On 7/16/21 11:14 AM, Daniel P. Berrangé wrote:
>>>>> I increasingly worry that we're making a mistake by going down the
>>>>> route of having custom smp_parse implementations per target, as this
>>>>> is showing signs of inconsistent behaviour and error reportings. I
>>>>> think the differences / restrictions have granularity at a different
>>>>> level that is being tested in many cases too.
>>>>>
>>>>> Whether threads != 1 is valid will likely vary depending on what
>>>>> CPU model is chosen, rather than what architecture is chosen.
>>>>> The same is true for dies != 1. We're not really checking this
>>>>> closely even in x86 - for example I can request nonsense such
>>>>> as a 25 year old i486 CPU model with hyperthreading and multiple
>>>>> dies
>>>>>
>>>>>     qemu-system-x86_64 -cpu 486 -smp 16,cores=4,dies=2,threads=2
>>>
>>> Now that's what I'd call an upgrade :)
>>>
>>>>>
>>>>> In this patch, there is no error reporting if the user specifies
>>>>> dies != 1 or threads != 1 - it just silently ignores the request
>>>>> which is not good.
>>>>
>>>> yes, I should change this
>>>>
>>>>>
>>>>> Some machine types may have constraints on CPU sockets.
>>>>>
>>>>> This can of course all be handled by custom smp_parse impls, but
>>>>> this is ultimately going to lead to alot of duplicated and
>>>>> inconsistent logic I fear.
>>>>>
>>>>> I wonder if we would be better off having machine class callback
>>>>> that can report topology constraints for the current configuration,
>>>>> along lines ofsmp_constraints(MachineState *ms,
>>>>>
>>>>>        smp_constraints(MachineState *ms,
>>>>>                        int *max_sockets,
>>>>>                        int *max_dies,
>>>>>                        int *max_cores,
>>>>>                        int *max_threads)
>>>>
>>>> I find the idee good, but what about making it really machine agnostic
>>>> by removing names and using a generic
>>>>
>>>>     smp_constraints(MachineState *ms,
>>>>             int *nb_levels,
>>>>             int *levels[]
>>>>             );
>>>>
>>>> Level can be replaced by another name like container.
>>>> The machine could also provide the level/container names according to
>>>> its internal documentation.
>>>
>>> In theory, this could give us more flexibility; however, wouldn't
>>> that still mean that the core needs to have some knowledge of the
>>> individual levels? We also have the command line parsing to consider,
>>> and that one uses concrete names (which may or may not make sense,
>>> depending on what machine you are trying to configure), and we'd still
>>> have to map these to 'levels'.
>>
>> Yeah, we need to deal with names in several places, so I don't think
>> abstracting it in one place is desirable, as it introduces the need
>> to convert between the two and potentially obscures the semantics.
>>
> 
> Converting with names looks possible to me, every architecture can 
> export a topology_name array or structure indicating names and other 
> informations like the maximum possible count of entries at this level.
> 
> We have now the SMPConfiguration, couldn't we use it for this?

Hum, I think I over estimated my understanding of what json is capable of.
Sorry, forget it.


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 2/9] s390x: toplogy: adding drawers and books to smp parsing

[Pierre Morel]

On 7/20/21 10:20 AM, Cornelia Huck wrote:
> On Tue, Jul 20 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 7/19/21 5:50 PM, Cornelia Huck wrote:
>>> On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>> Is the book/drawer thing architecture specific, or is it machine
>>>> type / CPU specific. ie do /all/ the s390x machine types / CPUS
>>>> QEMU support the book/drawer concept, or only a subset.
>>>
>>> Should not be by machine type, but might be by cpu model (e.g. older
>>> hardware lacking the needed support for exposing this to the guest.) IBM
>>> folks, please correct me if I'm wrong.
>>
>>
>> Looks correct to me this is an information indicated by a facility
>> introduced with Z10 if I do not make an error.
> 
> Hm. Would that become a problem if we made availability of parameters
> dependent upon a value in the machine (see the other thread I cc:ed you
> on?)
> 

Why?
The parameter can always be there, it is just that with older cpu model 
we will not report the topology information to the guest.

The discussion on dies and on smp_dies_supported in this thread will be 
interesting to follow because in my opinion dies for X or books/drawers 
for Z are to be treated equally.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 2/9] s390x: toplogy: adding drawers and books to smp parsing

[Cornelia Huck]

On Tue, Jul 20 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 7/20/21 10:20 AM, Cornelia Huck wrote:
>> On Tue, Jul 20 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:
>> 
>>> On 7/19/21 5:50 PM, Cornelia Huck wrote:
>>>> On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>>> Is the book/drawer thing architecture specific, or is it machine
>>>>> type / CPU specific. ie do /all/ the s390x machine types / CPUS
>>>>> QEMU support the book/drawer concept, or only a subset.
>>>>
>>>> Should not be by machine type, but might be by cpu model (e.g. older
>>>> hardware lacking the needed support for exposing this to the guest.) IBM
>>>> folks, please correct me if I'm wrong.
>>>
>>>
>>> Looks correct to me this is an information indicated by a facility
>>> introduced with Z10 if I do not make an error.
>> 
>> Hm. Would that become a problem if we made availability of parameters
>> dependent upon a value in the machine (see the other thread I cc:ed you
>> on?)
>> 
>
> Why?
> The parameter can always be there, it is just that with older cpu model 
> we will not report the topology information to the guest.

If we are fine with a configuration like that, sure.

>
> The discussion on dies and on smp_dies_supported in this thread will be 
> interesting to follow because in my opinion dies for X or books/drawers 
> for Z are to be treated equally.

Agreed. There was also talk of "clusters" in that thread, which I assume
are yet anothor machine specific parameter.

Re: [PATCH v1 2/9] s390x: toplogy: adding drawers and books to smp parsing

[Daniel P. Berrangé]

On Tue, Jul 20, 2021 at 10:46:31AM +0200, Pierre Morel wrote:
> 
> 
> On 7/20/21 10:20 AM, Cornelia Huck wrote:
> > On Tue, Jul 20 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:
> > 
> > > On 7/19/21 5:50 PM, Cornelia Huck wrote:
> > > > On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > > > Is the book/drawer thing architecture specific, or is it machine
> > > > > type / CPU specific. ie do /all/ the s390x machine types / CPUS
> > > > > QEMU support the book/drawer concept, or only a subset.
> > > > 
> > > > Should not be by machine type, but might be by cpu model (e.g. older
> > > > hardware lacking the needed support for exposing this to the guest.) IBM
> > > > folks, please correct me if I'm wrong.
> > > 
> > > 
> > > Looks correct to me this is an information indicated by a facility
> > > introduced with Z10 if I do not make an error.
> > 
> > Hm. Would that become a problem if we made availability of parameters
> > dependent upon a value in the machine (see the other thread I cc:ed you
> > on?)
> > 
> 
> Why?
> The parameter can always be there, it is just that with older cpu model we
> will not report the topology information to the guest.

I mostly see this as an error reporting problem, perhaps with an optional
capabilitty reporting facility.

eg if someone requests 'z9'  or older together with books > 1 or
drawers > 1, it would be ideal if QEMU reports an error message,
rather than silently ignoring it and not reporting this info to
the guest.

Taking it further, it some app queries CPU models  via QMP, it
could be desirable if the CPU model data returned indicates whether
drawers/books > 1 are conceptually available.  Likewise for dies
on x86, since I cna't imagine guest OS doing sensible things
with an i486 CPU and dies=2

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 2/9] s390x: toplogy: adding drawers and books to smp parsing

[Pierre Morel]

On 7/20/21 11:19 AM, Daniel P. Berrangé wrote:
> On Tue, Jul 20, 2021 at 10:46:31AM +0200, Pierre Morel wrote:
>>
>>
>> On 7/20/21 10:20 AM, Cornelia Huck wrote:
>>> On Tue, Jul 20 2021, Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>
>>>> On 7/19/21 5:50 PM, Cornelia Huck wrote:
>>>>> On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>>>> Is the book/drawer thing architecture specific, or is it machine
>>>>>> type / CPU specific. ie do /all/ the s390x machine types / CPUS
>>>>>> QEMU support the book/drawer concept, or only a subset.
>>>>>
>>>>> Should not be by machine type, but might be by cpu model (e.g. older
>>>>> hardware lacking the needed support for exposing this to the guest.) IBM
>>>>> folks, please correct me if I'm wrong.
>>>>
>>>>
>>>> Looks correct to me this is an information indicated by a facility
>>>> introduced with Z10 if I do not make an error.
>>>
>>> Hm. Would that become a problem if we made availability of parameters
>>> dependent upon a value in the machine (see the other thread I cc:ed you
>>> on?)
>>>
>>
>> Why?
>> The parameter can always be there, it is just that with older cpu model we
>> will not report the topology information to the guest.
> 
> I mostly see this as an error reporting problem, perhaps with an optional
> capabilitty reporting facility.
> 
> eg if someone requests 'z9'  or older together with books > 1 or
> drawers > 1, it would be ideal if QEMU reports an error message,
> rather than silently ignoring it and not reporting this info to
> the guest.
> 
> Taking it further, it some app queries CPU models  via QMP, it
> could be desirable if the CPU model data returned indicates whether
> drawers/books > 1 are conceptually available.  Likewise for dies
> on x86, since I cna't imagine guest OS doing sensible things
> with an i486 CPU and dies=2
> 
> Regards,
> Daniel
> 

Thanks,
I will rework this based on what is done on x86 and dies.

regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen