[PATCH v10 0/9] s390x: CPU Topology

[PATCH v10 0/9] 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.

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

2) The introduction of drawers and books is deferred to a later version.

3) A new property, topology-disable, is added for new machines for test
   purpose and migration to/from a host without facility 11 from/to a
   host with the facility 11.

Also a documentation has been added to the series.


To use the QEMU patches, you will need Linux V6-rc1 or newer,
or use the following Linux mainline patches:

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 of this series.

The admin will want to match the host and the guest topology, taking
into account that the guest does not recognize multithreading.
Consequently, two vCPU assigned to threads of the same real CPU should
preferably be assigned to the same socket of the guest machine.

Regards,
Pierre

Pierre Morel (9):
  s390x/cpu topology: core_id sets s390x CPU topology
  s390x/cpu topology: reporting the CPU 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: add topology-disable machine property
  s390x/cpu topology: add max_threads machine class attribute
  s390x/cpu_topology: activating CPU topology
  docs/s390x: document s390x cpu topology

 docs/system/s390x/cpu_topology.rst |  80 +++++++++
 include/hw/boards.h                |   3 +
 include/hw/s390x/cpu-topology.h    |  65 +++++++
 include/hw/s390x/s390-virtio-ccw.h |   9 +
 target/s390x/cpu.h                 |  50 ++++++
 target/s390x/kvm/kvm_s390x.h       |   1 +
 hw/core/machine.c                  |   5 +
 hw/s390x/cpu-topology.c            | 279 +++++++++++++++++++++++++++++
 hw/s390x/s390-virtio-ccw.c         |  85 ++++++++-
 target/s390x/cpu-sysemu.c          |  15 ++
 target/s390x/cpu_topology.c        | 109 +++++++++++
 target/s390x/kvm/kvm.c             |  56 +++++-
 util/qemu-config.c                 |   4 +
 hw/s390x/meson.build               |   1 +
 qemu-options.hx                    |   6 +-
 target/s390x/meson.build           |   1 +
 16 files changed, 766 insertions(+), 3 deletions(-)
 create mode 100644 docs/system/s390x/cpu_topology.rst
 create mode 100644 include/hw/s390x/cpu-topology.h
 create mode 100644 hw/s390x/cpu-topology.c
 create mode 100644 target/s390x/cpu_topology.c

-- 
2.31.1

Changelog:

- since v9

- remove books and drawers

- remove thread denying and replace with a merge
  of cores * threads to specify the CPUs available
  to the guest

- add a class option to avoid topology on older
  machines
  (Cedric)

- Allocate a SYSIB buffer of the maximal length to
  avoid overflow.
  (Nico, Janis)

- suppress redundancy of smp parameters in topology
  and use directly the machine smp structure

- Early check for topology support
  (Cedric)

- 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 v10 1/9] 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.
s390x/cpu topology: core_id sets s390x CPU topology

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

Let's build the topology based on the core_id.

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

diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
new file mode 100644
index 0000000000..66c171d0bc
--- /dev/null
+++ b/include/hw/s390x/cpu-topology.h
@@ -0,0 +1,45 @@
+/*
+ * 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
+
+#include "hw/qdev-core.h"
+#include "qom/object.h"
+
+typedef struct S390TopoContainer {
+    int active_count;
+} S390TopoContainer;
+
+#define S390_TOPOLOGY_CPU_IFL 0x03
+#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
+typedef struct S390TopoTLE {
+    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
+} S390TopoTLE;
+
+struct S390Topology {
+    SysBusDevice parent_obj;
+    int cpus;
+    S390TopoContainer *socket;
+    S390TopoTLE *tle;
+    MachineState *ms;
+};
+
+#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);
+
+static inline bool s390_has_topology(void)
+{
+    return false;
+}
+
+#endif
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
new file mode 100644
index 0000000000..42b22a1831
--- /dev/null
+++ b/hw/s390x/cpu-topology.c
@@ -0,0 +1,132 @@
+/*
+ * 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->cpus;
+
+    /*
+     * At the core level, each CPU is represented by a bit in a 64bit
+     * unsigned long which represent the presence 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 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.
+     * The only reason to have several S390TopologyCores inside a socket is
+     * to have more than 64 CPUs.
+     * 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.
+     */
+    bit = core_id;
+    origin = bit / 64;
+    bit %= 64;
+    bit = 63 - bit;
+
+    topo->socket[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);
+
+    topo->cpus = ms->smp.cores * ms->smp.threads;
+
+    topo->socket = g_new0(S390TopoContainer, ms->smp.sockets);
+    topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
+
+    topo->ms = ms;
+}
+
+/**
+ * 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/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 03855c7231..aa99a62e42 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;
 
@@ -94,6 +95,18 @@ static void s390_init_cpus(MachineState *machine)
     }
 }
 
+static void s390_init_topology(MachineState *machine)
+{
+    DeviceState *dev;
+
+    if (s390_has_topology()) {
+        dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
+        object_property_add_child(&machine->parent_obj,
+                                  TYPE_S390_CPU_TOPOLOGY, OBJECT(dev));
+        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    }
+}
+
 static const char *const reset_dev_types[] = {
     TYPE_VIRTUAL_CSS_BRIDGE,
     "s390-sclp-event-facility",
@@ -244,6 +257,9 @@ 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 */
+    s390_init_topology(machine);
+
     /* init CPUs (incl. CPU model) early so s390_has_feature() works */
     s390_init_cpus(machine);
 
@@ -306,6 +322,11 @@ 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 */
+    if (s390_has_topology()) {
+        s390_topology_new_cpu(cpu->env.core_id);
+    }
+
     if (dev->hotplugged) {
         raise_irq_cpu_hotplug();
     }
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index f291016fee..653f6ab488 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',
-- 
2.31.1

[PATCH v10 2/9] 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>
---
 include/hw/s390x/cpu-topology.h |   3 +
 target/s390x/cpu.h              |  48 ++++++++++++++
 hw/s390x/cpu-topology.c         |   8 ++-
 target/s390x/cpu_topology.c     | 109 ++++++++++++++++++++++++++++++++
 target/s390x/kvm/kvm.c          |   6 +-
 target/s390x/meson.build        |   1 +
 6 files changed, 172 insertions(+), 3 deletions(-)
 create mode 100644 target/s390x/cpu_topology.c

diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
index 66c171d0bc..61c11db017 100644
--- a/include/hw/s390x/cpu-topology.h
+++ b/include/hw/s390x/cpu-topology.h
@@ -13,6 +13,8 @@
 #include "hw/qdev-core.h"
 #include "qom/object.h"
 
+#define S390_TOPOLOGY_POLARITY_H  0x00
+
 typedef struct S390TopoContainer {
     int active_count;
 } S390TopoContainer;
@@ -29,6 +31,7 @@ struct S390Topology {
     S390TopoContainer *socket;
     S390TopoTLE *tle;
     MachineState *ms;
+    QemuMutex topo_mutex;
 };
 
 #define TYPE_S390_CPU_TOPOLOGY "s390-topology"
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7d6d01325b..d604aa9c78 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -565,6 +565,52 @@ typedef union SysIB {
 } SysIB;
 QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
 
+/* CPU type Topology List Entry */
+typedef struct SysIBTl_cpu {
+        uint8_t nl;
+        uint8_t reserved0[3];
+        uint8_t reserved1:5;
+        uint8_t dedicated:1;
+        uint8_t polarity:2;
+        uint8_t type;
+        uint16_t origin;
+        uint64_t mask;
+} QEMU_PACKED QEMU_ALIGNED(8) 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 QEMU_ALIGNED(8) SysIBTl_container;
+QEMU_BUILD_BUG_ON(sizeof(SysIBTl_container) != 8);
+
+#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;
+    char tle[0];
+} QEMU_PACKED QEMU_ALIGNED(8) SysIB_151x;
+QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
+
+/* Maxi size of a SYSIB structure is when all CPU are alone in a container */
+#define S390_TOPOLOGY_SYSIB_SIZE (sizeof(SysIB_151x) +                         \
+                                  S390_MAX_CPUS * (sizeof(SysIBTl_container) + \
+                                                   sizeof(SysIBTl_cpu)))
+
+
 /* MMU defines */
 #define ASCE_ORIGIN           (~0xfffULL) /* segment table origin             */
 #define ASCE_SUBSPACE         0x200       /* subspace group control           */
@@ -843,4 +889,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/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index 42b22a1831..c73cebfe6f 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -54,8 +54,6 @@ void s390_topology_new_cpu(int core_id)
         return;
     }
 
-    socket_id = core_id / topo->cpus;
-
     /*
      * At the core level, each CPU is represented by a bit in a 64bit
      * unsigned long which represent the presence of a CPU.
@@ -76,8 +74,13 @@ void s390_topology_new_cpu(int core_id)
     bit %= 64;
     bit = 63 - bit;
 
+    qemu_mutex_lock(&topo->topo_mutex);
+
+    socket_id = core_id / topo->cpus;
     topo->socket[socket_id].active_count++;
     set_bit(bit, &topo->tle[socket_id].mask[origin]);
+
+    qemu_mutex_unlock(&topo->topo_mutex);
 }
 
 /**
@@ -101,6 +104,7 @@ static void s390_topology_realize(DeviceState *dev, Error **errp)
     topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
 
     topo->ms = ms;
+    qemu_mutex_init(&topo->topo_mutex);
 }
 
 /**
diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
new file mode 100644
index 0000000000..df86a98f23
--- /dev/null
+++ b/target/s390x/cpu_topology.c
@@ -0,0 +1,109 @@
+/*
+ * 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"
+
+#define S390_TOPOLOGY_MAX_STSI_SIZE (S390_MAX_CPUS *              \
+                                     (sizeof(SysIB_151x) +        \
+                                      sizeof(SysIBTl_container) + \
+                                      sizeof(SysIBTl_cpu)))
+
+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_IFL;
+    tle->origin = cpu_to_be64(origin * 64);
+    tle->mask = cpu_to_be64(mask);
+    return p + sizeof(*tle);
+}
+
+static char *s390_top_set_level2(S390Topology *topo, char *p)
+{
+    MachineState *ms = topo->ms;
+    int i, origin;
+
+    for (i = 0; i < ms->smp.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 = 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();
+    MachineState *ms = topo->ms;
+    char *p = sysib->tle;
+
+    qemu_mutex_lock(&topo->topo_mutex);
+
+    sysib->mnest = level;
+    switch (level) {
+    case 2:
+        sysib->mag[TOPOLOGY_NR_MAG2] = ms->smp.sockets;
+        sysib->mag[TOPOLOGY_NR_MAG1] = topo->cpus;
+        p = s390_top_set_level2(topo, p);
+        break;
+    }
+
+    qemu_mutex_unlock(&topo->topo_mutex);
+
+    return p - (char *) sysib;
+}
+
+#define S390_TOPOLOGY_MAX_MNEST 2
+
+void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
+{
+    uint64_t page[S390_TOPOLOGY_SYSIB_SIZE / sizeof(uint64_t)] = {};
+    SysIB_151x *sysib = (SysIB_151x *) page;
+    int len;
+
+    if (s390_is_pv() || !s390_has_topology() ||
+        sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
+        setcc(cpu, 3);
+        return;
+    }
+
+    len = setup_stsi(sysib, sel2);
+
+    sysib->length = cpu_to_be16(len);
+    s390_cpu_virt_mem_write(cpu, addr, ar, sysib, len);
+    setcc(cpu, 0);
+}
+
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 v10 3/9] 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>
Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 target/s390x/cpu.h           |  1 +
 target/s390x/kvm/kvm_s390x.h |  1 +
 hw/s390x/cpu-topology.c      | 12 ++++++++++++
 hw/s390x/s390-virtio-ccw.c   |  1 +
 target/s390x/cpu-sysemu.c    |  7 +++++++
 target/s390x/kvm/kvm.c       | 23 +++++++++++++++++++++++
 6 files changed, 45 insertions(+)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index d604aa9c78..9b35795ac8 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -825,6 +825,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_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 */
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index c73cebfe6f..9f202621d0 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -107,6 +107,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
@@ -120,6 +131,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 aa99a62e42..362378454a 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -113,6 +113,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/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;
+}
-- 
2.31.1

[PATCH v10 4/9] 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>
---
 include/hw/s390x/cpu-topology.h |  1 +
 target/s390x/cpu.h              |  1 +
 hw/s390x/cpu-topology.c         | 79 +++++++++++++++++++++++++++++++++
 target/s390x/cpu-sysemu.c       |  8 ++++
 4 files changed, 89 insertions(+)

diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
index 61c11db017..35a8a981ec 100644
--- a/include/hw/s390x/cpu-topology.h
+++ b/include/hw/s390x/cpu-topology.h
@@ -28,6 +28,7 @@ typedef struct S390TopoTLE {
 struct S390Topology {
     SysBusDevice parent_obj;
     int cpus;
+    bool topology_needed;
     S390TopoContainer *socket;
     S390TopoTLE *tle;
     MachineState *ms;
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 9b35795ac8..8495bfafde 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -826,6 +826,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
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index 9f202621d0..349f0ad89d 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)
 {
@@ -118,6 +119,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_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_topology()) {
+        return 0;
+    }
+
+    /* We support CPU Topology, set the MTCR unconditionally */
+    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_topology();
+    return 0;
+}
+
+/**
+ * cpu_topology_needed:
+ * @opaque: The pointer to the S390Topology
+ *
+ * We always need to know if source and destination use the topology.
+ */
+static bool cpu_topology_needed(void *opaque)
+{
+    return true;
+}
+
+
+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
@@ -132,6 +210,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/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;
+}
-- 
2.31.1

[PATCH v10 5/9] 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>
Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 include/hw/s390x/s390-virtio-ccw.h |  6 ++++
 hw/s390x/cpu-topology.c            | 52 ++++++++++++++++++++++++++++++
 target/s390x/kvm/kvm.c             | 11 +++++++
 3 files changed, 69 insertions(+)

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/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index 349f0ad89d..2ad516a97d 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/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 9c994d27d5..49a99931a4 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,13 @@ static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
     }
 }
 
+static void 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);
+}
+
 static int handle_b9(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
 {
     int r = 0;
@@ -1480,6 +1488,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:
+        kvm_handle_ptf(cpu, run);
+        break;
     case PRIV_B9_EQBS:
         /* just inject exception */
         r = -1;
-- 
2.31.1

[PATCH v10 6/9] s390x/cpu topology: add topology-disable machine property

[Pierre Morel]

S390 CPU topology is only allowed for s390-virtio-ccw-7.3 and
newer S390 machines.
We keep the possibility to disable the topology on these newer
machines with the property topology-disable.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 include/hw/boards.h                |  3 ++
 include/hw/s390x/cpu-topology.h    | 18 +++++++++-
 include/hw/s390x/s390-virtio-ccw.h |  2 ++
 hw/core/machine.c                  |  5 +++
 hw/s390x/s390-virtio-ccw.c         | 53 +++++++++++++++++++++++++++++-
 util/qemu-config.c                 |  4 +++
 qemu-options.hx                    |  6 +++-
 7 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 311ed17e18..67147c47bf 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -379,6 +379,9 @@ struct MachineState {
     } \
     type_init(machine_initfn##_register_types)
 
+extern GlobalProperty hw_compat_7_2[];
+extern const size_t hw_compat_7_2_len;
+
 extern GlobalProperty hw_compat_7_1[];
 extern const size_t hw_compat_7_1_len;
 
diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
index 35a8a981ec..747c9ab4c6 100644
--- a/include/hw/s390x/cpu-topology.h
+++ b/include/hw/s390x/cpu-topology.h
@@ -12,6 +12,8 @@
 
 #include "hw/qdev-core.h"
 #include "qom/object.h"
+#include "cpu.h"
+#include "hw/s390x/s390-virtio-ccw.h"
 
 #define S390_TOPOLOGY_POLARITY_H  0x00
 
@@ -43,7 +45,21 @@ void s390_topology_new_cpu(int core_id);
 
 static inline bool s390_has_topology(void)
 {
-    return false;
+    static S390CcwMachineState *ccw;
+    Object *obj;
+
+    if (ccw) {
+        return !ccw->topology_disable;
+    }
+
+    /* we have to bail out for the "none" machine */
+    obj = object_dynamic_cast(qdev_get_machine(),
+                              TYPE_S390_CCW_MACHINE);
+    if (!obj) {
+        return false;
+    }
+    ccw = S390_CCW_MACHINE(obj);
+    return !ccw->topology_disable;
 }
 
 #endif
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 9e7a0d75bc..6c4b4645fc 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];
 };
 
@@ -46,6 +47,7 @@ struct S390CcwMachineClass {
     bool cpu_model_allowed;
     bool css_migration_enabled;
     bool hpage_1m_allowed;
+    bool topology_allowed;
 };
 
 /* runtime-instrumentation allowed by the machine */
diff --git a/hw/core/machine.c b/hw/core/machine.c
index aa520e74a8..93c497655e 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_2[] = {
+    { "s390-topology", "topology-disable", "true" },
+};
+const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
+
 GlobalProperty hw_compat_7_1[] = {};
 const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
 
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 362378454a..3a13fad4df 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -616,6 +616,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     s390mc->cpu_model_allowed = true;
     s390mc->css_migration_enabled = true;
     s390mc->hpage_1m_allowed = true;
+    s390mc->topology_allowed = true;
     mc->init = ccw_init;
     mc->reset = s390_machine_reset;
     mc->block_default_type = IF_VIRTIO;
@@ -726,6 +727,27 @@ bool hpage_1m_allowed(void)
     return get_machine_class()->hpage_1m_allowed;
 }
 
+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);
+
+    if (!get_machine_class()->topology_allowed) {
+        error_setg(errp, "Property topology-disable not available on machine %s",
+                   get_machine_class()->parent_class.name);
+        return;
+    }
+
+    ms->topology_disable = value;
+}
+
 static char *machine_get_loadparm(Object *obj, Error **errp)
 {
     S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
@@ -784,6 +806,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 CPU topology");
+    object_property_set_bool(obj, "topology-disable", false, NULL);
 }
 
 static const TypeInfo ccw_machine_info = {
@@ -836,14 +865,36 @@ bool css_migration_enabled(void)
     }                                                                         \
     type_init(ccw_machine_register_##suffix)
 
+static void ccw_machine_7_3_instance_options(MachineState *machine)
+{
+}
+
+static void ccw_machine_7_3_class_options(MachineClass *mc)
+{
+}
+DEFINE_CCW_MACHINE(7_3, "7.3", true);
+
 static void ccw_machine_7_2_instance_options(MachineState *machine)
 {
+    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
+
+    ccw_machine_7_3_instance_options(machine);
+    ms->topology_disable = true;
 }
 
 static void ccw_machine_7_2_class_options(MachineClass *mc)
 {
+    S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
+    static GlobalProperty compat[] = {
+        { TYPE_S390_CPU_TOPOLOGY, "topology-allowed", "off", },
+    };
+
+    ccw_machine_7_3_class_options(mc);
+    compat_props_add(mc->compat_props, hw_compat_7_2, hw_compat_7_2_len);
+    compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
+    s390mc->topology_allowed = false;
 }
-DEFINE_CCW_MACHINE(7_2, "7.2", true);
+DEFINE_CCW_MACHINE(7_2, "7.2", false);
 
 static void ccw_machine_7_1_instance_options(MachineState *machine)
 {
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 5325f6bf80..c19e8bc8f3 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -240,6 +240,10 @@ static QemuOptsList machine_opts = {
             .name = "zpcii-disable",
             .type = QEMU_OPT_BOOL,
             .help = "disable zPCI interpretation facilities",
+        },{
+            .name = "topology-disable",
+            .type = QEMU_OPT_BOOL,
+            .help = "disable CPU topology",
         },
         { /* End of list */ }
     }
diff --git a/qemu-options.hx b/qemu-options.hx
index 95b998a13b..c804b0f899 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -38,7 +38,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
     "                hmat=on|off controls ACPI HMAT support (default=off)\n"
     "                memory-backend='backend-id' specifies explicitly provided backend for main RAM (default=none)\n"
     "                cxl-fmw.0.targets.0=firsttarget,cxl-fmw.0.targets.1=secondtarget,cxl-fmw.0.size=size[,cxl-fmw.0.interleave-granularity=granularity]\n"
-    "                zpcii-disable=on|off disables zPCI interpretation facilities (default=off)\n",
+    "                zpcii-disable=on|off disables zPCI interpretation facilities (default=off)\n"
+    "                topology-disable=on|off disables CPU topology (default=off)\n",
     QEMU_ARCH_ALL)
 SRST
 ``-machine [type=]name[,prop=value[,...]]``
@@ -163,6 +164,9 @@ SRST
         Disables zPCI interpretation facilties on s390-ccw hosts.
         This feature can be used to disable hardware virtual assists
         related to zPCI devices. The default is off.
+
+    ``topology-disable=on|off``
+        Disables CPU topology on for S390 machines starting with s390-ccw-virtio-7.3.
 ERST
 
 DEF("M", HAS_ARG, QEMU_OPTION_M,
-- 
2.31.1

[PATCH v10 7/9] s390x/cpu topology: add max_threads machine class attribute

[Pierre Morel]

The S390 CPU topology accepts the smp.threads argument while
in reality it does not effectively allow multthreading.

Let's keep this behavior for machines older than 7.3 and
refuse to use threads in newer machines until multithreading
is really proposed to the guest by the machine.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 include/hw/s390x/s390-virtio-ccw.h |  1 +
 hw/s390x/s390-virtio-ccw.c         | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 6c4b4645fc..319dfac1bb 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -48,6 +48,7 @@ struct S390CcwMachineClass {
     bool css_migration_enabled;
     bool hpage_1m_allowed;
     bool topology_allowed;
+    int max_threads;
 };
 
 /* runtime-instrumentation allowed by the machine */
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 3a13fad4df..d6ce31d168 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -85,8 +85,15 @@ out:
 static void s390_init_cpus(MachineState *machine)
 {
     MachineClass *mc = MACHINE_GET_CLASS(machine);
+    S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
     int i;
 
+    if (machine->smp.threads > s390mc->max_threads) {
+        error_report("S390 does not support more than %d threads.",
+                     s390mc->max_threads);
+        exit(1);
+    }
+
     /* initialize possible_cpus */
     mc->possible_cpu_arch_ids(machine);
 
@@ -617,6 +624,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     s390mc->css_migration_enabled = true;
     s390mc->hpage_1m_allowed = true;
     s390mc->topology_allowed = true;
+    s390mc->max_threads = 1;
     mc->init = ccw_init;
     mc->reset = s390_machine_reset;
     mc->block_default_type = IF_VIRTIO;
@@ -887,12 +895,14 @@ static void ccw_machine_7_2_class_options(MachineClass *mc)
     S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
     static GlobalProperty compat[] = {
         { TYPE_S390_CPU_TOPOLOGY, "topology-allowed", "off", },
+        { TYPE_S390_CPU_TOPOLOGY, "max_threads", "off", },
     };
 
     ccw_machine_7_3_class_options(mc);
     compat_props_add(mc->compat_props, hw_compat_7_2, hw_compat_7_2_len);
     compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
     s390mc->topology_allowed = false;
+    s390mc->max_threads = S390_MAX_CPUS;
 }
 DEFINE_CCW_MACHINE(7_2, "7.2", false);
 
-- 
2.31.1

[PATCH v10 8/9] s390x/cpu_topology: activating CPU topology

[Pierre Morel]

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 available in QEMU and in KVM.

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

diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 49a99931a4..1c4e24c9a7 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -2464,6 +2464,22 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
         set_bit(S390_FEAT_UNPACK, model->features);
     }
 
+    /*
+     * If we have the CPU Topology allowed in QEMU and
+     * implemented in KVM, activate the CPU TOPOLOGY feature.
+     */
+    if (s390_has_topology()) {
+        if (!kvm_check_extension(kvm_state, KVM_CAP_S390_CPU_TOPOLOGY)) {
+            error_setg(errp, "KVM: S390 topology not present in kernel");
+            return;
+        }
+        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 v10 9/9] 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 | 80 ++++++++++++++++++++++++++++++
 1 file changed, 80 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..1dcd24cbbc
--- /dev/null
+++ b/docs/system/s390x/cpu_topology.rst
@@ -0,0 +1,80 @@
+CPU Topology on s390x
+=====================
+
+CPU Topology on S390x provides up to 5 levels of topology containers:
+nodes, drawers, books, sockets and CPUs.
+While the 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 parent container.
+
+Currently QEMU CPU topology uses a single level of container: the sockets.
+
+For backward compatibility, threads can be declared on the ``-smp`` command
+line. They will be seen as CPUs by the guest as long as multithreading
+is not really supported by QEMU for S390.
+
+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
+of QEMU, we use a capability, ``KVM_CAP_S390_CPU_TOPOLOGY``, to notify KVM
+QEMU use of the CPU Topology.
+
+Indicating the CPU topology to the Virtual Machine
+--------------------------------------------------
+
+The CPU Topology, can be specified on the QEMU command line
+with the ``-smp`` or the ``-device`` qemu command arguments.
+
+Like in :
+
+.. code-block:: sh
+    -smp cpus=5,sockets=8,cores=2,threads=2,maxcpus=32
+    -device host-s390x-cpu,core-id=14
+
+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 up to maxcpus.
+
+In the example above:
+
+* There are 5 cpus provided to the guest with the ``-smp`` command line
+  They will take the core-ids 0,1,2,3,4
+  As we have 2 threads in 2 cores in a socket, we have 4 cpus provided
+  to the guest in socket 0, with core-ids 0,1,2,3.
+  The last cpu, with core-id 4, will be on socket 1.
+
+* the core with ID 14 provided by the ``-device`` command line will
+  be placed in socket 3, with core-id 14
+
+* the core with ID 9 provided by the ``device_add`` qmp command will
+  be placed in socket 2, with core-id 9
+
+Note that the core ID is machine wide and the CPU TLE masks provided
+by the STSI instruction will be:
+
+* in socket 0: 0xf0000000 (core id 0,1,2,3)
+* in socket 1: 0x00400000 (core id 9)
+* in socket 1: 0x00020000 (core id 14)
+
+Migration
+---------
+
+For virtio-ccw machines older than s390-virtio-ccw-7.3, CPU Topoogy is
+unavailable.
+
+CPU Topoogy is by default enabled for s390-virtio-ccw-7.3 and newer machines.
+
+Disabling CPU topology can be done by setting the global option
+``topology-disable`` to ``on`` like in:
+
+.. code-block:: sh
+   -machine s390-ccw-virtio-7.3,accel=kvm,topology-disable=on
-- 
2.31.1

Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

[Cédric Le Goater]

Hello Pierre,

On 10/12/22 18:20, 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.
> s390x/cpu topology: core_id sets s390x CPU topology
> 
> In the S390x CPU topology the core_id specifies the CPU address
> and the position of the cpu withing the topology.
> 
> Let's build the topology based on the core_id.

The commit log is doubled.

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   include/hw/s390x/cpu-topology.h |  45 +++++++++++
>   hw/s390x/cpu-topology.c         | 132 ++++++++++++++++++++++++++++++++
>   hw/s390x/s390-virtio-ccw.c      |  21 +++++
>   hw/s390x/meson.build            |   1 +
>   4 files changed, 199 insertions(+)
>   create mode 100644 include/hw/s390x/cpu-topology.h
>   create mode 100644 hw/s390x/cpu-topology.c
> 
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> new file mode 100644
> index 0000000000..66c171d0bc
> --- /dev/null
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -0,0 +1,45 @@
> +/*
> + * 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
> +
> +#include "hw/qdev-core.h"
> +#include "qom/object.h"
> +
> +typedef struct S390TopoContainer {
> +    int active_count;
> +} S390TopoContainer;

This structure does not seem very useful.

> +
> +#define S390_TOPOLOGY_CPU_IFL 0x03
> +#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
> +typedef struct S390TopoTLE { 

The 'Topo' is redundant as TLE stands for 'topology-list entry'. This is minor.

> +    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
> +} S390TopoTLE;
> +
> +struct S390Topology {
> +    SysBusDevice parent_obj;
> +    int cpus;
> +    S390TopoContainer *socket;
> +    S390TopoTLE *tle;
> +    MachineState *ms;

hmm, it would be cleaner to introduce the fields and properties needed
by the S390Topology model and avoid dragging the machine object pointer.
AFAICT, these properties would be :

   "nr-cpus"
   "max-cpus"
   "nr-sockets"



> +};
> +
> +#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);
> +
> +static inline bool s390_has_topology(void)
> +{
> +    return false;
> +}
> +
> +#endif
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> new file mode 100644
> index 0000000000..42b22a1831
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c
> @@ -0,0 +1,132 @@
> +/*
> + * CPU Topology
> + *
> + * Copyright IBM Corp. 2022

The Copyright tag is different in the .h file.
  
> + * 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;

I am not convinced this routine is useful. The s390Topology pointer
could be stored under the machine state I think. It wouldn't be a
problem when CPUs are hot plugged since we have access to the machine
in the hot plug handler.

For the stsi call, 'struct ArchCPU' probably lacks a back pointer to
the machine objects with which CPU interact. These are typically
interrupt controllers or this new s390Topology model. You could add
the pointer there or, better, under a generic 'void *opaque' attribute.

That said, what you did works fine. The modeling could be cleaner.

> +}
> +
> +/*
> + * 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;
> +    }

I would move this test in the caller.

> +
> +    socket_id = core_id / topo->cpus;
> +
> +    /*
> +     * At the core level, each CPU is represented by a bit in a 64bit
> +     * unsigned long which represent the presence 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 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.
> +     * The only reason to have several S390TopologyCores inside a socket is
> +     * to have more than 64 CPUs.
> +     * 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.
> +     */
> +    bit = core_id;
> +    origin = bit / 64;
> +    bit %= 64;
> +    bit = 63 - bit;
> +
> +    topo->socket[socket_id].active_count++;
> +    set_bit(bit, &topo->tle[socket_id].mask[origin]);

here, the tle array is indexed with a socket id and ...

> +}
> +
> +/**
> + * 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);
> +
> +    topo->cpus = ms->smp.cores * ms->smp.threads;> +
> +    topo->socket = g_new0(S390TopoContainer, ms->smp.sockets);
> +    topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);


... here, the tle array is allocated with max_cpus and this looks
weird. I will dig the specs to try to understand.

> +
> +    topo->ms = ms;

See comment above regarding the properties.

> +}
> +
> +/**
> + * 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/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 03855c7231..aa99a62e42 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;
>   
> @@ -94,6 +95,18 @@ static void s390_init_cpus(MachineState *machine)
>       }
>   }
>   
> +static void s390_init_topology(MachineState *machine)
> +{
> +    DeviceState *dev;
> +
> +    if (s390_has_topology()) {
> +        dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
> +        object_property_add_child(&machine->parent_obj,
> +                                  TYPE_S390_CPU_TOPOLOGY, OBJECT(dev));
> +        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    }
> +}
> +
>   static const char *const reset_dev_types[] = {
>       TYPE_VIRTUAL_CSS_BRIDGE,
>       "s390-sclp-event-facility",
> @@ -244,6 +257,9 @@ 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 */

initialization

> +    s390_init_topology(machine);
> +
>       /* init CPUs (incl. CPU model) early so s390_has_feature() works */
>       s390_init_cpus(machine);
>   
> @@ -306,6 +322,11 @@ 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 */
> +    if (s390_has_topology()) {
> +        s390_topology_new_cpu(cpu->env.core_id);
> +    }
> +
>       if (dev->hotplugged) {
>           raise_irq_cpu_hotplug();
>       }
> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> index f291016fee..653f6ab488 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',

Re: [PATCH v10 2/9] s390x/cpu topology: reporting the CPU topology to the guest

[Cédric Le Goater]

On 10/12/22 18:21, 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>
> ---
>   include/hw/s390x/cpu-topology.h |   3 +
>   target/s390x/cpu.h              |  48 ++++++++++++++
>   hw/s390x/cpu-topology.c         |   8 ++-
>   target/s390x/cpu_topology.c     | 109 ++++++++++++++++++++++++++++++++
>   target/s390x/kvm/kvm.c          |   6 +-
>   target/s390x/meson.build        |   1 +
>   6 files changed, 172 insertions(+), 3 deletions(-)
>   create mode 100644 target/s390x/cpu_topology.c
> 
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> index 66c171d0bc..61c11db017 100644
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -13,6 +13,8 @@
>   #include "hw/qdev-core.h"
>   #include "qom/object.h"
>   
> +#define S390_TOPOLOGY_POLARITY_H  0x00

The defing looks like a header file guard. I would use

   S390_TOPOLOGY_HORIZONTAL_POLARITY

May be add the 3 vertical ones also, for completeness.

> +
>   typedef struct S390TopoContainer {
>       int active_count;
>   } S390TopoContainer;
> @@ -29,6 +31,7 @@ struct S390Topology {
>       S390TopoContainer *socket;
>       S390TopoTLE *tle;
>       MachineState *ms;
> +    QemuMutex topo_mutex;
>   };
>   
>   #define TYPE_S390_CPU_TOPOLOGY "s390-topology"
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 7d6d01325b..d604aa9c78 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -565,6 +565,52 @@ typedef union SysIB {
>   } SysIB;
>   QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
>   
> +/* CPU type Topology List Entry */
> +typedef struct SysIBTl_cpu {
> +        uint8_t nl;
> +        uint8_t reserved0[3];
> +        uint8_t reserved1:5;
> +        uint8_t dedicated:1;
> +        uint8_t polarity:2;
> +        uint8_t type;
> +        uint16_t origin;
> +        uint64_t mask;
> +} QEMU_PACKED QEMU_ALIGNED(8) 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 QEMU_ALIGNED(8) SysIBTl_container;
> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_container) != 8);
> +
> +#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

May be add a S390_ prefix. I don't think _NR is important for the
magnitude fields. And these are byte offsets.


> +/* 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;
> +    char tle[0];
> +} QEMU_PACKED QEMU_ALIGNED(8) SysIB_151x;
> +QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
> +
> +/* Maxi size of a SYSIB structure is when all CPU are alone in a container */
> +#define S390_TOPOLOGY_SYSIB_SIZE (sizeof(SysIB_151x) +                         \
> +                                  S390_MAX_CPUS * (sizeof(SysIBTl_container) + \
> +                                                   sizeof(SysIBTl_cpu)))
> +
> +
>   /* MMU defines */
>   #define ASCE_ORIGIN           (~0xfffULL) /* segment table origin             */
>   #define ASCE_SUBSPACE         0x200       /* subspace group control           */
> @@ -843,4 +889,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/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> index 42b22a1831..c73cebfe6f 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -54,8 +54,6 @@ void s390_topology_new_cpu(int core_id)
>           return;
>       }
>   
> -    socket_id = core_id / topo->cpus;
> -
>       /*
>        * At the core level, each CPU is represented by a bit in a 64bit
>        * unsigned long which represent the presence of a CPU.
> @@ -76,8 +74,13 @@ void s390_topology_new_cpu(int core_id)
>       bit %= 64;
>       bit = 63 - bit;
>   
> +    qemu_mutex_lock(&topo->topo_mutex);
> +
> +    socket_id = core_id / topo->cpus;
>       topo->socket[socket_id].active_count++;
>       set_bit(bit, &topo->tle[socket_id].mask[origin]);
> +
> +    qemu_mutex_unlock(&topo->topo_mutex);
>   }
>   
>   /**
> @@ -101,6 +104,7 @@ static void s390_topology_realize(DeviceState *dev, Error **errp)
>       topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
>   
>       topo->ms = ms;
> +    qemu_mutex_init(&topo->topo_mutex);
>   }
>   
>   /**
> diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
> new file mode 100644
> index 0000000000..df86a98f23
> --- /dev/null
> +++ b/target/s390x/cpu_topology.c
> @@ -0,0 +1,109 @@
> +/*
> + * QEMU S390x CPU Topology
> + *
> + * Copyright IBM Corp. 2022

Copyright tag

> + * 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"
> +
> +#define S390_TOPOLOGY_MAX_STSI_SIZE (S390_MAX_CPUS *              \
> +                                     (sizeof(SysIB_151x) +        \
> +                                      sizeof(SysIBTl_container) + \
> +                                      sizeof(SysIBTl_cpu)))

This is unused ?

> +
> +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_IFL;
> +    tle->origin = cpu_to_be64(origin * 64);
> +    tle->mask = cpu_to_be64(mask);
> +    return p + sizeof(*tle);
> +}

It would be nive to have some diagram on the field layout of a CPU tle
and container tle. It can be done as a follow up.

> +static char *s390_top_set_level2(S390Topology *topo, char *p)
> +{
> +    MachineState *ms = topo->ms;
> +    int i, origin;
> +
> +    for (i = 0; i < ms->smp.sockets; i++) {

a topo "nr-sockets" property would be better.
  
> +        if (!topo->socket[i].active_count) {

is 'active_count' only used to filter emtpy tles ? If so, I think this can
be done differently, without a state I mean.

> +            continue;
> +        }
> +        p = fill_container(p, 1, i);
> +        for (origin = 0; origin < S390_TOPOLOGY_MAX_ORIGIN; origin++) {

(I need to understand what 'origin' means. this is not obvious)

> +            uint64_t mask = 0L;
> +
> +            mask = 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();
> +    MachineState *ms = topo->ms;
> +    char *p = sysib->tle;
> +
> +    qemu_mutex_lock(&topo->topo_mutex);
> +
> +    sysib->mnest = level;
> +    switch (level) {
> +    case 2:
> +        sysib->mag[TOPOLOGY_NR_MAG2] = ms->smp.sockets;

a topo "nr-sockets" property would be better.

> +        sysib->mag[TOPOLOGY_NR_MAG1] = topo->cpus;

a topo "nr-cpus" property would be better.

> +        p = s390_top_set_level2(topo, p);
> +        break;
> +    }
> +
> +    qemu_mutex_unlock(&topo->topo_mutex);
> +
> +    return p - (char *) sysib;
> +}
> +
> +#define S390_TOPOLOGY_MAX_MNEST 2
> +
> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
> +{
> +    uint64_t page[S390_TOPOLOGY_SYSIB_SIZE / sizeof(uint64_t)] = {};
> +    SysIB_151x *sysib = (SysIB_151x *) page;
> +    int len;
> +
> +    if (s390_is_pv() || !s390_has_topology() ||
> +        sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
> +        setcc(cpu, 3);
> +        return;
> +    }
> +
> +    len = setup_stsi(sysib, sel2);
> +
> +    sysib->length = cpu_to_be16(len);
> +    s390_cpu_virt_mem_write(cpu, addr, ar, sysib, len);
> +    setcc(cpu, 0);
> +}
> +
> 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',

Do we really need a new file for the CPU topology ? I am asking because
insert_stsi_3_2_2() is in kvm.c and may be, instead, we should gather
all the stsi routines.

C.


>   ))
>   
>   s390x_user_ss = ss.source_set()

Re: [PATCH v10 3/9] s390x/cpu_topology: resetting the Topology-Change-Report

[Cédric Le Goater]

On 10/12/22 18:21, 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>
> Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
> Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>   target/s390x/cpu.h           |  1 +
>   target/s390x/kvm/kvm_s390x.h |  1 +
>   hw/s390x/cpu-topology.c      | 12 ++++++++++++
>   hw/s390x/s390-virtio-ccw.c   |  1 +
>   target/s390x/cpu-sysemu.c    |  7 +++++++
>   target/s390x/kvm/kvm.c       | 23 +++++++++++++++++++++++
>   6 files changed, 45 insertions(+)
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index d604aa9c78..9b35795ac8 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -825,6 +825,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_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 */
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> index c73cebfe6f..9f202621d0 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -107,6 +107,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
> @@ -120,6 +131,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 aa99a62e42..362378454a 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -113,6 +113,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);

I would catch and report the errors here.

> +    }
> +}
> 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;
> +}

Re: [PATCH v10 6/9] s390x/cpu topology: add topology-disable machine property

[Cédric Le Goater]

On 10/12/22 18:21, Pierre Morel wrote:
> S390 CPU topology is only allowed for s390-virtio-ccw-7.3 and
> newer S390 machines.
> We keep the possibility to disable the topology on these newer
> machines with the property topology-disable.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   include/hw/boards.h                |  3 ++
>   include/hw/s390x/cpu-topology.h    | 18 +++++++++-
>   include/hw/s390x/s390-virtio-ccw.h |  2 ++
>   hw/core/machine.c                  |  5 +++
>   hw/s390x/s390-virtio-ccw.c         | 53 +++++++++++++++++++++++++++++-
>   util/qemu-config.c                 |  4 +++
>   qemu-options.hx                    |  6 +++-
>   7 files changed, 88 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 311ed17e18..67147c47bf 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -379,6 +379,9 @@ struct MachineState {
>       } \
>       type_init(machine_initfn##_register_types)
>   
> +extern GlobalProperty hw_compat_7_2[];
> +extern const size_t hw_compat_7_2_len;

QEMU 7.2 is not out yet.

> +
>   extern GlobalProperty hw_compat_7_1[];
>   extern const size_t hw_compat_7_1_len;
>   
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> index 35a8a981ec..747c9ab4c6 100644
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -12,6 +12,8 @@
>   
>   #include "hw/qdev-core.h"
>   #include "qom/object.h"
> +#include "cpu.h"
> +#include "hw/s390x/s390-virtio-ccw.h"
>   
>   #define S390_TOPOLOGY_POLARITY_H  0x00
>   
> @@ -43,7 +45,21 @@ void s390_topology_new_cpu(int core_id);
>   
>   static inline bool s390_has_topology(void)
>   {
> -    return false;
> +    static S390CcwMachineState *ccw;

hmm, s390_has_topology is a static inline. It would be preferable to
change its definition to extern.

> +    Object *obj;
> +
> +    if (ccw) {
> +        return !ccw->topology_disable;
> +    }
> +
> +    /* we have to bail out for the "none" machine */
> +    obj = object_dynamic_cast(qdev_get_machine(),
> +                              TYPE_S390_CCW_MACHINE);
> +    if (!obj) {
> +        return false;
> +    }
> +    ccw = S390_CCW_MACHINE(obj);
> +    return !ccw->topology_disable;
>   }
>   
>   #endif
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 9e7a0d75bc..6c4b4645fc 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];
>   };
>   
> @@ -46,6 +47,7 @@ struct S390CcwMachineClass {
>       bool cpu_model_allowed;
>       bool css_migration_enabled;
>       bool hpage_1m_allowed;
> +    bool topology_allowed;

'topology_disable' in the state and 'topology_allowed' in the class.
This is confusing :/

you should add 'topology_allowed' in its own patch and maybe call
it 'topology_capable' ? it is a QEMU capability AIUI

>   };
>   
>   /* runtime-instrumentation allowed by the machine */
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index aa520e74a8..93c497655e 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_2[] = {
> +    { "s390-topology", "topology-disable", "true" },

May be use TYPE_S390_CPU_TOPOLOGY instead.

But again, this should only apply to 7.1 machines and below. 7.2 is
not out yet.


> +};
> +const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
> +
>   GlobalProperty hw_compat_7_1[] = {};
>   const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
>   
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 362378454a..3a13fad4df 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -616,6 +616,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>       s390mc->cpu_model_allowed = true;
>       s390mc->css_migration_enabled = true;
>       s390mc->hpage_1m_allowed = true;
> +    s390mc->topology_allowed = true;
>       mc->init = ccw_init;
>       mc->reset = s390_machine_reset;
>       mc->block_default_type = IF_VIRTIO;
> @@ -726,6 +727,27 @@ bool hpage_1m_allowed(void)
>       return get_machine_class()->hpage_1m_allowed;
>   }
>   
> +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);
> +
> +    if (!get_machine_class()->topology_allowed) {
> +        error_setg(errp, "Property topology-disable not available on machine %s",
> +                   get_machine_class()->parent_class.name);

OK. I get it now. May be we should consider adding the capability concept
David introduced in the pseries machine. Please take a look. That's not
for this patchset though. It would be too much work.

> +        return;
> +    }
> +
> +    ms->topology_disable = value;
> +}
> +
>   static char *machine_get_loadparm(Object *obj, Error **errp)
>   {
>       S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> @@ -784,6 +806,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 CPU topology");
> +    object_property_set_bool(obj, "topology-disable", false, NULL);

All the properties should be added in the machine class_init routine.
There is a preliminary cleanup patch required to move them all :/
   
>   }
>   
>   static const TypeInfo ccw_machine_info = {
> @@ -836,14 +865,36 @@ bool css_migration_enabled(void)
>       }                                                                         \
>       type_init(ccw_machine_register_##suffix)
>   
> +static void ccw_machine_7_3_instance_options(MachineState *machine)
> +{
> +}
> +
> +static void ccw_machine_7_3_class_options(MachineClass *mc)
> +{
> +}
> +DEFINE_CCW_MACHINE(7_3, "7.3", true);

That's too early.

> +
>   static void ccw_machine_7_2_instance_options(MachineState *machine)
>   {
> +    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
> +
> +    ccw_machine_7_3_instance_options(machine);
> +    ms->topology_disable = true;
>   }
>   
>   static void ccw_machine_7_2_class_options(MachineClass *mc)
>   {
> +    S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
> +    static GlobalProperty compat[] = {
> +        { TYPE_S390_CPU_TOPOLOGY, "topology-allowed", "off", },

hmm, "topology-allowed" is not a TYPE_S390_CPU_TOPOLOGY property.


> +    };
> +
> +    ccw_machine_7_3_class_options(mc);
> +    compat_props_add(mc->compat_props, hw_compat_7_2, hw_compat_7_2_len);
> +    compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
> +    s390mc->topology_allowed = false;
>   }
> -DEFINE_CCW_MACHINE(7_2, "7.2", true);
> +DEFINE_CCW_MACHINE(7_2, "7.2", false);
>   
>   static void ccw_machine_7_1_instance_options(MachineState *machine)
>   {
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index 5325f6bf80..c19e8bc8f3 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -240,6 +240,10 @@ static QemuOptsList machine_opts = {
>               .name = "zpcii-disable",
>               .type = QEMU_OPT_BOOL,
>               .help = "disable zPCI interpretation facilities",
> +        },{
> +            .name = "topology-disable",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "disable CPU topology",
>           },
>           { /* End of list */ }
>       }
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 95b998a13b..c804b0f899 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -38,7 +38,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>       "                hmat=on|off controls ACPI HMAT support (default=off)\n"
>       "                memory-backend='backend-id' specifies explicitly provided backend for main RAM (default=none)\n"
>       "                cxl-fmw.0.targets.0=firsttarget,cxl-fmw.0.targets.1=secondtarget,cxl-fmw.0.size=size[,cxl-fmw.0.interleave-granularity=granularity]\n"
> -    "                zpcii-disable=on|off disables zPCI interpretation facilities (default=off)\n",
> +    "                zpcii-disable=on|off disables zPCI interpretation facilities (default=off)\n"
> +    "                topology-disable=on|off disables CPU topology (default=off)\n",
>       QEMU_ARCH_ALL)
>   SRST
>   ``-machine [type=]name[,prop=value[,...]]``
> @@ -163,6 +164,9 @@ SRST
>           Disables zPCI interpretation facilties on s390-ccw hosts.
>           This feature can be used to disable hardware virtual assists
>           related to zPCI devices. The default is off.
> +
> +    ``topology-disable=on|off``
> +        Disables CPU topology on for S390 machines starting with s390-ccw-virtio-7.3.
>   ERST
>   
>   DEF("M", HAS_ARG, QEMU_OPTION_M,

Re: [PATCH v10 7/9] s390x/cpu topology: add max_threads machine class attribute

[Cédric Le Goater]

On 10/12/22 18:21, Pierre Morel wrote:
> The S390 CPU topology accepts the smp.threads argument while
> in reality it does not effectively allow multthreading.
> 
> Let's keep this behavior for machines older than 7.3 and
> refuse to use threads in newer machines until multithreading
> is really proposed to the guest by the machine.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   include/hw/s390x/s390-virtio-ccw.h |  1 +
>   hw/s390x/s390-virtio-ccw.c         | 10 ++++++++++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 6c4b4645fc..319dfac1bb 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -48,6 +48,7 @@ struct S390CcwMachineClass {
>       bool css_migration_enabled;
>       bool hpage_1m_allowed;
>       bool topology_allowed;
> +    int max_threads;
>   };
>   
>   /* runtime-instrumentation allowed by the machine */
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 3a13fad4df..d6ce31d168 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -85,8 +85,15 @@ out:
>   static void s390_init_cpus(MachineState *machine)
>   {
>       MachineClass *mc = MACHINE_GET_CLASS(machine);
> +    S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
>       int i;
>   
> +    if (machine->smp.threads > s390mc->max_threads) {
> +        error_report("S390 does not support more than %d threads.",
> +                     s390mc->max_threads);
> +        exit(1);
> +    }
> +
>       /* initialize possible_cpus */
>       mc->possible_cpu_arch_ids(machine);
>   
> @@ -617,6 +624,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>       s390mc->css_migration_enabled = true;
>       s390mc->hpage_1m_allowed = true;
>       s390mc->topology_allowed = true;
> +    s390mc->max_threads = 1;
>       mc->init = ccw_init;
>       mc->reset = s390_machine_reset;
>       mc->block_default_type = IF_VIRTIO;
> @@ -887,12 +895,14 @@ static void ccw_machine_7_2_class_options(MachineClass *mc)
>       S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
>       static GlobalProperty compat[] = {
>           { TYPE_S390_CPU_TOPOLOGY, "topology-allowed", "off", },
> +        { TYPE_S390_CPU_TOPOLOGY, "max_threads", "off", },

I don't understand this change.


C.


>       };
>   
>       ccw_machine_7_3_class_options(mc);
>       compat_props_add(mc->compat_props, hw_compat_7_2, hw_compat_7_2_len);
>       compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
>       s390mc->topology_allowed = false;
> +    s390mc->max_threads = S390_MAX_CPUS;
>   }
>   DEFINE_CCW_MACHINE(7_2, "7.2", false);
>   

Re: [PATCH v10 6/9] s390x/cpu topology: add topology-disable machine property

[Cédric Le Goater]

On 10/12/22 18:21, Pierre Morel wrote:
> S390 CPU topology is only allowed for s390-virtio-ccw-7.3 and
> newer S390 machines.
> We keep the possibility to disable the topology on these newer
> machines with the property topology-disable.

Isn't 'topology' enough for the property ? I don't think the
'-disable' prefix adds much to the meaning.
  
C.

Re: [PATCH v10 6/9] s390x/cpu topology: add topology-disable machine property

[Cornelia Huck]

On Tue, Oct 18 2022, Cédric Le Goater <clg@kaod.org> wrote:

> On 10/12/22 18:21, Pierre Morel wrote:
>> S390 CPU topology is only allowed for s390-virtio-ccw-7.3 and
>> newer S390 machines.
>> We keep the possibility to disable the topology on these newer
>> machines with the property topology-disable.
>> 
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   include/hw/boards.h                |  3 ++
>>   include/hw/s390x/cpu-topology.h    | 18 +++++++++-
>>   include/hw/s390x/s390-virtio-ccw.h |  2 ++
>>   hw/core/machine.c                  |  5 +++
>>   hw/s390x/s390-virtio-ccw.c         | 53 +++++++++++++++++++++++++++++-
>>   util/qemu-config.c                 |  4 +++
>>   qemu-options.hx                    |  6 +++-
>>   7 files changed, 88 insertions(+), 3 deletions(-)
>> 
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 311ed17e18..67147c47bf 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -379,6 +379,9 @@ struct MachineState {
>>       } \
>>       type_init(machine_initfn##_register_types)
>>   
>> +extern GlobalProperty hw_compat_7_2[];
>> +extern const size_t hw_compat_7_2_len;
>
> QEMU 7.2 is not out yet.

Yes, and the introduction of the new compat machines needs to go into a
separate patch. I'm usually preparing that patch while QEMU is in
freeze, but feel free to cook up a patch earlier if you need it.

(...)

>> +static void ccw_machine_7_3_instance_options(MachineState *machine)
>> +{
>> +}
>> +
>> +static void ccw_machine_7_3_class_options(MachineClass *mc)
>> +{
>> +}
>> +DEFINE_CCW_MACHINE(7_3, "7.3", true);
>
> That's too early.

Also, the next QEMU version will be 8.0, not 7.3 :)

Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

[Pierre Morel]

On 10/18/22 18:43, Cédric Le Goater wrote:
> Hello Pierre,
> 
> On 10/12/22 18:20, 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.
>> s390x/cpu topology: core_id sets s390x CPU topology
>>
>> In the S390x CPU topology the core_id specifies the CPU address
>> and the position of the cpu withing the topology.
>>
>> Let's build the topology based on the core_id.
> 
> The commit log is doubled.

Yes, thanks.

> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   include/hw/s390x/cpu-topology.h |  45 +++++++++++
>>   hw/s390x/cpu-topology.c         | 132 ++++++++++++++++++++++++++++++++
>>   hw/s390x/s390-virtio-ccw.c      |  21 +++++
>>   hw/s390x/meson.build            |   1 +
>>   4 files changed, 199 insertions(+)
>>   create mode 100644 include/hw/s390x/cpu-topology.h
>>   create mode 100644 hw/s390x/cpu-topology.c
>>
>> diff --git a/include/hw/s390x/cpu-topology.h 
>> b/include/hw/s390x/cpu-topology.h
>> new file mode 100644
>> index 0000000000..66c171d0bc
>> --- /dev/null
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -0,0 +1,45 @@
>> +/*
>> + * 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
>> +
>> +#include "hw/qdev-core.h"
>> +#include "qom/object.h"
>> +
>> +typedef struct S390TopoContainer {
>> +    int active_count;
>> +} S390TopoContainer;
> 
> This structure does not seem very useful.
> 
>> +
>> +#define S390_TOPOLOGY_CPU_IFL 0x03
>> +#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
>> +typedef struct S390TopoTLE { 
> 
> The 'Topo' is redundant as TLE stands for 'topology-list entry'. This is 
> minor.
> 
>> +    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
>> +} S390TopoTLE;
>> +
>> +struct S390Topology {
>> +    SysBusDevice parent_obj;
>> +    int cpus;
>> +    S390TopoContainer *socket;
>> +    S390TopoTLE *tle;
>> +    MachineState *ms;
> 
> hmm, it would be cleaner to introduce the fields and properties needed
> by the S390Topology model and avoid dragging the machine object pointer.
> AFAICT, these properties would be :
> 
>    "nr-cpus"
>    "max-cpus"
>    "nr-sockets"
> 

OK

> 
> 
>> +};
>> +
>> +#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);
>> +
>> +static inline bool s390_has_topology(void)
>> +{
>> +    return false;
>> +}
>> +
>> +#endif
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> new file mode 100644
>> index 0000000000..42b22a1831
>> --- /dev/null
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -0,0 +1,132 @@
>> +/*
>> + * CPU Topology
>> + *
>> + * Copyright IBM Corp. 2022
> 
> The Copyright tag is different in the .h file.

OK, I change this to be like in the header file it seems to be the most 
used format.

> 
>> + * 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;
> 
> I am not convinced this routine is useful. The s390Topology pointer
> could be stored under the machine state I think. It wouldn't be a
> problem when CPUs are hot plugged since we have access to the machine
> in the hot plug handler.

OK, I add a pointer to the machine state that will be initialised during 
s390_init_topology()

> 
> For the stsi call, 'struct ArchCPU' probably lacks a back pointer to
> the machine objects with which CPU interact. These are typically
> interrupt controllers or this new s390Topology model. You could add
> the pointer there or, better, under a generic 'void *opaque' attribute.
> 
> That said, what you did works fine. The modeling could be cleaner.

Yes. I think you are right and I add a opaque pointer to the topology.

> 
>> +}
>> +
>> +/*
>> + * 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;
>> +    }
> 
> I would move this test in the caller.

Check will disapear with the new implementation.

> 
>> +
>> +    socket_id = core_id / topo->cpus;
>> +
>> +    /*
>> +     * At the core level, each CPU is represented by a bit in a 64bit
>> +     * unsigned long which represent the presence 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 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.
>> +     * The only reason to have several S390TopologyCores inside a 
>> socket is
>> +     * to have more than 64 CPUs.
>> +     * 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.
>> +     */
>> +    bit = core_id;
>> +    origin = bit / 64;
>> +    bit %= 64;
>> +    bit = 63 - bit;
>> +
>> +    topo->socket[socket_id].active_count++;
>> +    set_bit(bit, &topo->tle[socket_id].mask[origin]);
> 
> here, the tle array is indexed with a socket id and ...

It was stupid to keep both structures.
I will keep only the socket structure and incorparate the TLE inside.

> 
>> +}
>> +
>> +/**
>> + * 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);
>> +
>> +    topo->cpus = ms->smp.cores * ms->smp.threads;> +
>> +    topo->socket = g_new0(S390TopoContainer, ms->smp.sockets);
>> +    topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
> 
> 
> ... here, the tle array is allocated with max_cpus and this looks
> weird. I will dig the specs to try to understand.

ack it looks weird. I keep only the socket structure

> 
>> +
>> +    topo->ms = ms;
> 
> See comment above regarding the properties.
> 
>> +}
>> +
>> +/**
>> + * 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/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 03855c7231..aa99a62e42 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;
>> @@ -94,6 +95,18 @@ static void s390_init_cpus(MachineState *machine)
>>       }
>>   }
>> +static void s390_init_topology(MachineState *machine)
>> +{
>> +    DeviceState *dev;
>> +
>> +    if (s390_has_topology()) {
>> +        dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
>> +        object_property_add_child(&machine->parent_obj,
>> +                                  TYPE_S390_CPU_TOPOLOGY, OBJECT(dev));
>> +        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> +    }
>> +}
>> +
>>   static const char *const reset_dev_types[] = {
>>       TYPE_VIRTUAL_CSS_BRIDGE,
>>       "s390-sclp-event-facility",
>> @@ -244,6 +257,9 @@ 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 */
> 
> initialization

yes, thanks


Thanks,

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v10 6/9] s390x/cpu topology: add topology-disable machine property

[Pierre Morel]

On 10/19/22 11:03, Cornelia Huck wrote:
> On Tue, Oct 18 2022, Cédric Le Goater <clg@kaod.org> wrote:
> 
>> On 10/12/22 18:21, Pierre Morel wrote:
>>> S390 CPU topology is only allowed for s390-virtio-ccw-7.3 and
>>> newer S390 machines.
>>> We keep the possibility to disable the topology on these newer
>>> machines with the property topology-disable.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>    include/hw/boards.h                |  3 ++
>>>    include/hw/s390x/cpu-topology.h    | 18 +++++++++-
>>>    include/hw/s390x/s390-virtio-ccw.h |  2 ++
>>>    hw/core/machine.c                  |  5 +++
>>>    hw/s390x/s390-virtio-ccw.c         | 53 +++++++++++++++++++++++++++++-
>>>    util/qemu-config.c                 |  4 +++
>>>    qemu-options.hx                    |  6 +++-
>>>    7 files changed, 88 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>>> index 311ed17e18..67147c47bf 100644
>>> --- a/include/hw/boards.h
>>> +++ b/include/hw/boards.h
>>> @@ -379,6 +379,9 @@ struct MachineState {
>>>        } \
>>>        type_init(machine_initfn##_register_types)
>>>    
>>> +extern GlobalProperty hw_compat_7_2[];
>>> +extern const size_t hw_compat_7_2_len;
>>
>> QEMU 7.2 is not out yet.
> 
> Yes, and the introduction of the new compat machines needs to go into a
> separate patch. I'm usually preparing that patch while QEMU is in
> freeze, but feel free to cook up a patch earlier if you need it.

OK, Thanks, I understand I put it in a separate file so it can be 
adapted at the moment the series will need to be merged.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

[Janis Schoetterl-Glausch]

On Wed, 2022-10-19 at 17:39 +0200, Pierre Morel wrote:
> 
> On 10/18/22 18:43, Cédric Le Goater wrote:
[...]
> > 
> > > diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> > > new file mode 100644
> > > index 0000000000..42b22a1831
> > > --- /dev/null
> > > +++ b/hw/s390x/cpu-topology.c
> > > @@ -0,0 +1,132 @@
> > > +/*
> > > + * CPU Topology
> > > + *
> > > + * Copyright IBM Corp. 2022
> > 
> > The Copyright tag is different in the .h file.
> 
> OK, I change this to be like in the header file it seems to be the most 
> used format.
> 
No, this form, with the date at the end, is the correct one.
> 

[...]

Re: [PATCH v10 6/9] s390x/cpu topology: add topology-disable machine property

[Pierre Morel]

On 10/18/22 19:34, Cédric Le Goater wrote:
> On 10/12/22 18:21, Pierre Morel wrote:
>> S390 CPU topology is only allowed for s390-virtio-ccw-7.3 and
>> newer S390 machines.
>> We keep the possibility to disable the topology on these newer
>> machines with the property topology-disable.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   include/hw/boards.h                |  3 ++
>>   include/hw/s390x/cpu-topology.h    | 18 +++++++++-
>>   include/hw/s390x/s390-virtio-ccw.h |  2 ++
>>   hw/core/machine.c                  |  5 +++
>>   hw/s390x/s390-virtio-ccw.c         | 53 +++++++++++++++++++++++++++++-
>>   util/qemu-config.c                 |  4 +++
>>   qemu-options.hx                    |  6 +++-
>>   7 files changed, 88 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 311ed17e18..67147c47bf 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -379,6 +379,9 @@ struct MachineState {
>>       } \
>>       type_init(machine_initfn##_register_types)
>> +extern GlobalProperty hw_compat_7_2[];
>> +extern const size_t hw_compat_7_2_len;
> 
> QEMU 7.2 is not out yet.

OK, thanks, I will use 7.2 instead of 7.3 as the new machine.
We see later if it goes to 8.0

> 
>> +
>>   extern GlobalProperty hw_compat_7_1[];
>>   extern const size_t hw_compat_7_1_len;
>> diff --git a/include/hw/s390x/cpu-topology.h 
>> b/include/hw/s390x/cpu-topology.h
>> index 35a8a981ec..747c9ab4c6 100644
>> --- a/include/hw/s390x/cpu-topology.h
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -12,6 +12,8 @@
>>   #include "hw/qdev-core.h"
>>   #include "qom/object.h"
>> +#include "cpu.h"
>> +#include "hw/s390x/s390-virtio-ccw.h"
>>   #define S390_TOPOLOGY_POLARITY_H  0x00
>> @@ -43,7 +45,21 @@ void s390_topology_new_cpu(int core_id);
>>   static inline bool s390_has_topology(void)
>>   {
>> -    return false;
>> +    static S390CcwMachineState *ccw;
> 
> hmm, s390_has_topology is a static inline. It would be preferable to
> change its definition to extern.

OK

> 
>> +    Object *obj;
>> +
>> +    if (ccw) {
>> +        return !ccw->topology_disable;
>> +    }
>> +
>> +    /* we have to bail out for the "none" machine */
>> +    obj = object_dynamic_cast(qdev_get_machine(),
>> +                              TYPE_S390_CCW_MACHINE);
>> +    if (!obj) {
>> +        return false;
>> +    }
>> +    ccw = S390_CCW_MACHINE(obj);
>> +    return !ccw->topology_disable;
>>   }
>>   #endif
>> diff --git a/include/hw/s390x/s390-virtio-ccw.h 
>> b/include/hw/s390x/s390-virtio-ccw.h
>> index 9e7a0d75bc..6c4b4645fc 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];
>>   };
>> @@ -46,6 +47,7 @@ struct S390CcwMachineClass {
>>       bool cpu_model_allowed;
>>       bool css_migration_enabled;
>>       bool hpage_1m_allowed;
>> +    bool topology_allowed;
> 
> 'topology_disable' in the state and 'topology_allowed' in the class.
> This is confusing :/
> 
> you should add 'topology_allowed' in its own patch and maybe call
> it 'topology_capable' ? it is a QEMU capability AIUI

yes, OK, I separate the two.

. topology_capable to know if the QEMU version is capable of handling 
topology

- topology_disable for migration if KVM is not able to handle the 
topology on one of the machines involved.

> 
>>   };
>>   /* runtime-instrumentation allowed by the machine */
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index aa520e74a8..93c497655e 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_2[] = {
>> +    { "s390-topology", "topology-disable", "true" },
> 
> May be use TYPE_S390_CPU_TOPOLOGY instead.
> 
> But again, this should only apply to 7.1 machines and below. 7.2 is
> not out yet.

OK

> 
> 
>> +};
>> +const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
>> +
>>   GlobalProperty hw_compat_7_1[] = {};
>>   const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 362378454a..3a13fad4df 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -616,6 +616,7 @@ static void ccw_machine_class_init(ObjectClass 
>> *oc, void *data)
>>       s390mc->cpu_model_allowed = true;
>>       s390mc->css_migration_enabled = true;
>>       s390mc->hpage_1m_allowed = true;
>> +    s390mc->topology_allowed = true;
>>       mc->init = ccw_init;
>>       mc->reset = s390_machine_reset;
>>       mc->block_default_type = IF_VIRTIO;
>> @@ -726,6 +727,27 @@ bool hpage_1m_allowed(void)
>>       return get_machine_class()->hpage_1m_allowed;
>>   }
>> +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);
>> +
>> +    if (!get_machine_class()->topology_allowed) {
>> +        error_setg(errp, "Property topology-disable not available on 
>> machine %s",
>> +                   get_machine_class()->parent_class.name);
> 
> OK. I get it now. May be we should consider adding the capability concept
> David introduced in the pseries machine. Please take a look. That's not
> for this patchset though. It would be too much work.

Yes, interesting, would be something to do in the near future.

> 
>> +        return;
>> +    }
>> +
>> +    ms->topology_disable = value;
>> +}
>> +
>>   static char *machine_get_loadparm(Object *obj, Error **errp)
>>   {
>>       S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
>> @@ -784,6 +806,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 CPU topology");
>> +    object_property_set_bool(obj, "topology-disable", false, NULL);
> 
> All the properties should be added in the machine class_init routine.
> There is a preliminary cleanup patch required to move them all :/

OK, I will move it to the class_init


>>   }
>>   static const TypeInfo ccw_machine_info = {
>> @@ -836,14 +865,36 @@ bool css_migration_enabled(void)
>>       
>> }                                                                         \
>>       type_init(ccw_machine_register_##suffix)
>> +static void ccw_machine_7_3_instance_options(MachineState *machine)
>> +{
>> +}
>> +
>> +static void ccw_machine_7_3_class_options(MachineClass *mc)
>> +{
>> +}
>> +DEFINE_CCW_MACHINE(7_3, "7.3", true);
> 
> That's too early.
> 
>> +
>>   static void ccw_machine_7_2_instance_options(MachineState *machine)
>>   {
>> +    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
>> +
>> +    ccw_machine_7_3_instance_options(machine);
>> +    ms->topology_disable = true;
>>   }
>>   static void ccw_machine_7_2_class_options(MachineClass *mc)
>>   {
>> +    S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
>> +    static GlobalProperty compat[] = {
>> +        { TYPE_S390_CPU_TOPOLOGY, "topology-allowed", "off", },
> 
> hmm, "topology-allowed" is not a TYPE_S390_CPU_TOPOLOGY property.

I do not understand what I did there and why.
I guess I better do nothing there.

Thanks for the comments,

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v10 6/9] s390x/cpu topology: add topology-disable machine property

[Pierre Morel]

On 10/18/22 19:51, Cédric Le Goater wrote:
> On 10/12/22 18:21, Pierre Morel wrote:
>> S390 CPU topology is only allowed for s390-virtio-ccw-7.3 and
>> newer S390 machines.
>> We keep the possibility to disable the topology on these newer
>> machines with the property topology-disable.
> 
> Isn't 'topology' enough for the property ? I don't think the
> '-disable' prefix adds much to the meaning.
> 
> C.
> 
> 

Agreed.

Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

[Cédric Le Goater]

On 10/19/22 17:39, Pierre Morel wrote:
> 
> 
> On 10/18/22 18:43, Cédric Le Goater wrote:
>> Hello Pierre,
>>
>> On 10/12/22 18:20, 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.
>>> s390x/cpu topology: core_id sets s390x CPU topology
>>>
>>> In the S390x CPU topology the core_id specifies the CPU address
>>> and the position of the cpu withing the topology.
>>>
>>> Let's build the topology based on the core_id.
>>
>> The commit log is doubled.
> 
> Yes, thanks.
> 
>>
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   include/hw/s390x/cpu-topology.h |  45 +++++++++++
>>>   hw/s390x/cpu-topology.c         | 132 ++++++++++++++++++++++++++++++++
>>>   hw/s390x/s390-virtio-ccw.c      |  21 +++++
>>>   hw/s390x/meson.build            |   1 +
>>>   4 files changed, 199 insertions(+)
>>>   create mode 100644 include/hw/s390x/cpu-topology.h
>>>   create mode 100644 hw/s390x/cpu-topology.c
>>>
>>> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
>>> new file mode 100644
>>> index 0000000000..66c171d0bc
>>> --- /dev/null
>>> +++ b/include/hw/s390x/cpu-topology.h
>>> @@ -0,0 +1,45 @@
>>> +/*
>>> + * 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
>>> +
>>> +#include "hw/qdev-core.h"
>>> +#include "qom/object.h"
>>> +
>>> +typedef struct S390TopoContainer {
>>> +    int active_count;
>>> +} S390TopoContainer;
>>
>> This structure does not seem very useful.
>>
>>> +
>>> +#define S390_TOPOLOGY_CPU_IFL 0x03
>>> +#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
>>> +typedef struct S390TopoTLE { 
>>
>> The 'Topo' is redundant as TLE stands for 'topology-list entry'. This is minor.
>>
>>> +    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
>>> +} S390TopoTLE;
>>> +
>>> +struct S390Topology {
>>> +    SysBusDevice parent_obj;
>>> +    int cpus;
>>> +    S390TopoContainer *socket;
>>> +    S390TopoTLE *tle;
>>> +    MachineState *ms;
>>
>> hmm, it would be cleaner to introduce the fields and properties needed
>> by the S390Topology model and avoid dragging the machine object pointer.
>> AFAICT, these properties would be :
>>
>>    "nr-cpus"
>>    "max-cpus"
>>    "nr-sockets"
>>
> 
> OK
> 
>>
>>
>>> +};
>>> +
>>> +#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);
>>> +
>>> +static inline bool s390_has_topology(void)
>>> +{
>>> +    return false;
>>> +}
>>> +
>>> +#endif
>>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>>> new file mode 100644
>>> index 0000000000..42b22a1831
>>> --- /dev/null
>>> +++ b/hw/s390x/cpu-topology.c
>>> @@ -0,0 +1,132 @@
>>> +/*
>>> + * CPU Topology
>>> + *
>>> + * Copyright IBM Corp. 2022
>>
>> The Copyright tag is different in the .h file.
> 
> OK, I change this to be like in the header file it seems to be the most used format.
> 
>>
>>> + * 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;
>>
>> I am not convinced this routine is useful. The s390Topology pointer
>> could be stored under the machine state I think. It wouldn't be a
>> problem when CPUs are hot plugged since we have access to the machine
>> in the hot plug handler.
> 
> OK, I add a pointer to the machine state that will be initialised during s390_init_topology()

LGTM.

> 
>>
>> For the stsi call, 'struct ArchCPU' probably lacks a back pointer to
>> the machine objects with which CPU interact. These are typically
>> interrupt controllers or this new s390Topology model. You could add
>> the pointer there or, better, under a generic 'void *opaque' attribute.
>>
>> That said, what you did works fine. The modeling could be cleaner.
> 
> Yes. I think you are right and I add a opaque pointer to the topology.

As an example, you could look at PPC where the PowerPCCPU CPU model is
shared between two differents machine, a baremetal one PowerNV and the
para-virtual one pSeries/sPAPR. Look for :

    pnv_cpu_state(PowerPCCPU *cpu)
    spapr_cpu_state(PowerPCCPU *cpu)

the machine CPU state is stored under an opaque cpu->machine_data which is
specific to each machine. It doesn't have to be as complex on s390 since
we only have one type of z-machine. An opaque is a good idea still.

Thanks,

C.

Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

[Janis Schoetterl-Glausch]

On Wed, 2022-10-12 at 18:20 +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.
> s390x/cpu topology: core_id sets s390x CPU topology
> 
> In the S390x CPU topology the core_id specifies the CPU address
> and the position of the cpu withing the topology.
> 
> Let's build the topology based on the core_id.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  include/hw/s390x/cpu-topology.h |  45 +++++++++++
>  hw/s390x/cpu-topology.c         | 132 ++++++++++++++++++++++++++++++++
>  hw/s390x/s390-virtio-ccw.c      |  21 +++++
>  hw/s390x/meson.build            |   1 +
>  4 files changed, 199 insertions(+)
>  create mode 100644 include/hw/s390x/cpu-topology.h
>  create mode 100644 hw/s390x/cpu-topology.c
> 
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> new file mode 100644
> index 0000000000..66c171d0bc
> --- /dev/null
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -0,0 +1,45 @@
> +/*
> + * 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
> +
> +#include "hw/qdev-core.h"
> +#include "qom/object.h"
> +
> +typedef struct S390TopoContainer {
> +    int active_count;
> +} S390TopoContainer;
> +
> +#define S390_TOPOLOGY_CPU_IFL 0x03
> +#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
> +typedef struct S390TopoTLE {
> +    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
> +} S390TopoTLE;

Since this actually represents multiple TLEs, you might want to change the
name of the struct to reflect this. S390TopoTLEList maybe?

> +
> +struct S390Topology {
> +    SysBusDevice parent_obj;
> +    int cpus;
> +    S390TopoContainer *socket;
> +    S390TopoTLE *tle;
> +    MachineState *ms;
> +};
> +
> +#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);
> +
> +static inline bool s390_has_topology(void)
> +{
> +    return false;
> +}
> +
> +#endif
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> new file mode 100644
> index 0000000000..42b22a1831
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c
> @@ -0,0 +1,132 @@
> +/*
> + * 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.

s/aruments/arguments/

> + * 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
> + */

Not sure if that comment helps if you don't already know how the topology list works.
> +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->cpus;
> +
> +    /*
> +     * At the core level, each CPU is represented by a bit in a 64bit
> +     * unsigned long which represent the presence of a CPU.
> +     * The firmware assume that all CPU in a CPU TLE have the same

s/firmware assume/architecture specifies/

> +     * type, polarization and are all dedicated or shared.
> +     * In that case the origin variable represents the offset of the first
> +     * CPU in the CPU container.

This sentence is repeated further down.

> +     * More than 64 CPUs per socket are represented in several CPU containers
> +     * inside the socket container.
> +     * The only reason to have several S390TopologyCores inside a socket is
> +     * to have more than 64 CPUs.
> +     * 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.
> +     */

In the last version you had:
+ /*
+ * 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.
+ */

I would modify it thus (with better line wrapping):
+ /*
+ * At the core level, each CPU is represented by a bit in a 64bit
+ * unsigned long.
+ * The architecture specifies that all CPU in a CPU TLE have the same
+ * type, polarization and are all dedicated or shared.
+ * In the case that a socket contains CPUs 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.
+ * Therefore, the only reason to have several S390TopologyCores inside a socket is
+ * to support CPU id differences > 64.
+ * In that case, the origin field in a container represents the offset of the first CPU
+ * in that CPU container, thereby allowing representation of all CPUs via multiple containers.
+ */

> +    bit = core_id;
> +    origin = bit / 64;
> +    bit %= 64;
> +    bit = 63 - bit;

I'm not convinced that that is more readable than just
 origin = core_id / 64;
 bit = 63 - (core_id % 64);

but that is for you to decide.
> +
> +    topo->socket[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);
> +
> +    topo->cpus = ms->smp.cores * ms->smp.threads;
> +
> +    topo->socket = g_new0(S390TopoContainer, ms->smp.sockets);
> +    topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);

As Cédric pointed out, the number of TLE(List)s should be the same as the
sockets.
> +
> +    topo->ms = ms;
> +}
[...]

Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

[Janis Schoetterl-Glausch]

On Wed, 2022-10-19 at 17:39 +0200, Pierre Morel wrote:
> 
> On 10/18/22 18:43, Cédric Le Goater wrote:
> 
> > > 
[...]
> > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > > ---
> > >   include/hw/s390x/cpu-topology.h |  45 +++++++++++
> > >   hw/s390x/cpu-topology.c         | 132 ++++++++++++++++++++++++++++++++
> > >   hw/s390x/s390-virtio-ccw.c      |  21 +++++
> > >   hw/s390x/meson.build            |   1 +
> > >   4 files changed, 199 insertions(+)
> > >   create mode 100644 include/hw/s390x/cpu-topology.h
> > >   create mode 100644 hw/s390x/cpu-topology.c
> > > 
[...]
> > > 
> > > +/*
> > > + * 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;
> > > +    }
> > 
> > I would move this test in the caller.
> 
> Check will disapear with the new implementation.
> 
> > 
> > > +
> > > +    socket_id = core_id / topo->cpus;
> > > +
> > > +    /*
> > > +     * At the core level, each CPU is represented by a bit in a 64bit
> > > +     * unsigned long which represent the presence 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 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.
> > > +     * The only reason to have several S390TopologyCores inside a 
> > > socket is
> > > +     * to have more than 64 CPUs.
> > > +     * 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.
> > > +     */
> > > +    bit = core_id;
> > > +    origin = bit / 64;
> > > +    bit %= 64;
> > > +    bit = 63 - bit;
> > > +
> > > +    topo->socket[socket_id].active_count++;
> > > +    set_bit(bit, &topo->tle[socket_id].mask[origin]);
> > 
> > here, the tle array is indexed with a socket id and ...
> 
> It was stupid to keep both structures.
> I will keep only the socket structure and incorparate the TLE inside.

I don't think it's stupid. Both are valid possibilities.
The first one treats sockets and books and drawers exactly the same, since
they are all just containers (once you introduce books and drawers).
The second treats sockets differently, because they're the leaf nodes of the
hierarchy in a certain sense (the leaf nodes of the "regular" hierarchy,
whereas the cpus are the real leaf nodes of the topology but special/not "regular").

I'd say the first is more natural from reading the PoP, but it might indeed be a bit
confusing when reading the code since there's a one to one correspondence between
sockets and TLE(List)s.
> 
> > 
> > > +}
> > > +
> > > +/**
> > > + * 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);
> > > +
> > > +    topo->cpus = ms->smp.cores * ms->smp.threads;> +
> > > +    topo->socket = g_new0(S390TopoContainer, ms->smp.sockets);
> > > +    topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
> > 
> > 
> > ... here, the tle array is allocated with max_cpus and this looks
> > weird. I will dig the specs to try to understand.
> 
> ack it looks weird. I keep only the socket structure

[...]

Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

[Janis Schoetterl-Glausch]

On Wed, 2022-10-12 at 18:20 +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.
> s390x/cpu topology: core_id sets s390x CPU topology
> 
> In the S390x CPU topology the core_id specifies the CPU address
> and the position of the cpu withing the topology.
> 
> Let's build the topology based on the core_id.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  include/hw/s390x/cpu-topology.h |  45 +++++++++++
>  hw/s390x/cpu-topology.c         | 132 ++++++++++++++++++++++++++++++++
>  hw/s390x/s390-virtio-ccw.c      |  21 +++++
>  hw/s390x/meson.build            |   1 +
>  4 files changed, 199 insertions(+)
>  create mode 100644 include/hw/s390x/cpu-topology.h
>  create mode 100644 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);
> +
> +    topo->cpus = ms->smp.cores * ms->smp.threads;

Currently threads are not supported, effectively increasing the number of cpus,
so this is currently correct. Once the machine version limits the threads to 1,
it is also correct. However, once we support multiple threads, this becomes incorrect.
I wonder if it's ok from a backward compatibility point of view to modify the smp values
by doing cores *= threads, threads = 1 for old machines.
Then you can just use the cores value and it is always correct.
In any case, if you keep it as is, I'd like to see a comment here saying that this
is correct only so long as we don't support threads.
> +
> +    topo->socket = g_new0(S390TopoContainer, ms->smp.sockets);
> +    topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
> +
> +    topo->ms = ms;
> +}
> +
[...]

Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

[Pierre Morel]

On 10/25/22 21:58, Janis Schoetterl-Glausch wrote:
> On Wed, 2022-10-12 at 18:20 +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.
>> s390x/cpu topology: core_id sets s390x CPU topology
>>
>> In the S390x CPU topology the core_id specifies the CPU address
>> and the position of the cpu withing the topology.
>>
>> Let's build the topology based on the core_id.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   include/hw/s390x/cpu-topology.h |  45 +++++++++++
>>   hw/s390x/cpu-topology.c         | 132 ++++++++++++++++++++++++++++++++
>>   hw/s390x/s390-virtio-ccw.c      |  21 +++++
>>   hw/s390x/meson.build            |   1 +
>>   4 files changed, 199 insertions(+)
>>   create mode 100644 include/hw/s390x/cpu-topology.h
>>   create mode 100644 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);
>> +
>> +    topo->cpus = ms->smp.cores * ms->smp.threads;
> 
> Currently threads are not supported, effectively increasing the number of cpus,
> so this is currently correct. Once the machine version limits the threads to 1,
> it is also correct. However, once we support multiple threads, this becomes incorrect.
> I wonder if it's ok from a backward compatibility point of view to modify the smp values
> by doing cores *= threads, threads = 1 for old machines.

Right, this will become incorrect with thread support.
What about having a dedicated function:

	topo->cpus = s390_get_cpus(ms);

This function will use the S390CcwMachineClass->max_thread introduced 
later to report the correct number of CPUs.


> Then you can just use the cores value and it is always correct.
> In any case, if you keep it as is, I'd like to see a comment here saying that this
> is correct only so long as we don't support threads.
>> +
>> +    topo->socket = g_new0(S390TopoContainer, ms->smp.sockets);
>> +    topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
>> +
>> +    topo->ms = ms;
>> +}
>> +
> [...]

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v10 7/9] s390x/cpu topology: add max_threads machine class attribute

[Pierre Morel]

On 10/18/22 19:36, Cédric Le Goater wrote:
> On 10/12/22 18:21, Pierre Morel wrote:
>> The S390 CPU topology accepts the smp.threads argument while
>> in reality it does not effectively allow multthreading.
>>
>> Let's keep this behavior for machines older than 7.3 and
>> refuse to use threads in newer machines until multithreading
>> is really proposed to the guest by the machine.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   include/hw/s390x/s390-virtio-ccw.h |  1 +
>>   hw/s390x/s390-virtio-ccw.c         | 10 ++++++++++
>>   2 files changed, 11 insertions(+)
>>
>> diff --git a/include/hw/s390x/s390-virtio-ccw.h 
>> b/include/hw/s390x/s390-virtio-ccw.h
>> index 6c4b4645fc..319dfac1bb 100644
>> --- a/include/hw/s390x/s390-virtio-ccw.h
>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>> @@ -48,6 +48,7 @@ struct S390CcwMachineClass {
>>       bool css_migration_enabled;
>>       bool hpage_1m_allowed;
>>       bool topology_allowed;
>> +    int max_threads;
>>   };
>>   /* runtime-instrumentation allowed by the machine */
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 3a13fad4df..d6ce31d168 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -85,8 +85,15 @@ out:
>>   static void s390_init_cpus(MachineState *machine)
>>   {
>>       MachineClass *mc = MACHINE_GET_CLASS(machine);
>> +    S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
>>       int i;
>> +    if (machine->smp.threads > s390mc->max_threads) {
>> +        error_report("S390 does not support more than %d threads.",
>> +                     s390mc->max_threads);
>> +        exit(1);
>> +    }
>> +
>>       /* initialize possible_cpus */
>>       mc->possible_cpu_arch_ids(machine);
>> @@ -617,6 +624,7 @@ static void ccw_machine_class_init(ObjectClass 
>> *oc, void *data)
>>       s390mc->css_migration_enabled = true;
>>       s390mc->hpage_1m_allowed = true;
>>       s390mc->topology_allowed = true;
>> +    s390mc->max_threads = 1;
>>       mc->init = ccw_init;
>>       mc->reset = s390_machine_reset;
>>       mc->block_default_type = IF_VIRTIO;
>> @@ -887,12 +895,14 @@ static void 
>> ccw_machine_7_2_class_options(MachineClass *mc)
>>       S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
>>       static GlobalProperty compat[] = {
>>           { TYPE_S390_CPU_TOPOLOGY, "topology-allowed", "off", },
>> +        { TYPE_S390_CPU_TOPOLOGY, "max_threads", "off", },
> 
> I don't understand this change.

hum, this was a try to understand how GlobalProperty_compat works and I 
forgot to remove it.
I must say I did not understand exactly how it works.

> 
> 
> C.
> 
> 
>>       };
>>       ccw_machine_7_3_class_options(mc);
>>       compat_props_add(mc->compat_props, hw_compat_7_2, 
>> hw_compat_7_2_len);
>>       compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
>>       s390mc->topology_allowed = false;
>> +    s390mc->max_threads = S390_MAX_CPUS;
>>   }
>>   DEFINE_CCW_MACHINE(7_2, "7.2", false);
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

[Thomas Huth]

On 24/10/2022 21.25, Janis Schoetterl-Glausch wrote:
> On Wed, 2022-10-12 at 18:20 +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.
>> s390x/cpu topology: core_id sets s390x CPU topology
>>
>> In the S390x CPU topology the core_id specifies the CPU address
>> and the position of the cpu withing the topology.
>>
>> Let's build the topology based on the core_id.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   include/hw/s390x/cpu-topology.h |  45 +++++++++++
>>   hw/s390x/cpu-topology.c         | 132 ++++++++++++++++++++++++++++++++
>>   hw/s390x/s390-virtio-ccw.c      |  21 +++++
>>   hw/s390x/meson.build            |   1 +
>>   4 files changed, 199 insertions(+)
>>   create mode 100644 include/hw/s390x/cpu-topology.h
>>   create mode 100644 hw/s390x/cpu-topology.c
>>
>> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
>> new file mode 100644
>> index 0000000000..66c171d0bc
>> --- /dev/null
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -0,0 +1,45 @@
>> +/*
>> + * 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
>> +
>> +#include "hw/qdev-core.h"
>> +#include "qom/object.h"
>> +
>> +typedef struct S390TopoContainer {
>> +    int active_count;
>> +} S390TopoContainer;
>> +
>> +#define S390_TOPOLOGY_CPU_IFL 0x03
>> +#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
>> +typedef struct S390TopoTLE {
>> +    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
>> +} S390TopoTLE;
> 
> Since this actually represents multiple TLEs, you might want to change the
> name of the struct to reflect this. S390TopoTLEList maybe?

Didn't TLE mean "Topology List Entry"? (by the way, Pierre, please explain 
this three letter acronym somewhere in this header in a comment)...

So expanding the TLE, this would mean S390TopoTopologyListEntryList ? ... 
this is getting weird... Also, this is not a "list" in the sense of a linked 
list, as one might expect at a first glance, so this is all very confusing 
here. Could you please come up with some better naming?

  Thomas

Re: [PATCH v10 2/9] s390x/cpu topology: reporting the CPU topology to the guest

[Thomas Huth]

On 12/10/2022 18.21, 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>
> ---
>   include/hw/s390x/cpu-topology.h |   3 +
>   target/s390x/cpu.h              |  48 ++++++++++++++
>   hw/s390x/cpu-topology.c         |   8 ++-
>   target/s390x/cpu_topology.c     | 109 ++++++++++++++++++++++++++++++++
>   target/s390x/kvm/kvm.c          |   6 +-
>   target/s390x/meson.build        |   1 +
>   6 files changed, 172 insertions(+), 3 deletions(-)
>   create mode 100644 target/s390x/cpu_topology.c
> 
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> index 66c171d0bc..61c11db017 100644
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -13,6 +13,8 @@
>   #include "hw/qdev-core.h"
>   #include "qom/object.h"
>   
> +#define S390_TOPOLOGY_POLARITY_H  0x00
> +
>   typedef struct S390TopoContainer {
>       int active_count;
>   } S390TopoContainer;
> @@ -29,6 +31,7 @@ struct S390Topology {
>       S390TopoContainer *socket;
>       S390TopoTLE *tle;
>       MachineState *ms;
> +    QemuMutex topo_mutex;
>   };
>   
>   #define TYPE_S390_CPU_TOPOLOGY "s390-topology"
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 7d6d01325b..d604aa9c78 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -565,6 +565,52 @@ typedef union SysIB {
>   } SysIB;
>   QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
>   
> +/* CPU type Topology List Entry */
> +typedef struct SysIBTl_cpu {
> +        uint8_t nl;
> +        uint8_t reserved0[3];
> +        uint8_t reserved1:5;
> +        uint8_t dedicated:1;
> +        uint8_t polarity:2;
> +        uint8_t type;
> +        uint16_t origin;
> +        uint64_t mask;
> +} QEMU_PACKED QEMU_ALIGNED(8) 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 QEMU_ALIGNED(8) SysIBTl_container;
> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_container) != 8);
> +
> +#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;
> +    char tle[0];
> +} QEMU_PACKED QEMU_ALIGNED(8) SysIB_151x;
> +QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
> +
> +/* Maxi size of a SYSIB structure is when all CPU are alone in a container */
> +#define S390_TOPOLOGY_SYSIB_SIZE (sizeof(SysIB_151x) +                         \
> +                                  S390_MAX_CPUS * (sizeof(SysIBTl_container) + \
> +                                                   sizeof(SysIBTl_cpu)))
> +
> +
>   /* MMU defines */
>   #define ASCE_ORIGIN           (~0xfffULL) /* segment table origin             */
>   #define ASCE_SUBSPACE         0x200       /* subspace group control           */
> @@ -843,4 +889,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/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> index 42b22a1831..c73cebfe6f 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -54,8 +54,6 @@ void s390_topology_new_cpu(int core_id)
>           return;
>       }
>   
> -    socket_id = core_id / topo->cpus;
> -
>       /*
>        * At the core level, each CPU is represented by a bit in a 64bit
>        * unsigned long which represent the presence of a CPU.
> @@ -76,8 +74,13 @@ void s390_topology_new_cpu(int core_id)
>       bit %= 64;
>       bit = 63 - bit;
>   
> +    qemu_mutex_lock(&topo->topo_mutex);
> +
> +    socket_id = core_id / topo->cpus;
>       topo->socket[socket_id].active_count++;
>       set_bit(bit, &topo->tle[socket_id].mask[origin]);
> +
> +    qemu_mutex_unlock(&topo->topo_mutex);
>   }
>   
>   /**
> @@ -101,6 +104,7 @@ static void s390_topology_realize(DeviceState *dev, Error **errp)
>       topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
>   
>       topo->ms = ms;
> +    qemu_mutex_init(&topo->topo_mutex);
>   }
>   
>   /**
> diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
> new file mode 100644
> index 0000000000..df86a98f23
> --- /dev/null
> +++ b/target/s390x/cpu_topology.c
> @@ -0,0 +1,109 @@
> +/*
> + * 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"
> +
> +#define S390_TOPOLOGY_MAX_STSI_SIZE (S390_MAX_CPUS *              \
> +                                     (sizeof(SysIB_151x) +        \
> +                                      sizeof(SysIBTl_container) + \
> +                                      sizeof(SysIBTl_cpu)))
> +
> +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_IFL;
> +    tle->origin = cpu_to_be64(origin * 64);
> +    tle->mask = cpu_to_be64(mask);
> +    return p + sizeof(*tle);
> +}
> +
> +static char *s390_top_set_level2(S390Topology *topo, char *p)
> +{
> +    MachineState *ms = topo->ms;
> +    int i, origin;
> +
> +    for (i = 0; i < ms->smp.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 = 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();
> +    MachineState *ms = topo->ms;
> +    char *p = sysib->tle;
> +
> +    qemu_mutex_lock(&topo->topo_mutex);
> +
> +    sysib->mnest = level;
> +    switch (level) {
> +    case 2:
> +        sysib->mag[TOPOLOGY_NR_MAG2] = ms->smp.sockets;
> +        sysib->mag[TOPOLOGY_NR_MAG1] = topo->cpus;
> +        p = s390_top_set_level2(topo, p);
> +        break;
> +    }
> +
> +    qemu_mutex_unlock(&topo->topo_mutex);

Could you elaborate (maybe in the commit description) why you need a 
separate mutex here? ... I'd expect that all the STSI stuff is run with the 
BQL (big qemu lock) held (see kvm_arch_handle_exit()), so yet another mutex 
sounds rendundant to me here?

  Thomas

Re: [PATCH v10 3/9] s390x/cpu_topology: resetting the Topology-Change-Report

[Thomas Huth]

On 12/10/2022 18.21, 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>
> Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
> Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>   target/s390x/cpu.h           |  1 +
>   target/s390x/kvm/kvm_s390x.h |  1 +
>   hw/s390x/cpu-topology.c      | 12 ++++++++++++
>   hw/s390x/s390-virtio-ccw.c   |  1 +
>   target/s390x/cpu-sysemu.c    |  7 +++++++
>   target/s390x/kvm/kvm.c       | 23 +++++++++++++++++++++++
>   6 files changed, 45 insertions(+)
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index d604aa9c78..9b35795ac8 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -825,6 +825,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_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 */
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> index c73cebfe6f..9f202621d0 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -107,6 +107,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
> @@ -120,6 +131,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 aa99a62e42..362378454a 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -113,6 +113,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/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;

EFAULT is something that indicates a bad address (e.g. a segmentation fault) 
... so this definitely sounds like a bad choice for an error code here.

  Thomas

Re: [PATCH v10 3/9] s390x/cpu_topology: resetting the Topology-Change-Report

[Pierre Morel]

On 10/27/22 10:14, Thomas Huth wrote:
> On 12/10/2022 18.21, 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>
>> Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
>> Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
>>   target/s390x/cpu.h           |  1 +
>>   target/s390x/kvm/kvm_s390x.h |  1 +
>>   hw/s390x/cpu-topology.c      | 12 ++++++++++++
>>   hw/s390x/s390-virtio-ccw.c   |  1 +
>>   target/s390x/cpu-sysemu.c    |  7 +++++++
>>   target/s390x/kvm/kvm.c       | 23 +++++++++++++++++++++++
>>   6 files changed, 45 insertions(+)
>>
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index d604aa9c78..9b35795ac8 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -825,6 +825,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_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 */
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> index c73cebfe6f..9f202621d0 100644
>> --- a/hw/s390x/cpu-topology.c
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -107,6 +107,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
>> @@ -120,6 +131,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 aa99a62e42..362378454a 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -113,6 +113,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/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;
> 
> EFAULT is something that indicates a bad address (e.g. a segmentation 
> fault) ... so this definitely sounds like a bad choice for an error code 
> here.

Hum, yes, ENODEV seems besser no?

> 
>   Thomas
> 
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

[Janis Schoetterl-Glausch]

On Thu, 2022-10-27 at 10:05 +0200, Thomas Huth wrote:
> On 24/10/2022 21.25, Janis Schoetterl-Glausch wrote:
> > On Wed, 2022-10-12 at 18:20 +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.
> > > s390x/cpu topology: core_id sets s390x CPU topology
> > > 
> > > In the S390x CPU topology the core_id specifies the CPU address
> > > and the position of the cpu withing the topology.
> > > 
> > > Let's build the topology based on the core_id.
> > > 
> > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > > ---
> > >   include/hw/s390x/cpu-topology.h |  45 +++++++++++
> > >   hw/s390x/cpu-topology.c         | 132 ++++++++++++++++++++++++++++++++
> > >   hw/s390x/s390-virtio-ccw.c      |  21 +++++
> > >   hw/s390x/meson.build            |   1 +
> > >   4 files changed, 199 insertions(+)
> > >   create mode 100644 include/hw/s390x/cpu-topology.h
> > >   create mode 100644 hw/s390x/cpu-topology.c
> > > 
> > > diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> > > new file mode 100644
> > > index 0000000000..66c171d0bc
> > > --- /dev/null
> > > +++ b/include/hw/s390x/cpu-topology.h
> > > @@ -0,0 +1,45 @@
> > > +/*
> > > + * 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
> > > +
> > > +#include "hw/qdev-core.h"
> > > +#include "qom/object.h"
> > > +
> > > +typedef struct S390TopoContainer {
> > > +    int active_count;
> > > +} S390TopoContainer;
> > > +
> > > +#define S390_TOPOLOGY_CPU_IFL 0x03
> > > +#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
> > > +typedef struct S390TopoTLE {
> > > +    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
> > > +} S390TopoTLE;
> > 
> > Since this actually represents multiple TLEs, you might want to change the
> > name of the struct to reflect this. S390TopoTLEList maybe?
> 
> Didn't TLE mean "Topology List Entry"? (by the way, Pierre, please explain 

Yes.

> this three letter acronym somewhere in this header in a comment)...
> 
> So expanding the TLE, this would mean S390TopoTopologyListEntryList ? ... 
> this is getting weird...

:D indeed. So the leaves of the topology tree as stored by STSI are lists
of CPU-type TLEs which aren't empty i.e. represent some cpus.
Whereas this struct is used to track which CPU-type TLEs need to be created.
It doesn't represent a TLE and doesn't represent the list of CPU-type TLEs.
So yeah, you're right, not a good name.

Off the top of my head I'd suggest S390TopoCPUSet. It's a bitmap, which is
kind of a set. Maybe S390TopoSocketCPUSet to reflect that it is the set of
CPUs in a socket, although, if we ever support different polarizations, etc.
that wouldn't really be true anymore, since that creates additional levels,
so maybe not. (In that case the leaf list of CPU-types TLEs is a flattened tree.)

> Also, this is not a "list" in the sense of a linked 
> list, as one might expect at a first glance, so this is all very confusing 
> here. Could you please come up with some better naming?
> 
>   Thomas
> 
> 

Re: [PATCH v10 3/9] s390x/cpu_topology: resetting the Topology-Change-Report

[Cédric Le Goater]

On 10/27/22 11:11, Pierre Morel wrote:
> 
> 
> On 10/27/22 10:14, Thomas Huth wrote:
>> On 12/10/2022 18.21, 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>
>>> Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
>>> Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>>> ---
>>>   target/s390x/cpu.h           |  1 +
>>>   target/s390x/kvm/kvm_s390x.h |  1 +
>>>   hw/s390x/cpu-topology.c      | 12 ++++++++++++
>>>   hw/s390x/s390-virtio-ccw.c   |  1 +
>>>   target/s390x/cpu-sysemu.c    |  7 +++++++
>>>   target/s390x/kvm/kvm.c       | 23 +++++++++++++++++++++++
>>>   6 files changed, 45 insertions(+)
>>>
>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>>> index d604aa9c78..9b35795ac8 100644
>>> --- a/target/s390x/cpu.h
>>> +++ b/target/s390x/cpu.h
>>> @@ -825,6 +825,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_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 */
>>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>>> index c73cebfe6f..9f202621d0 100644
>>> --- a/hw/s390x/cpu-topology.c
>>> +++ b/hw/s390x/cpu-topology.c
>>> @@ -107,6 +107,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
>>> @@ -120,6 +131,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 aa99a62e42..362378454a 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -113,6 +113,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/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;
>>
>> EFAULT is something that indicates a bad address (e.g. a segmentation fault) ... so this definitely sounds like a bad choice for an error code here.
> 
> Hum, yes, ENODEV seems besser no?

-ENOTSUP would be 'meilleur' may be ?  :)

C.


> 
>>
>>   Thomas
>>
>>
> 

Re: [PATCH v10 7/9] s390x/cpu topology: add max_threads machine class attribute

[Cédric Le Goater]

Hello Pierre,

On 10/12/22 18:21, Pierre Morel wrote:
> The S390 CPU topology accepts the smp.threads argument while
> in reality it does not effectively allow multthreading.
> 
> Let's keep this behavior for machines older than 7.3 and
> refuse to use threads in newer machines until multithreading
> is really proposed to the guest by the machine.

This change is unrelated to the rest of the series and we could merge it
for 7.2. We still have time for it.

Thanks,

C.

  
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   include/hw/s390x/s390-virtio-ccw.h |  1 +
>   hw/s390x/s390-virtio-ccw.c         | 10 ++++++++++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 6c4b4645fc..319dfac1bb 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -48,6 +48,7 @@ struct S390CcwMachineClass {
>       bool css_migration_enabled;
>       bool hpage_1m_allowed;
>       bool topology_allowed;
> +    int max_threads;
>   };
>   
>   /* runtime-instrumentation allowed by the machine */
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 3a13fad4df..d6ce31d168 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -85,8 +85,15 @@ out:
>   static void s390_init_cpus(MachineState *machine)
>   {
>       MachineClass *mc = MACHINE_GET_CLASS(machine);
> +    S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
>       int i;
>   
> +    if (machine->smp.threads > s390mc->max_threads) {
> +        error_report("S390 does not support more than %d threads.",
> +                     s390mc->max_threads);
> +        exit(1);
> +    }
> +
>       /* initialize possible_cpus */
>       mc->possible_cpu_arch_ids(machine);
>   
> @@ -617,6 +624,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>       s390mc->css_migration_enabled = true;
>       s390mc->hpage_1m_allowed = true;
>       s390mc->topology_allowed = true;
> +    s390mc->max_threads = 1;
>       mc->init = ccw_init;
>       mc->reset = s390_machine_reset;
>       mc->block_default_type = IF_VIRTIO;
> @@ -887,12 +895,14 @@ static void ccw_machine_7_2_class_options(MachineClass *mc)
>       S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
>       static GlobalProperty compat[] = {
>           { TYPE_S390_CPU_TOPOLOGY, "topology-allowed", "off", },
> +        { TYPE_S390_CPU_TOPOLOGY, "max_threads", "off", },
>       };
>   
>       ccw_machine_7_3_class_options(mc);
>       compat_props_add(mc->compat_props, hw_compat_7_2, hw_compat_7_2_len);
>       compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
>       s390mc->topology_allowed = false;
> +    s390mc->max_threads = S390_MAX_CPUS;
>   }
>   DEFINE_CCW_MACHINE(7_2, "7.2", false);
>   

Re: [PATCH v10 2/9] s390x/cpu topology: reporting the CPU topology to the guest

[Pierre Morel]

On 10/27/22 10:12, Thomas Huth wrote:
> On 12/10/2022 18.21, 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>
>> ---
>>   include/hw/s390x/cpu-topology.h |   3 +
>>   target/s390x/cpu.h              |  48 ++++++++++++++
>>   hw/s390x/cpu-topology.c         |   8 ++-
>>   target/s390x/cpu_topology.c     | 109 ++++++++++++++++++++++++++++++++
>>   target/s390x/kvm/kvm.c          |   6 +-
>>   target/s390x/meson.build        |   1 +
>>   6 files changed, 172 insertions(+), 3 deletions(-)
>>   create mode 100644 target/s390x/cpu_topology.c
>>
>> diff --git a/include/hw/s390x/cpu-topology.h 
>> b/include/hw/s390x/cpu-topology.h
>> index 66c171d0bc..61c11db017 100644
>> --- a/include/hw/s390x/cpu-topology.h
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -13,6 +13,8 @@
>>   #include "hw/qdev-core.h"
>>   #include "qom/object.h"
>> +#define S390_TOPOLOGY_POLARITY_H  0x00
>> +
>>   typedef struct S390TopoContainer {
>>       int active_count;
>>   } S390TopoContainer;
>> @@ -29,6 +31,7 @@ struct S390Topology {
>>       S390TopoContainer *socket;
>>       S390TopoTLE *tle;
>>       MachineState *ms;
>> +    QemuMutex topo_mutex;
>>   };
>>   #define TYPE_S390_CPU_TOPOLOGY "s390-topology"
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 7d6d01325b..d604aa9c78 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -565,6 +565,52 @@ typedef union SysIB {
>>   } SysIB;
>>   QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
>> +/* CPU type Topology List Entry */
>> +typedef struct SysIBTl_cpu {
>> +        uint8_t nl;
>> +        uint8_t reserved0[3];
>> +        uint8_t reserved1:5;
>> +        uint8_t dedicated:1;
>> +        uint8_t polarity:2;
>> +        uint8_t type;
>> +        uint16_t origin;
>> +        uint64_t mask;
>> +} QEMU_PACKED QEMU_ALIGNED(8) 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 QEMU_ALIGNED(8) SysIBTl_container;
>> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_container) != 8);
>> +
>> +#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;
>> +    char tle[0];
>> +} QEMU_PACKED QEMU_ALIGNED(8) SysIB_151x;
>> +QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
>> +
>> +/* Maxi size of a SYSIB structure is when all CPU are alone in a 
>> container */
>> +#define S390_TOPOLOGY_SYSIB_SIZE (sizeof(SysIB_151x) 
>> +                         \
>> +                                  S390_MAX_CPUS * 
>> (sizeof(SysIBTl_container) + \
>> +                                                   sizeof(SysIBTl_cpu)))
>> +
>> +
>>   /* MMU defines */
>>   #define ASCE_ORIGIN           (~0xfffULL) /* segment table 
>> origin             */
>>   #define ASCE_SUBSPACE         0x200       /* subspace group 
>> control           */
>> @@ -843,4 +889,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/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> index 42b22a1831..c73cebfe6f 100644
>> --- a/hw/s390x/cpu-topology.c
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -54,8 +54,6 @@ void s390_topology_new_cpu(int core_id)
>>           return;
>>       }
>> -    socket_id = core_id / topo->cpus;
>> -
>>       /*
>>        * At the core level, each CPU is represented by a bit in a 64bit
>>        * unsigned long which represent the presence of a CPU.
>> @@ -76,8 +74,13 @@ void s390_topology_new_cpu(int core_id)
>>       bit %= 64;
>>       bit = 63 - bit;
>> +    qemu_mutex_lock(&topo->topo_mutex);
>> +
>> +    socket_id = core_id / topo->cpus;
>>       topo->socket[socket_id].active_count++;
>>       set_bit(bit, &topo->tle[socket_id].mask[origin]);
>> +
>> +    qemu_mutex_unlock(&topo->topo_mutex);
>>   }
>>   /**
>> @@ -101,6 +104,7 @@ static void s390_topology_realize(DeviceState 
>> *dev, Error **errp)
>>       topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
>>       topo->ms = ms;
>> +    qemu_mutex_init(&topo->topo_mutex);
>>   }
>>   /**
>> diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
>> new file mode 100644
>> index 0000000000..df86a98f23
>> --- /dev/null
>> +++ b/target/s390x/cpu_topology.c
>> @@ -0,0 +1,109 @@
>> +/*
>> + * 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"
>> +
>> +#define S390_TOPOLOGY_MAX_STSI_SIZE (S390_MAX_CPUS *              \
>> +                                     (sizeof(SysIB_151x) +        \
>> +                                      sizeof(SysIBTl_container) + \
>> +                                      sizeof(SysIBTl_cpu)))
>> +
>> +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_IFL;
>> +    tle->origin = cpu_to_be64(origin * 64);
>> +    tle->mask = cpu_to_be64(mask);
>> +    return p + sizeof(*tle);
>> +}
>> +
>> +static char *s390_top_set_level2(S390Topology *topo, char *p)
>> +{
>> +    MachineState *ms = topo->ms;
>> +    int i, origin;
>> +
>> +    for (i = 0; i < ms->smp.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 = 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();
>> +    MachineState *ms = topo->ms;
>> +    char *p = sysib->tle;
>> +
>> +    qemu_mutex_lock(&topo->topo_mutex);
>> +
>> +    sysib->mnest = level;
>> +    switch (level) {
>> +    case 2:
>> +        sysib->mag[TOPOLOGY_NR_MAG2] = ms->smp.sockets;
>> +        sysib->mag[TOPOLOGY_NR_MAG1] = topo->cpus;
>> +        p = s390_top_set_level2(topo, p);
>> +        break;
>> +    }
>> +
>> +    qemu_mutex_unlock(&topo->topo_mutex);
> 
> Could you elaborate (maybe in the commit description) why you need a 
> separate mutex here? ... I'd expect that all the STSI stuff is run with 
> the BQL (big qemu lock) held (see kvm_arch_handle_exit()), so yet 
> another mutex sounds rendundant to me here?
> 
>   Thomas
> 

Right and since BQL is hold for the hotplug, there is no need.
Thanks.

Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v10 3/9] s390x/cpu_topology: resetting the Topology-Change-Report

[Pierre Morel]

On 10/27/22 11:58, Cédric Le Goater wrote:
> On 10/27/22 11:11, Pierre Morel wrote:
>>
>>
>> On 10/27/22 10:14, Thomas Huth wrote:
>>> On 12/10/2022 18.21, 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>
>>>> Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
>>>> Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>>>> ---
>>>>   target/s390x/cpu.h           |  1 +
>>>>   target/s390x/kvm/kvm_s390x.h |  1 +
>>>>   hw/s390x/cpu-topology.c      | 12 ++++++++++++
>>>>   hw/s390x/s390-virtio-ccw.c   |  1 +
>>>>   target/s390x/cpu-sysemu.c    |  7 +++++++
>>>>   target/s390x/kvm/kvm.c       | 23 +++++++++++++++++++++++
>>>>   6 files changed, 45 insertions(+)
>>>>
>>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>>>> index d604aa9c78..9b35795ac8 100644
>>>> --- a/target/s390x/cpu.h
>>>> +++ b/target/s390x/cpu.h
>>>> @@ -825,6 +825,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_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 */
>>>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>>>> index c73cebfe6f..9f202621d0 100644
>>>> --- a/hw/s390x/cpu-topology.c
>>>> +++ b/hw/s390x/cpu-topology.c
>>>> @@ -107,6 +107,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
>>>> @@ -120,6 +131,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 aa99a62e42..362378454a 100644
>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>> @@ -113,6 +113,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/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;
>>>
>>> EFAULT is something that indicates a bad address (e.g. a segmentation 
>>> fault) ... so this definitely sounds like a bad choice for an error 
>>> code here.
>>
>> Hum, yes, ENODEV seems besser no?
> 
> -ENOTSUP would be 'meilleur' may be ?  :)

yes better :)

thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v10 7/9] s390x/cpu topology: add max_threads machine class attribute

[Pierre Morel]

On 10/27/22 12:00, Cédric Le Goater wrote:
> Hello Pierre,
> 
> On 10/12/22 18:21, Pierre Morel wrote:
>> The S390 CPU topology accepts the smp.threads argument while
>> in reality it does not effectively allow multthreading.
>>
>> Let's keep this behavior for machines older than 7.3 and
>> refuse to use threads in newer machines until multithreading
>> is really proposed to the guest by the machine.
> 
> This change is unrelated to the rest of the series and we could merge it
> for 7.2. We still have time for it.

OK, then I send it on its own

Regards,
Pierre

...

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

[Janis Schoetterl-Glausch]

On Wed, 2022-10-26 at 10:34 +0200, Pierre Morel wrote:
> 
> On 10/25/22 21:58, Janis Schoetterl-Glausch wrote:
> > On Wed, 2022-10-12 at 18:20 +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.
> > > s390x/cpu topology: core_id sets s390x CPU topology
> > > 
> > > In the S390x CPU topology the core_id specifies the CPU address
> > > and the position of the cpu withing the topology.
> > > 
> > > Let's build the topology based on the core_id.
> > > 
> > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > > ---
> > >   include/hw/s390x/cpu-topology.h |  45 +++++++++++
> > >   hw/s390x/cpu-topology.c         | 132 ++++++++++++++++++++++++++++++++
> > >   hw/s390x/s390-virtio-ccw.c      |  21 +++++
> > >   hw/s390x/meson.build            |   1 +
> > >   4 files changed, 199 insertions(+)
> > >   create mode 100644 include/hw/s390x/cpu-topology.h
> > >   create mode 100644 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);
> > > +
> > > +    topo->cpus = ms->smp.cores * ms->smp.threads;
> > 
> > Currently threads are not supported, effectively increasing the number of cpus,
> > so this is currently correct. Once the machine version limits the threads to 1,
> > it is also correct. However, once we support multiple threads, this becomes incorrect.
> > I wonder if it's ok from a backward compatibility point of view to modify the smp values
> > by doing cores *= threads, threads = 1 for old machines.
> 
> Right, this will become incorrect with thread support.
> What about having a dedicated function:
> 
> 	topo->cpus = s390_get_cpus(ms);
> 
> This function will use the S390CcwMachineClass->max_thread introduced 
> later to report the correct number of CPUs.

I don't think max_threads is exactly what matters here, it's if
threads are supported or not or, if max_threads == 1 it doesn't matter.
The question is how best to do the check. You could check the machine version.
I wonder if you could add a feature bit for the multithreading facility that is
always false and use that.

I don't know if using a function makes a difference, that is if it is obvious on
introduction of multithreading support that the function needs to be updated.
(If it is implemented in a way that requires updating, if you check the machine
version it doesn't)
In any case, the name you suggested isn't very descriptive.
> 
> 
> > Then you can just use the cores value and it is always correct.
> > In any case, if you keep it as is, I'd like to see a comment here saying that this
> > is correct only so long as we don't support threads.
> > > +
> > > +    topo->socket = g_new0(S390TopoContainer, ms->smp.sockets);
> > > +    topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
> > > +
> > > +    topo->ms = ms;
> > > +}
> > > +
> > [...]
> 

Re: [PATCH v10 2/9] s390x/cpu topology: reporting the CPU topology to the guest

[Janis Schoetterl-Glausch]

On Wed, 2022-10-12 at 18:21 +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>
> ---
>  include/hw/s390x/cpu-topology.h |   3 +
>  target/s390x/cpu.h              |  48 ++++++++++++++
>  hw/s390x/cpu-topology.c         |   8 ++-
>  target/s390x/cpu_topology.c     | 109 ++++++++++++++++++++++++++++++++
>  target/s390x/kvm/kvm.c          |   6 +-
>  target/s390x/meson.build        |   1 +
>  6 files changed, 172 insertions(+), 3 deletions(-)
>  create mode 100644 target/s390x/cpu_topology.c
> 
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> index 66c171d0bc..61c11db017 100644
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -13,6 +13,8 @@
>  #include "hw/qdev-core.h"
>  #include "qom/object.h"
>  
> +#define S390_TOPOLOGY_POLARITY_H  0x00
> +
>  typedef struct S390TopoContainer {
>      int active_count;
>  } S390TopoContainer;
> @@ -29,6 +31,7 @@ struct S390Topology {
>      S390TopoContainer *socket;
>      S390TopoTLE *tle;
>      MachineState *ms;
> +    QemuMutex topo_mutex;
>  };
>  
>  #define TYPE_S390_CPU_TOPOLOGY "s390-topology"
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 7d6d01325b..d604aa9c78 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> 
[...]
> +
> +/* Maxi size of a SYSIB structure is when all CPU are alone in a container */

Max or Maximum.

> +#define S390_TOPOLOGY_SYSIB_SIZE (sizeof(SysIB_151x) +                         \
> +                                  S390_MAX_CPUS * (sizeof(SysIBTl_container) + \
> +                                                   sizeof(SysIBTl_cpu)))

Currently this is 16+248*3*8 == 5968 and will grow with books, drawer support to
16+248*5*8 == 9936 ...

[...]
> 
> +
> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
> +{
> +    uint64_t page[S390_TOPOLOGY_SYSIB_SIZE / sizeof(uint64_t)] = {};

... so calling this page is a bit misleading. Also why not make it a char[]?
And maybe use a union for type punning.

> +    SysIB_151x *sysib = (SysIB_151x *) page;
> +    int len;
> +
> +    if (s390_is_pv() || !s390_has_topology() ||
> +        sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
> +        setcc(cpu, 3);
> +        return;
> +    }
> +
> +    len = setup_stsi(sysib, sel2);

This should now be memory safe, but might be larger than 4k,
the maximum size of the SYSIB. I guess you want to set cc code 3
in this case and return.
> +
> +    sysib->length = cpu_to_be16(len);
> +    s390_cpu_virt_mem_write(cpu, addr, ar, sysib, len);
> +    setcc(cpu, 0);
> +}
> +
> 
[...]

Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

[Pierre Morel]

On 10/27/22 22:20, Janis Schoetterl-Glausch wrote:
> On Wed, 2022-10-26 at 10:34 +0200, Pierre Morel wrote:
>>
>> On 10/25/22 21:58, Janis Schoetterl-Glausch wrote:
>>> On Wed, 2022-10-12 at 18:20 +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.
>>>> s390x/cpu topology: core_id sets s390x CPU topology
>>>>
>>>> In the S390x CPU topology the core_id specifies the CPU address
>>>> and the position of the cpu withing the topology.
>>>>
>>>> Let's build the topology based on the core_id.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> ---
>>>>    include/hw/s390x/cpu-topology.h |  45 +++++++++++
>>>>    hw/s390x/cpu-topology.c         | 132 ++++++++++++++++++++++++++++++++
>>>>    hw/s390x/s390-virtio-ccw.c      |  21 +++++
>>>>    hw/s390x/meson.build            |   1 +
>>>>    4 files changed, 199 insertions(+)
>>>>    create mode 100644 include/hw/s390x/cpu-topology.h
>>>>    create mode 100644 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);
>>>> +
>>>> +    topo->cpus = ms->smp.cores * ms->smp.threads;
>>>
>>> Currently threads are not supported, effectively increasing the number of cpus,
>>> so this is currently correct. Once the machine version limits the threads to 1,
>>> it is also correct. However, once we support multiple threads, this becomes incorrect.
>>> I wonder if it's ok from a backward compatibility point of view to modify the smp values
>>> by doing cores *= threads, threads = 1 for old machines.
>>
>> Right, this will become incorrect with thread support.
>> What about having a dedicated function:
>>
>> 	topo->cpus = s390_get_cpus(ms);
>>
>> This function will use the S390CcwMachineClass->max_thread introduced
>> later to report the correct number of CPUs.
> 
> I don't think max_threads is exactly what matters here, it's if
> threads are supported or not or, if max_threads == 1 it doesn't matter.
> The question is how best to do the check. You could check the machine version.
> I wonder if you could add a feature bit for the multithreading facility that is
> always false and use that.
> 
> I don't know if using a function makes a difference, that is if it is obvious on
> introduction of multithreading support that the function needs to be updated.
> (If it is implemented in a way that requires updating, if you check the machine
> version it doesn't)
> In any case, the name you suggested isn't very descriptive.

I think we care about this machine and olders.
Olders do not support topology so this, Multithreading (MT) does not mater.
This machine support topology, if I follow Cedric advise, the 
"max_thread" will/may be introduce before the topology.

This in fact is not an implementation for MT or does not allow the 
implementation of MT it is only a way to get rid of the false 
information given to the user that we accept MT.

So I think that when we introduce MT we will take care of making things 
right at this place as in other places of the code.

What about we keep the original:

     topo->cpus = ms->smp.cores * ms->smp.threads;

Which does not do any arm to machines without MT ?

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v10 2/9] s390x/cpu topology: reporting the CPU topology to the guest

[Pierre Morel]

On 10/27/22 22:42, Janis Schoetterl-Glausch wrote:
> On Wed, 2022-10-12 at 18:21 +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>
>> ---
>>   include/hw/s390x/cpu-topology.h |   3 +
>>   target/s390x/cpu.h              |  48 ++++++++++++++
>>   hw/s390x/cpu-topology.c         |   8 ++-
>>   target/s390x/cpu_topology.c     | 109 ++++++++++++++++++++++++++++++++
>>   target/s390x/kvm/kvm.c          |   6 +-
>>   target/s390x/meson.build        |   1 +
>>   6 files changed, 172 insertions(+), 3 deletions(-)
>>   create mode 100644 target/s390x/cpu_topology.c
>>
>> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
>> index 66c171d0bc..61c11db017 100644
>> --- a/include/hw/s390x/cpu-topology.h
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -13,6 +13,8 @@
>>   #include "hw/qdev-core.h"
>>   #include "qom/object.h"
>>   
>> +#define S390_TOPOLOGY_POLARITY_H  0x00
>> +
>>   typedef struct S390TopoContainer {
>>       int active_count;
>>   } S390TopoContainer;
>> @@ -29,6 +31,7 @@ struct S390Topology {
>>       S390TopoContainer *socket;
>>       S390TopoTLE *tle;
>>       MachineState *ms;
>> +    QemuMutex topo_mutex;
>>   };
>>   
>>   #define TYPE_S390_CPU_TOPOLOGY "s390-topology"
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 7d6d01325b..d604aa9c78 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>>
> [...]
>> +
>> +/* Maxi size of a SYSIB structure is when all CPU are alone in a container */
> 
> Max or Maximum.
> 
>> +#define S390_TOPOLOGY_SYSIB_SIZE (sizeof(SysIB_151x) +                         \
>> +                                  S390_MAX_CPUS * (sizeof(SysIBTl_container) + \
>> +                                                   sizeof(SysIBTl_cpu)))
> 
> Currently this is 16+248*3*8 == 5968 and will grow with books, drawer support to
> 16+248*5*8 == 9936 ...
> 
> [...]
>>
>> +
>> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
>> +{
>> +    uint64_t page[S390_TOPOLOGY_SYSIB_SIZE / sizeof(uint64_t)] = {};
> 
> ... so calling this page is a bit misleading. Also why not make it a char[]?
> And maybe use a union for type punning.

OK, what about:

     union {
         char place_holder[S390_TOPOLOGY_SYSIB_SIZE];
         SysIB_151x sysib;
     } buffer QEMU_ALIGNED(8);


> 
>> +    SysIB_151x *sysib = (SysIB_151x *) page;
>> +    int len;
>> +
>> +    if (s390_is_pv() || !s390_has_topology() ||
>> +        sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
>> +        setcc(cpu, 3);
>> +        return;
>> +    }
>> +
>> +    len = setup_stsi(sysib, sel2);
> 
> This should now be memory safe, but might be larger than 4k,
> the maximum size of the SYSIB. I guess you want to set cc code 3
> in this case and return.

I do not find why the SYSIB can not be larger than 4k.
Can you point me to this restriction?


Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v10 2/9] s390x/cpu topology: reporting the CPU topology to the guest

[Janis Schoetterl-Glausch]

On Fri, 2022-10-28 at 12:00 +0200, Pierre Morel wrote:
> 
> On 10/27/22 22:42, Janis Schoetterl-Glausch wrote:
> > On Wed, 2022-10-12 at 18:21 +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>
> > > ---
> > >   include/hw/s390x/cpu-topology.h |   3 +
> > >   target/s390x/cpu.h              |  48 ++++++++++++++
> > >   hw/s390x/cpu-topology.c         |   8 ++-
> > >   target/s390x/cpu_topology.c     | 109 ++++++++++++++++++++++++++++++++
> > >   target/s390x/kvm/kvm.c          |   6 +-
> > >   target/s390x/meson.build        |   1 +
> > >   6 files changed, 172 insertions(+), 3 deletions(-)
> > >   create mode 100644 target/s390x/cpu_topology.c
> > > 
> > > diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> > > index 66c171d0bc..61c11db017 100644
> > > --- a/include/hw/s390x/cpu-topology.h
> > > +++ b/include/hw/s390x/cpu-topology.h
> > > @@ -13,6 +13,8 @@
> > >   #include "hw/qdev-core.h"
> > >   #include "qom/object.h"
> > >   
> > > +#define S390_TOPOLOGY_POLARITY_H  0x00
> > > +
> > >   typedef struct S390TopoContainer {
> > >       int active_count;
> > >   } S390TopoContainer;
> > > @@ -29,6 +31,7 @@ struct S390Topology {
> > >       S390TopoContainer *socket;
> > >       S390TopoTLE *tle;
> > >       MachineState *ms;
> > > +    QemuMutex topo_mutex;
> > >   };
> > >   
> > >   #define TYPE_S390_CPU_TOPOLOGY "s390-topology"
> > > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> > > index 7d6d01325b..d604aa9c78 100644
> > > --- a/target/s390x/cpu.h
> > > +++ b/target/s390x/cpu.h
> > > 
> > [...]
> > > +
> > > +/* Maxi size of a SYSIB structure is when all CPU are alone in a container */
> > 
> > Max or Maximum.
> > 
> > > +#define S390_TOPOLOGY_SYSIB_SIZE (sizeof(SysIB_151x) +                         \
> > > +                                  S390_MAX_CPUS * (sizeof(SysIBTl_container) + \
> > > +                                                   sizeof(SysIBTl_cpu)))
> > 
> > Currently this is 16+248*3*8 == 5968 and will grow with books, drawer support to
> > 16+248*5*8 == 9936 ...
> > 
> > [...]
> > > 
> > > +
> > > +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
> > > +{
> > > +    uint64_t page[S390_TOPOLOGY_SYSIB_SIZE / sizeof(uint64_t)] = {};
> > 
> > ... so calling this page is a bit misleading. Also why not make it a char[]?
> > And maybe use a union for type punning.
> 
> OK, what about:
> 
>      union {
>          char place_holder[S390_TOPOLOGY_SYSIB_SIZE];
>          SysIB_151x sysib;
>      } buffer QEMU_ALIGNED(8);
> 
I don't think you need the QEMU_ALIGNED since SysIB_151x already has it. Not that it hurts to be
explicit. If you declared the tle member as uint64_t[], you should get the correct alignment
automatically and can then drop the explicit one.
Btw, [] seems to be preferred over [0], at least there is a commit doing a conversion:
f7795e4096 ("misc: Replace zero-length arrays with flexible array member (automatic)")
> 
> > 
> > > +    SysIB_151x *sysib = (SysIB_151x *) page;
> > > +    int len;
> > > +
> > > +    if (s390_is_pv() || !s390_has_topology() ||
> > > +        sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
> > > +        setcc(cpu, 3);
> > > +        return;
> > > +    }
> > > +
> > > +    len = setup_stsi(sysib, sel2);
> > 
> > This should now be memory safe, but might be larger than 4k,
> > the maximum size of the SYSIB. I guess you want to set cc code 3
> > in this case and return.
> 
> I do not find why the SYSIB can not be larger than 4k.
> Can you point me to this restriction?

Says so at the top of the description of STSI:

The SYSIB is 4K bytes and must begin at a 4 K-byte
boundary; otherwise, a specification exception may
be recognized.

Also the graphics show that it is 1024 words long.
> 
> 
> Regards,
> Pierre
> 

Re: [PATCH v10 2/9] s390x/cpu topology: reporting the CPU topology to the guest

[Pierre Morel]

On 11/7/22 14:20, Janis Schoetterl-Glausch wrote:
> On Fri, 2022-10-28 at 12:00 +0200, Pierre Morel wrote:
>>
>> On 10/27/22 22:42, Janis Schoetterl-Glausch wrote:
>>> On Wed, 2022-10-12 at 18:21 +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>
>>>> ---
>>>>    include/hw/s390x/cpu-topology.h |   3 +
>>>>    target/s390x/cpu.h              |  48 ++++++++++++++
>>>>    hw/s390x/cpu-topology.c         |   8 ++-
>>>>    target/s390x/cpu_topology.c     | 109 ++++++++++++++++++++++++++++++++
>>>>    target/s390x/kvm/kvm.c          |   6 +-
>>>>    target/s390x/meson.build        |   1 +
>>>>    6 files changed, 172 insertions(+), 3 deletions(-)
>>>>    create mode 100644 target/s390x/cpu_topology.c
>>>>
>>>> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
>>>> index 66c171d0bc..61c11db017 100644
>>>> --- a/include/hw/s390x/cpu-topology.h
>>>> +++ b/include/hw/s390x/cpu-topology.h
>>>> @@ -13,6 +13,8 @@
>>>>    #include "hw/qdev-core.h"
>>>>    #include "qom/object.h"
>>>>    
>>>> +#define S390_TOPOLOGY_POLARITY_H  0x00
>>>> +
>>>>    typedef struct S390TopoContainer {
>>>>        int active_count;
>>>>    } S390TopoContainer;
>>>> @@ -29,6 +31,7 @@ struct S390Topology {
>>>>        S390TopoContainer *socket;
>>>>        S390TopoTLE *tle;
>>>>        MachineState *ms;
>>>> +    QemuMutex topo_mutex;
>>>>    };
>>>>    
>>>>    #define TYPE_S390_CPU_TOPOLOGY "s390-topology"
>>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>>>> index 7d6d01325b..d604aa9c78 100644
>>>> --- a/target/s390x/cpu.h
>>>> +++ b/target/s390x/cpu.h
>>>>
>>> [...]
>>>> +
>>>> +/* Maxi size of a SYSIB structure is when all CPU are alone in a container */
>>>
>>> Max or Maximum.
>>>
>>>> +#define S390_TOPOLOGY_SYSIB_SIZE (sizeof(SysIB_151x) +                         \
>>>> +                                  S390_MAX_CPUS * (sizeof(SysIBTl_container) + \
>>>> +                                                   sizeof(SysIBTl_cpu)))
>>>
>>> Currently this is 16+248*3*8 == 5968 and will grow with books, drawer support to
>>> 16+248*5*8 == 9936 ...
>>>
>>> [...]
>>>>
>>>> +
>>>> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
>>>> +{
>>>> +    uint64_t page[S390_TOPOLOGY_SYSIB_SIZE / sizeof(uint64_t)] = {};
>>>
>>> ... so calling this page is a bit misleading. Also why not make it a char[]?
>>> And maybe use a union for type punning.
>>
>> OK, what about:
>>
>>       union {
>>           char place_holder[S390_TOPOLOGY_SYSIB_SIZE];
>>           SysIB_151x sysib;
>>       } buffer QEMU_ALIGNED(8);
>>
> I don't think you need the QEMU_ALIGNED since SysIB_151x already has it. Not that it hurts to be
> explicit. If you declared the tle member as uint64_t[], you should get the correct alignment
> automatically and can then drop the explicit one.

I find the explicit statement better. Why make it non explicit?

> Btw, [] seems to be preferred over [0], at least there is a commit doing a conversion:
> f7795e4096 ("misc: Replace zero-length arrays with flexible array member (automatic)")

OK

>>
>>>
>>>> +    SysIB_151x *sysib = (SysIB_151x *) page;
>>>> +    int len;
>>>> +
>>>> +    if (s390_is_pv() || !s390_has_topology() ||
>>>> +        sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
>>>> +        setcc(cpu, 3);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    len = setup_stsi(sysib, sel2);
>>>
>>> This should now be memory safe, but might be larger than 4k,
>>> the maximum size of the SYSIB. I guess you want to set cc code 3
>>> in this case and return.
>>
>> I do not find why the SYSIB can not be larger than 4k.
>> Can you point me to this restriction?
> 
> Says so at the top of the description of STSI:
> 
> The SYSIB is 4K bytes and must begin at a 4 K-byte
> boundary; otherwise, a specification exception may
> be recognized.

Right, I guess I can not read.

So I will return CC=3 in case the length is greater than 4K


thanks,
Regards,

Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

[Janis Schoetterl-Glausch]

On Fri, 2022-10-28 at 11:30 +0200, Pierre Morel wrote:
> 
> On 10/27/22 22:20, Janis Schoetterl-Glausch wrote:
> > On Wed, 2022-10-26 at 10:34 +0200, Pierre Morel wrote:
> > > 
> > > On 10/25/22 21:58, Janis Schoetterl-Glausch wrote:
> > > > On Wed, 2022-10-12 at 18:20 +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.
> > > > > s390x/cpu topology: core_id sets s390x CPU topology
> > > > > 
> > > > > In the S390x CPU topology the core_id specifies the CPU address
> > > > > and the position of the cpu withing the topology.
> > > > > 
> > > > > Let's build the topology based on the core_id.
> > > > > 
> > > > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > > > > ---
> > > > >    include/hw/s390x/cpu-topology.h |  45 +++++++++++
> > > > >    hw/s390x/cpu-topology.c         | 132 ++++++++++++++++++++++++++++++++
> > > > >    hw/s390x/s390-virtio-ccw.c      |  21 +++++
> > > > >    hw/s390x/meson.build            |   1 +
> > > > >    4 files changed, 199 insertions(+)
> > > > >    create mode 100644 include/hw/s390x/cpu-topology.h
> > > > >    create mode 100644 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);
> > > > > +
> > > > > +    topo->cpus = ms->smp.cores * ms->smp.threads;
> > > > 
> > > > Currently threads are not supported, effectively increasing the number of cpus,
> > > > so this is currently correct. Once the machine version limits the threads to 1,
> > > > it is also correct. However, once we support multiple threads, this becomes incorrect.
> > > > I wonder if it's ok from a backward compatibility point of view to modify the smp values
> > > > by doing cores *= threads, threads = 1 for old machines.
> > > 
> > > Right, this will become incorrect with thread support.
> > > What about having a dedicated function:
> > > 
> > > 	topo->cpus = s390_get_cpus(ms);
> > > 
> > > This function will use the S390CcwMachineClass->max_thread introduced
> > > later to report the correct number of CPUs.
> > 
> > I don't think max_threads is exactly what matters here, it's if
> > threads are supported or not or, if max_threads == 1 it doesn't matter.
> > The question is how best to do the check. You could check the machine version.
> > I wonder if you could add a feature bit for the multithreading facility that is
> > always false and use that.
> > 
> > I don't know if using a function makes a difference, that is if it is obvious on
> > introduction of multithreading support that the function needs to be updated.
> > (If it is implemented in a way that requires updating, if you check the machine
> > version it doesn't)
> > In any case, the name you suggested isn't very descriptive.
> 
> I think we care about this machine and olders.
> Olders do not support topology so this, Multithreading (MT) does not mater.
> This machine support topology, if I follow Cedric advise, the 
> "max_thread" will/may be introduce before the topology.
> 
> This in fact is not an implementation for MT or does not allow the 
> implementation of MT it is only a way to get rid of the false 
> information given to the user that we accept MT.
> 
> So I think that when we introduce MT we will take care of making things 
> right at this place as in other places of the code.
> 
> What about we keep the original:
> 
>      topo->cpus = ms->smp.cores * ms->smp.threads;

If topology is only supported for new machines and not the old machines
for which you set max_threads to a compatibility value (max cpus), then
you should just ignore the threads, cpus == cores.
(There might not be any point in keeping a topo->cpus member in this case, I haven't checked)
> 
> Which does not do any arm to machines without MT ?
> 
> Regards,
> Pierre
> 

Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

[Pierre Morel]

On 11/7/22 19:04, Janis Schoetterl-Glausch wrote:
> On Fri, 2022-10-28 at 11:30 +0200, Pierre Morel wrote:
>>
>> On 10/27/22 22:20, Janis Schoetterl-Glausch wrote:
>>> On Wed, 2022-10-26 at 10:34 +0200, Pierre Morel wrote:
>>>>
>>>> On 10/25/22 21:58, Janis Schoetterl-Glausch wrote:
>>>>> On Wed, 2022-10-12 at 18:20 +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.
>>>>>> s390x/cpu topology: core_id sets s390x CPU topology
>>>>>>
>>>>>> In the S390x CPU topology the core_id specifies the CPU address
>>>>>> and the position of the cpu withing the topology.
>>>>>>
>>>>>> Let's build the topology based on the core_id.
>>>>>>
>>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>>>> ---
>>>>>>     include/hw/s390x/cpu-topology.h |  45 +++++++++++
>>>>>>     hw/s390x/cpu-topology.c         | 132 ++++++++++++++++++++++++++++++++
>>>>>>     hw/s390x/s390-virtio-ccw.c      |  21 +++++
>>>>>>     hw/s390x/meson.build            |   1 +
>>>>>>     4 files changed, 199 insertions(+)
>>>>>>     create mode 100644 include/hw/s390x/cpu-topology.h
>>>>>>     create mode 100644 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);
>>>>>> +
>>>>>> +    topo->cpus = ms->smp.cores * ms->smp.threads;
>>>>>
>>>>> Currently threads are not supported, effectively increasing the number of cpus,
>>>>> so this is currently correct. Once the machine version limits the threads to 1,
>>>>> it is also correct. However, once we support multiple threads, this becomes incorrect.
>>>>> I wonder if it's ok from a backward compatibility point of view to modify the smp values
>>>>> by doing cores *= threads, threads = 1 for old machines.
>>>>
>>>> Right, this will become incorrect with thread support.
>>>> What about having a dedicated function:
>>>>
>>>> 	topo->cpus = s390_get_cpus(ms);
>>>>
>>>> This function will use the S390CcwMachineClass->max_thread introduced
>>>> later to report the correct number of CPUs.
>>>
>>> I don't think max_threads is exactly what matters here, it's if
>>> threads are supported or not or, if max_threads == 1 it doesn't matter.
>>> The question is how best to do the check. You could check the machine version.
>>> I wonder if you could add a feature bit for the multithreading facility that is
>>> always false and use that.
>>>
>>> I don't know if using a function makes a difference, that is if it is obvious on
>>> introduction of multithreading support that the function needs to be updated.
>>> (If it is implemented in a way that requires updating, if you check the machine
>>> version it doesn't)
>>> In any case, the name you suggested isn't very descriptive.
>>
>> I think we care about this machine and olders.
>> Olders do not support topology so this, Multithreading (MT) does not mater.
>> This machine support topology, if I follow Cedric advise, the
>> "max_thread" will/may be introduce before the topology.
>>
>> This in fact is not an implementation for MT or does not allow the
>> implementation of MT it is only a way to get rid of the false
>> information given to the user that we accept MT.
>>
>> So I think that when we introduce MT we will take care of making things
>> right at this place as in other places of the code.
>>
>> What about we keep the original:
>>
>>       topo->cpus = ms->smp.cores * ms->smp.threads;
> 
> If topology is only supported for new machines and not the old machines
> for which you set max_threads to a compatibility value (max cpus), then
> you should just ignore the threads, cpus == cores.
> (There might not be any point in keeping a topo->cpus member in this case, I haven't checked)

Right but, I need the nr_cpus in the topology so I prefer to keep it.

However, smp.threads has nothing to do there anymore as you pointed.
I think that nr_cpus should may be named nr_cores and should be set to 
smp.cores.

Thanks,
Regards,

Pierre


-- 
Pierre Morel
IBM Lab Boeblingen