[PATCH v9 00/10] s390x: CPU Topology

[PATCH v9 00/10] s390x: CPU Topology

[Pierre Morel]

Hi,

The implementation of the CPU Topology in QEMU has been drastically
modified since the last patch series and the number of LOCs has been
greatly reduced.

Unnecessary objects have been removed, only a single S390Topology object
is created to support migration and reset.

Also a documentation has been added to the series.


To use these patches, you will need Linux V6-rc1 or newer.

Mainline patches needed are:

f5ecfee94493 2022-07-20 KVM: s390: resetting the Topology-Change-Report    
24fe0195bc19 2022-07-20 KVM: s390: guest support for topology function     
0130337ec45b 2022-07-20 KVM: s390: Cleanup ipte lock access and SIIF fac.. 

Currently this code is for KVM only, I have no idea if it is interesting
to provide a TCG patch. If ever it will be done in another series.

To have a better understanding of the S390x CPU Topology and its
implementation in QEMU you can have a look at the documentation in the
last patch.

New in this series
==================

  s390x/cpus: Make absence of multithreading clear

This patch makes clear that CPU-multithreading is not supported in
the guest.

  s390x/cpu topology: core_id sets s390x CPU topology

This patch uses the core_id to build the container topology
and the placement of the CPU inside the container.

  s390x/cpu topology: reporting the CPU topology to the guest

This patch is based on the fact that the CPU type for guests
is always IFL, CPUs are always dedicated and the polarity is
always horizontal.
This may change in the future.

  hw/core: introducing drawer and books for s390x
  s390x/cpu: reporting drawers and books topology to the guest

These two patches extend the topology handling to add two
new containers levels above sockets: books and drawers.

The subject of the last patches is clear enough (I hope).

Regards,
Pierre

Pierre Morel (10):
  s390x/cpus: Make absence of multithreading clear
  s390x/cpu topology: core_id sets s390x CPU topology
  s390x/cpu topology: reporting the CPU topology to the guest
  hw/core: introducing drawer and books for s390x
  s390x/cpu: reporting drawers and books topology to the guest
  s390x/cpu_topology: resetting the Topology-Change-Report
  s390x/cpu_topology: CPU topology migration
  target/s390x: interception of PTF instruction
  s390x/cpu_topology: activating CPU topology
  docs/s390x: document s390x cpu topology

 docs/system/s390x/cpu_topology.rst |  88 +++++++++
 hw/core/machine-smp.c              |  48 ++++-
 hw/core/machine.c                  |   9 +
 hw/s390x/cpu-topology.c            | 293 +++++++++++++++++++++++++++++
 hw/s390x/meson.build               |   1 +
 hw/s390x/s390-virtio-ccw.c         |  61 +++++-
 include/hw/boards.h                |  11 ++
 include/hw/s390x/cpu-topology.h    |  53 ++++++
 include/hw/s390x/s390-virtio-ccw.h |   7 +
 qapi/machine.json                  |  14 +-
 qemu-options.hx                    |   6 +-
 softmmu/vl.c                       |   6 +
 target/s390x/cpu-sysemu.c          |  15 ++
 target/s390x/cpu.h                 |  51 +++++
 target/s390x/cpu_topology.c        | 150 +++++++++++++++
 target/s390x/kvm/kvm.c             |  56 +++++-
 target/s390x/kvm/kvm_s390x.h       |   1 +
 target/s390x/meson.build           |   1 +
 18 files changed, 858 insertions(+), 13 deletions(-)
 create mode 100644 docs/system/s390x/cpu_topology.rst
 create mode 100644 hw/s390x/cpu-topology.c
 create mode 100644 include/hw/s390x/cpu-topology.h
 create mode 100644 target/s390x/cpu_topology.c

-- 
2.31.1

Changelog:

- since v8

- Linux patches are now mainline

- simplification of the implementation
  (Janis)

- Migration, new machine definition
  (Thomas)

- Documentation

- since v7

- Coherence with the Linux patch series changes for MTCR get
  (Pierre)

- check return values during new CPU creation
  (Thomas)

- Improving codding style and argument usages
  (Thomas)

- since v6

- Changes on smp args in qemu-options
  (Daniel)
  
- changed comments in machine.jason
  (Daniel)
 
- Added reset
  (Janosch)

- since v5

- rebasing on newer QEMU version

- reworked most lines above 80 characters.

- since v4

- Added drawer and books to topology

- Added numa topology

- Added documentation

- since v3

- Added migration
  (Thomas)

- Separated STSI instruction from KVM to prepare TCG
  (Thomas)

- Take care of endianess to prepare TCG
  (Thomas)

- Added comments on STSI CPU container and PFT instruction
  (Thomas)

- Moved enabling the instructions as the last patch
  (Thomas)

[PATCH v9 01/10] s390x/cpus: Make absence of multithreading clear

[Pierre Morel]

S390x do not support multithreading in the guest.
Do not let admin falsely specify multithreading on QEMU
smp commandline.

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

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 70229b102b..b5ca154e2f 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine)
     MachineClass *mc = MACHINE_GET_CLASS(machine);
     int i;
 
+    /* Explicitely do not support threads */
+    assert(machine->smp.threads == 1);
+
     /* initialize possible_cpus */
     mc->possible_cpu_arch_ids(machine);
 
-- 
2.31.1

[PATCH v9 02/10] s390x/cpu topology: core_id sets s390x CPU topology

[Pierre Morel]

In the S390x CPU topology the core_id specifies the CPU address
and the position of the core withing the topology.

Let's build the topology based on the core_id.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/s390x/cpu-topology.c         | 135 ++++++++++++++++++++++++++++++++
 hw/s390x/meson.build            |   1 +
 hw/s390x/s390-virtio-ccw.c      |  10 +++
 include/hw/s390x/cpu-topology.h |  42 ++++++++++
 4 files changed, 188 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..a6ca006ec5
--- /dev/null
+++ b/hw/s390x/cpu-topology.c
@@ -0,0 +1,135 @@
+/*
+ * CPU Topology
+ *
+ * Copyright IBM Corp. 2022
+ * 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/qdev-properties.h"
+#include "hw/boards.h"
+#include "qemu/typedefs.h"
+#include "target/s390x/cpu.h"
+#include "hw/s390x/s390-virtio-ccw.h"
+#include "hw/s390x/cpu-topology.h"
+
+S390Topology *s390_get_topology(void)
+{
+    static S390Topology *s390Topology;
+
+    if (!s390Topology) {
+        s390Topology = S390_CPU_TOPOLOGY(
+            object_resolve_path(TYPE_S390_CPU_TOPOLOGY, NULL));
+    }
+
+    return s390Topology;
+}
+
+/*
+ * s390_topology_new_cpu:
+ * @core_id: the core ID is machine wide
+ *
+ * The topology returned by s390_get_topology(), gives us the CPU
+ * topology established by the -smp QEMU aruments.
+ * The core-id gives:
+ *  - the Container TLE (Topology List Entry) containing the CPU TLE.
+ *  - in the CPU TLE the origin, or offset of the first bit in the core mask
+ *  - the bit in the CPU TLE core mask
+ */
+void s390_topology_new_cpu(int core_id)
+{
+    S390Topology *topo = s390_get_topology();
+    int socket_id;
+    int bit, origin;
+
+    /* In the case no Topology is used nothing is to be done here */
+    if (!topo) {
+        return;
+    }
+
+    socket_id = core_id / topo->cores;
+
+    bit = core_id;
+    origin = bit / 64;
+    bit %= 64;
+    bit = 63 - bit;
+
+    /*
+     * At the core level, each CPU is represented by a bit in a 64bit
+     * unsigned long. Set on plug and clear on unplug of a CPU.
+     * The firmware assume that all CPU in a CPU TLE have the same
+     * type, polarization and are all dedicated or shared.
+     * In the case a socket contains CPU with different type, polarization
+     * or entitlement then they will be defined in different CPU containers.
+     * Currently we assume all CPU are identical IFL CPUs and that they are
+     * all dedicated CPUs.
+     * The only reason to have several S390TopologyCores inside a socket is
+     * to have more than 64 CPUs.
+     * In that case the origin field, representing the offset of the first CPU
+     * in the CPU container allows to represent up to the maximal number of
+     * CPU inside several CPU containers inside the socket container.
+     */
+    topo->socket[socket_id].active_count++;
+    topo->tle[socket_id].active_count++;
+    set_bit(bit, &topo->tle[socket_id].mask[origin]);
+}
+
+/**
+ * s390_topology_realize:
+ * @dev: the device state
+ * @errp: the error pointer (not used)
+ *
+ * During realize the machine CPU topology is initialized with the
+ * QEMU -smp parameters.
+ * The maximum count of CPU TLE in the all Topology can not be greater
+ * than the maximum CPUs.
+ */
+static void s390_topology_realize(DeviceState *dev, Error **errp)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+    S390Topology *topo = S390_CPU_TOPOLOGY(dev);
+    int n;
+
+    topo->sockets = ms->smp.sockets;
+    topo->cores = ms->smp.cores;
+    topo->tles = ms->smp.max_cpus;
+
+    n = topo->sockets;
+    topo->socket = g_malloc0(n * sizeof(S390TopoContainer));
+    topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE));
+}
+
+/**
+ * topology_class_init:
+ * @oc: Object class
+ * @data: (not used)
+ *
+ * A very simple object we will need for reset and migration.
+ */
+static void topology_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = s390_topology_realize;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+}
+
+static const TypeInfo cpu_topology_info = {
+    .name          = TYPE_S390_CPU_TOPOLOGY,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(S390Topology),
+    .class_init    = topology_class_init,
+};
+
+static void topology_register(void)
+{
+    type_register_static(&cpu_topology_info);
+}
+type_init(topology_register);
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index de28a90a57..96d7d7d231 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 b5ca154e2f..15cefd104b 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -43,6 +43,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;
 
@@ -247,6 +248,12 @@ static void ccw_init(MachineState *machine)
     /* init memory + setup max page size. Required for the CPU model */
     s390_memory_init(machine->ram);
 
+    /* Adding the topology must be done before CPU intialization*/
+    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
+    object_property_add_child(qdev_get_machine(), TYPE_S390_CPU_TOPOLOGY,
+                              OBJECT(dev));
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+
     /* init CPUs (incl. CPU model) early so s390_has_feature() works */
     s390_init_cpus(machine);
 
@@ -309,6 +316,9 @@ 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);
 
+    /* Inserting the CPU in the Topology can not fail */
+    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..6911f975f4
--- /dev/null
+++ b/include/hw/s390x/cpu-topology.h
@@ -0,0 +1,42 @@
+/*
+ * CPU Topology
+ *
+ * Copyright 2022 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
+
+typedef struct S390TopoContainer {
+    int active_count;
+} S390TopoContainer;
+
+#define S390_TOPOLOGY_MAX_ORIGIN (1 + S390_MAX_CPUS / 64)
+typedef struct S390TopoTLE {
+    int active_count;
+    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
+} S390TopoTLE;
+
+#include "hw/qdev-core.h"
+#include "qom/object.h"
+
+struct S390Topology {
+    SysBusDevice parent_obj;
+    int sockets;
+    int cores;
+    int tles;
+    S390TopoContainer *socket;
+    S390TopoTLE *tle;
+};
+typedef struct S390Topology S390Topology;
+
+#define TYPE_S390_CPU_TOPOLOGY "s390-topology"
+OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY)
+
+S390Topology *s390_get_topology(void);
+void s390_topology_new_cpu(int core_id);
+
+#endif
-- 
2.31.1

[PATCH v9 03/10] s390x/cpu topology: reporting the CPU topology to the guest

[Pierre Morel]

The guest can use the STSI instruction to get a buffer filled
with the CPU topology description.

Let us implement the STSI instruction for the basis CPU topology
level, level 2.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/s390x/cpu-topology.c         |   4 ++
 include/hw/s390x/cpu-topology.h |   5 ++
 target/s390x/cpu.h              |  49 +++++++++++++++
 target/s390x/cpu_topology.c     | 108 ++++++++++++++++++++++++++++++++
 target/s390x/kvm/kvm.c          |   6 +-
 target/s390x/meson.build        |   1 +
 6 files changed, 172 insertions(+), 1 deletion(-)
 create mode 100644 target/s390x/cpu_topology.c

diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index a6ca006ec5..e2fd5c7e44 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -76,9 +76,11 @@ void s390_topology_new_cpu(int core_id)
      * in the CPU container allows to represent up to the maximal number of
      * CPU inside several CPU containers inside the socket container.
      */
+    qemu_mutex_lock(&topo->topo_mutex);
     topo->socket[socket_id].active_count++;
     topo->tle[socket_id].active_count++;
     set_bit(bit, &topo->tle[socket_id].mask[origin]);
+    qemu_mutex_unlock(&topo->topo_mutex);
 }
 
 /**
@@ -104,6 +106,8 @@ static void s390_topology_realize(DeviceState *dev, Error **errp)
     n = topo->sockets;
     topo->socket = g_malloc0(n * sizeof(S390TopoContainer));
     topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE));
+
+    qemu_mutex_init(&topo->topo_mutex);
 }
 
 /**
diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
index 6911f975f4..0b7f3d10b2 100644
--- a/include/hw/s390x/cpu-topology.h
+++ b/include/hw/s390x/cpu-topology.h
@@ -10,6 +10,10 @@
 #ifndef HW_S390X_CPU_TOPOLOGY_H
 #define HW_S390X_CPU_TOPOLOGY_H
 
+#define S390_TOPOLOGY_CPU_TYPE    0x03
+
+#define S390_TOPOLOGY_POLARITY_H  0x00
+
 typedef struct S390TopoContainer {
     int active_count;
 } S390TopoContainer;
@@ -30,6 +34,7 @@ struct S390Topology {
     int tles;
     S390TopoContainer *socket;
     S390TopoTLE *tle;
+    QemuMutex topo_mutex;
 };
 typedef struct S390Topology S390Topology;
 
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7d6d01325b..c61fe9b563 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -565,6 +565,53 @@ 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;
+} SysIBTl_cpu;
+QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16);
+
+/* Container type Topology List Entry */
+typedef struct SysIBTl_container {
+        uint8_t nl;
+        uint8_t reserved[6];
+        uint8_t id;
+} QEMU_PACKED SysIBTl_container;
+QEMU_BUILD_BUG_ON(sizeof(SysIBTl_container) != 8);
+
+/* Generic Topology List Entry */
+typedef union SysIBTl_entry {
+        uint8_t nl;
+        SysIBTl_container container;
+        SysIBTl_cpu cpu;
+} 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  reserved0[2];
+    uint16_t length;
+    uint8_t  mag[TOPOLOGY_NR_MAG];
+    uint8_t  reserved1;
+    uint8_t  mnest;
+    uint32_t reserved2;
+    SysIBTl_entry tle[0];
+} SysIB_151x;
+QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
+
 /* MMU defines */
 #define ASCE_ORIGIN           (~0xfffULL) /* segment table origin             */
 #define ASCE_SUBSPACE         0x200       /* subspace group control           */
@@ -843,4 +890,6 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
 
 #include "exec/cpu-all.h"
 
+void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar);
+
 #endif
diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
new file mode 100644
index 0000000000..56865dafc6
--- /dev/null
+++ b/target/s390x/cpu_topology.c
@@ -0,0 +1,108 @@
+/*
+ * QEMU S390x CPU Topology
+ *
+ * Copyright IBM Corp. 2022
+ * 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 "cpu.h"
+#include "hw/s390x/pv.h"
+#include "hw/sysbus.h"
+#include "hw/s390x/cpu-topology.h"
+#include "hw/s390x/sclp.h"
+
+static char *fill_container(char *p, int level, int id)
+{
+    SysIBTl_container *tle = (SysIBTl_container *)p;
+
+    tle->nl = level;
+    tle->id = id;
+    return p + sizeof(*tle);
+}
+
+static char *fill_tle_cpu(char *p, uint64_t mask, int origin)
+{
+    SysIBTl_cpu *tle = (SysIBTl_cpu *)p;
+
+    tle->nl = 0;
+    tle->dedicated = 1;
+    tle->polarity = S390_TOPOLOGY_POLARITY_H;
+    tle->type = S390_TOPOLOGY_CPU_TYPE;
+    tle->origin = origin * 64;
+    tle->mask = be64_to_cpu(mask);
+    return p + sizeof(*tle);
+}
+
+static char *s390_top_set_level2(S390Topology *topo, char *p)
+{
+    int i, origin;
+
+    for (i = 0; i < topo->sockets; i++) {
+        if (!topo->socket[i].active_count) {
+            continue;
+        }
+        p = fill_container(p, 1, i);
+        for (origin = 0; origin < S390_TOPOLOGY_MAX_ORIGIN; origin++) {
+            uint64_t mask = 0L;
+
+            mask = be64_to_cpu(topo->tle[i].mask[origin]);
+            if (mask) {
+                p = fill_tle_cpu(p, mask, origin);
+            }
+        }
+    }
+    return p;
+}
+
+static int setup_stsi(SysIB_151x *sysib, int level)
+{
+    S390Topology *topo = s390_get_topology();
+    char *p = (char *)sysib->tle;
+
+    qemu_mutex_lock(&topo->topo_mutex);
+
+    sysib->mnest = level;
+    switch (level) {
+    case 2:
+        sysib->mag[TOPOLOGY_NR_MAG2] = topo->sockets;
+        sysib->mag[TOPOLOGY_NR_MAG1] = topo->cores;
+        p = s390_top_set_level2(topo, p);
+        break;
+    }
+
+    qemu_mutex_unlock(&topo->topo_mutex);
+
+    return p - (char *)sysib->tle;
+}
+
+#define S390_TOPOLOGY_MAX_MNEST 2
+void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
+{
+    SysIB_151x *sysib;
+    int len = sizeof(*sysib);
+
+    if (s390_is_pv() || sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
+        setcc(cpu, 3);
+        return;
+    }
+
+    sysib = g_malloc0(TARGET_PAGE_SIZE);
+
+    len += setup_stsi(sysib, sel2);
+    if (len > TARGET_PAGE_SIZE) {
+        setcc(cpu, 3);
+        goto out_free;
+    }
+
+    sysib->length = be16_to_cpu(len);
+    s390_cpu_virt_mem_write(cpu, addr, ar, sysib, len);
+    setcc(cpu, 0);
+
+out_free:
+    g_free(sysib);
+}
+
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 6a8dbadf7e..f96630440b 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -51,6 +51,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
@@ -1917,9 +1918,12 @@ static int handle_stsi(S390CPU *cpu)
         if (run->s390_stsi.sel1 != 2 || run->s390_stsi.sel2 != 2) {
             return 0;
         }
-        /* 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;
     }
diff --git a/target/s390x/meson.build b/target/s390x/meson.build
index 84c1402a6a..890ccfa789 100644
--- a/target/s390x/meson.build
+++ b/target/s390x/meson.build
@@ -29,6 +29,7 @@ s390x_softmmu_ss.add(files(
   'sigp.c',
   'cpu-sysemu.c',
   'cpu_models_sysemu.c',
+  'cpu_topology.c',
 ))
 
 s390x_user_ss = ss.source_set()
-- 
2.31.1

[PATCH v9 04/10] hw/core: introducing drawer and books for s390x

[Pierre Morel]

S390x defines two topology levels above sockets: nbooks and drawers.

Let's add these two levels inside the CPU topology implementation.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/core/machine-smp.c | 48 ++++++++++++++++++++++++++++++++++++-------
 hw/core/machine.c     |  4 ++++
 include/hw/boards.h   |  8 ++++++++
 qapi/machine.json     | 14 +++++++++++--
 qemu-options.hx       |  6 ++++--
 softmmu/vl.c          |  6 ++++++
 6 files changed, 75 insertions(+), 11 deletions(-)

diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index b39ed21e65..26150c748f 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -31,6 +31,14 @@ static char *cpu_hierarchy_to_string(MachineState *ms)
     MachineClass *mc = MACHINE_GET_CLASS(ms);
     GString *s = g_string_new(NULL);
 
+    if (mc->smp_props.drawers_supported) {
+        g_string_append_printf(s, " * drawers (%u)", ms->smp.drawers);
+    }
+
+    if (mc->smp_props.books_supported) {
+        g_string_append_printf(s, " * books (%u)", ms->smp.books);
+    }
+
     g_string_append_printf(s, "sockets (%u)", ms->smp.sockets);
 
     if (mc->smp_props.dies_supported) {
@@ -73,6 +81,8 @@ void machine_parse_smp_config(MachineState *ms,
 {
     MachineClass *mc = MACHINE_GET_CLASS(ms);
     unsigned cpus    = config->has_cpus ? config->cpus : 0;
+    unsigned drawers = config->has_drawers ? config->drawers : 0;
+    unsigned books   = config->has_books ? config->books : 0;
     unsigned sockets = config->has_sockets ? config->sockets : 0;
     unsigned dies    = config->has_dies ? config->dies : 0;
     unsigned clusters = config->has_clusters ? config->clusters : 0;
@@ -85,6 +95,8 @@ void machine_parse_smp_config(MachineState *ms,
      * explicit configuration like "cpus=0" is not allowed.
      */
     if ((config->has_cpus && config->cpus == 0) ||
+        (config->has_drawers && config->drawers == 0) ||
+        (config->has_books && config->books == 0) ||
         (config->has_sockets && config->sockets == 0) ||
         (config->has_dies && config->dies == 0) ||
         (config->has_clusters && config->clusters == 0) ||
@@ -111,6 +123,20 @@ void machine_parse_smp_config(MachineState *ms,
     dies = dies > 0 ? dies : 1;
     clusters = clusters > 0 ? clusters : 1;
 
+    if (!mc->smp_props.books_supported && books > 1) {
+        error_setg(errp, "books not supported by this machine's CPU topology");
+        return;
+    }
+
+    books = books > 0 ? books : 1;
+
+    if (!mc->smp_props.drawers_supported && drawers > 1) {
+        error_setg(errp, "drawers not supported by this machine's CPU topology");
+        return;
+    }
+
+    drawers = drawers > 0 ? drawers : 1;
+
     /* compute missing values based on the provided ones */
     if (cpus == 0 && maxcpus == 0) {
         sockets = sockets > 0 ? sockets : 1;
@@ -124,33 +150,41 @@ void machine_parse_smp_config(MachineState *ms,
             if (sockets == 0) {
                 cores = cores > 0 ? cores : 1;
                 threads = threads > 0 ? threads : 1;
-                sockets = maxcpus / (dies * clusters * cores * threads);
+                sockets = maxcpus /
+                          (drawers * books * dies * clusters * cores * threads);
             } else if (cores == 0) {
                 threads = threads > 0 ? threads : 1;
-                cores = maxcpus / (sockets * dies * clusters * threads);
+                cores = maxcpus /
+                        (drawers * books * sockets * dies * clusters * threads);
             }
         } else {
             /* prefer cores over sockets since 6.2 */
             if (cores == 0) {
                 sockets = sockets > 0 ? sockets : 1;
                 threads = threads > 0 ? threads : 1;
-                cores = maxcpus / (sockets * dies * clusters * threads);
+                cores = maxcpus /
+                        (drawers * books * sockets * dies * clusters * threads);
             } else if (sockets == 0) {
                 threads = threads > 0 ? threads : 1;
-                sockets = maxcpus / (dies * clusters * cores * threads);
+                sockets = maxcpus /
+                         (drawers * books * dies * clusters * cores * threads);
             }
         }
 
         /* try to calculate omitted threads at last */
         if (threads == 0) {
-            threads = maxcpus / (sockets * dies * clusters * cores);
+            threads = maxcpus /
+                      (drawers * books * sockets * dies * clusters * cores);
         }
     }
 
-    maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * clusters * cores * threads;
+    maxcpus = maxcpus > 0 ? maxcpus : drawers * books * sockets * dies *
+                                      clusters * cores * threads;
     cpus = cpus > 0 ? cpus : maxcpus;
 
     ms->smp.cpus = cpus;
+    ms->smp.drawers = drawers;
+    ms->smp.books = books;
     ms->smp.sockets = sockets;
     ms->smp.dies = dies;
     ms->smp.clusters = clusters;
@@ -159,7 +193,7 @@ void machine_parse_smp_config(MachineState *ms,
     ms->smp.max_cpus = maxcpus;
 
     /* sanity-check of the computed topology */
-    if (sockets * dies * clusters * cores * threads != maxcpus) {
+    if (drawers * books * sockets * dies * clusters * cores * threads != maxcpus) {
         g_autofree char *topo_msg = cpu_hierarchy_to_string(ms);
         error_setg(errp, "Invalid CPU topology: "
                    "product of the hierarchy must match maxcpus: "
diff --git a/hw/core/machine.c b/hw/core/machine.c
index a673302cce..4c5c8d1655 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -821,6 +821,8 @@ static void machine_get_smp(Object *obj, Visitor *v, const char *name,
     MachineState *ms = MACHINE(obj);
     SMPConfiguration *config = &(SMPConfiguration){
         .has_cpus = true, .cpus = ms->smp.cpus,
+        .has_drawers = true, .drawers = ms->smp.drawers,
+        .has_books = true, .books = ms->smp.books,
         .has_sockets = true, .sockets = ms->smp.sockets,
         .has_dies = true, .dies = ms->smp.dies,
         .has_clusters = true, .clusters = ms->smp.clusters,
@@ -1087,6 +1089,8 @@ static void machine_initfn(Object *obj)
     /* default to mc->default_cpus */
     ms->smp.cpus = mc->default_cpus;
     ms->smp.max_cpus = mc->default_cpus;
+    ms->smp.drawers = 1;
+    ms->smp.books = 1;
     ms->smp.sockets = 1;
     ms->smp.dies = 1;
     ms->smp.clusters = 1;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 7b416c9787..69e20c1252 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -130,11 +130,15 @@ typedef struct {
  * @prefer_sockets - whether sockets are preferred over cores in smp parsing
  * @dies_supported - whether dies are supported by the machine
  * @clusters_supported - whether clusters are supported by the machine
+ * @books_supported - whether books are supported by the machine
+ * @drawers_supported - whether drawers are supported by the machine
  */
 typedef struct {
     bool prefer_sockets;
     bool dies_supported;
     bool clusters_supported;
+    bool books_supported;
+    bool drawers_supported;
 } SMPCompatProps;
 
 /**
@@ -299,6 +303,8 @@ typedef struct DeviceMemoryState {
 /**
  * CpuTopology:
  * @cpus: the number of present logical processors on the machine
+ * @drawers: the number of drawers on the machine
+ * @books: the number of books on the machine
  * @sockets: the number of sockets on the machine
  * @dies: the number of dies in one socket
  * @clusters: the number of clusters in one die
@@ -308,6 +314,8 @@ typedef struct DeviceMemoryState {
  */
 typedef struct CpuTopology {
     unsigned int cpus;
+    unsigned int drawers;
+    unsigned int books;
     unsigned int sockets;
     unsigned int dies;
     unsigned int clusters;
diff --git a/qapi/machine.json b/qapi/machine.json
index 6afd1936b0..bdd92e3cb1 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -900,13 +900,15 @@
 # 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
+# @drawer-id: drawer number within node/board the CPU belongs to
+# @book-id: book number within drawer/node/board the CPU belongs to
+# @socket-id: socket number within book/node/board the CPU belongs to
 # @die-id: die number within socket the CPU belongs to (since 4.1)
 # @cluster-id: cluster number within die the CPU belongs to (since 7.1)
 # @core-id: core number within cluster the CPU belongs to
 # @thread-id: thread number within core the CPU belongs to
 #
-# Note: currently there are 6 properties that could be present
+# Note: currently there are 7 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
@@ -916,6 +918,8 @@
 ##
 { 'struct': 'CpuInstanceProperties',
   'data': { '*node-id': 'int',
+            '*drawer-id': 'int',
+            '*book-id': 'int',
             '*socket-id': 'int',
             '*die-id': 'int',
             '*cluster-id': 'int',
@@ -1465,6 +1469,10 @@
 #
 # @cpus: number of virtual CPUs in the virtual machine
 #
+# @drawers: number of drawers in the CPU topology
+#
+# @books: number of books in the CPU topology
+#
 # @sockets: number of sockets in the CPU topology
 #
 # @dies: number of dies per socket in the CPU topology
@@ -1481,6 +1489,8 @@
 ##
 { 'struct': 'SMPConfiguration', 'data': {
      '*cpus': 'int',
+     '*drawers': 'int',
+     '*books': 'int',
      '*sockets': 'int',
      '*dies': 'int',
      '*clusters': 'int',
diff --git a/qemu-options.hx b/qemu-options.hx
index 311e75a98e..e78e51ab49 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -245,11 +245,13 @@ SRST
 ERST
 
 DEF("smp", HAS_ARG, QEMU_OPTION_smp,
-    "-smp [[cpus=]n][,maxcpus=maxcpus][,sockets=sockets][,dies=dies][,clusters=clusters][,cores=cores][,threads=threads]\n"
+    "-smp [[cpus=]n][,maxcpus=maxcpus][,drawers=drawers][,books=books][,sockets=sockets][,dies=dies][,clusters=clusters][,cores=cores][,threads=threads]\n"
     "                set the number of initial CPUs to 'n' [default=1]\n"
     "                maxcpus= maximum number of total CPUs, including\n"
     "                offline CPUs for hotplug, etc\n"
-    "                sockets= number of sockets on the machine board\n"
+    "                drawers= number of drawers on the machine board\n"
+    "                books= number of books in one drawer\n"
+    "                sockets= number of sockets in one book\n"
     "                dies= number of dies in one socket\n"
     "                clusters= number of clusters in one die\n"
     "                cores= number of cores in one cluster\n"
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 706bd7cff7..80ea35fa26 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -726,6 +726,12 @@ static QemuOptsList qemu_smp_opts = {
         {
             .name = "cpus",
             .type = QEMU_OPT_NUMBER,
+        }, {
+            .name = "drawers",
+            .type = QEMU_OPT_NUMBER,
+        }, {
+            .name = "books",
+            .type = QEMU_OPT_NUMBER,
         }, {
             .name = "sockets",
             .type = QEMU_OPT_NUMBER,
-- 
2.31.1

[PATCH v9 05/10] s390x/cpu: reporting drawers and books topology to the guest

[Pierre Morel]

The guest can ask for a topology report on drawer's or book's
level.
Let's implement the STSI instruction's handling for the corresponding
selector values.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/s390x/cpu-topology.c         | 19 +++++++---
 hw/s390x/s390-virtio-ccw.c      |  2 ++
 include/hw/s390x/cpu-topology.h |  7 +++-
 target/s390x/cpu_topology.c     | 64 +++++++++++++++++++++++++++------
 4 files changed, 76 insertions(+), 16 deletions(-)

diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index e2fd5c7e44..bb9ae63483 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -46,7 +46,7 @@ S390Topology *s390_get_topology(void)
 void s390_topology_new_cpu(int core_id)
 {
     S390Topology *topo = s390_get_topology();
-    int socket_id;
+    int socket_id, book_id, drawer_id;
     int bit, origin;
 
     /* In the case no Topology is used nothing is to be done here */
@@ -55,6 +55,8 @@ void s390_topology_new_cpu(int core_id)
     }
 
     socket_id = core_id / topo->cores;
+    book_id = socket_id / topo->sockets;
+    drawer_id = book_id / topo->books;
 
     bit = core_id;
     origin = bit / 64;
@@ -77,6 +79,8 @@ void s390_topology_new_cpu(int core_id)
      * CPU inside several CPU containers inside the socket container.
      */
     qemu_mutex_lock(&topo->topo_mutex);
+    topo->drawer[drawer_id].active_count++;
+    topo->book[book_id].active_count++;
     topo->socket[socket_id].active_count++;
     topo->tle[socket_id].active_count++;
     set_bit(bit, &topo->tle[socket_id].mask[origin]);
@@ -99,13 +103,20 @@ static void s390_topology_realize(DeviceState *dev, Error **errp)
     S390Topology *topo = S390_CPU_TOPOLOGY(dev);
     int n;
 
+    topo->drawers = ms->smp.drawers;
+    topo->books = ms->smp.books;
+    topo->total_books = topo->books * topo->drawers;
     topo->sockets = ms->smp.sockets;
+    topo->total_sockets = topo->sockets * topo->books * topo->drawers;
     topo->cores = ms->smp.cores;
-    topo->tles = ms->smp.max_cpus;
 
-    n = topo->sockets;
+    n = topo->drawers;
+    topo->drawer = g_malloc0(n * sizeof(S390TopoContainer));
+    n *= topo->books;
+    topo->book = g_malloc0(n * sizeof(S390TopoContainer));
+    n *= topo->sockets;
     topo->socket = g_malloc0(n * sizeof(S390TopoContainer));
-    topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE));
+    topo->tle = g_malloc0(n * sizeof(S390TopoTLE));
 
     qemu_mutex_init(&topo->topo_mutex);
 }
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 15cefd104b..3f28e28d47 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -626,6 +626,8 @@ 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_props.books_supported = true;
+    mc->smp_props.drawers_supported = true;
 }
 
 static inline bool machine_get_aes_key_wrap(Object *obj, Error **errp)
diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
index 0b7f3d10b2..4f8ac39ca0 100644
--- a/include/hw/s390x/cpu-topology.h
+++ b/include/hw/s390x/cpu-topology.h
@@ -29,9 +29,14 @@ typedef struct S390TopoTLE {
 
 struct S390Topology {
     SysBusDevice parent_obj;
+    int total_books;
+    int total_sockets;
+    int drawers;
+    int books;
     int sockets;
     int cores;
-    int tles;
+    S390TopoContainer *drawer;
+    S390TopoContainer *book;
     S390TopoContainer *socket;
     S390TopoTLE *tle;
     QemuMutex topo_mutex;
diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
index 56865dafc6..305fbb9734 100644
--- a/target/s390x/cpu_topology.c
+++ b/target/s390x/cpu_topology.c
@@ -37,19 +37,18 @@ static char *fill_tle_cpu(char *p, uint64_t mask, int origin)
     return p + sizeof(*tle);
 }
 
-static char *s390_top_set_level2(S390Topology *topo, char *p)
+static char *s390_top_set_level2(S390Topology *topo, char *p, int fs, int ns)
 {
-    int i, origin;
+    int socket, origin;
+    uint64_t mask;
 
-    for (i = 0; i < topo->sockets; i++) {
-        if (!topo->socket[i].active_count) {
+    for (socket = fs; socket < fs + ns; socket++) {
+        if (!topo->socket[socket].active_count) {
             continue;
         }
-        p = fill_container(p, 1, i);
+        p = fill_container(p, 1, socket);
         for (origin = 0; origin < S390_TOPOLOGY_MAX_ORIGIN; origin++) {
-            uint64_t mask = 0L;
-
-            mask = be64_to_cpu(topo->tle[i].mask[origin]);
+            mask = be64_to_cpu(topo->tle[socket].mask[origin]);
             if (mask) {
                 p = fill_tle_cpu(p, mask, origin);
             }
@@ -58,19 +57,63 @@ static char *s390_top_set_level2(S390Topology *topo, char *p)
     return p;
 }
 
+static char *s390_top_set_level3(S390Topology *topo, char *p, int fb, int nb)
+{
+    int book, fs = 0;
+
+    for (book = fb; book < fb + nb; book++, fs += topo->sockets) {
+        if (!topo->book[book].active_count) {
+            continue;
+        }
+        p = fill_container(p, 2, book);
+    p = s390_top_set_level2(topo, p, fs, topo->sockets);
+    }
+    return p;
+}
+
+static char *s390_top_set_level4(S390Topology *topo, char *p)
+{
+    int drawer, fb = 0;
+
+    for (drawer = 0; drawer < topo->drawers; drawer++, fb += topo->books) {
+        if (!topo->drawer[drawer].active_count) {
+            continue;
+        }
+        p = fill_container(p, 3, drawer);
+        p = s390_top_set_level3(topo, p, fb, topo->books);
+    }
+    return p;
+}
+
 static int setup_stsi(SysIB_151x *sysib, int level)
 {
     S390Topology *topo = s390_get_topology();
     char *p = (char *)sysib->tle;
+    int max_containers;
 
     qemu_mutex_lock(&topo->topo_mutex);
 
     sysib->mnest = level;
     switch (level) {
     case 2:
+        max_containers = topo->sockets * topo->books * topo->drawers;
+        sysib->mag[TOPOLOGY_NR_MAG2] = max_containers;
+        sysib->mag[TOPOLOGY_NR_MAG1] = topo->cores;
+        p = s390_top_set_level2(topo, p, 0, max_containers);
+        break;
+    case 3:
+        max_containers = topo->books * topo->drawers;
+        sysib->mag[TOPOLOGY_NR_MAG3] = max_containers;
         sysib->mag[TOPOLOGY_NR_MAG2] = topo->sockets;
         sysib->mag[TOPOLOGY_NR_MAG1] = topo->cores;
-        p = s390_top_set_level2(topo, p);
+        p = s390_top_set_level3(topo, p, 0, max_containers);
+        break;
+    case 4:
+        sysib->mag[TOPOLOGY_NR_MAG4] = topo->drawers;
+        sysib->mag[TOPOLOGY_NR_MAG3] = topo->books;
+        sysib->mag[TOPOLOGY_NR_MAG2] = topo->sockets;
+        sysib->mag[TOPOLOGY_NR_MAG1] = topo->cores;
+        p = s390_top_set_level4(topo, p);
         break;
     }
 
@@ -79,7 +122,7 @@ static int setup_stsi(SysIB_151x *sysib, int level)
     return p - (char *)sysib->tle;
 }
 
-#define S390_TOPOLOGY_MAX_MNEST 2
+#define S390_TOPOLOGY_MAX_MNEST 4
 void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
 {
     SysIB_151x *sysib;
@@ -105,4 +148,3 @@ void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
 out_free:
     g_free(sysib);
 }
-
-- 
2.31.1

[PATCH v9 06/10] s390x/cpu_topology: resetting the Topology-Change-Report

[Pierre Morel]

During a subsystem reset the Topology-Change-Report is cleared
by the machine.
Let's ask KVM to clear the Modified Topology Change Report (MTCR)
 bit of the SCA in the case of a subsystem reset.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/s390x/cpu-topology.c      | 12 ++++++++++++
 hw/s390x/s390-virtio-ccw.c   |  1 +
 target/s390x/cpu-sysemu.c    |  7 +++++++
 target/s390x/cpu.h           |  1 +
 target/s390x/kvm/kvm.c       | 23 +++++++++++++++++++++++
 target/s390x/kvm/kvm_s390x.h |  1 +
 6 files changed, 45 insertions(+)

diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index bb9ae63483..6098d6ea1f 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -121,6 +121,17 @@ static void s390_topology_realize(DeviceState *dev, Error **errp)
     qemu_mutex_init(&topo->topo_mutex);
 }
 
+/**
+ * s390_topology_reset:
+ * @dev: the device
+ *
+ * Calls the sysemu topology reset
+ */
+static void s390_topology_reset(DeviceState *dev)
+{
+    s390_cpu_topology_reset();
+}
+
 /**
  * topology_class_init:
  * @oc: Object class
@@ -134,6 +145,7 @@ static void topology_class_init(ObjectClass *oc, void *data)
 
     dc->realize = s390_topology_realize;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+    dc->reset = s390_topology_reset;
 }
 
 static const TypeInfo cpu_topology_info = {
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 3f28e28d47..1fa98740de 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -104,6 +104,7 @@ static const char *const reset_dev_types[] = {
     "s390-flic",
     "diag288",
     TYPE_S390_PCI_HOST_BRIDGE,
+    TYPE_S390_CPU_TOPOLOGY,
 };
 
 static void subsystem_reset(void)
diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
index 948e4bd3e0..707c0b658c 100644
--- a/target/s390x/cpu-sysemu.c
+++ b/target/s390x/cpu-sysemu.c
@@ -306,3 +306,10 @@ void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg)
         kvm_s390_set_diag318(cs, arg.host_ulong);
     }
 }
+
+void s390_cpu_topology_reset(void)
+{
+    if (kvm_enabled()) {
+        kvm_s390_topology_set_mtcr(0);
+    }
+}
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index c61fe9b563..cff5cc0415 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -826,6 +826,7 @@ void s390_enable_css_support(S390CPU *cpu);
 void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg);
 int s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch_id,
                                 int vq, bool assign);
+void s390_cpu_topology_reset(void);
 #ifndef CONFIG_USER_ONLY
 unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu);
 #else
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index f96630440b..9c994d27d5 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -2585,3 +2585,26 @@ int kvm_s390_get_zpci_op(void)
 {
     return cap_zpci_op;
 }
+
+int kvm_s390_topology_set_mtcr(uint64_t attr)
+{
+    struct kvm_device_attr attribute = {
+        .group = KVM_S390_VM_CPU_TOPOLOGY,
+        .attr  = attr,
+    };
+    int ret;
+
+    if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
+        return -EFAULT;
+    }
+    if (!kvm_vm_check_attr(kvm_state, KVM_S390_VM_CPU_TOPOLOGY, attr)) {
+        return -ENOENT;
+    }
+
+    ret = kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attribute);
+    if (ret) {
+        error_report("Failed to set cpu topology attribute %lu: %s",
+                     attr, strerror(-ret));
+    }
+    return ret;
+}
diff --git a/target/s390x/kvm/kvm_s390x.h b/target/s390x/kvm/kvm_s390x.h
index aaae8570de..a13c8fb9a3 100644
--- a/target/s390x/kvm/kvm_s390x.h
+++ b/target/s390x/kvm/kvm_s390x.h
@@ -46,5 +46,6 @@ void kvm_s390_crypto_reset(void);
 void kvm_s390_restart_interrupt(S390CPU *cpu);
 void kvm_s390_stop_interrupt(S390CPU *cpu);
 void kvm_s390_set_diag318(CPUState *cs, uint64_t diag318_info);
+int kvm_s390_topology_set_mtcr(uint64_t attr);
 
 #endif /* KVM_S390X_H */
-- 
2.31.1

[PATCH v9 07/10] s390x/cpu_topology: CPU topology migration

[Pierre Morel]

The migration can only take place if both source and destination
of the migration both use or both do not use the CPU topology
facility.

We indicate a change in topology during migration postload for the
case the topology changed between source and destination.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/s390x/cpu-topology.c         | 79 +++++++++++++++++++++++++++++++++
 include/hw/s390x/cpu-topology.h |  1 +
 target/s390x/cpu-sysemu.c       |  8 ++++
 target/s390x/cpu.h              |  1 +
 4 files changed, 89 insertions(+)

diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index 6098d6ea1f..b6bf839e40 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -19,6 +19,7 @@
 #include "target/s390x/cpu.h"
 #include "hw/s390x/s390-virtio-ccw.h"
 #include "hw/s390x/cpu-topology.h"
+#include "migration/vmstate.h"
 
 S390Topology *s390_get_topology(void)
 {
@@ -132,6 +133,83 @@ static void s390_topology_reset(DeviceState *dev)
     s390_cpu_topology_reset();
 }
 
+/**
+ * cpu_topology_postload
+ * @opaque: a pointer to the S390Topology
+ * @version_id: version identifier
+ *
+ * We check that the topology is used or is not used
+ * on both side identically.
+ *
+ * If the topology is in use we set the Modified Topology Change Report
+ * on the destination host.
+ */
+static int cpu_topology_postload(void *opaque, int version_id)
+{
+    S390Topology *topo = opaque;
+    int ret;
+
+    if (topo->topology_needed != s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
+        if (topo->topology_needed) {
+            error_report("Topology facility is needed in destination");
+        } else {
+            error_report("Topology facility can not be used in destination");
+        }
+        return -EINVAL;
+    }
+
+    /* We do not support CPU Topology, all is good */
+    if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
+        return 0;
+    }
+
+    /* We support CPU Topology, set the MTCR */
+    ret = s390_cpu_topology_mtcr_set();
+    if (ret) {
+        error_report("Failed to set MTCR: %s", strerror(-ret));
+    }
+    return ret;
+}
+
+/**
+ * cpu_topology_presave:
+ * @opaque: The pointer to the S390Topology
+ *
+ * Save the usage of the CPU Topology in the VM State.
+ */
+static int cpu_topology_presave(void *opaque)
+{
+    S390Topology *topo = opaque;
+
+    topo->topology_needed = s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY);
+    return 0;
+}
+
+/**
+ * cpu_topology_needed:
+ * @opaque: The pointer to the S390Topology
+ *
+ * If we use the CPU Topology on the source it will be needed on the destination.
+ */
+static bool cpu_topology_needed(void *opaque)
+{
+    return s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY);
+}
+
+
+const VMStateDescription vmstate_cpu_topology = {
+    .name = "cpu_topology",
+    .version_id = 1,
+    .post_load = cpu_topology_postload,
+    .pre_save = cpu_topology_presave,
+    .minimum_version_id = 1,
+    .needed = cpu_topology_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(topology_needed, S390Topology),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 /**
  * topology_class_init:
  * @oc: Object class
@@ -146,6 +224,7 @@ static void topology_class_init(ObjectClass *oc, void *data)
     dc->realize = s390_topology_realize;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     dc->reset = s390_topology_reset;
+    dc->vmsd = &vmstate_cpu_topology;
 }
 
 static const TypeInfo cpu_topology_info = {
diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
index 4f8ac39ca0..a15567baca 100644
--- a/include/hw/s390x/cpu-topology.h
+++ b/include/hw/s390x/cpu-topology.h
@@ -29,6 +29,7 @@ typedef struct S390TopoTLE {
 
 struct S390Topology {
     SysBusDevice parent_obj;
+    bool topology_needed;
     int total_books;
     int total_sockets;
     int drawers;
diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
index 707c0b658c..78cb11c0f8 100644
--- a/target/s390x/cpu-sysemu.c
+++ b/target/s390x/cpu-sysemu.c
@@ -313,3 +313,11 @@ void s390_cpu_topology_reset(void)
         kvm_s390_topology_set_mtcr(0);
     }
 }
+
+int s390_cpu_topology_mtcr_set(void)
+{
+    if (kvm_enabled()) {
+        return kvm_s390_topology_set_mtcr(1);
+    }
+    return -ENOENT;
+}
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index cff5cc0415..0eb2d219d3 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -827,6 +827,7 @@ void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg);
 int s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch_id,
                                 int vq, bool assign);
 void s390_cpu_topology_reset(void);
+int s390_cpu_topology_mtcr_set(void);
 #ifndef CONFIG_USER_ONLY
 unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu);
 #else
-- 
2.31.1

[PATCH v9 08/10] target/s390x: interception of PTF instruction

[Pierre Morel]

When the host supports the CPU topology facility, the PTF
instruction with function code 2 is interpreted by the SIE,
provided that the userland hypervizor activates the interpretation
by using the KVM_CAP_S390_CPU_TOPOLOGY KVM extension.

The PTF instructions with function code 0 and 1 are intercepted
and must be emulated by the userland hypervizor.

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

diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index b6bf839e40..7dcaa28ca3 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -20,6 +20,58 @@
 #include "hw/s390x/s390-virtio-ccw.h"
 #include "hw/s390x/cpu-topology.h"
 #include "migration/vmstate.h"
+#include "target/s390x/cpu.h"
+#include "hw/s390x/s390-virtio-ccw.h"
+
+/*
+ * s390_handle_ptf:
+ *
+ * @register 1: contains the function code
+ *
+ * Function codes 0 and 1 handle the CPU polarization.
+ * We assume an horizontal topology, the only one supported currently
+ * by Linux, consequently we answer to function code 0, requesting
+ * horizontal polarization that it is already the current polarization
+ * and reject vertical polarization request without further explanation.
+ *
+ * Function code 2 is handling topology changes and is interpreted
+ * by the SIE.
+ */
+void 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_OPERATION, ra);
+        return;
+    }
+
+    if (env->psw.mask & PSW_MASK_PSTATE) {
+        s390_program_interrupt(env, PGM_PRIVILEGED, ra);
+        return;
+    }
+
+    if (reg & ~S390_TOPO_FC_MASK) {
+        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+        return;
+    }
+
+    switch (fc) {
+    case 0:    /* Horizontal polarization is already set */
+        env->regs[r1] |= S390_PTF_REASON_DONE;
+        setcc(cpu, 2);
+        break;
+    case 1:    /* Vertical polarization is not supported */
+        env->regs[r1] |= S390_PTF_REASON_NONE;
+        setcc(cpu, 2);
+        break;
+    default:
+        /* Note that fc == 2 is interpreted by the SIE */
+        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+    }
+}
 
 S390Topology *s390_get_topology(void)
 {
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 8a0090a071..9e7a0d75bc 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -31,6 +31,12 @@ struct S390CcwMachineState {
     uint8_t loadparm[8];
 };
 
+#define S390_PTF_REASON_NONE (0x00 << 8)
+#define S390_PTF_REASON_DONE (0x01 << 8)
+#define S390_PTF_REASON_BUSY (0x02 << 8)
+#define S390_TOPO_FC_MASK 0xffUL
+void s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra);
+
 struct S390CcwMachineClass {
     /*< private >*/
     MachineClass parent_class;
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 9c994d27d5..cb14bcc012 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -97,6 +97,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
@@ -1463,6 +1464,15 @@ static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
     }
 }
 
+static int kvm_handle_ptf(S390CPU *cpu, struct kvm_run *run)
+{
+    uint8_t r1 = (run->s390_sieic.ipb >> 20) & 0x0f;
+
+    s390_handle_ptf(cpu, r1, RA_IGNORED);
+
+    return 0;
+}
+
 static int handle_b9(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
 {
     int r = 0;
@@ -1480,6 +1490,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.31.1

[PATCH v9 09/10] s390x/cpu_topology: activating CPU topology

[Pierre Morel]

Starting with a new machine, s390-virtio-ccw-7.2, the machine
property topology-disable is set to false while it is kept to
true for older machine.
This allows migrating older machine without disabling the ctop
CPU feature for older machine, thus keeping existing start scripts.

The KVM capability, KVM_CAP_S390_CPU_TOPOLOGY is used to
activate the S390_FEAT_CONFIGURATION_TOPOLOGY feature and
the topology facility for the guest in the case the topology
is not disabled.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/core/machine.c                  |  5 +++
 hw/s390x/s390-virtio-ccw.c         | 55 ++++++++++++++++++++++++++----
 include/hw/boards.h                |  3 ++
 include/hw/s390x/s390-virtio-ccw.h |  1 +
 target/s390x/kvm/kvm.c             | 14 ++++++++
 5 files changed, 72 insertions(+), 6 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 4c5c8d1655..cbcdd40763 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -40,6 +40,11 @@
 #include "hw/virtio/virtio-pci.h"
 #include "qom/object_interfaces.h"
 
+GlobalProperty hw_compat_7_1[] = {
+    { "s390x-cpu", "ctop", "off"},
+};
+const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
+
 GlobalProperty hw_compat_7_0[] = {
     { "arm-gicv3-common", "force-8-bit-prio", "on" },
     { "nvme-ns", "eui64-default", "on"},
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 1fa98740de..3078e68df7 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -249,11 +249,16 @@ static void ccw_init(MachineState *machine)
     /* init memory + setup max page size. Required for the CPU model */
     s390_memory_init(machine->ram);
 
-    /* Adding the topology must be done before CPU intialization*/
-    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
-    object_property_add_child(qdev_get_machine(), TYPE_S390_CPU_TOPOLOGY,
-                              OBJECT(dev));
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    /*
+     * Adding the topology must be done before CPU intialization but
+     * only in the case it is not disabled for migration purpose.
+     */
+    if (!S390_CCW_MACHINE(machine)->topology_disable) {
+        dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
+        object_property_add_child(qdev_get_machine(), TYPE_S390_CPU_TOPOLOGY,
+                                  OBJECT(dev));
+        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    }
 
     /* init CPUs (incl. CPU model) early so s390_has_feature() works */
     s390_init_cpus(machine);
@@ -676,6 +681,21 @@ static inline void machine_set_zpcii_disable(Object *obj, bool value,
     ms->zpcii_disable = value;
 }
 
+static inline bool machine_get_topology_disable(Object *obj, Error **errp)
+{
+    S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
+
+    return ms->topology_disable;
+}
+
+static inline void machine_set_topology_disable(Object *obj, bool value,
+                                                Error **errp)
+{
+    S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
+
+    ms->topology_disable = value;
+}
+
 static S390CcwMachineClass *current_mc;
 
 /*
@@ -778,6 +798,13 @@ static inline void s390_machine_initfn(Object *obj)
     object_property_set_description(obj, "zpcii-disable",
             "disable zPCI interpretation facilties");
     object_property_set_bool(obj, "zpcii-disable", false, NULL);
+
+    object_property_add_bool(obj, "topology-disable",
+                             machine_get_topology_disable,
+                             machine_set_topology_disable);
+    object_property_set_description(obj, "topology-disable",
+            "disable zPCI interpretation facilties");
+    object_property_set_bool(obj, "topology-disable", false, NULL);
 }
 
 static const TypeInfo ccw_machine_info = {
@@ -830,14 +857,29 @@ bool css_migration_enabled(void)
     }                                                                         \
     type_init(ccw_machine_register_##suffix)
 
+static void ccw_machine_7_2_instance_options(MachineState *machine)
+{
+}
+
+static void ccw_machine_7_2_class_options(MachineClass *mc)
+{
+}
+DEFINE_CCW_MACHINE(7_2, "7.2", true);
+
 static void ccw_machine_7_1_instance_options(MachineState *machine)
 {
+    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
+
+    ccw_machine_7_2_instance_options(machine);
+    ms->topology_disable = true;
 }
 
 static void ccw_machine_7_1_class_options(MachineClass *mc)
 {
+    ccw_machine_7_2_class_options(mc);
+    compat_props_add(mc->compat_props, hw_compat_7_1, hw_compat_7_1_len);
 }
-DEFINE_CCW_MACHINE(7_1, "7.1", true);
+DEFINE_CCW_MACHINE(7_1, "7.1", false);
 
 static void ccw_machine_7_0_instance_options(MachineState *machine)
 {
@@ -847,6 +889,7 @@ static void ccw_machine_7_0_instance_options(MachineState *machine)
     ccw_machine_7_1_instance_options(machine);
     s390_set_qemu_cpu_model(0x8561, 15, 1, qemu_cpu_feat);
     ms->zpcii_disable = true;
+
 }
 
 static void ccw_machine_7_0_class_options(MachineClass *mc)
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 69e20c1252..6e9803aa2d 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -387,6 +387,9 @@ struct MachineState {
     } \
     type_init(machine_initfn##_register_types)
 
+extern GlobalProperty hw_compat_7_1[];
+extern const size_t hw_compat_7_1_len;
+
 extern GlobalProperty hw_compat_7_0[];
 extern const size_t hw_compat_7_0_len;
 
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 9e7a0d75bc..b14660eecb 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -28,6 +28,7 @@ struct S390CcwMachineState {
     bool dea_key_wrap;
     bool pv;
     bool zpcii_disable;
+    bool topology_disable;
     uint8_t loadparm[8];
 };
 
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index cb14bcc012..6b7efee511 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -2385,6 +2385,7 @@ bool kvm_s390_cpu_models_supported(void)
 
 void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
 {
+    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
     struct kvm_s390_vm_cpu_machine prop = {};
     struct kvm_device_attr attr = {
         .group = KVM_S390_VM_CPU_MODEL,
@@ -2466,6 +2467,19 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
         set_bit(S390_FEAT_UNPACK, model->features);
     }
 
+    /*
+     * If we have the CPU Topology implemented in KVM activate
+     * the CPU TOPOLOGY feature.
+     */
+    if ((!ms->topology_disable) &&
+        kvm_check_extension(kvm_state, KVM_CAP_S390_CPU_TOPOLOGY)) {
+        if (kvm_vm_enable_cap(kvm_state, KVM_CAP_S390_CPU_TOPOLOGY, 0) < 0) {
+            error_setg(errp, "KVM: Error enabling KVM_CAP_S390_CPU_TOPOLOGY");
+            return;
+        }
+        set_bit(S390_FEAT_CONFIGURATION_TOPOLOGY, model->features);
+    }
+
     /* We emulate a zPCI bus and AEN, therefore we don't need HW support */
     set_bit(S390_FEAT_ZPCI, model->features);
     set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
-- 
2.31.1

[PATCH v9 10/10] docs/s390x: document s390x cpu topology

[Pierre Morel]

Add some basic examples for the definition of cpu topology
in s390x.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 docs/system/s390x/cpu_topology.rst | 88 ++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)
 create mode 100644 docs/system/s390x/cpu_topology.rst

diff --git a/docs/system/s390x/cpu_topology.rst b/docs/system/s390x/cpu_topology.rst
new file mode 100644
index 0000000000..00977d4319
--- /dev/null
+++ b/docs/system/s390x/cpu_topology.rst
@@ -0,0 +1,88 @@
+CPU Topology on s390x
+=====================
+
+CPU Topology on S390x provides up to 4 levels of topology containers:
+drawers, books, sockets and CPUs.
+While the three higher level containers, Containers Topology List Entries,
+(Containers TLE) define a tree hierarchy, the lowest level of topology
+definition, the CPU Topology List Entry (CPU TLE), provides the placement
+of the CPUs inside the last container.
+
+Prerequisites
+-------------
+
+To use CPU Topology a Linux QEMU/KVM machine providing the CPU Topology facility
+(STFLE bit 11) is required.
+
+However, since this facility has been enabled by default in an early version,
+the capability ``KVM_CAP_S390_CPU_TOPOLOGY`` is needed to indicate to KVM
+that QEMU support CPU Topology.
+
+Indicating the CPU topology to the Virtual Machine
+--------------------------------------------------
+
+The CPU Topology, number of drawers, number of books per drawers, number of
+sockets per book and number of cores per sockets is specified with the
+``-smp`` qemu command arguments.
+
+Like in :
+
+.. code-block:: sh
+    -smp cpus=1,drawers=3,books=4,sockets=2,cores=8,maxcpus=192
+
+If drawers or books are not specified, their default to 1.
+
+New CPUs can be plugged using the device_add hmp command like in:
+
+.. code-block:: sh
+   (qemu) device_add host-s390x-cpu,core-id=9
+
+The core-id defines the placement of the core in the topology by
+starting with core 0 in socket 0, book 0 and drawer 0 up to the maximum
+core number of the last socket of the last book in the last drawer.
+
+In the example above:
+
+* the core with ID 9 will be placed in container (0,0,1), as core 9
+  of CPU TLE 0 of socket 1 in book 0 from drawer 0.
+* the core ID 0 is defined by the -smp cpus=1 command and will be
+  placed as core 0 in CPU TLE 0 of container (0,0,0)
+
+Note that the core ID is machine wide and the CPU TLE masks provided
+by the STSI instruction will be:
+
+* in socket 0: 0x80000000 (core id 0)
+* in socket 1: 0x00400000 (core id 9)
+
+Indicating the CPU topology to the Guest
+----------------------------------------
+
+The guest can query for topology changes using the PTF instruction.
+In case of a topology change it can request the new topology by issuing
+STSI instructions specifying the level of detail required, drawer with
+STSI(15.1.4) or books STSI(15.1.3).
+
+The virtual machine will fill the provided buffer with the count of
+drawers (MAG4), books per drawer (MAG3), sockets per book (MAG2) and
+cores per socket (MAG1).
+
+Note that the STSI(15.1.2) is special in two ways:
+
+* When the firmware detect a change in the values calculated for STSI(15.1.2)
+  it will trigger the report of the topology change for the PTF instruction.
+
+Migration
+---------
+
+For virtio-ccw machines older than s390-virtio-ccw-7.2, CPU Topoogy is
+by default disabled.
+
+CPU Topoogy is by default enabled for s390-virtio-ccw-7.2 and newer machines.
+
+Enabling the CPU topology on older Machine is done by setting the global
+option ''topology-disable'' to false before enabling cpu topology with the
+cpu feature "ctop" like in:
+
+.. code-block:: sh
+   -machine s390-ccw-virtio-3.0,accel=kvm,topology-disable=false
+   -cpu z14,ctop=on
-- 
2.31.1

Re: [PATCH v9 01/10] s390x/cpus: Make absence of multithreading clear

[Nico Boehr]

Quoting Pierre Morel (2022-09-02 09:55:22)
> S390x do not support multithreading in the guest.
> Do not let admin falsely specify multithreading on QEMU
> smp commandline.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 70229b102b..b5ca154e2f 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine)
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>      int i;
>  
> +    /* Explicitely do not support threads */
          ^
          Explicitly

> +    assert(machine->smp.threads == 1);

It might be nicer to give a better error message to the user.
What do you think about something like (broken whitespace ahead):

    if (machine->smp.threads != 1) {
        error_setg(&error_fatal, "More than one thread specified, but multithreading unsupported");
        return;
    }

Re: [PATCH v9 01/10] s390x/cpus: Make absence of multithreading clear

[Pierre Morel]

On 9/5/22 13:32, Nico Boehr wrote:
> Quoting Pierre Morel (2022-09-02 09:55:22)
>> S390x do not support multithreading in the guest.
>> Do not let admin falsely specify multithreading on QEMU
>> smp commandline.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/s390-virtio-ccw.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 70229b102b..b5ca154e2f 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine)
>>       MachineClass *mc = MACHINE_GET_CLASS(machine);
>>       int i;
>>   
>> +    /* Explicitely do not support threads */
>            ^
>            Explicitly
> 
>> +    assert(machine->smp.threads == 1);
> 
> It might be nicer to give a better error message to the user.
> What do you think about something like (broken whitespace ahead):
> 
>      if (machine->smp.threads != 1) {if (machine->smp.threads != 1) {
>          error_setg(&error_fatal, "More than one thread specified, but multithreading unsupported");
>          return;
>      }
> 


OK, I think I wanted to do this and I changed my mind, obviously, I do 
not recall why.
I will do almost the same but after a look at error.h I will use 
error_report()/exit() instead of error_setg()/return as in:


+    /* Explicitly do not support threads */
+    if (machine->smp.threads != 1) {
+        error_report("More than one thread specified, but 
multithreading unsupported");
+        exit(1);
+    }


Thanks,

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 01/10] s390x/cpus: Make absence of multithreading clear

[Janis Schoetterl-Glausch]

On Mon, 2022-09-05 at 17:10 +0200, Pierre Morel wrote:
> 
> On 9/5/22 13:32, Nico Boehr wrote:
> > Quoting Pierre Morel (2022-09-02 09:55:22)
> > > S390x do not support multithreading in the guest.
> > > Do not let admin falsely specify multithreading on QEMU
> > > smp commandline.
> > > 
> > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > > ---
> > >   hw/s390x/s390-virtio-ccw.c | 3 +++
> > >   1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > > index 70229b102b..b5ca154e2f 100644
> > > --- a/hw/s390x/s390-virtio-ccw.c
> > > +++ b/hw/s390x/s390-virtio-ccw.c
> > > @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine)
> > >       MachineClass *mc = MACHINE_GET_CLASS(machine);
> > >       int i;
> > >   
> > > +    /* Explicitely do not support threads */
> >            ^
> >            Explicitly
> > 
> > > +    assert(machine->smp.threads == 1);
> > 
> > It might be nicer to give a better error message to the user.
> > What do you think about something like (broken whitespace ahead):
> > 
> >      if (machine->smp.threads != 1) {if (machine->smp.threads != 1) {
> >          error_setg(&error_fatal, "More than one thread specified, but multithreading unsupported");
> >          return;
> >      }
> > 
> 
> 
> OK, I think I wanted to do this and I changed my mind, obviously, I do 
> not recall why.
> I will do almost the same but after a look at error.h I will use 
> error_report()/exit() instead of error_setg()/return as in:
> 
> 
> +    /* Explicitly do not support threads */
> +    if (machine->smp.threads != 1) {
> +        error_report("More than one thread specified, but 
> multithreading unsupported");
> +        exit(1);
> +    }

I agree that an assert is not a good solution, and I'm not sure
aborting is a good idea either.
I'm assuming that currently if you specify threads > 0 qemu will run
with the number of CPUs multiplied by threads (compared to threads=1).
If that is true, then a new qemu version will break existing
invocations.

An alternative would be to print a warning and do:
cores *= threads
threads = 1

The questions would be what the best place to do that is.
I guess we'd need a new compat variable if that's done in machine-smp.c
> 
> 
> Thanks,
> 
> Regards,
> Pierre
> 

Re: [PATCH v9 09/10] s390x/cpu_topology: activating CPU topology

[Pierre Morel]

On 9/2/22 09:55, Pierre Morel wrote:
> Starting with a new machine, s390-virtio-ccw-7.2, the machine
> property topology-disable is set to false while it is kept to
> true for older machine.
> This allows migrating older machine without disabling the ctop
> CPU feature for older machine, thus keeping existing start scripts.
> 
> The KVM capability, KVM_CAP_S390_CPU_TOPOLOGY is used to
> activate the S390_FEAT_CONFIGURATION_TOPOLOGY feature and
> the topology facility for the guest in the case the topology
> is not disabled.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---

Sorry, forget this patch I made a stupid rebase error and lost half of 
the code.

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 01/10] s390x/cpus: Make absence of multithreading clear

[Pierre Morel]

On 9/5/22 17:23, Janis Schoetterl-Glausch wrote:
> On Mon, 2022-09-05 at 17:10 +0200, Pierre Morel wrote:
>>
>> On 9/5/22 13:32, Nico Boehr wrote:
>>> Quoting Pierre Morel (2022-09-02 09:55:22)
>>>> S390x do not support multithreading in the guest.
>>>> Do not let admin falsely specify multithreading on QEMU
>>>> smp commandline.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> ---
>>>>    hw/s390x/s390-virtio-ccw.c | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>> index 70229b102b..b5ca154e2f 100644
>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>> @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine)
>>>>        MachineClass *mc = MACHINE_GET_CLASS(machine);
>>>>        int i;
>>>>    
>>>> +    /* Explicitely do not support threads */
>>>             ^
>>>             Explicitly
>>>
>>>> +    assert(machine->smp.threads == 1);
>>>
>>> It might be nicer to give a better error message to the user.
>>> What do you think about something like (broken whitespace ahead):
>>>
>>>       if (machine->smp.threads != 1) {if (machine->smp.threads != 1) {
>>>           error_setg(&error_fatal, "More than one thread specified, but multithreading unsupported");
>>>           return;
>>>       }
>>>
>>
>>
>> OK, I think I wanted to do this and I changed my mind, obviously, I do
>> not recall why.
>> I will do almost the same but after a look at error.h I will use
>> error_report()/exit() instead of error_setg()/return as in:
>>
>>
>> +    /* Explicitly do not support threads */
>> +    if (machine->smp.threads != 1) {
>> +        error_report("More than one thread specified, but
>> multithreading unsupported");
>> +        exit(1);
>> +    }
> 
> I agree that an assert is not a good solution, and I'm not sure
> aborting is a good idea either.
> I'm assuming that currently if you specify threads > 0 qemu will run
> with the number of CPUs multiplied by threads (compared to threads=1).
> If that is true, then a new qemu version will break existing
> invocations.
> 
> An alternative would be to print a warning and do:
> cores *= threads
> threads = 1
> 
> The questions would be what the best place to do that is.
> I guess we'd need a new compat variable if that's done in machine-smp.c

Right, I think we can use the new "topology_disable" machine property.


>>
>>
>> Thanks,
>>
>> Regards,
>> Pierre
>>
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 02/10] s390x/cpu topology: core_id sets s390x CPU topology

[Janis Schoetterl-Glausch]

On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
> In the S390x CPU topology the core_id specifies the CPU address
> and the position of the core withing the topology.
> 
> Let's build the topology based on the core_id.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  hw/s390x/cpu-topology.c         | 135 ++++++++++++++++++++++++++++++++
>  hw/s390x/meson.build            |   1 +
>  hw/s390x/s390-virtio-ccw.c      |  10 +++
>  include/hw/s390x/cpu-topology.h |  42 ++++++++++
>  4 files changed, 188 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
> 
[...]

> +/**
> + * s390_topology_realize:
> + * @dev: the device state
> + * @errp: the error pointer (not used)
> + *
> + * During realize the machine CPU topology is initialized with the
> + * QEMU -smp parameters.
> + * The maximum count of CPU TLE in the all Topology can not be greater
> + * than the maximum CPUs.
> + */
> +static void s390_topology_realize(DeviceState *dev, Error **errp)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    S390Topology *topo = S390_CPU_TOPOLOGY(dev);
> +    int n;
> +
> +    topo->sockets = ms->smp.sockets;
> +    topo->cores = ms->smp.cores;
> +    topo->tles = ms->smp.max_cpus;
> +
> +    n = topo->sockets;
> +    topo->socket = g_malloc0(n * sizeof(S390TopoContainer));
> +    topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE));

Seems like a good use case for g_new0.

[...]
> 
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> new file mode 100644
> index 0000000000..6911f975f4
> --- /dev/null
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -0,0 +1,42 @@
> +/*
> + * CPU Topology
> + *
> + * Copyright 2022 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

Is there a reason this is before the includes?
> +
> +typedef struct S390TopoContainer {
> +    int active_count;
> +} S390TopoContainer;
> +
> +#define S390_TOPOLOGY_MAX_ORIGIN (1 + S390_MAX_CPUS / 64)

This is correct because cpu_id == core_id for s390, right?
So the cpu limit also applies to the core id.
You could do ((S390_MAX_CPUS + 63) / 64) instead.
But if you chose this for simplicity's sake, I'm fine with it.

> +typedef struct S390TopoTLE {
> +    int active_count;

Do you use (read) this field somewhere?
Is this in anticipation of there being multiple TLE arrays, for
different polarizations, etc? If so I would defer this for later.

> +    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
> +} S390TopoTLE;
> +
> +#include "hw/qdev-core.h"
> +#include "qom/object.h"
> +
> +struct S390Topology {
> +    SysBusDevice parent_obj;
> +    int sockets;
> +    int cores;

These are just cached values from machine_state.smp, right?
Not sure if I like the redundancy, it doesn't aid in comprehension.

> +    int tles;
> +    S390TopoContainer *socket;
> +    S390TopoTLE *tle;
> +};
> +typedef struct S390Topology S390Topology;

The DECLARE macro takes care of this typedef.

> +
> +#define TYPE_S390_CPU_TOPOLOGY "s390-topology"
> +OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY)
> +
> +S390Topology *s390_get_topology(void);
> +void s390_topology_new_cpu(int core_id);
> +
> +#endif

Re: [PATCH v9 02/10] s390x/cpu topology: core_id sets s390x CPU topology

[Nico Boehr]

Quoting Pierre Morel (2022-09-02 09:55:23)
[...]
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> new file mode 100644
> index 0000000000..a6ca006ec5
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c
[...]
> +void s390_topology_new_cpu(int core_id)
> +{
[...]
> +    socket_id = core_id / topo->cores;

The comment below is essential for understanding all of this. Move it before this line.

> +
> +    bit = core_id;
> +    origin = bit / 64;
> +    bit %= 64;
> +    bit = 63 - bit;
> +
> +    /*
> +     * At the core level, each CPU is represented by a bit in a 64bit
> +     * unsigned long. Set on plug and clear on unplug of a CPU.

cleared                                  ^

[...]
> +     * In that case the origin field, representing the offset of the first CPU
> +     * in the CPU container allows to represent up to the maximal number of
> +     * CPU inside several CPU containers inside the socket container.

How about:
"In that case the origin variable represents the offset of the first CPU in the
CPU container. More than 64 CPUs per socket are represented in several CPU
containers inside the socket container."

> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index b5ca154e2f..15cefd104b 100644
[...]
> @@ -247,6 +248,12 @@ static void ccw_init(MachineState *machine)
>      /* init memory + setup max page size. Required for the CPU model */
>      s390_memory_init(machine->ram);
>  
> +    /* Adding the topology must be done before CPU intialization*/

space                                                              ^

Re: [PATCH v9 03/10] s390x/cpu topology: reporting the CPU topology to the guest

[Nico Boehr]

Quoting Pierre Morel (2022-09-02 09:55:24)
> The guest can use the STSI instruction to get a buffer filled
> with the CPU topology description.
> 
> Let us implement the STSI instruction for the basis CPU topology
> level, level 2.

I like this. It is so much simpler. Thanks.

[...]
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> index a6ca006ec5..e2fd5c7e44 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -76,9 +76,11 @@ void s390_topology_new_cpu(int core_id)
>       * in the CPU container allows to represent up to the maximal number of
>       * CPU inside several CPU containers inside the socket container.
>       */
> +    qemu_mutex_lock(&topo->topo_mutex);

You access topo->cores above. Do you need the mutex for that? I guess not since
it can't change at runtime (right?), so maybe it is worth documenting what the
topo_mutex actually protects or you just take the mutex at the start of the
function.

[...]
> diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
> new file mode 100644
> index 0000000000..56865dafc6
> --- /dev/null
> +++ b/target/s390x/cpu_topology.c
[...]
> +static char *fill_tle_cpu(char *p, uint64_t mask, int origin)
> +{
> +    SysIBTl_cpu *tle = (SysIBTl_cpu *)p;
> +
> +    tle->nl = 0;
> +    tle->dedicated = 1;
> +    tle->polarity = S390_TOPOLOGY_POLARITY_H;
> +    tle->type = S390_TOPOLOGY_CPU_TYPE;
> +    tle->origin = origin * 64;

origin would also need a byte order conversion.

> +    tle->mask = be64_to_cpu(mask);

cpu_to_be64()

[...]
> +static char *s390_top_set_level2(S390Topology *topo, char *p)
> +{
> +    int i, origin;
> +
> +    for (i = 0; i < topo->sockets; i++) {
> +        if (!topo->socket[i].active_count) {
> +            continue;
> +        }
> +        p = fill_container(p, 1, i);
> +        for (origin = 0; origin < S390_TOPOLOGY_MAX_ORIGIN; origin++) {
> +            uint64_t mask = 0L;
> +
> +            mask = be64_to_cpu(topo->tle[i].mask[origin]);

Don't you already do the endianness conversion in fill_tle_cpu()?

[...]
> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
> +{
> +    SysIB_151x *sysib;
> +    int len = sizeof(*sysib);
> +
> +    if (s390_is_pv() || sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
> +        setcc(cpu, 3);
> +        return;
> +    }
> +
> +    sysib = g_malloc0(TARGET_PAGE_SIZE);
> +
> +    len += setup_stsi(sysib, sel2);
> +    if (len > TARGET_PAGE_SIZE) {
> +        setcc(cpu, 3);
> +        goto out_free;
> +    }

Maybe I don't get it, but isn't it kind of late for this check? You would
already have written beyond the end of the buffer at this point in time...

> +
> +    sysib->length = be16_to_cpu(len);

cpu_to_be16()

Re: [PATCH v9 06/10] s390x/cpu_topology: resetting the Topology-Change-Report

[Nico Boehr]

Quoting Pierre Morel (2022-09-02 09:55:27)
> During a subsystem reset the Topology-Change-Report is cleared
> by the machine.
> Let's ask KVM to clear the Modified Topology Change Report (MTCR)
>  bit of the SCA in the case of a subsystem reset.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [PATCH v9 04/10] hw/core: introducing drawer and books for s390x

[Markus Armbruster]

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

> S390x defines two topology levels above sockets: nbooks and drawers.

nbooks or books?

> Let's add these two levels inside the CPU topology implementation.
>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---

[...]

> diff --git a/qapi/machine.json b/qapi/machine.json
> index 6afd1936b0..bdd92e3cb1 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -900,13 +900,15 @@
>  # 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
> +# @drawer-id: drawer number within node/board the CPU belongs to
> +# @book-id: book number within drawer/node/board the CPU belongs to
> +# @socket-id: socket number within book/node/board the CPU belongs to
>  # @die-id: die number within socket the CPU belongs to (since 4.1)
>  # @cluster-id: cluster number within die the CPU belongs to (since 7.1)
>  # @core-id: core number within cluster the CPU belongs to
>  # @thread-id: thread number within core the CPU belongs to
>  #
> -# Note: currently there are 6 properties that could be present
> +# Note: currently there are 7 properties that could be present

Should this be 8?

>  #       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.

Not your patch's fault, but the second sentence is less than clear.
What are "the filed names"?  A typo perhaps?

> @@ -916,6 +918,8 @@
>  ##
>  { 'struct': 'CpuInstanceProperties',
>    'data': { '*node-id': 'int',
> +            '*drawer-id': 'int',
> +            '*book-id': 'int',
>              '*socket-id': 'int',
>              '*die-id': 'int',
>              '*cluster-id': 'int',
> @@ -1465,6 +1469,10 @@
>  #
>  # @cpus: number of virtual CPUs in the virtual machine
>  #
> +# @drawers: number of drawers in the CPU topology
> +#
> +# @books: number of books in the CPU topology
> +#
>  # @sockets: number of sockets in the CPU topology
>  #
>  # @dies: number of dies per socket in the CPU topology
> @@ -1481,6 +1489,8 @@
>  ##
>  { 'struct': 'SMPConfiguration', 'data': {
>       '*cpus': 'int',
> +     '*drawers': 'int',
> +     '*books': 'int',
>       '*sockets': 'int',
>       '*dies': 'int',
>       '*clusters': 'int',

[...]

Re: [PATCH v9 03/10] s390x/cpu topology: reporting the CPU topology to the guest

[Janis Schoetterl-Glausch]

On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
> The guest can use the STSI instruction to get a buffer filled
> with the CPU topology description.
> 
> Let us implement the STSI instruction for the basis CPU topology
> level, level 2.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  hw/s390x/cpu-topology.c         |   4 ++
>  include/hw/s390x/cpu-topology.h |   5 ++
>  target/s390x/cpu.h              |  49 +++++++++++++++
>  target/s390x/cpu_topology.c     | 108 ++++++++++++++++++++++++++++++++
>  target/s390x/kvm/kvm.c          |   6 +-
>  target/s390x/meson.build        |   1 +
>  6 files changed, 172 insertions(+), 1 deletion(-)
>  create mode 100644 target/s390x/cpu_topology.c
> 
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> index a6ca006ec5..e2fd5c7e44 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -76,9 +76,11 @@ void s390_topology_new_cpu(int core_id)
>       * in the CPU container allows to represent up to the maximal number of
>       * CPU inside several CPU containers inside the socket container.
>       */
> +    qemu_mutex_lock(&topo->topo_mutex);

You could use a reader writer lock for this, if qemu has that (I didn't
find any tho).

>      topo->socket[socket_id].active_count++;
>      topo->tle[socket_id].active_count++;
>      set_bit(bit, &topo->tle[socket_id].mask[origin]);
> +    qemu_mutex_unlock(&topo->topo_mutex);
>  }
>  
>  /**
> @@ -104,6 +106,8 @@ static void s390_topology_realize(DeviceState *dev, Error **errp)
>      n = topo->sockets;
>      topo->socket = g_malloc0(n * sizeof(S390TopoContainer));
>      topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE));
> +
> +    qemu_mutex_init(&topo->topo_mutex);
>  }
>  
>  /**
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> index 6911f975f4..0b7f3d10b2 100644
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -10,6 +10,10 @@
>  #ifndef HW_S390X_CPU_TOPOLOGY_H
>  #define HW_S390X_CPU_TOPOLOGY_H
>  
> +#define S390_TOPOLOGY_CPU_TYPE    0x03

IMO you should add the name of cpu type 0x03 to the name of the
constant, even if there is only one right now.
You did the same for the polarity after all.

> +
> +#define S390_TOPOLOGY_POLARITY_H  0x00
> +
>  typedef struct S390TopoContainer {
>      int active_count;
>  } S390TopoContainer;
> @@ -30,6 +34,7 @@ struct S390Topology {
>      int tles;
>      S390TopoContainer *socket;
>      S390TopoTLE *tle;
> +    QemuMutex topo_mutex;
>  };
>  typedef struct S390Topology S390Topology;
>  
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 7d6d01325b..c61fe9b563 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -565,6 +565,53 @@ 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;
> +} SysIBTl_cpu;
> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16);
> +
> +/* Container type Topology List Entry */
> +typedef struct SysIBTl_container {
> +        uint8_t nl;
> +        uint8_t reserved[6];
> +        uint8_t id;
> +} QEMU_PACKED SysIBTl_container;
> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_container) != 8);
> +
> +/* Generic Topology List Entry */
> +typedef union SysIBTl_entry {
> +        uint8_t nl;
> +        SysIBTl_container container;
> +        SysIBTl_cpu cpu;
> +} SysIBTl_entry;

This isn't used for anything but the declaration in SysIB_151x, is it?
> +
> +#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  reserved0[2];
> +    uint16_t length;
> +    uint8_t  mag[TOPOLOGY_NR_MAG];
> +    uint8_t  reserved1;
> +    uint8_t  mnest;
> +    uint32_t reserved2;
> +    SysIBTl_entry tle[0];

I would just use uint64_t[0] as type or uint64_t[] whichever is qemu
style.

> +} SysIB_151x;
> +QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
> +
>  /* MMU defines */
>  #define ASCE_ORIGIN           (~0xfffULL) /* segment table origin             */
>  #define ASCE_SUBSPACE         0x200       /* subspace group control           */
> @@ -843,4 +890,6 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
>  
>  #include "exec/cpu-all.h"
>  
> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar);
> +
>  #endif
> diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
> new file mode 100644
> index 0000000000..56865dafc6
> --- /dev/null
> +++ b/target/s390x/cpu_topology.c
> @@ -0,0 +1,108 @@
> +/*
> + * QEMU S390x CPU Topology
> + *
> + * Copyright IBM Corp. 2022
> + * 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 "cpu.h"
> +#include "hw/s390x/pv.h"
> +#include "hw/sysbus.h"
> +#include "hw/s390x/cpu-topology.h"
> +#include "hw/s390x/sclp.h"
> +
> +static char *fill_container(char *p, int level, int id)
> +{
> +    SysIBTl_container *tle = (SysIBTl_container *)p;
> +
> +    tle->nl = level;
> +    tle->id = id;
> +    return p + sizeof(*tle);
> +}
> +
> +static char *fill_tle_cpu(char *p, uint64_t mask, int origin)
> +{
> +    SysIBTl_cpu *tle = (SysIBTl_cpu *)p;
> +
> +    tle->nl = 0;
> +    tle->dedicated = 1;
> +    tle->polarity = S390_TOPOLOGY_POLARITY_H;
> +    tle->type = S390_TOPOLOGY_CPU_TYPE;
> +    tle->origin = origin * 64;
> +    tle->mask = be64_to_cpu(mask);

You convert endianess for mask here...

> +    return p + sizeof(*tle);
> +}
> +
> +static char *s390_top_set_level2(S390Topology *topo, char *p)
> +{
> +    int i, origin;
> +
> +    for (i = 0; i < topo->sockets; i++) {
> +        if (!topo->socket[i].active_count) {
> +            continue;
> +        }
> +        p = fill_container(p, 1, i);
> +        for (origin = 0; origin < S390_TOPOLOGY_MAX_ORIGIN; origin++) {
> +            uint64_t mask = 0L;
> +
> +            mask = be64_to_cpu(topo->tle[i].mask[origin]);

...and here. So one has to go, I guess this one.
Also using cpu_to_be64 seems more intuitive to me.

> +            if (mask) {
> +                p = fill_tle_cpu(p, mask, origin);
> +            }
> +        }
> +    }
> +    return p;
> +}
> +
> +static int setup_stsi(SysIB_151x *sysib, int level)
> +{
> +    S390Topology *topo = s390_get_topology();
> +    char *p = (char *)sysib->tle;
> +
> +    qemu_mutex_lock(&topo->topo_mutex);
> +
> +    sysib->mnest = level;
> +    switch (level) {
> +    case 2:
> +        sysib->mag[TOPOLOGY_NR_MAG2] = topo->sockets;
> +        sysib->mag[TOPOLOGY_NR_MAG1] = topo->cores;
> +        p = s390_top_set_level2(topo, p);
> +        break;
> +    }
> +
> +    qemu_mutex_unlock(&topo->topo_mutex);
> +
> +    return p - (char *)sysib->tle;
> +}
> +
> +#define S390_TOPOLOGY_MAX_MNEST 2
> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
> +{
> +    SysIB_151x *sysib;
> +    int len = sizeof(*sysib);
> +
> +    if (s390_is_pv() || sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
> +        setcc(cpu, 3);
> +        return;
> +    }
> +
> +    sysib = g_malloc0(TARGET_PAGE_SIZE);

What made you decide against stack allocating this?

> +
> +    len += setup_stsi(sysib, sel2);
> +    if (len > TARGET_PAGE_SIZE) {

If you do the check here it's too late.

> +        setcc(cpu, 3);
> +        goto out_free;
> +    }
> +
> +    sysib->length = be16_to_cpu(len);
> +    s390_cpu_virt_mem_write(cpu, addr, ar, sysib, len);

If the return value of this is <0 it's an error condition.
If you ignore the value we'll keep running.

> +    setcc(cpu, 0);

Is it correct to set the cc value even if s390_cpu_virt_mem_write
causes an exception?
> +
> +out_free:
> +    g_free(sysib);
> +}
> +
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 6a8dbadf7e..f96630440b 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -51,6 +51,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
> @@ -1917,9 +1918,12 @@ static int handle_stsi(S390CPU *cpu)
>          if (run->s390_stsi.sel1 != 2 || run->s390_stsi.sel2 != 2) {
>              return 0;
>          }
> -        /* 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;
>      }
> diff --git a/target/s390x/meson.build b/target/s390x/meson.build
> index 84c1402a6a..890ccfa789 100644
> --- a/target/s390x/meson.build
> +++ b/target/s390x/meson.build
> @@ -29,6 +29,7 @@ s390x_softmmu_ss.add(files(
>    'sigp.c',
>    'cpu-sysemu.c',
>    'cpu_models_sysemu.c',
> +  'cpu_topology.c',
>  ))
>  
>  s390x_user_ss = ss.source_set()

Re: [PATCH v9 03/10] s390x/cpu topology: reporting the CPU topology to the guest

[Janis Schoetterl-Glausch]

On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
> The guest can use the STSI instruction to get a buffer filled
> with the CPU topology description.
> 
> Let us implement the STSI instruction for the basis CPU topology
> level, level 2.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  hw/s390x/cpu-topology.c         |   4 ++
>  include/hw/s390x/cpu-topology.h |   5 ++
>  target/s390x/cpu.h              |  49 +++++++++++++++
>  target/s390x/cpu_topology.c     | 108 ++++++++++++++++++++++++++++++++
>  target/s390x/kvm/kvm.c          |   6 +-
>  target/s390x/meson.build        |   1 +
>  6 files changed, 172 insertions(+), 1 deletion(-)
>  create mode 100644 target/s390x/cpu_topology.c
> 
[...]

> diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c

[...]

> +static char *fill_tle_cpu(char *p, uint64_t mask, int origin)
> +{
> +    SysIBTl_cpu *tle = (SysIBTl_cpu *)p;
> +
> +    tle->nl = 0;
> +    tle->dedicated = 1;
> +    tle->polarity = S390_TOPOLOGY_POLARITY_H;
> +    tle->type = S390_TOPOLOGY_CPU_TYPE;
> +    tle->origin = origin * 64;

origin is a multibyte field too, so needs a conversion too.

> +    tle->mask = be64_to_cpu(mask);
> +    return p + sizeof(*tle);
> +}
> +
[...]

Re: [PATCH v9 05/10] s390x/cpu: reporting drawers and books topology to the guest

[Janis Schoetterl-Glausch]

On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
> The guest can ask for a topology report on drawer's or book's
> level.
> Let's implement the STSI instruction's handling for the corresponding
> selector values.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  hw/s390x/cpu-topology.c         | 19 +++++++---
>  hw/s390x/s390-virtio-ccw.c      |  2 ++
>  include/hw/s390x/cpu-topology.h |  7 +++-
>  target/s390x/cpu_topology.c     | 64 +++++++++++++++++++++++++++------
>  4 files changed, 76 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> index e2fd5c7e44..bb9ae63483 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> 
[...]

> @@ -99,13 +103,20 @@ static void s390_topology_realize(DeviceState *dev, Error **errp)
>      S390Topology *topo = S390_CPU_TOPOLOGY(dev);
>      int n;
>  
> +    topo->drawers = ms->smp.drawers;
> +    topo->books = ms->smp.books;
> +    topo->total_books = topo->books * topo->drawers;
>      topo->sockets = ms->smp.sockets;
> +    topo->total_sockets = topo->sockets * topo->books * topo->drawers;
>      topo->cores = ms->smp.cores;
> -    topo->tles = ms->smp.max_cpus;
>  
> -    n = topo->sockets;
> +    n = topo->drawers;
> +    topo->drawer = g_malloc0(n * sizeof(S390TopoContainer));
> +    n *= topo->books;
> +    topo->book = g_malloc0(n * sizeof(S390TopoContainer));
> +    n *= topo->sockets;
>      topo->socket = g_malloc0(n * sizeof(S390TopoContainer));
> -    topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE));
> +    topo->tle = g_malloc0(n * sizeof(S390TopoTLE));

Same question here about using g_new0.
>  
>      qemu_mutex_init(&topo->topo_mutex);
>  }
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 15cefd104b..3f28e28d47 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -626,6 +626,8 @@ 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_props.books_supported = true;
> +    mc->smp_props.drawers_supported = true;
>  }
>  
>  static inline bool machine_get_aes_key_wrap(Object *obj, Error **errp)
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> index 0b7f3d10b2..4f8ac39ca0 100644
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -29,9 +29,14 @@ typedef struct S390TopoTLE {
>  
>  struct S390Topology {
>      SysBusDevice parent_obj;
> +    int total_books;
> +    int total_sockets;

What are these used for? I'm not seeing anything.

> +    int drawers;
> +    int books;
>      int sockets;
>      int cores;
> -    int tles;

You remove this in this patch and you didn't really need it before.
As far as I can tell it was just used for calculating the number of
tles to allocate and you could use a local variable instead.
So I would get rid of it in the patch that introduced it.

> +    S390TopoContainer *drawer;
> +    S390TopoContainer *book;
>      S390TopoContainer *socket;
>      S390TopoTLE *tle;
>      QemuMutex topo_mutex;
> diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
> index 56865dafc6..305fbb9734 100644
> --- a/target/s390x/cpu_topology.c
> +++ b/target/s390x/cpu_topology.c
> @@ -37,19 +37,18 @@ static char *fill_tle_cpu(char *p, uint64_t mask, int origin)
>      return p + sizeof(*tle);
>  }
>  
> -static char *s390_top_set_level2(S390Topology *topo, char *p)
> +static char *s390_top_set_level2(S390Topology *topo, char *p, int fs, int ns)
>  {

I wouldn't hate more verbose names for fs and ns. start_socket,
num_socket maybe? Same for fb, nb, but it is your call, it's not really
hard to understand the code.

> -    int i, origin;
> +    int socket, origin;
> +    uint64_t mask;
>  
> -    for (i = 0; i < topo->sockets; i++) {
> -        if (!topo->socket[i].active_count) {
> +    for (socket = fs; socket < fs + ns; socket++) {
> +        if (!topo->socket[socket].active_count) {
>              continue;
>          }
> -        p = fill_container(p, 1, i);
> +        p = fill_container(p, 1, socket);

Have you considered using an enum for the level constants?

>          for (origin = 0; origin < S390_TOPOLOGY_MAX_ORIGIN; origin++) {
> -            uint64_t mask = 0L;
> -
> -            mask = be64_to_cpu(topo->tle[i].mask[origin]);
> +            mask = be64_to_cpu(topo->tle[socket].mask[origin]);
>              if (mask) {
>                  p = fill_tle_cpu(p, mask, origin);
>              }
> @@ -58,19 +57,63 @@ static char *s390_top_set_level2(S390Topology *topo, char *p)
>      return p;
>  }
>  
> +static char *s390_top_set_level3(S390Topology *topo, char *p, int fb, int nb)
> +{
> +    int book, fs = 0;
> +
> +    for (book = fb; book < fb + nb; book++, fs += topo->sockets) {
> +        if (!topo->book[book].active_count) {
> +            continue;
> +        }
> +        p = fill_container(p, 2, book);
> +    p = s390_top_set_level2(topo, p, fs, topo->sockets);

Indent is off.

> +    }
> +    return p;
> +}
> +
> +static char *s390_top_set_level4(S390Topology *topo, char *p)
> +{
> +    int drawer, fb = 0;
> +
> +    for (drawer = 0; drawer < topo->drawers; drawer++, fb += topo->books) {
> +        if (!topo->drawer[drawer].active_count) {
> +            continue;
> +        }
> +        p = fill_container(p, 3, drawer);
> +        p = s390_top_set_level3(topo, p, fb, topo->books);
> +    }
> +    return p;
> +}
> +
>  static int setup_stsi(SysIB_151x *sysib, int level)
>  {
>      S390Topology *topo = s390_get_topology();
>      char *p = (char *)sysib->tle;
> +    int max_containers;
>  
>      qemu_mutex_lock(&topo->topo_mutex);
>  
>      sysib->mnest = level;
>      switch (level) {
>      case 2:
> +        max_containers = topo->sockets * topo->books * topo->drawers;
> +        sysib->mag[TOPOLOGY_NR_MAG2] = max_containers;
> +        sysib->mag[TOPOLOGY_NR_MAG1] = topo->cores;
> +        p = s390_top_set_level2(topo, p, 0, max_containers);

Isn't this logic change already required for the patch that introduced
stsi 15.1.2 handling?

> +        break;
> +    case 3:
> +        max_containers = topo->books * topo->drawers;
> +        sysib->mag[TOPOLOGY_NR_MAG3] = max_containers;
>          sysib->mag[TOPOLOGY_NR_MAG2] = topo->sockets;
>          sysib->mag[TOPOLOGY_NR_MAG1] = topo->cores;
> -        p = s390_top_set_level2(topo, p);
> +        p = s390_top_set_level3(topo, p, 0, max_containers);
> +        break;
> +    case 4:
> +        sysib->mag[TOPOLOGY_NR_MAG4] = topo->drawers;
> +        sysib->mag[TOPOLOGY_NR_MAG3] = topo->books;
> +        sysib->mag[TOPOLOGY_NR_MAG2] = topo->sockets;
> +        sysib->mag[TOPOLOGY_NR_MAG1] = topo->cores;
> +        p = s390_top_set_level4(topo, p);
>          break;
>      }
>  
> @@ -79,7 +122,7 @@ static int setup_stsi(SysIB_151x *sysib, int level)
>      return p - (char *)sysib->tle;
>  }
>  
> -#define S390_TOPOLOGY_MAX_MNEST 2
> +#define S390_TOPOLOGY_MAX_MNEST 4

AFAIK you're only allowed to increase this if the maximum mnest
facility is installed. If it isn't, only level 2 is supported.
Which would mean that this patch doesn't do anything.

>  void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
>  {
>      SysIB_151x *sysib;
> @@ -105,4 +148,3 @@ void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
>  out_free:
>      g_free(sysib);
>  }
> -

Re: [PATCH v9 06/10] s390x/cpu_topology: resetting the Topology-Change-Report

[Janis Schoetterl-Glausch]

On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
> During a subsystem reset the Topology-Change-Report is cleared
> by the machine.
> Let's ask KVM to clear the Modified Topology Change Report (MTCR)
>  bit of the SCA in the case of a subsystem reset.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  hw/s390x/cpu-topology.c      | 12 ++++++++++++
>  hw/s390x/s390-virtio-ccw.c   |  1 +
>  target/s390x/cpu-sysemu.c    |  7 +++++++
>  target/s390x/cpu.h           |  1 +
>  target/s390x/kvm/kvm.c       | 23 +++++++++++++++++++++++
>  target/s390x/kvm/kvm_s390x.h |  1 +
>  6 files changed, 45 insertions(+)

[...]

> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index f96630440b..9c994d27d5 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -2585,3 +2585,26 @@ int kvm_s390_get_zpci_op(void)
>  {
>      return cap_zpci_op;
>  }
> +
> +int kvm_s390_topology_set_mtcr(uint64_t attr)
> +{
> +    struct kvm_device_attr attribute = {
> +        .group = KVM_S390_VM_CPU_TOPOLOGY,
> +        .attr  = attr,
> +    };
> +    int ret;
> +
> +    if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
> +        return -EFAULT;

Why EFAULT?
The return value is just ignored when resetting, isn't it?
I wonder if it would be better not to.
Is it necessary because you're detecting the feature after you've
already created the S390Topology instance?
And you're doing that because that's just the order in which QEMU does
things? So the machine class is inited before the cpu model?
I wonder if there is a nice way to create the S390Topology only if the
feature is selected.

Anyway:
Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

> +    }
> +    if (!kvm_vm_check_attr(kvm_state, KVM_S390_VM_CPU_TOPOLOGY, attr)) {
> +        return -ENOENT;
> +    }
> +
> +    ret = kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attribute);
> +    if (ret) {
> +        error_report("Failed to set cpu topology attribute %lu: %s",
> +                     attr, strerror(-ret));
> +    }
> +    return ret;
> +}
> 
[...]

Re: [PATCH v9 07/10] s390x/cpu_topology: CPU topology migration

[Janis Schoetterl-Glausch]

On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
> The migration can only take place if both source and destination
> of the migration both use or both do not use the CPU topology
> facility.
> 
> We indicate a change in topology during migration postload for the
> case the topology changed between source and destination.

You always set the report bit after migration, right?
In the last series you actually migrated the bit.
Why the change? With the code you have actually migrating the bit isn't
hard.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  hw/s390x/cpu-topology.c         | 79 +++++++++++++++++++++++++++++++++
>  include/hw/s390x/cpu-topology.h |  1 +
>  target/s390x/cpu-sysemu.c       |  8 ++++
>  target/s390x/cpu.h              |  1 +
>  4 files changed, 89 insertions(+)
> 
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> index 6098d6ea1f..b6bf839e40 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -19,6 +19,7 @@
>  #include "target/s390x/cpu.h"
>  #include "hw/s390x/s390-virtio-ccw.h"
>  #include "hw/s390x/cpu-topology.h"
> +#include "migration/vmstate.h"
>  
>  S390Topology *s390_get_topology(void)
>  {
> @@ -132,6 +133,83 @@ static void s390_topology_reset(DeviceState *dev)
>      s390_cpu_topology_reset();
>  }
>  
> +/**
> + * cpu_topology_postload
> + * @opaque: a pointer to the S390Topology
> + * @version_id: version identifier
> + *
> + * We check that the topology is used or is not used
> + * on both side identically.
> + *
> + * If the topology is in use we set the Modified Topology Change Report
> + * on the destination host.
> + */
> +static int cpu_topology_postload(void *opaque, int version_id)
> +{
> +    S390Topology *topo = opaque;
> +    int ret;
> +
> +    if (topo->topology_needed != s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {

Does this function even run if topology_needed is false?
In that case there is no data saved, so no reason to load it either.
If so you can only check that both the source and the destination have
the feature enabled. You would need to always send the topology VMSD in
order to check that the feature is disabled.

Does qemu allow you to attempt to migrate to a host with another cpu
model?
If it disallowes that you wouldn't need to do any checks, right?

> +        if (topo->topology_needed) {
> +            error_report("Topology facility is needed in destination");
> +        } else {
> +            error_report("Topology facility can not be used in destination");
> +        }
> +        return -EINVAL;
> +    }
> +
> +    /* We do not support CPU Topology, all is good */
> +    if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
> +        return 0;
> +    }
> +
> +    /* We support CPU Topology, set the MTCR */
> +    ret = s390_cpu_topology_mtcr_set();
> +    if (ret) {
> +        error_report("Failed to set MTCR: %s", strerror(-ret));
> +    }
> +    return ret;
> +}
> +
> +/**
> + * cpu_topology_presave:
> + * @opaque: The pointer to the S390Topology
> + *
> + * Save the usage of the CPU Topology in the VM State.
> + */
> +static int cpu_topology_presave(void *opaque)
> +{
> +    S390Topology *topo = opaque;
> +
> +    topo->topology_needed = s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY);
> +    return 0;
> +}
> +
> +/**
> + * cpu_topology_needed:
> + * @opaque: The pointer to the S390Topology
> + *
> + * If we use the CPU Topology on the source it will be needed on the destination.
> + */
> +static bool cpu_topology_needed(void *opaque)
> +{
> +    return s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY);
> +}
> +
> +
> +const VMStateDescription vmstate_cpu_topology = {
> +    .name = "cpu_topology",
> +    .version_id = 1,
> +    .post_load = cpu_topology_postload,
> +    .pre_save = cpu_topology_presave,
> +    .minimum_version_id = 1,
> +    .needed = cpu_topology_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BOOL(topology_needed, S390Topology),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
[...]

Re: [PATCH v9 08/10] target/s390x: interception of PTF instruction

[Janis Schoetterl-Glausch]

On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
> When the host supports the CPU topology facility, the PTF
> instruction with function code 2 is interpreted by the SIE,
> provided that the userland hypervizor activates the interpretation
> by using the KVM_CAP_S390_CPU_TOPOLOGY KVM extension.
> 
> The PTF instructions with function code 0 and 1 are intercepted
> and must be emulated by the userland hypervizor.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

See note below.
> ---
>  hw/s390x/cpu-topology.c            | 52 ++++++++++++++++++++++++++++++
>  include/hw/s390x/s390-virtio-ccw.h |  6 ++++
>  target/s390x/kvm/kvm.c             | 13 ++++++++
>  3 files changed, 71 insertions(+)
> 
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> index b6bf839e40..7dcaa28ca3 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -20,6 +20,58 @@
>  #include "hw/s390x/s390-virtio-ccw.h"
>  #include "hw/s390x/cpu-topology.h"
>  #include "migration/vmstate.h"
> +#include "target/s390x/cpu.h"
> +#include "hw/s390x/s390-virtio-ccw.h"
> +
> +/*
> + * s390_handle_ptf:
> + *
> + * @register 1: contains the function code
> + *
> + * Function codes 0 and 1 handle the CPU polarization.
> + * We assume an horizontal topology, the only one supported currently
> + * by Linux, consequently we answer to function code 0, requesting
> + * horizontal polarization that it is already the current polarization
> + * and reject vertical polarization request without further explanation.
> + *
> + * Function code 2 is handling topology changes and is interpreted
> + * by the SIE.
> + */
> +void 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_OPERATION, ra);
> +        return;

I'm either expecting this function to return -1 here...
> +    }
> +
> +    if (env->psw.mask & PSW_MASK_PSTATE) {
> +        s390_program_interrupt(env, PGM_PRIVILEGED, ra);
> +        return;
> +    }
> +
> +    if (reg & ~S390_TOPO_FC_MASK) {
> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> +        return;
> +    }
> +
> +    switch (fc) {
> +    case 0:    /* Horizontal polarization is already set */
> +        env->regs[r1] |= S390_PTF_REASON_DONE;
> +        setcc(cpu, 2);
> +        break;
> +    case 1:    /* Vertical polarization is not supported */
> +        env->regs[r1] |= S390_PTF_REASON_NONE;
> +        setcc(cpu, 2);
> +        break;
> +    default:
> +        /* Note that fc == 2 is interpreted by the SIE */
> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> +    }
> +}

[...]
>  
> +static int kvm_handle_ptf(S390CPU *cpu, struct kvm_run *run)
> +{
> +    uint8_t r1 = (run->s390_sieic.ipb >> 20) & 0x0f;
> +
> +    s390_handle_ptf(cpu, r1, RA_IGNORED);

... and this being returned here...
> +
> +    return 0;

... or this function being void.
> +}
> +
>  static int handle_b9(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
>  {
>      int r = 0;
> @@ -1480,6 +1490,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;

Re: [PATCH v9 10/10] docs/s390x: document s390x cpu topology

[Janis Schoetterl-Glausch]

On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
> Add some basic examples for the definition of cpu topology
> in s390x.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  docs/system/s390x/cpu_topology.rst | 88 ++++++++++++++++++++++++++++++
>  1 file changed, 88 insertions(+)
>  create mode 100644 docs/system/s390x/cpu_topology.rst
> 
> diff --git a/docs/system/s390x/cpu_topology.rst b/docs/system/s390x/cpu_topology.rst
> new file mode 100644
> index 0000000000..00977d4319
> --- /dev/null
> +++ b/docs/system/s390x/cpu_topology.rst
> @@ -0,0 +1,88 @@
> +CPU Topology on s390x
> +=====================
> +
> +CPU Topology on S390x provides up to 4 levels of topology containers:
> +drawers, books, sockets and CPUs.
> +While the three higher level containers, Containers Topology List Entries,
> +(Containers TLE) define a tree hierarchy, the lowest level of topology
> +definition, the CPU Topology List Entry (CPU TLE), provides the placement
> +of the CPUs inside the last container.

inside the parent container
> +
> +Prerequisites
> +-------------
> +
> +To use CPU Topology a Linux QEMU/KVM machine providing the CPU Topology facility
> +(STFLE bit 11) is required.
> +
> +However, since this facility has been enabled by default in an early version,
> +the capability ``KVM_CAP_S390_CPU_TOPOLOGY`` is needed to indicate to KVM
> +that QEMU support CPU Topology.

I don't understand this paragraph. Early version of what?
> +
> +Indicating the CPU topology to the Virtual Machine
> +--------------------------------------------------
> +
> +The CPU Topology, number of drawers, number of books per drawers, number of
> +sockets per book and number of cores per sockets is specified with the
> +``-smp`` qemu command arguments.
> +
> +Like in :
> +
> +.. code-block:: sh
> +    -smp cpus=1,drawers=3,books=4,sockets=2,cores=8,maxcpus=192
> +
> +If drawers or books are not specified, their default to 1.
> +
> +New CPUs can be plugged using the device_add hmp command like in:
> +
> +.. code-block:: sh
> +   (qemu) device_add host-s390x-cpu,core-id=9
> +
> +The core-id defines the placement of the core in the topology by
> +starting with core 0 in socket 0, book 0 and drawer 0 up to the maximum
> +core number of the last socket of the last book in the last drawer.
> +
> +In the example above:
> +
> +* the core with ID 9 will be placed in container (0,0,1), as core 9
> +  of CPU TLE 0 of socket 1 in book 0 from drawer 0.
> +* the core ID 0 is defined by the -smp cpus=1 command and will be
> +  placed as core 0 in CPU TLE 0 of container (0,0,0)
> +
> +Note that the core ID is machine wide and the CPU TLE masks provided
> +by the STSI instruction will be:
> +
> +* in socket 0: 0x80000000 (core id 0)
> +* in socket 1: 0x00400000 (core id 9)
> +
> +Indicating the CPU topology to the Guest
> +----------------------------------------
> +
> +The guest can query for topology changes using the PTF instruction.
> +In case of a topology change it can request the new topology by issuing
> +STSI instructions specifying the level of detail required, drawer with
> +STSI(15.1.4) or books STSI(15.1.3).
> +
> +The virtual machine will fill the provided buffer with the count of
> +drawers (MAG4), books per drawer (MAG3), sockets per book (MAG2) and
> +cores per socket (MAG1).
> +
> +Note that the STSI(15.1.2) is special in two ways:
> +
> +* When the firmware detect a change in the values calculated for STSI(15.1.2)
> +  it will trigger the report of the topology change for the PTF instruction.

I don't know if we need this section, after all documenting this is the
job of the principles of operation. You could just refer to the
relevant sections.
> +
> +Migration
> +---------
> +
> +For virtio-ccw machines older than s390-virtio-ccw-7.2, CPU Topoogy is
> +by default disabled.
> +
> +CPU Topoogy is by default enabled for s390-virtio-ccw-7.2 and newer machines.
> +
> +Enabling the CPU topology on older Machine is done by setting the global
> +option ''topology-disable'' to false before enabling cpu topology with the
> +cpu feature "ctop" like in:
> +
> +.. code-block:: sh
> +   -machine s390-ccw-virtio-3.0,accel=kvm,topology-disable=false
> +   -cpu z14,ctop=on

Re: [PATCH v9 10/10] docs/s390x: document s390x cpu topology

[Janis Schoetterl-Glausch]

On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
> Add some basic examples for the definition of cpu topology
> in s390x.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  docs/system/s390x/cpu_topology.rst | 88 ++++++++++++++++++++++++++++++
>  1 file changed, 88 insertions(+)
>  create mode 100644 docs/system/s390x/cpu_topology.rst
> 
> diff --git a/docs/system/s390x/cpu_topology.rst b/docs/system/s390x/cpu_topology.rst
> new file mode 100644
> index 0000000000..00977d4319
> --- /dev/null
> +++ b/docs/system/s390x/cpu_topology.rst
> @@ -0,0 +1,88 @@

[...]

> +Indicating the CPU topology to the Virtual Machine
> +--------------------------------------------------
> +
> +The CPU Topology, number of drawers, number of books per drawers, number of
> +sockets per book and number of cores per sockets is specified with the
> +``-smp`` qemu command arguments.
> +
> +Like in :
> +
> +.. code-block:: sh
> +    -smp cpus=1,drawers=3,books=4,sockets=2,cores=8,maxcpus=192
> +
> +If drawers or books are not specified, their default to 1.

Forgot this:
s/their default/they default/


[...]

Re: [PATCH v9 00/10] s390x: CPU Topology

[Janis Schoetterl-Glausch]

I found this version much easier to understand than the previous one.

You could consider splitting up the series into two.
One that introduces support for STSI, PTF, migration, etc.
And a second one that adds support for the maximum-MNist facility and
drawers and books.

This would also make bisecting a bit nicer because it moves the feature
enablement closer to the commits adding the support.

Right now, with this series, the topology is static and cannot change.
Most of the value of making the topology visible to the guest is for it
to mirror reality, and a static topology is a hindrance for that.
I'm completely fine with having a static topology as a stepping stone
to a dynamic one.
However I think we should have a rough plan or maybe even a prototype
for how we turn a static into a dynamic topology before we merge this
series in order to avoid designing us into a corner.
What do you think?

On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
> Hi,
> 
> The implementation of the CPU Topology in QEMU has been drastically
> modified since the last patch series and the number of LOCs has been
> greatly reduced.
> 
> Unnecessary objects have been removed, only a single S390Topology object
> is created to support migration and reset.
> 
> Also a documentation has been added to the series.
> 
> 
> To use these patches, you will need Linux V6-rc1 or newer.
> 
> Mainline patches needed are:
> 
> f5ecfee94493 2022-07-20 KVM: s390: resetting the Topology-Change-Report    
> 24fe0195bc19 2022-07-20 KVM: s390: guest support for topology function     
> 0130337ec45b 2022-07-20 KVM: s390: Cleanup ipte lock access and SIIF fac.. 
> 
> Currently this code is for KVM only, I have no idea if it is interesting
> to provide a TCG patch. If ever it will be done in another series.
> 
> To have a better understanding of the S390x CPU Topology and its
> implementation in QEMU you can have a look at the documentation in the
> last patch.
> 
> New in this series
> ==================
> 
>   s390x/cpus: Make absence of multithreading clear
> 
> This patch makes clear that CPU-multithreading is not supported in
> the guest.
> 
>   s390x/cpu topology: core_id sets s390x CPU topology
> 
> This patch uses the core_id to build the container topology
> and the placement of the CPU inside the container.
> 
>   s390x/cpu topology: reporting the CPU topology to the guest
> 
> This patch is based on the fact that the CPU type for guests
> is always IFL, CPUs are always dedicated and the polarity is
> always horizontal.
> This may change in the future.
> 
>   hw/core: introducing drawer and books for s390x
>   s390x/cpu: reporting drawers and books topology to the guest
> 
> These two patches extend the topology handling to add two
> new containers levels above sockets: books and drawers.
> 
> The subject of the last patches is clear enough (I hope).
> 
> Regards,
> Pierre
> 
> Pierre Morel (10):
>   s390x/cpus: Make absence of multithreading clear
>   s390x/cpu topology: core_id sets s390x CPU topology
>   s390x/cpu topology: reporting the CPU topology to the guest
>   hw/core: introducing drawer and books for s390x
>   s390x/cpu: reporting drawers and books topology to the guest
>   s390x/cpu_topology: resetting the Topology-Change-Report
>   s390x/cpu_topology: CPU topology migration
>   target/s390x: interception of PTF instruction
>   s390x/cpu_topology: activating CPU topology
>   docs/s390x: document s390x cpu topology
> 
>  docs/system/s390x/cpu_topology.rst |  88 +++++++++
>  hw/core/machine-smp.c              |  48 ++++-
>  hw/core/machine.c                  |   9 +
>  hw/s390x/cpu-topology.c            | 293 +++++++++++++++++++++++++++++
>  hw/s390x/meson.build               |   1 +
>  hw/s390x/s390-virtio-ccw.c         |  61 +++++-
>  include/hw/boards.h                |  11 ++
>  include/hw/s390x/cpu-topology.h    |  53 ++++++
>  include/hw/s390x/s390-virtio-ccw.h |   7 +
>  qapi/machine.json                  |  14 +-
>  qemu-options.hx                    |   6 +-
>  softmmu/vl.c                       |   6 +
>  target/s390x/cpu-sysemu.c          |  15 ++
>  target/s390x/cpu.h                 |  51 +++++
>  target/s390x/cpu_topology.c        | 150 +++++++++++++++
>  target/s390x/kvm/kvm.c             |  56 +++++-
>  target/s390x/kvm/kvm_s390x.h       |   1 +
>  target/s390x/meson.build           |   1 +
>  18 files changed, 858 insertions(+), 13 deletions(-)
>  create mode 100644 docs/system/s390x/cpu_topology.rst
>  create mode 100644 hw/s390x/cpu-topology.c
>  create mode 100644 include/hw/s390x/cpu-topology.h
>  create mode 100644 target/s390x/cpu_topology.c
> 

Re: [PATCH v9 02/10] s390x/cpu topology: core_id sets s390x CPU topology

[Pierre Morel]

On 9/5/22 20:11, Janis Schoetterl-Glausch wrote:
> On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
>> In the S390x CPU topology the core_id specifies the CPU address
>> and the position of the core withing the topology.
>>
>> Let's build the topology based on the core_id.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/cpu-topology.c         | 135 ++++++++++++++++++++++++++++++++
>>   hw/s390x/meson.build            |   1 +
>>   hw/s390x/s390-virtio-ccw.c      |  10 +++
>>   include/hw/s390x/cpu-topology.h |  42 ++++++++++
>>   4 files changed, 188 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
>>
> [...]
> 
>> +/**
>> + * s390_topology_realize:
>> + * @dev: the device state
>> + * @errp: the error pointer (not used)
>> + *
>> + * During realize the machine CPU topology is initialized with the
>> + * QEMU -smp parameters.
>> + * The maximum count of CPU TLE in the all Topology can not be greater
>> + * than the maximum CPUs.
>> + */
>> +static void s390_topology_realize(DeviceState *dev, Error **errp)
>> +{
>> +    MachineState *ms = MACHINE(qdev_get_machine());
>> +    S390Topology *topo = S390_CPU_TOPOLOGY(dev);
>> +    int n;
>> +
>> +    topo->sockets = ms->smp.sockets;
>> +    topo->cores = ms->smp.cores;
>> +    topo->tles = ms->smp.max_cpus;
>> +
>> +    n = topo->sockets;
>> +    topo->socket = g_malloc0(n * sizeof(S390TopoContainer));
>> +    topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE));
> 
> Seems like a good use case for g_new0.

Yes


> 
> [...]
>>
>> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
>> new file mode 100644
>> index 0000000000..6911f975f4
>> --- /dev/null
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -0,0 +1,42 @@
>> +/*
>> + * CPU Topology
>> + *
>> + * Copyright 2022 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
> 
> Is there a reason this is before the includes?
>> +
>> +typedef struct S390TopoContainer {
>> +    int active_count;
>> +} S390TopoContainer;
>> +
>> +#define S390_TOPOLOGY_MAX_ORIGIN (1 + S390_MAX_CPUS / 64)
> 
> This is correct because cpu_id == core_id for s390, right?
> So the cpu limit also applies to the core id.
> You could do ((S390_MAX_CPUS + 63) / 64) instead.
> But if you chose this for simplicity's sake, I'm fine with it.

hum, I think your proposition is right and mine is false.
If S390_MAX_CPUS is 64 we have only 1 origin.
I count 2 but you count 1.

So I change this.

> 
>> +typedef struct S390TopoTLE {
>> +    int active_count;
> 
> Do you use (read) this field somewhere?
> Is this in anticipation of there being multiple TLE arrays, for
> different polarizations, etc? If so I would defer this for later.

OK

> 
>> +    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
>> +} S390TopoTLE;
>> +
>> +#include "hw/qdev-core.h"
>> +#include "qom/object.h"
>> +
>> +struct S390Topology {
>> +    SysBusDevice parent_obj;
>> +    int sockets;
>> +    int cores;
> 
> These are just cached values from machine_state.smp, right?
> Not sure if I like the redundancy, it doesn't aid in comprehension.
> 
>> +    int tles;
>> +    S390TopoContainer *socket;
>> +    S390TopoTLE *tle;
>> +};
>> +typedef struct S390Topology S390Topology;
> 
> The DECLARE macro takes care of this typedef.

right

> 
>> +
>> +#define TYPE_S390_CPU_TOPOLOGY "s390-topology"
>> +OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY)
>> +
>> +S390Topology *s390_get_topology(void);
>> +void s390_topology_new_cpu(int core_id);
>> +
>> +#endif
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 02/10] s390x/cpu topology: core_id sets s390x CPU topology

[Pierre Morel]

On 9/6/22 07:58, Nico Boehr wrote:
> Quoting Pierre Morel (2022-09-02 09:55:23)
> [...]
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> new file mode 100644
>> index 0000000000..a6ca006ec5
>> --- /dev/null
>> +++ b/hw/s390x/cpu-topology.c
> [...]
>> +void s390_topology_new_cpu(int core_id)
>> +{
> [...]
>> +    socket_id = core_id / topo->cores;
> 
> The comment below is essential for understanding all of this. Move it before this line.
> 

OK

>> +
>> +    bit = core_id;
>> +    origin = bit / 64;
>> +    bit %= 64;
>> +    bit = 63 - bit;
>> +
>> +    /*
>> +     * At the core level, each CPU is represented by a bit in a 64bit
>> +     * unsigned long. Set on plug and clear on unplug of a CPU.
> 
> cleared                                  ^
> 
> [...]
>> +     * In that case the origin field, representing the offset of the first CPU
>> +     * in the CPU container allows to represent up to the maximal number of
>> +     * CPU inside several CPU containers inside the socket container.
> 
> How about:
> "In that case the origin variable represents the offset of the first CPU in the
> CPU container. More than 64 CPUs per socket are represented in several CPU
> containers inside the socket container."

Yes, better, thanks I take it


> 
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index b5ca154e2f..15cefd104b 100644
> [...]
>> @@ -247,6 +248,12 @@ static void ccw_init(MachineState *machine)
>>       /* init memory + setup max page size. Required for the CPU model */
>>       s390_memory_init(machine->ram);
>>   
>> +    /* Adding the topology must be done before CPU intialization*/
> 
> space                                                              ^
> 

Yes thanks

regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 01/10] s390x/cpus: Make absence of multithreading clear

[Cédric Le Goater]

On 9/5/22 17:10, Pierre Morel wrote:
> 
> 
> On 9/5/22 13:32, Nico Boehr wrote:
>> Quoting Pierre Morel (2022-09-02 09:55:22)
>>> S390x do not support multithreading in the guest.
>>> Do not let admin falsely specify multithreading on QEMU
>>> smp commandline.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   hw/s390x/s390-virtio-ccw.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index 70229b102b..b5ca154e2f 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine)
>>>       MachineClass *mc = MACHINE_GET_CLASS(machine);
>>>       int i;
>>> +    /* Explicitely do not support threads */
>>            ^
>>            Explicitly
>>
>>> +    assert(machine->smp.threads == 1);
>>
>> It might be nicer to give a better error message to the user.
>> What do you think about something like (broken whitespace ahead):
>>
>>      if (machine->smp.threads != 1) {if (machine->smp.threads != 1) {
>>          error_setg(&error_fatal, "More than one thread specified, but multithreading unsupported");
>>          return;
>>      }
>>
> 
> 
> OK, I think I wanted to do this and I changed my mind, obviously, I do not recall why.
> I will do almost the same but after a look at error.h I will use error_report()/exit() instead of error_setg()/return as in:
> 
> 
> +    /* Explicitly do not support threads */
> +    if (machine->smp.threads != 1) {
> +        error_report("More than one thread specified, but multithreading unsupported");
> +        exit(1);
> +    }


or add an 'Error **errp' parameter to s390_init_cpus() and use error_setg()
as initially proposed. s390x_new_cpu() would benefit from it also.

Thanks,

C.


> 
> 
> Thanks,
> 
> Regards,
> Pierre
> 

Re: [PATCH v9 02/10] s390x/cpu topology: core_id sets s390x CPU topology

[Cédric Le Goater]

On 9/2/22 09:55, Pierre Morel wrote:
> In the S390x CPU topology the core_id specifies the CPU address
> and the position of the core withing the topology.
> 
> Let's build the topology based on the core_id.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   hw/s390x/cpu-topology.c         | 135 ++++++++++++++++++++++++++++++++
>   hw/s390x/meson.build            |   1 +
>   hw/s390x/s390-virtio-ccw.c      |  10 +++
>   include/hw/s390x/cpu-topology.h |  42 ++++++++++
>   4 files changed, 188 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..a6ca006ec5
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c
> @@ -0,0 +1,135 @@
> +/*
> + * CPU Topology
> + *
> + * Copyright IBM Corp. 2022
> + * 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/qdev-properties.h"
> +#include "hw/boards.h"
> +#include "qemu/typedefs.h"
> +#include "target/s390x/cpu.h"
> +#include "hw/s390x/s390-virtio-ccw.h"
> +#include "hw/s390x/cpu-topology.h"
> +
> +S390Topology *s390_get_topology(void)
> +{
> +    static S390Topology *s390Topology;
> +
> +    if (!s390Topology) {
> +        s390Topology = S390_CPU_TOPOLOGY(
> +            object_resolve_path(TYPE_S390_CPU_TOPOLOGY, NULL));
> +    }
> +
> +    return s390Topology;
> +}
> +
> +/*
> + * s390_topology_new_cpu:
> + * @core_id: the core ID is machine wide
> + *
> + * The topology returned by s390_get_topology(), gives us the CPU
> + * topology established by the -smp QEMU aruments.
> + * The core-id gives:
> + *  - the Container TLE (Topology List Entry) containing the CPU TLE.
> + *  - in the CPU TLE the origin, or offset of the first bit in the core mask
> + *  - the bit in the CPU TLE core mask
> + */
> +void s390_topology_new_cpu(int core_id)
> +{
> +    S390Topology *topo = s390_get_topology();
> +    int socket_id;
> +    int bit, origin;
> +
> +    /* In the case no Topology is used nothing is to be done here */
> +    if (!topo) {
> +        return;
> +    }
> +
> +    socket_id = core_id / topo->cores;
> +
> +    bit = core_id;
> +    origin = bit / 64;
> +    bit %= 64;
> +    bit = 63 - bit;
> +
> +    /*
> +     * At the core level, each CPU is represented by a bit in a 64bit
> +     * unsigned long. Set on plug and clear on unplug of a CPU.

Do we have CPU unplug on s390x ?

> +     * The firmware assume that all CPU in a CPU TLE have the same
> +     * type, polarization and are all dedicated or shared.
> +     * In the case a socket contains CPU with different type, polarization
> +     * or entitlement then they will be defined in different CPU containers.
> +     * Currently we assume all CPU are identical IFL CPUs and that they are
> +     * all dedicated CPUs.
> +     * The only reason to have several S390TopologyCores inside a socket is
> +     * to have more than 64 CPUs.
> +     * In that case the origin field, representing the offset of the first CPU
> +     * in the CPU container allows to represent up to the maximal number of
> +     * CPU inside several CPU containers inside the socket container.
> +     */
> +    topo->socket[socket_id].active_count++;
> +    topo->tle[socket_id].active_count++;
> +    set_bit(bit, &topo->tle[socket_id].mask[origin]);
> +}
> +
> +/**
> + * s390_topology_realize:
> + * @dev: the device state
> + * @errp: the error pointer (not used)
> + *
> + * During realize the machine CPU topology is initialized with the
> + * QEMU -smp parameters.
> + * The maximum count of CPU TLE in the all Topology can not be greater
> + * than the maximum CPUs.
> + */
> +static void s390_topology_realize(DeviceState *dev, Error **errp)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());

Using qdev_get_machine() is suspicious :)

> +    S390Topology *topo = S390_CPU_TOPOLOGY(dev);
> +    int n;
> +
> +    topo->sockets = ms->smp.sockets;
> +    topo->cores = ms->smp.cores;
> +    topo->tles = ms->smp.max_cpus;

These look like object properties to me.

> +
> +    n = topo->sockets;
> +    topo->socket = g_malloc0(n * sizeof(S390TopoContainer));
> +    topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE));
> +}
> +
> +/**
> + * topology_class_init:
> + * @oc: Object class
> + * @data: (not used)
> + *
> + * A very simple object we will need for reset and migration.
> + */
> +static void topology_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = s390_topology_realize;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +}

no vmstate ?

> +static const TypeInfo cpu_topology_info = {
> +    .name          = TYPE_S390_CPU_TOPOLOGY,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(S390Topology),
> +    .class_init    = topology_class_init,
> +};
> +
> +static void topology_register(void)
> +{
> +    type_register_static(&cpu_topology_info);
> +}
> +type_init(topology_register);
> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> index de28a90a57..96d7d7d231 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 b5ca154e2f..15cefd104b 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -43,6 +43,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;
>   
> @@ -247,6 +248,12 @@ static void ccw_init(MachineState *machine)
>       /* init memory + setup max page size. Required for the CPU model */
>       s390_memory_init(machine->ram);
>   
> +    /* Adding the topology must be done before CPU intialization*/
> +    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
> +    object_property_add_child(qdev_get_machine(), TYPE_S390_CPU_TOPOLOGY,

No need to use qdev_get_machine(), you have 'machine' above.

> +                              OBJECT(dev));
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);


why not store a TYPE_S390_CPU_TOPOLOGY object pointer under the machine
state for later use ?

> +
>       /* init CPUs (incl. CPU model) early so s390_has_feature() works */
>       s390_init_cpus(machine);
>   
> @@ -309,6 +316,9 @@ 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);
>   
> +    /* Inserting the CPU in the Topology can not fail */
> +    s390_topology_new_cpu(cpu->env.core_id);
> +

in which case, we could use the topo object pointer to insert a new CPU
id and drop s390_get_topology() which looks overkill.

I would add the test :

    if (!S390_CCW_MACHINE(machine)->topology_disable) {

before inserting to be consistent. But I am anticipating some other
patch.

C.


>       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..6911f975f4
> --- /dev/null
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -0,0 +1,42 @@
> +/*
> + * CPU Topology
> + *
> + * Copyright 2022 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
> +
> +typedef struct S390TopoContainer {
> +    int active_count;
> +} S390TopoContainer;
> +
> +#define S390_TOPOLOGY_MAX_ORIGIN (1 + S390_MAX_CPUS / 64)
> +typedef struct S390TopoTLE {
> +    int active_count;
> +    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
> +} S390TopoTLE;
> +
> +#include "hw/qdev-core.h"
> +#include "qom/object.h"
> +
> +struct S390Topology {
> +    SysBusDevice parent_obj;
> +    int sockets;
> +    int cores;
> +    int tles;
> +    S390TopoContainer *socket;
> +    S390TopoTLE *tle;
> +};
> +typedef struct S390Topology S390Topology;
> +
> +#define TYPE_S390_CPU_TOPOLOGY "s390-topology"
> +OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY)
> +
> +S390Topology *s390_get_topology(void);
> +void s390_topology_new_cpu(int core_id);
> +
> +#endif

Re: [PATCH v9 09/10] s390x/cpu_topology: activating CPU topology

[Cédric Le Goater]

On 9/2/22 09:55, Pierre Morel wrote:
> Starting with a new machine, s390-virtio-ccw-7.2, the machine
> property topology-disable is set to false while it is kept to
> true for older machine.

We probably need a machine class option also because we don't want
this to be possible :

    -M s390-ccw-virtio-7.1,topology-disable=false


> This allows migrating older machine without disabling the ctop
> CPU feature for older machine, thus keeping existing start scripts.
> 
> The KVM capability, KVM_CAP_S390_CPU_TOPOLOGY is used to
> activate the S390_FEAT_CONFIGURATION_TOPOLOGY feature and
> the topology facility for the guest in the case the topology
> is not disabled.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   hw/core/machine.c                  |  5 +++
>   hw/s390x/s390-virtio-ccw.c         | 55 ++++++++++++++++++++++++++----
>   include/hw/boards.h                |  3 ++
>   include/hw/s390x/s390-virtio-ccw.h |  1 +
>   target/s390x/kvm/kvm.c             | 14 ++++++++
>   5 files changed, 72 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 4c5c8d1655..cbcdd40763 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -40,6 +40,11 @@
>   #include "hw/virtio/virtio-pci.h"
>   #include "qom/object_interfaces.h"
>   
> +GlobalProperty hw_compat_7_1[] = {
> +    { "s390x-cpu", "ctop", "off"},
> +};
> +const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
> +
>   GlobalProperty hw_compat_7_0[] = {
>       { "arm-gicv3-common", "force-8-bit-prio", "on" },
>       { "nvme-ns", "eui64-default", "on"},
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 1fa98740de..3078e68df7 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -249,11 +249,16 @@ static void ccw_init(MachineState *machine)
>       /* init memory + setup max page size. Required for the CPU model */
>       s390_memory_init(machine->ram);
>   
> -    /* Adding the topology must be done before CPU intialization*/
> -    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
> -    object_property_add_child(qdev_get_machine(), TYPE_S390_CPU_TOPOLOGY,
> -                              OBJECT(dev));
> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    /*
> +     * Adding the topology must be done before CPU intialization but
> +     * only in the case it is not disabled for migration purpose.
> +     */
> +    if (!S390_CCW_MACHINE(machine)->topology_disable) {
> +        dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
> +        object_property_add_child(qdev_get_machine(), TYPE_S390_CPU_TOPOLOGY,
> +                                  OBJECT(dev));
> +        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    }
>   
>       /* init CPUs (incl. CPU model) early so s390_has_feature() works */
>       s390_init_cpus(machine);
> @@ -676,6 +681,21 @@ static inline void machine_set_zpcii_disable(Object *obj, bool value,
>       ms->zpcii_disable = value;
>   }
>   
> +static inline bool machine_get_topology_disable(Object *obj, Error **errp)
> +{
> +    S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> +
> +    return ms->topology_disable;
> +}
> +
> +static inline void machine_set_topology_disable(Object *obj, bool value,
> +                                                Error **errp)
> +{
> +    S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> +
> +    ms->topology_disable = value;
> +}
> +
>   static S390CcwMachineClass *current_mc;
>   
>   /*
> @@ -778,6 +798,13 @@ static inline void s390_machine_initfn(Object *obj)
>       object_property_set_description(obj, "zpcii-disable",
>               "disable zPCI interpretation facilties");
>       object_property_set_bool(obj, "zpcii-disable", false, NULL);
> +
> +    object_property_add_bool(obj, "topology-disable",
> +                             machine_get_topology_disable,
> +                             machine_set_topology_disable);
> +    object_property_set_description(obj, "topology-disable",
> +            "disable zPCI interpretation facilties");
> +    object_property_set_bool(obj, "topology-disable", false, NULL);
>   }
>   
>   static const TypeInfo ccw_machine_info = {
> @@ -830,14 +857,29 @@ bool css_migration_enabled(void)
>       }                                                                         \
>       type_init(ccw_machine_register_##suffix)
>   
> +static void ccw_machine_7_2_instance_options(MachineState *machine)
> +{
> +}
> +
> +static void ccw_machine_7_2_class_options(MachineClass *mc)
> +{
> +}
> +DEFINE_CCW_MACHINE(7_2, "7.2", true);
> +
>   static void ccw_machine_7_1_instance_options(MachineState *machine)
>   {
> +    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
> +
> +    ccw_machine_7_2_instance_options(machine);
> +    ms->topology_disable = true;
>   }
>   
>   static void ccw_machine_7_1_class_options(MachineClass *mc)
>   {
> +    ccw_machine_7_2_class_options(mc);
> +    compat_props_add(mc->compat_props, hw_compat_7_1, hw_compat_7_1_len);
>   }
> -DEFINE_CCW_MACHINE(7_1, "7.1", true);
> +DEFINE_CCW_MACHINE(7_1, "7.1", false);
>   
>   static void ccw_machine_7_0_instance_options(MachineState *machine)
>   {
> @@ -847,6 +889,7 @@ static void ccw_machine_7_0_instance_options(MachineState *machine)
>       ccw_machine_7_1_instance_options(machine);
>       s390_set_qemu_cpu_model(0x8561, 15, 1, qemu_cpu_feat);
>       ms->zpcii_disable = true;
> +
>   }
>   
>   static void ccw_machine_7_0_class_options(MachineClass *mc)
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 69e20c1252..6e9803aa2d 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -387,6 +387,9 @@ struct MachineState {
>       } \
>       type_init(machine_initfn##_register_types)
>   
> +extern GlobalProperty hw_compat_7_1[];
> +extern const size_t hw_compat_7_1_len;
> +
>   extern GlobalProperty hw_compat_7_0[];
>   extern const size_t hw_compat_7_0_len;
>   
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 9e7a0d75bc..b14660eecb 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -28,6 +28,7 @@ struct S390CcwMachineState {
>       bool dea_key_wrap;
>       bool pv;
>       bool zpcii_disable;
> +    bool topology_disable;
>       uint8_t loadparm[8];
>   };
>   
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index cb14bcc012..6b7efee511 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -2385,6 +2385,7 @@ bool kvm_s390_cpu_models_supported(void)
>   
>   void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>   {
> +    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
>       struct kvm_s390_vm_cpu_machine prop = {};
>       struct kvm_device_attr attr = {
>           .group = KVM_S390_VM_CPU_MODEL,
> @@ -2466,6 +2467,19 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>           set_bit(S390_FEAT_UNPACK, model->features);
>       }
>   
> +    /*
> +     * If we have the CPU Topology implemented in KVM activate
> +     * the CPU TOPOLOGY feature.
> +     */
> +    if ((!ms->topology_disable) &&

'topology_disable' is a platform level configuration. May be instead,
the feature could be cleared at the machine level ?

Thanks,

C.

> +        kvm_check_extension(kvm_state, KVM_CAP_S390_CPU_TOPOLOGY)) {
> +        if (kvm_vm_enable_cap(kvm_state, KVM_CAP_S390_CPU_TOPOLOGY, 0) < 0) {
> +            error_setg(errp, "KVM: Error enabling KVM_CAP_S390_CPU_TOPOLOGY");
> +            return;
> +        }
> +        set_bit(S390_FEAT_CONFIGURATION_TOPOLOGY, model->features);
> +    }
> +
>       /* We emulate a zPCI bus and AEN, therefore we don't need HW support */
>       set_bit(S390_FEAT_ZPCI, model->features);
>       set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);

Re: [PATCH v9 09/10] s390x/cpu_topology: activating CPU topology

[Pierre Morel]

Hi Cedric,

On 9/27/22 16:41, Cédric Le Goater wrote:
> On 9/2/22 09:55, Pierre Morel wrote:
>> Starting with a new machine, s390-virtio-ccw-7.2, the machine
>> property topology-disable is set to false while it is kept to
>> true for older machine.
> 
> We probably need a machine class option also because we don't want
> this to be possible :
> 
>     -M s390-ccw-virtio-7.1,topology-disable=false

hum, you are right it has little interest to do this.

> 
> 
>> This allows migrating older machine without disabling the ctop
>> CPU feature for older machine, thus keeping existing start scripts.
>>
>> The KVM capability, KVM_CAP_S390_CPU_TOPOLOGY is used to
>> activate the S390_FEAT_CONFIGURATION_TOPOLOGY feature and
>> the topology facility for the guest in the case the topology
>> is not disabled.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/core/machine.c                  |  5 +++
>>   hw/s390x/s390-virtio-ccw.c         | 55 ++++++++++++++++++++++++++----
>>   include/hw/boards.h                |  3 ++
>>   include/hw/s390x/s390-virtio-ccw.h |  1 +
>>   target/s390x/kvm/kvm.c             | 14 ++++++++
>>   5 files changed, 72 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 4c5c8d1655..cbcdd40763 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -40,6 +40,11 @@
>>   #include "hw/virtio/virtio-pci.h"
>>   #include "qom/object_interfaces.h"
>> +GlobalProperty hw_compat_7_1[] = {
>> +    { "s390x-cpu", "ctop", "off"},
>> +};
>> +const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
>> +
>>   GlobalProperty hw_compat_7_0[] = {
>>       { "arm-gicv3-common", "force-8-bit-prio", "on" },
>>       { "nvme-ns", "eui64-default", "on"},
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 1fa98740de..3078e68df7 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -249,11 +249,16 @@ static void ccw_init(MachineState *machine)
>>       /* init memory + setup max page size. Required for the CPU model */
>>       s390_memory_init(machine->ram);
>> -    /* Adding the topology must be done before CPU intialization*/
>> -    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
>> -    object_property_add_child(qdev_get_machine(), 
>> TYPE_S390_CPU_TOPOLOGY,
>> -                              OBJECT(dev));
>> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> +    /*
>> +     * Adding the topology must be done before CPU intialization but
>> +     * only in the case it is not disabled for migration purpose.
>> +     */
>> +    if (!S390_CCW_MACHINE(machine)->topology_disable) {
>> +        dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
>> +        object_property_add_child(qdev_get_machine(), 
>> TYPE_S390_CPU_TOPOLOGY,
>> +                                  OBJECT(dev));
>> +        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> +    }
>>       /* init CPUs (incl. CPU model) early so s390_has_feature() works */
>>       s390_init_cpus(machine);
>> @@ -676,6 +681,21 @@ static inline void 
>> machine_set_zpcii_disable(Object *obj, bool value,
>>       ms->zpcii_disable = value;
>>   }
>> +static inline bool machine_get_topology_disable(Object *obj, Error 
>> **errp)
>> +{
>> +    S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
>> +
>> +    return ms->topology_disable;
>> +}
>> +
>> +static inline void machine_set_topology_disable(Object *obj, bool value,
>> +                                                Error **errp)
>> +{
>> +    S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
>> +
>> +    ms->topology_disable = value;
>> +}
>> +
>>   static S390CcwMachineClass *current_mc;
>>   /*
>> @@ -778,6 +798,13 @@ static inline void s390_machine_initfn(Object *obj)
>>       object_property_set_description(obj, "zpcii-disable",
>>               "disable zPCI interpretation facilties");
>>       object_property_set_bool(obj, "zpcii-disable", false, NULL);
>> +
>> +    object_property_add_bool(obj, "topology-disable",
>> +                             machine_get_topology_disable,
>> +                             machine_set_topology_disable);
>> +    object_property_set_description(obj, "topology-disable",
>> +            "disable zPCI interpretation facilties");
>> +    object_property_set_bool(obj, "topology-disable", false, NULL);
>>   }
>>   static const TypeInfo ccw_machine_info = {
>> @@ -830,14 +857,29 @@ bool css_migration_enabled(void)
>>       
>> }                                                                         \
>>       type_init(ccw_machine_register_##suffix)
>> +static void ccw_machine_7_2_instance_options(MachineState *machine)
>> +{
>> +}
>> +
>> +static void ccw_machine_7_2_class_options(MachineClass *mc)
>> +{
>> +}
>> +DEFINE_CCW_MACHINE(7_2, "7.2", true);
>> +
>>   static void ccw_machine_7_1_instance_options(MachineState *machine)
>>   {
>> +    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
>> +
>> +    ccw_machine_7_2_instance_options(machine);
>> +    ms->topology_disable = true;
>>   }
>>   static void ccw_machine_7_1_class_options(MachineClass *mc)
>>   {
>> +    ccw_machine_7_2_class_options(mc);
>> +    compat_props_add(mc->compat_props, hw_compat_7_1, 
>> hw_compat_7_1_len);
>>   }
>> -DEFINE_CCW_MACHINE(7_1, "7.1", true);
>> +DEFINE_CCW_MACHINE(7_1, "7.1", false);
>>   static void ccw_machine_7_0_instance_options(MachineState *machine)
>>   {
>> @@ -847,6 +889,7 @@ static void 
>> ccw_machine_7_0_instance_options(MachineState *machine)
>>       ccw_machine_7_1_instance_options(machine);
>>       s390_set_qemu_cpu_model(0x8561, 15, 1, qemu_cpu_feat);
>>       ms->zpcii_disable = true;
>> +
>>   }
>>   static void ccw_machine_7_0_class_options(MachineClass *mc)
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 69e20c1252..6e9803aa2d 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -387,6 +387,9 @@ struct MachineState {
>>       } \
>>       type_init(machine_initfn##_register_types)
>> +extern GlobalProperty hw_compat_7_1[];
>> +extern const size_t hw_compat_7_1_len;
>> +
>>   extern GlobalProperty hw_compat_7_0[];
>>   extern const size_t hw_compat_7_0_len;
>> diff --git a/include/hw/s390x/s390-virtio-ccw.h 
>> b/include/hw/s390x/s390-virtio-ccw.h
>> index 9e7a0d75bc..b14660eecb 100644
>> --- a/include/hw/s390x/s390-virtio-ccw.h
>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>> @@ -28,6 +28,7 @@ struct S390CcwMachineState {
>>       bool dea_key_wrap;
>>       bool pv;
>>       bool zpcii_disable;
>> +    bool topology_disable;
>>       uint8_t loadparm[8];
>>   };
>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>> index cb14bcc012..6b7efee511 100644
>> --- a/target/s390x/kvm/kvm.c
>> +++ b/target/s390x/kvm/kvm.c
>> @@ -2385,6 +2385,7 @@ bool kvm_s390_cpu_models_supported(void)
>>   void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>>   {
>> +    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
>>       struct kvm_s390_vm_cpu_machine prop = {};
>>       struct kvm_device_attr attr = {
>>           .group = KVM_S390_VM_CPU_MODEL,
>> @@ -2466,6 +2467,19 @@ void kvm_s390_get_host_cpu_model(S390CPUModel 
>> *model, Error **errp)
>>           set_bit(S390_FEAT_UNPACK, model->features);
>>       }
>> +    /*
>> +     * If we have the CPU Topology implemented in KVM activate
>> +     * the CPU TOPOLOGY feature.
>> +     */
>> +    if ((!ms->topology_disable) &&
> 
> 'topology_disable' is a platform level configuration. May be instead,
> the feature could be cleared at the machine level ?

thanks, I will try this way.

regards,
Pierre



-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 10/10] docs/s390x: document s390x cpu topology

[Pierre Morel]

On 9/12/22 15:41, Janis Schoetterl-Glausch wrote:
> On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
>> Add some basic examples for the definition of cpu topology
>> in s390x.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   docs/system/s390x/cpu_topology.rst | 88 ++++++++++++++++++++++++++++++
>>   1 file changed, 88 insertions(+)
>>   create mode 100644 docs/system/s390x/cpu_topology.rst
>>
>> diff --git a/docs/system/s390x/cpu_topology.rst b/docs/system/s390x/cpu_topology.rst
>> new file mode 100644
>> index 0000000000..00977d4319
>> --- /dev/null
>> +++ b/docs/system/s390x/cpu_topology.rst
>> @@ -0,0 +1,88 @@
>> +CPU Topology on s390x
>> +=====================
>> +
>> +CPU Topology on S390x provides up to 4 levels of topology containers:
>> +drawers, books, sockets and CPUs.
>> +While the three higher level containers, Containers Topology List Entries,
>> +(Containers TLE) define a tree hierarchy, the lowest level of topology
>> +definition, the CPU Topology List Entry (CPU TLE), provides the placement
>> +of the CPUs inside the last container.
> 
> inside the parent container

OK

>> +
>> +Prerequisites
>> +-------------
>> +
>> +To use CPU Topology a Linux QEMU/KVM machine providing the CPU Topology facility
>> +(STFLE bit 11) is required.
>> +
>> +However, since this facility has been enabled by default in an early version,
>> +the capability ``KVM_CAP_S390_CPU_TOPOLOGY`` is needed to indicate to KVM
>> +that QEMU support CPU Topology.
> 
> I don't understand this paragraph. Early version of what?

of QEMU, I add this

>> +
>> +Indicating the CPU topology to the Virtual Machine
>> +--------------------------------------------------
>> +
>> +The CPU Topology, number of drawers, number of books per drawers, number of
>> +sockets per book and number of cores per sockets is specified with the
>> +``-smp`` qemu command arguments.
>> +
>> +Like in :
>> +
>> +.. code-block:: sh
>> +    -smp cpus=1,drawers=3,books=4,sockets=2,cores=8,maxcpus=192
>> +
>> +If drawers or books are not specified, their default to 1.
>> +
>> +New CPUs can be plugged using the device_add hmp command like in:
>> +
>> +.. code-block:: sh
>> +   (qemu) device_add host-s390x-cpu,core-id=9
>> +
>> +The core-id defines the placement of the core in the topology by
>> +starting with core 0 in socket 0, book 0 and drawer 0 up to the maximum
>> +core number of the last socket of the last book in the last drawer.
>> +
>> +In the example above:
>> +
>> +* the core with ID 9 will be placed in container (0,0,1), as core 9
>> +  of CPU TLE 0 of socket 1 in book 0 from drawer 0.
>> +* the core ID 0 is defined by the -smp cpus=1 command and will be
>> +  placed as core 0 in CPU TLE 0 of container (0,0,0)
>> +
>> +Note that the core ID is machine wide and the CPU TLE masks provided
>> +by the STSI instruction will be:
>> +
>> +* in socket 0: 0x80000000 (core id 0)
>> +* in socket 1: 0x00400000 (core id 9)
>> +
>> +Indicating the CPU topology to the Guest
>> +----------------------------------------
>> +
>> +The guest can query for topology changes using the PTF instruction.
>> +In case of a topology change it can request the new topology by issuing
>> +STSI instructions specifying the level of detail required, drawer with
>> +STSI(15.1.4) or books STSI(15.1.3).
>> +
>> +The virtual machine will fill the provided buffer with the count of
>> +drawers (MAG4), books per drawer (MAG3), sockets per book (MAG2) and
>> +cores per socket (MAG1).
>> +
>> +Note that the STSI(15.1.2) is special in two ways:
>> +
>> +* When the firmware detect a change in the values calculated for STSI(15.1.2)
>> +  it will trigger the report of the topology change for the PTF instruction.
> 
> I don't know if we need this section, after all documenting this is the
> job of the principles of operation. You could just refer to the
> relevant sections.

OK, I will make it shorter

Thanks,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 00/10] s390x: CPU Topology

[Pierre Morel]

On 9/12/22 16:38, Janis Schoetterl-Glausch wrote:
> I found this version much easier to understand than the previous one.
> 
> You could consider splitting up the series into two.
> One that introduces support for STSI, PTF, migration, etc.
> And a second one that adds support for the maximum-MNist facility and
> drawers and books.

I agree, sending the first part as supporting only sockets already is an 
enhancement that could be proposed on its own.

> 
> This would also make bisecting a bit nicer because it moves the feature
> enablement closer to the commits adding the support.
> 
> Right now, with this series, the topology is static and cannot change.

If we ignore changes on hotplug, yes we consider the topology fixed and 
that the vCPU does not migrate.

> Most of the value of making the topology visible to the guest is for it
> to mirror reality, and a static topology is a hindrance for that.
> I'm completely fine with having a static topology as a stepping stone
> to a dynamic one.
> However I think we should have a rough plan or maybe even a prototype
> for how we turn a static into a dynamic topology before we merge this
> series in order to avoid designing us into a corner.
> What do you think?

Yes, we can discuss this internally before moving there.

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 07/10] s390x/cpu_topology: CPU topology migration

[Pierre Morel]

On 9/8/22 20:04, Janis Schoetterl-Glausch wrote:
> On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
>> The migration can only take place if both source and destination
>> of the migration both use or both do not use the CPU topology
>> facility.
>>
>> We indicate a change in topology during migration postload for the
>> case the topology changed between source and destination.
> 
> You always set the report bit after migration, right?
> In the last series you actually migrated the bit.
> Why the change? With the code you have actually migrating the bit isn't
> hard.

As for the moment the vCPU do not migrate from real CPU I thought based 
on the remark of Nico that there is no need to set the bit after a 
migration.

>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/cpu-topology.c         | 79 +++++++++++++++++++++++++++++++++
>>   include/hw/s390x/cpu-topology.h |  1 +
>>   target/s390x/cpu-sysemu.c       |  8 ++++
>>   target/s390x/cpu.h              |  1 +
>>   4 files changed, 89 insertions(+)
>>
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> index 6098d6ea1f..b6bf839e40 100644
>> --- a/hw/s390x/cpu-topology.c
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -19,6 +19,7 @@
>>   #include "target/s390x/cpu.h"
>>   #include "hw/s390x/s390-virtio-ccw.h"
>>   #include "hw/s390x/cpu-topology.h"
>> +#include "migration/vmstate.h"
>>   
>>   S390Topology *s390_get_topology(void)
>>   {
>> @@ -132,6 +133,83 @@ static void s390_topology_reset(DeviceState *dev)
>>       s390_cpu_topology_reset();
>>   }
>>   
>> +/**
>> + * cpu_topology_postload
>> + * @opaque: a pointer to the S390Topology
>> + * @version_id: version identifier
>> + *
>> + * We check that the topology is used or is not used
>> + * on both side identically.
>> + *
>> + * If the topology is in use we set the Modified Topology Change Report
>> + * on the destination host.
>> + */
>> +static int cpu_topology_postload(void *opaque, int version_id)
>> +{
>> +    S390Topology *topo = opaque;
>> +    int ret;
>> +
>> +    if (topo->topology_needed != s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
> 
> Does this function even run if topology_needed is false?
> In that case there is no data saved, so no reason to load it either.
> If so you can only check that both the source and the destination have
> the feature enabled. You would need to always send the topology VMSD in
> order to check that the feature is disabled.
> 
> Does qemu allow you to attempt to migrate to a host with another cpu
> model?
> If it disallowes that you wouldn't need to do any checks, right?

hum yes I must rework this

Thanks,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 06/10] s390x/cpu_topology: resetting the Topology-Change-Report

[Pierre Morel]

On 9/6/22 10:27, Nico Boehr wrote:
> Quoting Pierre Morel (2022-09-02 09:55:27)
>> During a subsystem reset the Topology-Change-Report is cleared
>> by the machine.
>> Let's ask KVM to clear the Modified Topology Change Report (MTCR)
>>   bit of the SCA in the case of a subsystem reset.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> 
> Reviewed-by: Nico Boehr <nrb@linux.ibm.com>


Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 06/10] s390x/cpu_topology: resetting the Topology-Change-Report

[Pierre Morel]

On 9/8/22 09:57, Janis Schoetterl-Glausch wrote:
> On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
>> During a subsystem reset the Topology-Change-Report is cleared
>> by the machine.
>> Let's ask KVM to clear the Modified Topology Change Report (MTCR)
>>   bit of the SCA in the case of a subsystem reset.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/cpu-topology.c      | 12 ++++++++++++
>>   hw/s390x/s390-virtio-ccw.c   |  1 +
>>   target/s390x/cpu-sysemu.c    |  7 +++++++
>>   target/s390x/cpu.h           |  1 +
>>   target/s390x/kvm/kvm.c       | 23 +++++++++++++++++++++++
>>   target/s390x/kvm/kvm_s390x.h |  1 +
>>   6 files changed, 45 insertions(+)
> 
> [...]
> 
>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>> index f96630440b..9c994d27d5 100644
>> --- a/target/s390x/kvm/kvm.c
>> +++ b/target/s390x/kvm/kvm.c
>> @@ -2585,3 +2585,26 @@ int kvm_s390_get_zpci_op(void)
>>   {
>>       return cap_zpci_op;
>>   }
>> +
>> +int kvm_s390_topology_set_mtcr(uint64_t attr)
>> +{
>> +    struct kvm_device_attr attribute = {
>> +        .group = KVM_S390_VM_CPU_TOPOLOGY,
>> +        .attr  = attr,
>> +    };
>> +    int ret;
>> +
>> +    if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
>> +        return -EFAULT;
> 
> Why EFAULT?
any proposition?

> The return value is just ignored when resetting, isn't it?

In migration the same function is used and we need to return an error there.
But if we would not use it, after the comments you did for migration, we 
may indeed not need it, then I guess using error_report and a void 
function may be better.

> I wonder if it would be better not to.
> Is it necessary because you're detecting the feature after you've
> already created the S390Topology instance?
> And you're doing that because that's just the order in which QEMU does
> things? So the machine class is inited before the cpu model?
> I wonder if there is a nice way to create the S390Topology only if the
> feature is selected.

I do not know if it is possible. I will look.

> 
> Anyway:
> Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

Thanks,
Pierre

> 
>> +    }
>> +    if (!kvm_vm_check_attr(kvm_state, KVM_S390_VM_CPU_TOPOLOGY, attr)) {
>> +        return -ENOENT;
>> +    }
>> +
>> +    ret = kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attribute);
>> +    if (ret) {
>> +        error_report("Failed to set cpu topology attribute %lu: %s",
>> +                     attr, strerror(-ret));
>> +    }
>> +    return ret;
>> +}
>>
> [...]
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 05/10] s390x/cpu: reporting drawers and books topology to the guest

[Pierre Morel]

On 9/7/22 12:36, Janis Schoetterl-Glausch wrote:
> On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
>> The guest can ask for a topology report on drawer's or book's
>> level.
>> Let's implement the STSI instruction's handling for the corresponding
>> selector values.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/cpu-topology.c         | 19 +++++++---
>>   hw/s390x/s390-virtio-ccw.c      |  2 ++
>>   include/hw/s390x/cpu-topology.h |  7 +++-
>>   target/s390x/cpu_topology.c     | 64 +++++++++++++++++++++++++++------
>>   4 files changed, 76 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> index e2fd5c7e44..bb9ae63483 100644
>> --- a/hw/s390x/cpu-topology.c
>> +++ b/hw/s390x/cpu-topology.c
>>
> [...]
> 
>> @@ -99,13 +103,20 @@ static void s390_topology_realize(DeviceState *dev, Error **errp)
>>       S390Topology *topo = S390_CPU_TOPOLOGY(dev);
>>       int n;
>>   
>> +    topo->drawers = ms->smp.drawers;
>> +    topo->books = ms->smp.books;
>> +    topo->total_books = topo->books * topo->drawers;
>>       topo->sockets = ms->smp.sockets;
>> +    topo->total_sockets = topo->sockets * topo->books * topo->drawers;
>>       topo->cores = ms->smp.cores;
>> -    topo->tles = ms->smp.max_cpus;
>>   
>> -    n = topo->sockets;
>> +    n = topo->drawers;
>> +    topo->drawer = g_malloc0(n * sizeof(S390TopoContainer));
>> +    n *= topo->books;
>> +    topo->book = g_malloc0(n * sizeof(S390TopoContainer));
>> +    n *= topo->sockets;
>>       topo->socket = g_malloc0(n * sizeof(S390TopoContainer));
>> -    topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE));
>> +    topo->tle = g_malloc0(n * sizeof(S390TopoTLE));
> 
> Same question here about using g_new0.

yes, g_new0 is better here

>>   
>>       qemu_mutex_init(&topo->topo_mutex);
>>   }
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 15cefd104b..3f28e28d47 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -626,6 +626,8 @@ 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_props.books_supported = true;
>> +    mc->smp_props.drawers_supported = true;
>>   }
>>   
>>   static inline bool machine_get_aes_key_wrap(Object *obj, Error **errp)
>> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
>> index 0b7f3d10b2..4f8ac39ca0 100644
>> --- a/include/hw/s390x/cpu-topology.h
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -29,9 +29,14 @@ typedef struct S390TopoTLE {
>>   
>>   struct S390Topology {
>>       SysBusDevice parent_obj;
>> +    int total_books;
>> +    int total_sockets;
> 
> What are these used for? I'm not seeing anything.
> 
>> +    int drawers;
>> +    int books;
>>       int sockets;
>>       int cores;
>> -    int tles;
> 
> You remove this in this patch and you didn't really need it before.
> As far as I can tell it was just used for calculating the number of
> tles to allocate and you could use a local variable instead.
> So I would get rid of it in the patch that introduced it.

yes


> 
>> +    S390TopoContainer *drawer;
>> +    S390TopoContainer *book;
>>       S390TopoContainer *socket;
>>       S390TopoTLE *tle;
>>       QemuMutex topo_mutex;
>> diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
>> index 56865dafc6..305fbb9734 100644
>> --- a/target/s390x/cpu_topology.c
>> +++ b/target/s390x/cpu_topology.c
>> @@ -37,19 +37,18 @@ static char *fill_tle_cpu(char *p, uint64_t mask, int origin)
>>       return p + sizeof(*tle);
>>   }
>>   
>> -static char *s390_top_set_level2(S390Topology *topo, char *p)
>> +static char *s390_top_set_level2(S390Topology *topo, char *p, int fs, int ns)
>>   {
> 
> I wouldn't hate more verbose names for fs and ns. start_socket,
> num_socket maybe? Same for fb, nb, but it is your call, it's not really
> hard to understand the code.

I prefer to keep the names short but I will have a second thought.

> 
>> -    int i, origin;
>> +    int socket, origin;
>> +    uint64_t mask;
>>   
>> -    for (i = 0; i < topo->sockets; i++) {
>> -        if (!topo->socket[i].active_count) {
>> +    for (socket = fs; socket < fs + ns; socket++) {
>> +        if (!topo->socket[socket].active_count) {
>>               continue;
>>           }
>> -        p = fill_container(p, 1, i);
>> +        p = fill_container(p, 1, socket);
> 
> Have you considered using an enum for the level constants?

did not but yes it would be better.

> 
>>           for (origin = 0; origin < S390_TOPOLOGY_MAX_ORIGIN; origin++) {
>> -            uint64_t mask = 0L;
>> -
>> -            mask = be64_to_cpu(topo->tle[i].mask[origin]);
>> +            mask = be64_to_cpu(topo->tle[socket].mask[origin]);
>>               if (mask) {
>>                   p = fill_tle_cpu(p, mask, origin);
>>               }
>> @@ -58,19 +57,63 @@ static char *s390_top_set_level2(S390Topology *topo, char *p)
>>       return p;
>>   }
>>   
>> +static char *s390_top_set_level3(S390Topology *topo, char *p, int fb, int nb)
>> +{
>> +    int book, fs = 0;
>> +
>> +    for (book = fb; book < fb + nb; book++, fs += topo->sockets) {
>> +        if (!topo->book[book].active_count) {
>> +            continue;
>> +        }
>> +        p = fill_container(p, 2, book);
>> +    p = s390_top_set_level2(topo, p, fs, topo->sockets);
> 
> Indent is off.

thx

> 
>> +    }
>> +    return p;
>> +}
>> +
>> +static char *s390_top_set_level4(S390Topology *topo, char *p)
>> +{
>> +    int drawer, fb = 0;
>> +
>> +    for (drawer = 0; drawer < topo->drawers; drawer++, fb += topo->books) {
>> +        if (!topo->drawer[drawer].active_count) {
>> +            continue;
>> +        }
>> +        p = fill_container(p, 3, drawer);
>> +        p = s390_top_set_level3(topo, p, fb, topo->books);
>> +    }
>> +    return p;
>> +}
>> +
>>   static int setup_stsi(SysIB_151x *sysib, int level)
>>   {
>>       S390Topology *topo = s390_get_topology();
>>       char *p = (char *)sysib->tle;
>> +    int max_containers;
>>   
>>       qemu_mutex_lock(&topo->topo_mutex);
>>   
>>       sysib->mnest = level;
>>       switch (level) {
>>       case 2:
>> +        max_containers = topo->sockets * topo->books * topo->drawers;
>> +        sysib->mag[TOPOLOGY_NR_MAG2] = max_containers;
>> +        sysib->mag[TOPOLOGY_NR_MAG1] = topo->cores;
>> +        p = s390_top_set_level2(topo, p, 0, max_containers);
> 
> Isn't this logic change already required for the patch that introduced
> stsi 15.1.2 handling?

yes, thx

> 
>> +        break;
>> +    case 3:
>> +        max_containers = topo->books * topo->drawers;
>> +        sysib->mag[TOPOLOGY_NR_MAG3] = max_containers;
>>           sysib->mag[TOPOLOGY_NR_MAG2] = topo->sockets;
>>           sysib->mag[TOPOLOGY_NR_MAG1] = topo->cores;
>> -        p = s390_top_set_level2(topo, p);
>> +        p = s390_top_set_level3(topo, p, 0, max_containers);
>> +        break;
>> +    case 4:
>> +        sysib->mag[TOPOLOGY_NR_MAG4] = topo->drawers;
>> +        sysib->mag[TOPOLOGY_NR_MAG3] = topo->books;
>> +        sysib->mag[TOPOLOGY_NR_MAG2] = topo->sockets;
>> +        sysib->mag[TOPOLOGY_NR_MAG1] = topo->cores;
>> +        p = s390_top_set_level4(topo, p);
>>           break;
>>       }
>>   
>> @@ -79,7 +122,7 @@ static int setup_stsi(SysIB_151x *sysib, int level)
>>       return p - (char *)sysib->tle;
>>   }
>>   
>> -#define S390_TOPOLOGY_MAX_MNEST 2
>> +#define S390_TOPOLOGY_MAX_MNEST 4
> 
> AFAIK you're only allowed to increase this if the maximum mnest
> facility is installed. If it isn't, only level 2 is supported.
> Which would mean that this patch doesn't do anything.

AFAIU this is model dependant but I have to rework the MNEST entry of 
the SYSIB anyway.

> 
>>   void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
>>   {
>>       SysIB_151x *sysib;
>> @@ -105,4 +148,3 @@ void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
>>   out_free:
>>       g_free(sysib);
>>   }
>> -
> 

Thanks for the comments

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 04/10] hw/core: introducing drawer and books for s390x

[Pierre Morel]

On 9/6/22 10:59, Markus Armbruster wrote:
> Pierre Morel <pmorel@linux.ibm.com> writes:
> 
>> S390x defines two topology levels above sockets: nbooks and drawers.
> 
> nbooks or books?
> 
>> Let's add these two levels inside the CPU topology implementation.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
> 
> [...]
> 
>> diff --git a/qapi/machine.json b/qapi/machine.json
>> index 6afd1936b0..bdd92e3cb1 100644
>> --- a/qapi/machine.json
>> +++ b/qapi/machine.json
>> @@ -900,13 +900,15 @@
>>   # 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
>> +# @drawer-id: drawer number within node/board the CPU belongs to
>> +# @book-id: book number within drawer/node/board the CPU belongs to
>> +# @socket-id: socket number within book/node/board the CPU belongs to
>>   # @die-id: die number within socket the CPU belongs to (since 4.1)
>>   # @cluster-id: cluster number within die the CPU belongs to (since 7.1)
>>   # @core-id: core number within cluster the CPU belongs to
>>   # @thread-id: thread number within core the CPU belongs to
>>   #
>> -# Note: currently there are 6 properties that could be present
>> +# Note: currently there are 7 properties that could be present
> 
> Should this be 8?

when one can count .. yes :)

Thanks, I will update in next spin

> 
>>   #       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.
> 
> Not your patch's fault, but the second sentence is less than clear.
> What are "the filed names"?  A typo perhaps?

I understood that it means the names in the structure here under which 
are filed inside the single quotes.


In this case, may be "filed names" should be replaced by
"names in the CpuInstanceProperties structure"


> 
>> @@ -916,6 +918,8 @@
>>   ##
>>   { 'struct': 'CpuInstanceProperties',
>>     'data': { '*node-id': 'int',
>> +            '*drawer-id': 'int',
>> +            '*book-id': 'int',
>>               '*socket-id': 'int',
>>               '*die-id': 'int',
>>               '*cluster-id': 'int',
>> @@ -1465,6 +1469,10 @@
>>   #

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 04/10] hw/core: introducing drawer and books for s390x

[Pierre Morel]

On 9/6/22 10:59, Markus Armbruster wrote:
> Pierre Morel <pmorel@linux.ibm.com> writes:
> 
>> S390x defines two topology levels above sockets: nbooks and drawers.
> 
> nbooks or books?

Sorry, forgot this.

Yes typo, I mean "books"

Thanks,
Pierre



-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 03/10] s390x/cpu topology: reporting the CPU topology to the guest

[Pierre Morel]

On 9/7/22 12:26, Janis Schoetterl-Glausch wrote:
> On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
>> The guest can use the STSI instruction to get a buffer filled
>> with the CPU topology description.
>>
>> Let us implement the STSI instruction for the basis CPU topology
>> level, level 2.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/cpu-topology.c         |   4 ++
>>   include/hw/s390x/cpu-topology.h |   5 ++
>>   target/s390x/cpu.h              |  49 +++++++++++++++
>>   target/s390x/cpu_topology.c     | 108 ++++++++++++++++++++++++++++++++
>>   target/s390x/kvm/kvm.c          |   6 +-
>>   target/s390x/meson.build        |   1 +
>>   6 files changed, 172 insertions(+), 1 deletion(-)
>>   create mode 100644 target/s390x/cpu_topology.c
>>
> [...]
> 
>> diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
> 
> [...]
> 
>> +static char *fill_tle_cpu(char *p, uint64_t mask, int origin)
>> +{
>> +    SysIBTl_cpu *tle = (SysIBTl_cpu *)p;
>> +
>> +    tle->nl = 0;
>> +    tle->dedicated = 1;
>> +    tle->polarity = S390_TOPOLOGY_POLARITY_H;
>> +    tle->type = S390_TOPOLOGY_CPU_TYPE;
>> +    tle->origin = origin * 64;
> 
> origin is a multibyte field too, so needs a conversion too.


right,

Thanks,
Pierre

> 
>> +    tle->mask = be64_to_cpu(mask);
>> +    return p + sizeof(*tle);
>> +}
>> +
> [...]

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 03/10] s390x/cpu topology: reporting the CPU topology to the guest

[Pierre Morel]

On 9/6/22 13:49, Janis Schoetterl-Glausch wrote:
> On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
>> The guest can use the STSI instruction to get a buffer filled
>> with the CPU topology description.
>>
>> Let us implement the STSI instruction for the basis CPU topology
>> level, level 2.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/cpu-topology.c         |   4 ++
>>   include/hw/s390x/cpu-topology.h |   5 ++
>>   target/s390x/cpu.h              |  49 +++++++++++++++
>>   target/s390x/cpu_topology.c     | 108 ++++++++++++++++++++++++++++++++
>>   target/s390x/kvm/kvm.c          |   6 +-
>>   target/s390x/meson.build        |   1 +
>>   6 files changed, 172 insertions(+), 1 deletion(-)
>>   create mode 100644 target/s390x/cpu_topology.c
>>
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> index a6ca006ec5..e2fd5c7e44 100644
>> --- a/hw/s390x/cpu-topology.c
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -76,9 +76,11 @@ void s390_topology_new_cpu(int core_id)
>>        * in the CPU container allows to represent up to the maximal number of
>>        * CPU inside several CPU containers inside the socket container.
>>        */
>> +    qemu_mutex_lock(&topo->topo_mutex);
> 
> You could use a reader writer lock for this, if qemu has that (I didn't
> find any tho).

I can use RCU but we could also consider that the write path is very short.

> 
>>       topo->socket[socket_id].active_count++;
>>       topo->tle[socket_id].active_count++;
>>       set_bit(bit, &topo->tle[socket_id].mask[origin]);
>> +    qemu_mutex_unlock(&topo->topo_mutex);
>>   }
>>   
>>   /**
>> @@ -104,6 +106,8 @@ static void s390_topology_realize(DeviceState *dev, Error **errp)
>>       n = topo->sockets;
>>       topo->socket = g_malloc0(n * sizeof(S390TopoContainer));
>>       topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE));
>> +
>> +    qemu_mutex_init(&topo->topo_mutex);
>>   }
>>   
>>   /**
>> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
>> index 6911f975f4..0b7f3d10b2 100644
>> --- a/include/hw/s390x/cpu-topology.h
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -10,6 +10,10 @@
>>   #ifndef HW_S390X_CPU_TOPOLOGY_H
>>   #define HW_S390X_CPU_TOPOLOGY_H
>>   
>> +#define S390_TOPOLOGY_CPU_TYPE    0x03
> 
> IMO you should add the name of cpu type 0x03 to the name of the
> constant, even if there is only one right now.
> You did the same for the polarity after all.

OK right
#define S390_TOPOLOGY_CPU_IFL 0x03

> 
>> +
>> +#define S390_TOPOLOGY_POLARITY_H  0x00
>> +
>>   typedef struct S390TopoContainer {
>>       int active_count;
>>   } S390TopoContainer;
>> @@ -30,6 +34,7 @@ struct S390Topology {
>>       int tles;
>>       S390TopoContainer *socket;
>>       S390TopoTLE *tle;
>> +    QemuMutex topo_mutex;
>>   };
>>   typedef struct S390Topology S390Topology;
>>   
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 7d6d01325b..c61fe9b563 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -565,6 +565,53 @@ 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;
>> +} SysIBTl_cpu;
>> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16);
>> +
>> +/* Container type Topology List Entry */
>> +typedef struct SysIBTl_container {
>> +        uint8_t nl;
>> +        uint8_t reserved[6];
>> +        uint8_t id;
>> +} QEMU_PACKED SysIBTl_container;
>> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_container) != 8);
>> +
>> +/* Generic Topology List Entry */
>> +typedef union SysIBTl_entry {
>> +        uint8_t nl;
>> +        SysIBTl_container container;
>> +        SysIBTl_cpu cpu;
>> +} SysIBTl_entry;
> 
> This isn't used for anything but the declaration in SysIB_151x, is it?

right.

>> +
>> +#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  reserved0[2];
>> +    uint16_t length;
>> +    uint8_t  mag[TOPOLOGY_NR_MAG];
>> +    uint8_t  reserved1;
>> +    uint8_t  mnest;
>> +    uint32_t reserved2;
>> +    SysIBTl_entry tle[0];
> 
> I would just use uint64_t[0] as type or uint64_t[] whichever is qemu
> style.

OK



> 
>> +} SysIB_151x;
>> +QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
>> +
>>   /* MMU defines */
>>   #define ASCE_ORIGIN           (~0xfffULL) /* segment table origin             */
>>   #define ASCE_SUBSPACE         0x200       /* subspace group control           */
>> @@ -843,4 +890,6 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
>>   
>>   #include "exec/cpu-all.h"
>>   
>> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar);
>> +
>>   #endif
>> diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
>> new file mode 100644
>> index 0000000000..56865dafc6
>> --- /dev/null
>> +++ b/target/s390x/cpu_topology.c
>> @@ -0,0 +1,108 @@
>> +/*
>> + * QEMU S390x CPU Topology
>> + *
>> + * Copyright IBM Corp. 2022
>> + * 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 "cpu.h"
>> +#include "hw/s390x/pv.h"
>> +#include "hw/sysbus.h"
>> +#include "hw/s390x/cpu-topology.h"
>> +#include "hw/s390x/sclp.h"
>> +
>> +static char *fill_container(char *p, int level, int id)
>> +{
>> +    SysIBTl_container *tle = (SysIBTl_container *)p;
>> +
>> +    tle->nl = level;
>> +    tle->id = id;
>> +    return p + sizeof(*tle);
>> +}
>> +
>> +static char *fill_tle_cpu(char *p, uint64_t mask, int origin)
>> +{
>> +    SysIBTl_cpu *tle = (SysIBTl_cpu *)p;
>> +
>> +    tle->nl = 0;
>> +    tle->dedicated = 1;
>> +    tle->polarity = S390_TOPOLOGY_POLARITY_H;
>> +    tle->type = S390_TOPOLOGY_CPU_TYPE;
>> +    tle->origin = origin * 64;
>> +    tle->mask = be64_to_cpu(mask);
> 
> You convert endianess for mask here...
> 
>> +    return p + sizeof(*tle);
>> +}
>> +
>> +static char *s390_top_set_level2(S390Topology *topo, char *p)
>> +{
>> +    int i, origin;
>> +
>> +    for (i = 0; i < topo->sockets; i++) {
>> +        if (!topo->socket[i].active_count) {
>> +            continue;
>> +        }
>> +        p = fill_container(p, 1, i);
>> +        for (origin = 0; origin < S390_TOPOLOGY_MAX_ORIGIN; origin++) {
>> +            uint64_t mask = 0L;
>> +
>> +            mask = be64_to_cpu(topo->tle[i].mask[origin]);
> 
> ...and here. So one has to go, I guess this one.
> Also using cpu_to_be64 seems more intuitive to me.

cpu_to_be64 is the right thing to do.
Also yes, I suppress this one.

> 
>> +            if (mask) {
>> +                p = fill_tle_cpu(p, mask, origin);
>> +            }
>> +        }
>> +    }
>> +    return p;
>> +}
>> +
>> +static int setup_stsi(SysIB_151x *sysib, int level)
>> +{
>> +    S390Topology *topo = s390_get_topology();
>> +    char *p = (char *)sysib->tle;
>> +
>> +    qemu_mutex_lock(&topo->topo_mutex);
>> +
>> +    sysib->mnest = level;
>> +    switch (level) {
>> +    case 2:
>> +        sysib->mag[TOPOLOGY_NR_MAG2] = topo->sockets;
>> +        sysib->mag[TOPOLOGY_NR_MAG1] = topo->cores;
>> +        p = s390_top_set_level2(topo, p);
>> +        break;
>> +    }
>> +
>> +    qemu_mutex_unlock(&topo->topo_mutex);
>> +
>> +    return p - (char *)sysib->tle;
>> +}
>> +
>> +#define S390_TOPOLOGY_MAX_MNEST 2
>> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
>> +{
>> +    SysIB_151x *sysib;
>> +    int len = sizeof(*sysib);
>> +
>> +    if (s390_is_pv() || sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
>> +        setcc(cpu, 3);
>> +        return;
>> +    }
>> +
>> +    sysib = g_malloc0(TARGET_PAGE_SIZE);
> 
> What made you decide against stack allocating this?

I just did not think about it.
I will change it.

> 
>> +
>> +    len += setup_stsi(sysib, sel2);
>> +    if (len > TARGET_PAGE_SIZE) {
> 
> If you do the check here it's too late.

yes.

> 
>> +        setcc(cpu, 3);
>> +        goto out_free;
>> +    }
>> +
>> +    sysib->length = be16_to_cpu(len);
>> +    s390_cpu_virt_mem_write(cpu, addr, ar, sysib, len);
> 
> If the return value of this is <0 it's an error condition.
> If you ignore the value we'll keep running.

I understood that exceptions are handled inside 
s390_cpu_virt_mem_write() and we have no CC to report.

> 
>> +    setcc(cpu, 0);
> 
> Is it correct to set the cc value even if s390_cpu_virt_mem_write
> causes an exception?

I guess that if it caused an exception we do not get so far.
Am I wrong?

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 03/10] s390x/cpu topology: reporting the CPU topology to the guest

[Pierre Morel]

On 9/6/22 10:17, Nico Boehr wrote:
> Quoting Pierre Morel (2022-09-02 09:55:24)
>> The guest can use the STSI instruction to get a buffer filled
>> with the CPU topology description.
>>
>> Let us implement the STSI instruction for the basis CPU topology
>> level, level 2.
> 
> I like this. It is so much simpler. Thanks.
> 
> [...]
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> index a6ca006ec5..e2fd5c7e44 100644
>> --- a/hw/s390x/cpu-topology.c
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -76,9 +76,11 @@ void s390_topology_new_cpu(int core_id)
>>        * in the CPU container allows to represent up to the maximal number of
>>        * CPU inside several CPU containers inside the socket container.
>>        */
>> +    qemu_mutex_lock(&topo->topo_mutex);
> 
> You access topo->cores above. Do you need the mutex for that? I guess not since
> it can't change at runtime (right?), so maybe it is worth documenting what the
> topo_mutex actually protects or you just take the mutex at the start of the
> function.

You are right one should always do that.
I will add this.

> 
> [...]
>> diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
>> new file mode 100644
>> index 0000000000..56865dafc6
>> --- /dev/null
>> +++ b/target/s390x/cpu_topology.c
> [...]
>> +static char *fill_tle_cpu(char *p, uint64_t mask, int origin)
>> +{
>> +    SysIBTl_cpu *tle = (SysIBTl_cpu *)p;
>> +
>> +    tle->nl = 0;
>> +    tle->dedicated = 1;
>> +    tle->polarity = S390_TOPOLOGY_POLARITY_H;
>> +    tle->type = S390_TOPOLOGY_CPU_TYPE;
>> +    tle->origin = origin * 64;
> 
> origin would also need a byte order conversion.

yes

> 
>> +    tle->mask = be64_to_cpu(mask);
> 
> cpu_to_be64()

yes

> 
> [...]
>> +static char *s390_top_set_level2(S390Topology *topo, char *p)
>> +{
>> +    int i, origin;
>> +
>> +    for (i = 0; i < topo->sockets; i++) {
>> +        if (!topo->socket[i].active_count) {
>> +            continue;
>> +        }
>> +        p = fill_container(p, 1, i);
>> +        for (origin = 0; origin < S390_TOPOLOGY_MAX_ORIGIN; origin++) {
>> +            uint64_t mask = 0L;
>> +
>> +            mask = be64_to_cpu(topo->tle[i].mask[origin]);
> 
> Don't you already do the endianness conversion in fill_tle_cpu()?

yes

> 
> [...]
>> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
>> +{
>> +    SysIB_151x *sysib;
>> +    int len = sizeof(*sysib);
>> +
>> +    if (s390_is_pv() || sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
>> +        setcc(cpu, 3);
>> +        return;
>> +    }
>> +
>> +    sysib = g_malloc0(TARGET_PAGE_SIZE);
>> +
>> +    len += setup_stsi(sysib, sel2);
>> +    if (len > TARGET_PAGE_SIZE) {
>> +        setcc(cpu, 3);
>> +        goto out_free;
>> +    }
> 
> Maybe I don't get it, but isn't it kind of late for this check? You would
> already have written beyond the end of the buffer at this point in time...

it is


Thanks for your comments.

regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 02/10] s390x/cpu topology: core_id sets s390x CPU topology

[Pierre Morel]

On 9/27/22 14:03, Cédric Le Goater wrote:
> On 9/2/22 09:55, Pierre Morel wrote:
>> In the S390x CPU topology the core_id specifies the CPU address
>> and the position of the core withing the topology.
>>
>> Let's build the topology based on the core_id.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/cpu-topology.c         | 135 ++++++++++++++++++++++++++++++++
>>   hw/s390x/meson.build            |   1 +
>>   hw/s390x/s390-virtio-ccw.c      |  10 +++
>>   include/hw/s390x/cpu-topology.h |  42 ++++++++++
>>   4 files changed, 188 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..a6ca006ec5
>> --- /dev/null
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -0,0 +1,135 @@
>> +/*
>> + * CPU Topology
>> + *
>> + * Copyright IBM Corp. 2022
>> + * 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/qdev-properties.h"
>> +#include "hw/boards.h"
>> +#include "qemu/typedefs.h"
>> +#include "target/s390x/cpu.h"
>> +#include "hw/s390x/s390-virtio-ccw.h"
>> +#include "hw/s390x/cpu-topology.h"
>> +
>> +S390Topology *s390_get_topology(void)
>> +{
>> +    static S390Topology *s390Topology;
>> +
>> +    if (!s390Topology) {
>> +        s390Topology = S390_CPU_TOPOLOGY(
>> +            object_resolve_path(TYPE_S390_CPU_TOPOLOGY, NULL));
>> +    }
>> +
>> +    return s390Topology;
>> +}
>> +
>> +/*
>> + * s390_topology_new_cpu:
>> + * @core_id: the core ID is machine wide
>> + *
>> + * The topology returned by s390_get_topology(), gives us the CPU
>> + * topology established by the -smp QEMU aruments.
>> + * The core-id gives:
>> + *  - the Container TLE (Topology List Entry) containing the CPU TLE.
>> + *  - in the CPU TLE the origin, or offset of the first bit in the 
>> core mask
>> + *  - the bit in the CPU TLE core mask
>> + */
>> +void s390_topology_new_cpu(int core_id)
>> +{
>> +    S390Topology *topo = s390_get_topology();
>> +    int socket_id;
>> +    int bit, origin;
>> +
>> +    /* In the case no Topology is used nothing is to be done here */
>> +    if (!topo) {
>> +        return;
>> +    }
>> +
>> +    socket_id = core_id / topo->cores;
>> +
>> +    bit = core_id;
>> +    origin = bit / 64;
>> +    bit %= 64;
>> +    bit = 63 - bit;
>> +
>> +    /*
>> +     * At the core level, each CPU is represented by a bit in a 64bit
>> +     * unsigned long. Set on plug and clear on unplug of a CPU.
> 
> Do we have CPU unplug on s390x ?

not in QEMU.

I will change this sentence.

> 
>> +     * The firmware assume that all CPU in a CPU TLE have the same
>> +     * type, polarization and are all dedicated or shared.
>> +     * In the case a socket contains CPU with different type, 
>> polarization
>> +     * or entitlement then they will be defined in different CPU 
>> containers.
>> +     * Currently we assume all CPU are identical IFL CPUs and that 
>> they are
>> +     * all dedicated CPUs.
>> +     * The only reason to have several S390TopologyCores inside a 
>> socket is
>> +     * to have more than 64 CPUs.
>> +     * In that case the origin field, representing the offset of the 
>> first CPU
>> +     * in the CPU container allows to represent up to the maximal 
>> number of
>> +     * CPU inside several CPU containers inside the socket container.
>> +     */
>> +    topo->socket[socket_id].active_count++;
>> +    topo->tle[socket_id].active_count++;
>> +    set_bit(bit, &topo->tle[socket_id].mask[origin]);
>> +}
>> +
>> +/**
>> + * s390_topology_realize:
>> + * @dev: the device state
>> + * @errp: the error pointer (not used)
>> + *
>> + * During realize the machine CPU topology is initialized with the
>> + * QEMU -smp parameters.
>> + * The maximum count of CPU TLE in the all Topology can not be greater
>> + * than the maximum CPUs.
>> + */
>> +static void s390_topology_realize(DeviceState *dev, Error **errp)
>> +{
>> +    MachineState *ms = MACHINE(qdev_get_machine());
> 
> Using qdev_get_machine() is suspicious :)

OK

> 
>> +    S390Topology *topo = S390_CPU_TOPOLOGY(dev);
>> +    int n;
>> +
>> +    topo->sockets = ms->smp.sockets;
>> +    topo->cores = ms->smp.cores;
>> +    topo->tles = ms->smp.max_cpus;
> 
> These look like object properties to me.

It is a temporary store to keep them at hand.
I will see if I keep them afterall.
If I keep them I will make them property.

> 
>> +
>> +    n = topo->sockets;
>> +    topo->socket = g_malloc0(n * sizeof(S390TopoContainer));
>> +    topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE));
>> +}
>> +
>> +/**
>> + * topology_class_init:
>> + * @oc: Object class
>> + * @data: (not used)
>> + *
>> + * A very simple object we will need for reset and migration.
>> + */
>> +static void topology_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +
>> +    dc->realize = s390_topology_realize;
>> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>> +}
> 
> no vmstate ?

it is added in a later patch

> 
>> +static const TypeInfo cpu_topology_info = {
>> +    .name          = TYPE_S390_CPU_TOPOLOGY,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(S390Topology),
>> +    .class_init    = topology_class_init,
>> +};
>> +
>> +static void topology_register(void)
>> +{
>> +    type_register_static(&cpu_topology_info);
>> +}
>> +type_init(topology_register);
>> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
>> index de28a90a57..96d7d7d231 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 b5ca154e2f..15cefd104b 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -43,6 +43,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;
>> @@ -247,6 +248,12 @@ static void ccw_init(MachineState *machine)
>>       /* init memory + setup max page size. Required for the CPU model */
>>       s390_memory_init(machine->ram);
>> +    /* Adding the topology must be done before CPU intialization*/
>> +    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
>> +    object_property_add_child(qdev_get_machine(), 
>> TYPE_S390_CPU_TOPOLOGY,
> 
> No need to use qdev_get_machine(), you have 'machine' above.

OK

> 
>> +                              OBJECT(dev));
>> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> 
> 
> why not store a TYPE_S390_CPU_TOPOLOGY object pointer under the machine
> state for later use ?

May be, I will think about it.

> 
>> +
>>       /* init CPUs (incl. CPU model) early so s390_has_feature() works */
>>       s390_init_cpus(machine);
>> @@ -309,6 +316,9 @@ 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);
>> +    /* Inserting the CPU in the Topology can not fail */
>> +    s390_topology_new_cpu(cpu->env.core_id);
>> +
> 
> in which case, we could use the topo object pointer to insert a new CPU
> id and drop s390_get_topology() which looks overkill.
> 
> I would add the test :
> 
>     if (!S390_CCW_MACHINE(machine)->topology_disable) {
> 
> before inserting to be consistent. But I am anticipating some other
> patch.

It belongs here for bisect so I will add it here.
Thanks.

> 
> C.
> 
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 01/10] s390x/cpus: Make absence of multithreading clear

[Pierre Morel]

On 9/27/22 11:44, Cédric Le Goater wrote:
> On 9/5/22 17:10, Pierre Morel wrote:
>>
>>
>> On 9/5/22 13:32, Nico Boehr wrote:
>>> Quoting Pierre Morel (2022-09-02 09:55:22)
>>>> S390x do not support multithreading in the guest.
>>>> Do not let admin falsely specify multithreading on QEMU
>>>> smp commandline.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> ---
>>>>   hw/s390x/s390-virtio-ccw.c | 3 +++
>>>>   1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>> index 70229b102b..b5ca154e2f 100644
>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>> @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine)
>>>>       MachineClass *mc = MACHINE_GET_CLASS(machine);
>>>>       int i;
>>>> +    /* Explicitely do not support threads */
>>>            ^
>>>            Explicitly
>>>
>>>> +    assert(machine->smp.threads == 1);
>>>
>>> It might be nicer to give a better error message to the user.
>>> What do you think about something like (broken whitespace ahead):
>>>
>>>      if (machine->smp.threads != 1) {if (machine->smp.threads != 1) {
>>>          error_setg(&error_fatal, "More than one thread specified, 
>>> but multithreading unsupported");
>>>          return;
>>>      }
>>>
>>
>>
>> OK, I think I wanted to do this and I changed my mind, obviously, I do 
>> not recall why.
>> I will do almost the same but after a look at error.h I will use 
>> error_report()/exit() instead of error_setg()/return as in:
>>
>>
>> +    /* Explicitly do not support threads */
>> +    if (machine->smp.threads != 1) {
>> +        error_report("More than one thread specified, but 
>> multithreading unsupported");
>> +        exit(1);
>> +    }
> 
> 
> or add an 'Error **errp' parameter to s390_init_cpus() and use error_setg()
> as initially proposed. s390x_new_cpu() would benefit from it also.
> 
OK, Thanks,

Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 08/10] target/s390x: interception of PTF instruction

[Pierre Morel]

On 9/9/22 18:50, Janis Schoetterl-Glausch wrote:
> On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
>> When the host supports the CPU topology facility, the PTF
>> instruction with function code 2 is interpreted by the SIE,
>> provided that the userland hypervizor activates the interpretation
>> by using the KVM_CAP_S390_CPU_TOPOLOGY KVM extension.
>>
>> The PTF instructions with function code 0 and 1 are intercepted
>> and must be emulated by the userland hypervizor.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> 
> Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

Thanks

> 
> See note below.
>> ---
>>   hw/s390x/cpu-topology.c            | 52 ++++++++++++++++++++++++++++++
>>   include/hw/s390x/s390-virtio-ccw.h |  6 ++++
>>   target/s390x/kvm/kvm.c             | 13 ++++++++
>>   3 files changed, 71 insertions(+)
>>
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> index b6bf839e40..7dcaa28ca3 100644
>> --- a/hw/s390x/cpu-topology.c
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -20,6 +20,58 @@
>>   #include "hw/s390x/s390-virtio-ccw.h"
>>   #include "hw/s390x/cpu-topology.h"
>>   #include "migration/vmstate.h"
>> +#include "target/s390x/cpu.h"
>> +#include "hw/s390x/s390-virtio-ccw.h"
>> +
>> +/*
>> + * s390_handle_ptf:
>> + *
>> + * @register 1: contains the function code
>> + *
>> + * Function codes 0 and 1 handle the CPU polarization.
>> + * We assume an horizontal topology, the only one supported currently
>> + * by Linux, consequently we answer to function code 0, requesting
>> + * horizontal polarization that it is already the current polarization
>> + * and reject vertical polarization request without further explanation.
>> + *
>> + * Function code 2 is handling topology changes and is interpreted
>> + * by the SIE.
>> + */
>> +void 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_OPERATION, ra);
>> +        return;
> 
> I'm either expecting this function to return -1 here...
>> +    }
>> +
>> +    if (env->psw.mask & PSW_MASK_PSTATE) {
>> +        s390_program_interrupt(env, PGM_PRIVILEGED, ra);
>> +        return;
>> +    }
>> +
>> +    if (reg & ~S390_TOPO_FC_MASK) {
>> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> +        return;
>> +    }
>> +
>> +    switch (fc) {
>> +    case 0:    /* Horizontal polarization is already set */
>> +        env->regs[r1] |= S390_PTF_REASON_DONE;
>> +        setcc(cpu, 2);
>> +        break;
>> +    case 1:    /* Vertical polarization is not supported */
>> +        env->regs[r1] |= S390_PTF_REASON_NONE;
>> +        setcc(cpu, 2);
>> +        break;
>> +    default:
>> +        /* Note that fc == 2 is interpreted by the SIE */
>> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> +    }
>> +}
> 
> [...]
>>   
>> +static int kvm_handle_ptf(S390CPU *cpu, struct kvm_run *run)
>> +{
>> +    uint8_t r1 = (run->s390_sieic.ipb >> 20) & 0x0f;
>> +
>> +    s390_handle_ptf(cpu, r1, RA_IGNORED);
> 
> ... and this being returned here...
>> +
>> +    return 0;
> 
> ... or this function being void.
>> +}


Yes right, thanks

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 01/10] s390x/cpus: Make absence of multithreading clear

[Pierre Morel]

More thinking about this I will drop this patch for backward 
compatibility and in topology masks treat CPUs as being cores*threads



On 9/28/22 15:21, Pierre Morel wrote:
> 
> 
> On 9/27/22 11:44, Cédric Le Goater wrote:
>> On 9/5/22 17:10, Pierre Morel wrote:
>>>
>>>
>>> On 9/5/22 13:32, Nico Boehr wrote:
>>>> Quoting Pierre Morel (2022-09-02 09:55:22)
>>>>> S390x do not support multithreading in the guest.
>>>>> Do not let admin falsely specify multithreading on QEMU
>>>>> smp commandline.
>>>>>
>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>>> ---
>>>>>   hw/s390x/s390-virtio-ccw.c | 3 +++
>>>>>   1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>>> index 70229b102b..b5ca154e2f 100644
>>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>>> @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine)
>>>>>       MachineClass *mc = MACHINE_GET_CLASS(machine);
>>>>>       int i;
>>>>> +    /* Explicitely do not support threads */
>>>>            ^
>>>>            Explicitly
>>>>
>>>>> +    assert(machine->smp.threads == 1);
>>>>
>>>> It might be nicer to give a better error message to the user.
>>>> What do you think about something like (broken whitespace ahead):
>>>>
>>>>      if (machine->smp.threads != 1) {if (machine->smp.threads != 1) {
>>>>          error_setg(&error_fatal, "More than one thread specified, 
>>>> but multithreading unsupported");
>>>>          return;
>>>>      }
>>>>
>>>
>>>
>>> OK, I think I wanted to do this and I changed my mind, obviously, I 
>>> do not recall why.
>>> I will do almost the same but after a look at error.h I will use 
>>> error_report()/exit() instead of error_setg()/return as in:
>>>
>>>
>>> +    /* Explicitly do not support threads */
>>> +    if (machine->smp.threads != 1) {
>>> +        error_report("More than one thread specified, but 
>>> multithreading unsupported");
>>> +        exit(1);
>>> +    }
>>
>>
>> or add an 'Error **errp' parameter to s390_init_cpus() and use 
>> error_setg()
>> as initially proposed. s390x_new_cpu() would benefit from it also.
>>
> OK, Thanks,
> 
> Pierre
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 01/10] s390x/cpus: Make absence of multithreading clear

[Cédric Le Goater]

On 9/28/22 18:16, Pierre Morel wrote:
> More thinking about this I will drop this patch for backward compatibility and in topology masks treat CPUs as being cores*threads

yes. You never know, people might have set threads=2 in their
domain file (like me). You could give the user a warning though,
with warn_report().

Thanks,

C.

  
> 
> 
> 
> On 9/28/22 15:21, Pierre Morel wrote:
>>
>>
>> On 9/27/22 11:44, Cédric Le Goater wrote:
>>> On 9/5/22 17:10, Pierre Morel wrote:
>>>>
>>>>
>>>> On 9/5/22 13:32, Nico Boehr wrote:
>>>>> Quoting Pierre Morel (2022-09-02 09:55:22)
>>>>>> S390x do not support multithreading in the guest.
>>>>>> Do not let admin falsely specify multithreading on QEMU
>>>>>> smp commandline.
>>>>>>
>>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>>>> ---
>>>>>>   hw/s390x/s390-virtio-ccw.c | 3 +++
>>>>>>   1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>>>> index 70229b102b..b5ca154e2f 100644
>>>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>>>> @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine)
>>>>>>       MachineClass *mc = MACHINE_GET_CLASS(machine);
>>>>>>       int i;
>>>>>> +    /* Explicitely do not support threads */
>>>>>            ^
>>>>>            Explicitly
>>>>>
>>>>>> +    assert(machine->smp.threads == 1);
>>>>>
>>>>> It might be nicer to give a better error message to the user.
>>>>> What do you think about something like (broken whitespace ahead):
>>>>>
>>>>>      if (machine->smp.threads != 1) {if (machine->smp.threads != 1) {
>>>>>          error_setg(&error_fatal, "More than one thread specified, but multithreading unsupported");
>>>>>          return;
>>>>>      }
>>>>>
>>>>
>>>>
>>>> OK, I think I wanted to do this and I changed my mind, obviously, I do not recall why.
>>>> I will do almost the same but after a look at error.h I will use error_report()/exit() instead of error_setg()/return as in:
>>>>
>>>>
>>>> +    /* Explicitly do not support threads */
>>>> +    if (machine->smp.threads != 1) {
>>>> +        error_report("More than one thread specified, but multithreading unsupported");
>>>> +        exit(1);
>>>> +    }
>>>
>>>
>>> or add an 'Error **errp' parameter to s390_init_cpus() and use error_setg()
>>> as initially proposed. s390x_new_cpu() would benefit from it also.
>>>
>> OK, Thanks,
>>
>> Pierre
>>
> 

Re: [PATCH v9 01/10] s390x/cpus: Make absence of multithreading clear

[Daniel P. Berrangé]

On Fri, Sep 02, 2022 at 09:55:22AM +0200, Pierre Morel wrote:
> S390x do not support multithreading in the guest.
> Do not let admin falsely specify multithreading on QEMU
> smp commandline.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 70229b102b..b5ca154e2f 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine)
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>      int i;
>  
> +    /* Explicitely do not support threads */
> +    assert(machine->smp.threads == 1);

What is the functional effect for currently released QEMU versions
if a user has set threads == 2  for an s390 machine ?  Is the
threads setting simply ignored ?

If we want to eliminate this mistake, then there's two possible
options

  * If it had no effect, treat this like a deprecation process
    where we print a warning for 2 releases, and then turn the
    warning into an error. Gives a little grace to fix the config
    mistakes some users might have made, at a time convenient to
    them.

Or

  * If it had effect and we need migration compatibility then forbid
    threads > 1 only for new machine type versions, so existing
    deployed guests are not changed.

With 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 v9 07/10] s390x/cpu_topology: CPU topology migration

[Pierre Morel]

On 9/28/22 10:34, Pierre Morel wrote:
> 
> 
> On 9/8/22 20:04, Janis Schoetterl-Glausch wrote:
>> On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
>>> The migration can only take place if both source and destination
>>> of the migration both use or both do not use the CPU topology
>>> facility.
>>>
>>> We indicate a change in topology during migration postload for the
>>> case the topology changed between source and destination.
>>
>> You always set the report bit after migration, right?
>> In the last series you actually migrated the bit.
>> Why the change? With the code you have actually migrating the bit isn't
>> hard.
> 
> As for the moment the vCPU do not migrate from real CPU I thought based 
> on the remark of Nico that there is no need to set the bit after a 
> migration.
> 
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   hw/s390x/cpu-topology.c         | 79 +++++++++++++++++++++++++++++++++
>>>   include/hw/s390x/cpu-topology.h |  1 +
>>>   target/s390x/cpu-sysemu.c       |  8 ++++
>>>   target/s390x/cpu.h              |  1 +
>>>   4 files changed, 89 insertions(+)
>>>
>>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>>> index 6098d6ea1f..b6bf839e40 100644
>>> --- a/hw/s390x/cpu-topology.c
>>> +++ b/hw/s390x/cpu-topology.c
>>> @@ -19,6 +19,7 @@
>>>   #include "target/s390x/cpu.h"
>>>   #include "hw/s390x/s390-virtio-ccw.h"
>>>   #include "hw/s390x/cpu-topology.h"
>>> +#include "migration/vmstate.h"
>>>   S390Topology *s390_get_topology(void)
>>>   {
>>> @@ -132,6 +133,83 @@ static void s390_topology_reset(DeviceState *dev)
>>>       s390_cpu_topology_reset();
>>>   }
>>> +/**
>>> + * cpu_topology_postload
>>> + * @opaque: a pointer to the S390Topology
>>> + * @version_id: version identifier
>>> + *
>>> + * We check that the topology is used or is not used
>>> + * on both side identically.
>>> + *
>>> + * If the topology is in use we set the Modified Topology Change Report
>>> + * on the destination host.
>>> + */
>>> +static int cpu_topology_postload(void *opaque, int version_id)
>>> +{
>>> +    S390Topology *topo = opaque;
>>> +    int ret;
>>> +
>>> +    if (topo->topology_needed != 
>>> s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
>>
>> Does this function even run if topology_needed is false?
>> In that case there is no data saved, so no reason to load it either.
>> If so you can only check that both the source and the destination have
>> the feature enabled. You would need to always send the topology VMSD in
>> order to check that the feature is disabled.
>>
>> Does qemu allow you to attempt to migrate to a host with another cpu
>> model?
>> If it disallowes that you wouldn't need to do any checks, right?
> 
> hum yes I must rework this
> 
> Thanks,
> Pierre
> 
> 

I think my answer was wrong but it helped to correct a bug:
- Only machines > 7.x will be able to use topology and it is correct 
that migration to another machine is not possible.

- However we keep the possibility to not use the topology in 7.x and we 
are not able to migrate if one end use the topology and the other do not.

So I think cpu_topology_needed() must return true (no test needed) but 
the implementation of presave and postload are OK.

Regards,
Pierre



-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 01/10] s390x/cpus: Make absence of multithreading clear

[Pierre Morel]

On 9/28/22 20:11, Daniel P. Berrangé wrote:
> On Fri, Sep 02, 2022 at 09:55:22AM +0200, Pierre Morel wrote:
>> S390x do not support multithreading in the guest.
>> Do not let admin falsely specify multithreading on QEMU
>> smp commandline.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/s390-virtio-ccw.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 70229b102b..b5ca154e2f 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine)
>>       MachineClass *mc = MACHINE_GET_CLASS(machine);
>>       int i;
>>   
>> +    /* Explicitely do not support threads */
>> +    assert(machine->smp.threads == 1);
> 
> What is the functional effect for currently released QEMU versions
> if a user has set threads == 2  for an s390 machine ?  Is the
> threads setting simply ignored ?

It is not ignored, the number of CPUs per sockets seen by the guest is 
cores*threads

> 
> If we want to eliminate this mistake, then there's two possible
> options
> 
>    * If it had no effect, treat this like a deprecation process
>      where we print a warning for 2 releases, and then turn the
>      warning into an error. Gives a little grace to fix the config
>      mistakes some users might have made, at a time convenient to
>      them.
> 
> Or
> 
>    * If it had effect and we need migration compatibility then forbid
>      threads > 1 only for new machine type versions, so existing
>      deployed guests are not changed.
> 
> With regards,
> Daniel

Thanks for your comments Daniel.
I will need to forbid threads > 1 for new machine.

regards,
Pierre




-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 01/10] s390x/cpus: Make absence of multithreading clear

[Pierre Morel]

On 9/28/22 18:28, Cédric Le Goater wrote:
> On 9/28/22 18:16, Pierre Morel wrote:
>> More thinking about this I will drop this patch for backward 
>> compatibility and in topology masks treat CPUs as being cores*threads
> 
> yes. You never know, people might have set threads=2 in their
> domain file (like me). You could give the user a warning though,
> with warn_report().

More thinking, I come back to the first idea after Daniel comment and 
protect the change with a new machine type version.


> 
> Thanks,
> 
> C.
> 
> 
>>

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 01/10] s390x/cpus: Make absence of multithreading clear

[Cédric Le Goater]

On 10/11/22 09:21, Pierre Morel wrote:
> 
> 
> On 9/28/22 18:28, Cédric Le Goater wrote:
>> On 9/28/22 18:16, Pierre Morel wrote:
>>> More thinking about this I will drop this patch for backward compatibility and in topology masks treat CPUs as being cores*threads
>>
>> yes. You never know, people might have set threads=2 in their
>> domain file (like me). You could give the user a warning though,
>> with warn_report().
> 
> More thinking, I come back to the first idea after Daniel comment and protect the change with a new machine type version.

yes. That would be another machine class attribute to set in the new machine,
may be 'max_threads' to compare with the user provided value.

C.

> 
> 
>>
>> Thanks,
>>
>> C.
>>
>>
>>>
> 

Re: [PATCH v9 00/10] s390x: CPU Topology

[Christian Borntraeger]

Am 02.09.22 um 09:55 schrieb Pierre Morel:
> Hi,
> 
> The implementation of the CPU Topology in QEMU has been drastically
> modified since the last patch series and the number of LOCs has been
> greatly reduced.
> 
> Unnecessary objects have been removed, only a single S390Topology object
> is created to support migration and reset.
> 
> Also a documentation has been added to the series.
> 
> 
> To use these patches, you will need Linux V6-rc1 or newer.
> 
> Mainline patches needed are:
> 
> f5ecfee94493 2022-07-20 KVM: s390: resetting the Topology-Change-Report
> 24fe0195bc19 2022-07-20 KVM: s390: guest support for topology function
> 0130337ec45b 2022-07-20 KVM: s390: Cleanup ipte lock access and SIIF fac..
> 
> Currently this code is for KVM only, I have no idea if it is interesting
> to provide a TCG patch. If ever it will be done in another series.
> 
> To have a better understanding of the S390x CPU Topology and its
> implementation in QEMU you can have a look at the documentation in the
> last patch.
> 
> New in this series
> ==================
> 
>    s390x/cpus: Make absence of multithreading clear
> 
> This patch makes clear that CPU-multithreading is not supported in
> the guest.
> 
>    s390x/cpu topology: core_id sets s390x CPU topology
> 
> This patch uses the core_id to build the container topology
> and the placement of the CPU inside the container.
> 
>    s390x/cpu topology: reporting the CPU topology to the guest
> 
> This patch is based on the fact that the CPU type for guests
> is always IFL, CPUs are always dedicated and the polarity is
> always horizontal.
> This may change in the future.
> 
>    hw/core: introducing drawer and books for s390x
>    s390x/cpu: reporting drawers and books topology to the guest
> 
> These two patches extend the topology handling to add two
> new containers levels above sockets: books and drawers.
> 
> The subject of the last patches is clear enough (I hope).
> 
> Regards,
> Pierre
> 
> Pierre Morel (10):
>    s390x/cpus: Make absence of multithreading clear
>    s390x/cpu topology: core_id sets s390x CPU topology
>    s390x/cpu topology: reporting the CPU topology to the guest
>    hw/core: introducing drawer and books for s390x
>    s390x/cpu: reporting drawers and books topology to the guest
>    s390x/cpu_topology: resetting the Topology-Change-Report
>    s390x/cpu_topology: CPU topology migration
>    target/s390x: interception of PTF instruction
>    s390x/cpu_topology: activating CPU topology


Do we really need a machine property? As far as I can see, old QEMU
cannot  activate the ctop facility with old and new kernel unless it
enables CAP_S390_CPU_TOPOLOGY. I do get
oldqemu .... -cpu z14,ctop=on
qemu-system-s390x: Some features requested in the CPU model are not available in the configuration: ctop

With the newer QEMU we can. So maybe we can simply have a topology (and
then a cpu model feature) in new QEMUs and non in old. the cpu model
would then also fence migration from enabled to disabled.

Re: [PATCH v9 00/10] s390x: CPU Topology

[Pierre Morel]

On 11/16/22 17:51, Christian Borntraeger wrote:
> Am 02.09.22 um 09:55 schrieb Pierre Morel:
>> Hi,
>>
>> The implementation of the CPU Topology in QEMU has been drastically
>> modified since the last patch series and the number of LOCs has been
>> greatly reduced.
>>
>> Unnecessary objects have been removed, only a single S390Topology object
>> is created to support migration and reset.
>>
>> Also a documentation has been added to the series.
>>
>>
>> To use these patches, you will need Linux V6-rc1 or newer.
>>
>> Mainline patches needed are:
>>
>> f5ecfee94493 2022-07-20 KVM: s390: resetting the Topology-Change-Report
>> 24fe0195bc19 2022-07-20 KVM: s390: guest support for topology function
>> 0130337ec45b 2022-07-20 KVM: s390: Cleanup ipte lock access and SIIF 
>> fac..
>>
>> Currently this code is for KVM only, I have no idea if it is interesting
>> to provide a TCG patch. If ever it will be done in another series.
>>
>> To have a better understanding of the S390x CPU Topology and its
>> implementation in QEMU you can have a look at the documentation in the
>> last patch.
>>
>> New in this series
>> ==================
>>
>>    s390x/cpus: Make absence of multithreading clear
>>
>> This patch makes clear that CPU-multithreading is not supported in
>> the guest.
>>
>>    s390x/cpu topology: core_id sets s390x CPU topology
>>
>> This patch uses the core_id to build the container topology
>> and the placement of the CPU inside the container.
>>
>>    s390x/cpu topology: reporting the CPU topology to the guest
>>
>> This patch is based on the fact that the CPU type for guests
>> is always IFL, CPUs are always dedicated and the polarity is
>> always horizontal.
>> This may change in the future.
>>
>>    hw/core: introducing drawer and books for s390x
>>    s390x/cpu: reporting drawers and books topology to the guest
>>
>> These two patches extend the topology handling to add two
>> new containers levels above sockets: books and drawers.
>>
>> The subject of the last patches is clear enough (I hope).
>>
>> Regards,
>> Pierre
>>
>> Pierre Morel (10):
>>    s390x/cpus: Make absence of multithreading clear
>>    s390x/cpu topology: core_id sets s390x CPU topology
>>    s390x/cpu topology: reporting the CPU topology to the guest
>>    hw/core: introducing drawer and books for s390x
>>    s390x/cpu: reporting drawers and books topology to the guest
>>    s390x/cpu_topology: resetting the Topology-Change-Report
>>    s390x/cpu_topology: CPU topology migration
>>    target/s390x: interception of PTF instruction
>>    s390x/cpu_topology: activating CPU topology
> 
> 
> Do we really need a machine property? As far as I can see, old QEMU
> cannot  activate the ctop facility with old and new kernel unless it
> enables CAP_S390_CPU_TOPOLOGY. I do get
> oldqemu .... -cpu z14,ctop=on
> qemu-system-s390x: Some features requested in the CPU model are not 
> available in the configuration: ctop
> 
> With the newer QEMU we can. So maybe we can simply have a topology (and
> then a cpu model feature) in new QEMUs and non in old. the cpu model
> would then also fence migration from enabled to disabled.

OK, I can check this.
In this case migration with topology will be if I understand correctly:

NEW_QEMU/old_machine <-> NEW_QEMU/old_machine OK
While
OLD_QEMU/old_machine <-> NEW_QEMU/old_machine KO
NEW_QEMU/old_machine <-> OLD_QEMU/old_machine KO

Is this something we can accept?

regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 00/10] s390x: CPU Topology

[Pierre Morel]

On 11/17/22 10:31, Pierre Morel wrote:
> 
> 
> On 11/16/22 17:51, Christian Borntraeger wrote:
>> Am 02.09.22 um 09:55 schrieb Pierre Morel:
>>> Hi,
>>>
>>> The implementation of the CPU Topology in QEMU has been drastically
>>> modified since the last patch series and the number of LOCs has been
>>> greatly reduced.
>>>
>>> Unnecessary objects have been removed, only a single S390Topology object
>>> is created to support migration and reset.
>>>
>>> Also a documentation has been added to the series.
>>>
>>>
>>> To use these patches, you will need Linux V6-rc1 or newer.
>>>
>>> Mainline patches needed are:
>>>
>>> f5ecfee94493 2022-07-20 KVM: s390: resetting the Topology-Change-Report
>>> 24fe0195bc19 2022-07-20 KVM: s390: guest support for topology function
>>> 0130337ec45b 2022-07-20 KVM: s390: Cleanup ipte lock access and SIIF 
>>> fac..
>>>
>>> Currently this code is for KVM only, I have no idea if it is interesting
>>> to provide a TCG patch. If ever it will be done in another series.
>>>
>>> To have a better understanding of the S390x CPU Topology and its
>>> implementation in QEMU you can have a look at the documentation in the
>>> last patch.
>>>
>>> New in this series
>>> ==================
>>>
>>>    s390x/cpus: Make absence of multithreading clear
>>>
>>> This patch makes clear that CPU-multithreading is not supported in
>>> the guest.
>>>
>>>    s390x/cpu topology: core_id sets s390x CPU topology
>>>
>>> This patch uses the core_id to build the container topology
>>> and the placement of the CPU inside the container.
>>>
>>>    s390x/cpu topology: reporting the CPU topology to the guest
>>>
>>> This patch is based on the fact that the CPU type for guests
>>> is always IFL, CPUs are always dedicated and the polarity is
>>> always horizontal.
>>> This may change in the future.
>>>
>>>    hw/core: introducing drawer and books for s390x
>>>    s390x/cpu: reporting drawers and books topology to the guest
>>>
>>> These two patches extend the topology handling to add two
>>> new containers levels above sockets: books and drawers.
>>>
>>> The subject of the last patches is clear enough (I hope).
>>>
>>> Regards,
>>> Pierre
>>>
>>> Pierre Morel (10):
>>>    s390x/cpus: Make absence of multithreading clear
>>>    s390x/cpu topology: core_id sets s390x CPU topology
>>>    s390x/cpu topology: reporting the CPU topology to the guest
>>>    hw/core: introducing drawer and books for s390x
>>>    s390x/cpu: reporting drawers and books topology to the guest
>>>    s390x/cpu_topology: resetting the Topology-Change-Report
>>>    s390x/cpu_topology: CPU topology migration
>>>    target/s390x: interception of PTF instruction
>>>    s390x/cpu_topology: activating CPU topology
>>
>>
>> Do we really need a machine property? As far as I can see, old QEMU
>> cannot  activate the ctop facility with old and new kernel unless it
>> enables CAP_S390_CPU_TOPOLOGY. I do get
>> oldqemu .... -cpu z14,ctop=on
>> qemu-system-s390x: Some features requested in the CPU model are not 
>> available in the configuration: ctop
>>
>> With the newer QEMU we can. So maybe we can simply have a topology (and
>> then a cpu model feature) in new QEMUs and non in old. the cpu model
>> would then also fence migration from enabled to disabled.
> 
> OK, I can check this.
> In this case migration with topology will be if I understand correctly:
> 
> NEW_QEMU/old_machine <-> NEW_QEMU/old_machine OK
> While
> OLD_QEMU/old_machine <-> NEW_QEMU/old_machine KO
> NEW_QEMU/old_machine <-> OLD_QEMU/old_machine KO

I forgot to say that I mean in the examples above without using a flag.

Of course using a flag like -ctop=off in NEW_QEMU/new_machine allows
to migrate from and to old_machines in an old QEMU.

Also I had the same behavior already in V9 by having a VMState without 
the creation of a machine property, a new cpu feature and a new cpu flag.






> 
> Is this something we can accept?
> 
> regards,
> Pierre
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 00/10] s390x: CPU Topology

[Pierre Morel]

Gentle ping.

Did I understand the problem or am I wrong?


On 11/17/22 17:38, Pierre Morel wrote:
> 
> 
> On 11/17/22 10:31, Pierre Morel wrote:
>>
>>
>> On 11/16/22 17:51, Christian Borntraeger wrote:
>>> Am 02.09.22 um 09:55 schrieb Pierre Morel:
>>>> Hi,
>>>>
>>>> The implementation of the CPU Topology in QEMU has been drastically
>>>> modified since the last patch series and the number of LOCs has been
>>>> greatly reduced.
>>>>
>>>> Unnecessary objects have been removed, only a single S390Topology 
>>>> object
>>>> is created to support migration and reset.
>>>>
>>>> Also a documentation has been added to the series.
>>>>
>>>>
>>>> To use these patches, you will need Linux V6-rc1 or newer.
>>>>
>>>> Mainline patches needed are:
>>>>
>>>> f5ecfee94493 2022-07-20 KVM: s390: resetting the Topology-Change-Report
>>>> 24fe0195bc19 2022-07-20 KVM: s390: guest support for topology function
>>>> 0130337ec45b 2022-07-20 KVM: s390: Cleanup ipte lock access and SIIF 
>>>> fac..
>>>>
>>>> Currently this code is for KVM only, I have no idea if it is 
>>>> interesting
>>>> to provide a TCG patch. If ever it will be done in another series.
>>>>
>>>> To have a better understanding of the S390x CPU Topology and its
>>>> implementation in QEMU you can have a look at the documentation in the
>>>> last patch.
>>>>
>>>> New in this series
>>>> ==================
>>>>
>>>>    s390x/cpus: Make absence of multithreading clear
>>>>
>>>> This patch makes clear that CPU-multithreading is not supported in
>>>> the guest.
>>>>
>>>>    s390x/cpu topology: core_id sets s390x CPU topology
>>>>
>>>> This patch uses the core_id to build the container topology
>>>> and the placement of the CPU inside the container.
>>>>
>>>>    s390x/cpu topology: reporting the CPU topology to the guest
>>>>
>>>> This patch is based on the fact that the CPU type for guests
>>>> is always IFL, CPUs are always dedicated and the polarity is
>>>> always horizontal.
>>>> This may change in the future.
>>>>
>>>>    hw/core: introducing drawer and books for s390x
>>>>    s390x/cpu: reporting drawers and books topology to the guest
>>>>
>>>> These two patches extend the topology handling to add two
>>>> new containers levels above sockets: books and drawers.
>>>>
>>>> The subject of the last patches is clear enough (I hope).
>>>>
>>>> Regards,
>>>> Pierre
>>>>
>>>> Pierre Morel (10):
>>>>    s390x/cpus: Make absence of multithreading clear
>>>>    s390x/cpu topology: core_id sets s390x CPU topology
>>>>    s390x/cpu topology: reporting the CPU topology to the guest
>>>>    hw/core: introducing drawer and books for s390x
>>>>    s390x/cpu: reporting drawers and books topology to the guest
>>>>    s390x/cpu_topology: resetting the Topology-Change-Report
>>>>    s390x/cpu_topology: CPU topology migration
>>>>    target/s390x: interception of PTF instruction
>>>>    s390x/cpu_topology: activating CPU topology
>>>
>>>
>>> Do we really need a machine property? As far as I can see, old QEMU
>>> cannot  activate the ctop facility with old and new kernel unless it
>>> enables CAP_S390_CPU_TOPOLOGY. I do get
>>> oldqemu .... -cpu z14,ctop=on
>>> qemu-system-s390x: Some features requested in the CPU model are not 
>>> available in the configuration: ctop
>>>
>>> With the newer QEMU we can. So maybe we can simply have a topology (and
>>> then a cpu model feature) in new QEMUs and non in old. the cpu model
>>> would then also fence migration from enabled to disabled.
>>
>> OK, I can check this.
>> In this case migration with topology will be if I understand correctly:
>>
>> NEW_QEMU/old_machine <-> NEW_QEMU/old_machine OK
>> While
>> OLD_QEMU/old_machine <-> NEW_QEMU/old_machine KO
>> NEW_QEMU/old_machine <-> OLD_QEMU/old_machine KO
> 
> I forgot to say that I mean in the examples above without using a flag.
> 
> Of course using a flag like -ctop=off in NEW_QEMU/new_machine allows
> to migrate from and to old_machines in an old QEMU.
> 
> Also I had the same behavior already in V9 by having a VMState without 
> the creation of a machine property, a new cpu feature and a new cpu flag.
> 
> 
> 
> 
> 
> 
>>
>> Is this something we can accept?
>>
>> regards,
>> Pierre
>>
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 00/10] s390x: CPU Topology

[Pierre Morel]

On 11/24/22 10:25, Pierre Morel wrote:
> 
> Gentle ping.
> 
> Did I understand the problem or am I wrong?
> 

I guess I was wrong, so I send a new series next week.

Regards,
Pierre

> 
> On 11/17/22 17:38, Pierre Morel wrote:
>>
>>
>> On 11/17/22 10:31, Pierre Morel wrote:
>>>
>>>
>>> On 11/16/22 17:51, Christian Borntraeger wrote:
>>>> Am 02.09.22 um 09:55 schrieb Pierre Morel:
>>>>> Hi,
>>>>>
>>>>> The implementation of the CPU Topology in QEMU has been drastically
>>>>> modified since the last patch series and the number of LOCs has been
>>>>> greatly reduced.
>>>>>
>>>>> Unnecessary objects have been removed, only a single S390Topology 
>>>>> object
>>>>> is created to support migration and reset.
>>>>>
>>>>> Also a documentation has been added to the series.
>>>>>
>>>>>
>>>>> To use these patches, you will need Linux V6-rc1 or newer.
>>>>>
>>>>> Mainline patches needed are:
>>>>>
>>>>> f5ecfee94493 2022-07-20 KVM: s390: resetting the 
>>>>> Topology-Change-Report
>>>>> 24fe0195bc19 2022-07-20 KVM: s390: guest support for topology function
>>>>> 0130337ec45b 2022-07-20 KVM: s390: Cleanup ipte lock access and 
>>>>> SIIF fac..
>>>>>
>>>>> Currently this code is for KVM only, I have no idea if it is 
>>>>> interesting
>>>>> to provide a TCG patch. If ever it will be done in another series.
>>>>>
>>>>> To have a better understanding of the S390x CPU Topology and its
>>>>> implementation in QEMU you can have a look at the documentation in the
>>>>> last patch.
>>>>>
>>>>> New in this series
>>>>> ==================
>>>>>
>>>>>    s390x/cpus: Make absence of multithreading clear
>>>>>
>>>>> This patch makes clear that CPU-multithreading is not supported in
>>>>> the guest.
>>>>>
>>>>>    s390x/cpu topology: core_id sets s390x CPU topology
>>>>>
>>>>> This patch uses the core_id to build the container topology
>>>>> and the placement of the CPU inside the container.
>>>>>
>>>>>    s390x/cpu topology: reporting the CPU topology to the guest
>>>>>
>>>>> This patch is based on the fact that the CPU type for guests
>>>>> is always IFL, CPUs are always dedicated and the polarity is
>>>>> always horizontal.
>>>>> This may change in the future.
>>>>>
>>>>>    hw/core: introducing drawer and books for s390x
>>>>>    s390x/cpu: reporting drawers and books topology to the guest
>>>>>
>>>>> These two patches extend the topology handling to add two
>>>>> new containers levels above sockets: books and drawers.
>>>>>
>>>>> The subject of the last patches is clear enough (I hope).
>>>>>
>>>>> Regards,
>>>>> Pierre
>>>>>
>>>>> Pierre Morel (10):
>>>>>    s390x/cpus: Make absence of multithreading clear
>>>>>    s390x/cpu topology: core_id sets s390x CPU topology
>>>>>    s390x/cpu topology: reporting the CPU topology to the guest
>>>>>    hw/core: introducing drawer and books for s390x
>>>>>    s390x/cpu: reporting drawers and books topology to the guest
>>>>>    s390x/cpu_topology: resetting the Topology-Change-Report
>>>>>    s390x/cpu_topology: CPU topology migration
>>>>>    target/s390x: interception of PTF instruction
>>>>>    s390x/cpu_topology: activating CPU topology
>>>>
>>>>
>>>> Do we really need a machine property? As far as I can see, old QEMU
>>>> cannot  activate the ctop facility with old and new kernel unless it
>>>> enables CAP_S390_CPU_TOPOLOGY. I do get
>>>> oldqemu .... -cpu z14,ctop=on
>>>> qemu-system-s390x: Some features requested in the CPU model are not 
>>>> available in the configuration: ctop
>>>>
>>>> With the newer QEMU we can. So maybe we can simply have a topology (and
>>>> then a cpu model feature) in new QEMUs and non in old. the cpu model
>>>> would then also fence migration from enabled to disabled.
>>>
>>> OK, I can check this.
>>> In this case migration with topology will be if I understand correctly:
>>>
>>> NEW_QEMU/old_machine <-> NEW_QEMU/old_machine OK
>>> While
>>> OLD_QEMU/old_machine <-> NEW_QEMU/old_machine KO
>>> NEW_QEMU/old_machine <-> OLD_QEMU/old_machine KO
>>
>> I forgot to say that I mean in the examples above without using a flag.
>>
>> Of course using a flag like -ctop=off in NEW_QEMU/new_machine allows
>> to migrate from and to old_machines in an old QEMU.
>>
>> Also I had the same behavior already in V9 by having a VMState without 
>> the creation of a machine property, a new cpu feature and a new cpu flag.
>>
>>
>>
>>
>>
>>
>>>
>>> Is this something we can accept?
>>>
>>> regards,
>>> Pierre
>>>
>>
> 

-- 
Pierre Morel
IBM Lab Boeblingen