[PATCH v12 0/7] s390x: CPU Topology

[PATCH v12 0/7] s390x: CPU Topology

[Pierre Morel]

Hi,

The implementation of the CPU Topology in QEMU has been modified
since the last patch series.

- The two preliminary patches have been accepted and are no longer
  part of this series.

- The topology machine property has been abandoned

- the topology_capable QEMU capability has been abandoned

- both where replaced with a new CPU feature, topology-disable
  to fence per default the ctop topology information feature.

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 (7):
  s390x/cpu topology: Creating CPU topology device
  s390x/cpu topology: reporting the CPU topology to the guest
  s390x/cpu_topology: resetting the Topology-Change-Report
  s390x/cpu_topology: CPU topology migration
  s390x/cpu_topology: interception of PTF instruction
  s390x/cpu_topology: activating CPU topology
  docs/s390x: document s390x cpu topology

 docs/system/s390x/cpu-topology.rst  |  80 +++++++++++
 include/hw/s390x/cpu-topology.h     |  41 ++++++
 include/hw/s390x/s390-virtio-ccw.h  |   7 +
 target/s390x/cpu.h                  |  79 +++++++++++
 target/s390x/kvm/kvm_s390x.h        |   1 +
 target/s390x/cpu_features_def.h.inc |   1 +
 hw/s390x/cpu-topology.c             | 200 ++++++++++++++++++++++++++++
 hw/s390x/s390-virtio-ccw.c          |  38 +++++-
 target/s390x/cpu-sysemu.c           |  21 +++
 target/s390x/cpu_models.c           |  17 +++
 target/s390x/cpu_topology.c         | 191 ++++++++++++++++++++++++++
 target/s390x/gen-features.c         |   3 +
 target/s390x/kvm/kvm.c              |  48 ++++++-
 hw/s390x/meson.build                |   1 +
 target/s390x/meson.build            |   1 +
 15 files changed, 721 insertions(+), 8 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

- since v11

- new CPU flag "disable-topology"
  I would have take "topology" if I was able to have
  it false on default.
  (Christian, Thomas)

- Build the topology during the interception of the
  STSI instruction.
  (Cedric)

- return CC3 in case the calculated SYSIB length is
  greater than 4096.
  (Janis)

- minor corections on documentation

- since v10

- change machine attribute "topology-disable" to "topology"
  (Cedric)
- Add preliminary patch for machine properties
  (Cedric)
- Use next machine as 7.2
  (Cedric / Connie)
- Remove unecessary mutex
  (Thomas)
- use ENOTSUP return value for kvm_s390_topology_set_mtcr()
  (Cedric)
- Add explanation on container and cpu TLEs
  (Thomas)
- use again cpu and socket count in topology structure
  (Cedric)
- Suppress the S390TopoTLE structure and integrate
  the TLE masks to the socket structure.
  (-)
- the STSI instruction now finds the topology from the machine
  (Cedric)

- 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 v12 1/7] s390x/cpu topology: Creating CPU topology device

[Pierre Morel]

We will need a Topology device to transfer the topology
during migration and to implement machine reset.

The device creation is fenced by s390_has_topology().

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 include/hw/s390x/cpu-topology.h    | 44 +++++++++++++++
 include/hw/s390x/s390-virtio-ccw.h |  1 +
 hw/s390x/cpu-topology.c            | 87 ++++++++++++++++++++++++++++++
 hw/s390x/s390-virtio-ccw.c         | 25 +++++++++
 hw/s390x/meson.build               |  1 +
 5 files changed, 158 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..e88059ccec
--- /dev/null
+++ b/include/hw/s390x/cpu-topology.h
@@ -0,0 +1,44 @@
+/*
+ * CPU Topology
+ *
+ * Copyright IBM Corp. 2022
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#ifndef HW_S390X_CPU_TOPOLOGY_H
+#define HW_S390X_CPU_TOPOLOGY_H
+
+#include "hw/qdev-core.h"
+#include "qom/object.h"
+
+#define S390_TOPOLOGY_CPU_IFL 0x03
+#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
+
+#define S390_TOPOLOGY_POLARITY_HORIZONTAL      0x00
+#define S390_TOPOLOGY_POLARITY_VERTICAL_LOW    0x01
+#define S390_TOPOLOGY_POLARITY_VERTICAL_MEDIUM 0x02
+#define S390_TOPOLOGY_POLARITY_VERTICAL_HIGH   0x03
+
+typedef struct S390TopoSocket {
+    int active_count;
+    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
+} S390TopoSocket;
+
+struct S390Topology {
+    SysBusDevice parent_obj;
+    uint32_t num_cores;
+    uint32_t num_sockets;
+    S390TopoSocket *socket;
+};
+
+#define TYPE_S390_CPU_TOPOLOGY "s390-topology"
+OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY)
+
+static inline bool s390_has_topology(void)
+{
+    return false;
+}
+
+#endif
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 9bba21a916..47ce0aa6fa 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;
     uint8_t loadparm[8];
+    DeviceState *topology;
 };
 
 struct S390CcwMachineClass {
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
new file mode 100644
index 0000000000..bbf97cd66a
--- /dev/null
+++ b/hw/s390x/cpu-topology.c
@@ -0,0 +1,87 @@
+/*
+ * 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"
+
+/**
+ * s390_topology_realize:
+ * @dev: the device state
+ *
+ * We free the socket array allocated in realize.
+ */
+static void s390_topology_unrealize(DeviceState *dev)
+{
+    S390Topology *topo = S390_CPU_TOPOLOGY(dev);
+
+    g_free(topo->socket);
+}
+
+/**
+ * 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)
+{
+    S390Topology *topo = S390_CPU_TOPOLOGY(dev);
+
+    topo->socket = g_new0(S390TopoSocket, topo->num_sockets);
+}
+
+static Property s390_topology_properties[] = {
+    DEFINE_PROP_UINT32("num-cores", S390Topology, num_cores, 1),
+    DEFINE_PROP_UINT32("num-sockets", S390Topology, num_sockets, 1),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+/**
+ * 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;
+    dc->unrealize = s390_topology_unrealize;
+    device_class_set_props(dc, s390_topology_properties);
+    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 2e64ffab45..973bbdd36e 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -44,6 +44,7 @@
 #include "hw/s390x/pv.h"
 #include "migration/blocker.h"
 #include "qapi/visitor.h"
+#include "hw/s390x/cpu-topology.h"
 
 static Error *pv_mig_blocker;
 
@@ -102,6 +103,24 @@ static void s390_init_cpus(MachineState *machine)
     }
 }
 
+static DeviceState *s390_init_topology(MachineState *machine, Error **errp)
+{
+    DeviceState *dev;
+
+    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
+
+    object_property_add_child(&machine->parent_obj,
+                              TYPE_S390_CPU_TOPOLOGY, OBJECT(dev));
+    object_property_set_int(OBJECT(dev), "num-cores",
+                            machine->smp.cores * machine->smp.threads, errp);
+    object_property_set_int(OBJECT(dev), "num-sockets",
+                            machine->smp.sockets, errp);
+
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);
+
+    return dev;
+}
+
 static const char *const reset_dev_types[] = {
     TYPE_VIRTUAL_CSS_BRIDGE,
     "s390-sclp-event-facility",
@@ -255,6 +274,12 @@ static void ccw_init(MachineState *machine)
     /* init CPUs (incl. CPU model) early so s390_has_feature() works */
     s390_init_cpus(machine);
 
+    /* Need CPU model to be determined before we can set up topology */
+    if (s390_has_topology()) {
+        S390_CCW_MACHINE(machine)->topology = s390_init_topology(machine,
+                                                                 &error_fatal);
+    }
+
     /* Need CPU model to be determined before we can set up PV */
     s390_pv_init(machine->cgs, &error_fatal);
 
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 v12 2/7] s390x/cpu topology: reporting the CPU topology to the guest

[Pierre Morel]

The guest uses the STSI instruction to get information on the
CPU topology.

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

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 target/s390x/cpu.h          |  77 +++++++++++++++
 hw/s390x/s390-virtio-ccw.c  |  12 +--
 target/s390x/cpu_topology.c | 186 ++++++++++++++++++++++++++++++++++++
 target/s390x/kvm/kvm.c      |   6 +-
 target/s390x/meson.build    |   1 +
 5 files changed, 274 insertions(+), 8 deletions(-)
 create mode 100644 target/s390x/cpu_topology.c

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7d6d01325b..dd878ac916 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -175,6 +175,7 @@ struct ArchCPU {
     /* needed for live migration */
     void *irqstate;
     uint32_t irqstate_saved_size;
+    void *machine_data;
 };
 
 
@@ -565,6 +566,80 @@ typedef union SysIB {
 } SysIB;
 QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
 
+/*
+ * CPU Topology List provided by STSI with fc=15 provides a list
+ * of two different Topology List Entries (TLE) types to specify
+ * the topology hierarchy.
+ *
+ * - Container Topology List Entry
+ *   Defines a container to contain other Topology List Entries
+ *   of any type, nested containers or CPU.
+ * - CPU Topology List Entry
+ *   Specifies the CPUs position, type, entitlement and polarization
+ *   of the CPUs contained in the last Container TLE.
+ *
+ * There can be theoretically up to five levels of containers, QEMU
+ * uses only one level, the socket level.
+ *
+ * A container of with a nesting level (NL) greater than 1 can only
+ * contain another container of nesting level NL-1.
+ *
+ * A container of nesting level 1 (socket), contains as many CPU TLE
+ * as needed to describe the position and qualities of all CPUs inside
+ * the container.
+ * The qualities of a CPU are polarization, entitlement and type.
+ *
+ * The CPU TLE defines the position of the CPUs of identical qualities
+ * using a 64bits mask which first bit has its offset defined by
+ * the CPU address orgin field of the CPU TLE like in:
+ * CPU address = origin * 64 + bit position within the mask
+ *
+ */
+/* 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);
+
+/* 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);
+
+#define S390_TOPOLOGY_MAG  6
+#define S390_TOPOLOGY_MAG6 0
+#define S390_TOPOLOGY_MAG5 1
+#define S390_TOPOLOGY_MAG4 2
+#define S390_TOPOLOGY_MAG3 3
+#define S390_TOPOLOGY_MAG2 4
+#define S390_TOPOLOGY_MAG1 5
+/* Configuration topology */
+typedef struct SysIB_151x {
+    uint8_t  reserved0[2];
+    uint16_t length;
+    uint8_t  mag[S390_TOPOLOGY_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);
+
+/* Max 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 +918,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/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 973bbdd36e..4be07959fd 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -64,11 +64,10 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
     return S390_CPU(ms->possible_cpus->cpus[cpu_addr].cpu);
 }
 
-static S390CPU *s390x_new_cpu(const char *typename, uint32_t core_id,
-                              Error **errp)
+static void s390x_new_cpu(MachineState *ms, uint32_t core_id, Error **errp)
 {
-    S390CPU *cpu = S390_CPU(object_new(typename));
-    S390CPU *ret = NULL;
+    S390CcwMachineState *s390ms = S390_CCW_MACHINE(ms);
+    S390CPU *cpu = S390_CPU(object_new(ms->cpu_type));
 
     if (!object_property_set_int(OBJECT(cpu), "core-id", core_id, errp)) {
         goto out;
@@ -76,11 +75,10 @@ static S390CPU *s390x_new_cpu(const char *typename, uint32_t core_id,
     if (!qdev_realize(DEVICE(cpu), NULL, errp)) {
         goto out;
     }
-    ret = cpu;
+    cpu->machine_data = s390ms;
 
 out:
     object_unref(OBJECT(cpu));
-    return ret;
 }
 
 static void s390_init_cpus(MachineState *machine)
@@ -99,7 +97,7 @@ static void s390_init_cpus(MachineState *machine)
     mc->possible_cpu_arch_ids(machine);
 
     for (i = 0; i < machine->smp.cpus; i++) {
-        s390x_new_cpu(machine->cpu_type, i, &error_fatal);
+        s390x_new_cpu(machine, i, &error_fatal);
     }
 }
 
diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
new file mode 100644
index 0000000000..b81f016ba1
--- /dev/null
+++ b/target/s390x/cpu_topology.c
@@ -0,0 +1,186 @@
+/*
+ * 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"
+
+/*
+ * s390_topology_add_cpu:
+ * @topo: pointer to the topology
+ * @cpu : pointer to the new CPU
+ *
+ * The topology pointed by S390CPU, gives us the CPU topology
+ * established by the -smp QEMU aruments.
+ * The core-id is used to calculate the position of the CPU inside
+ * the topology:
+ *  - the socket, container TLE, containing the CPU, we have one socket
+ *    for every num_cores cores.
+ *  - the CPU TLE inside the socket, we have potentionly up to 4 CPU TLE
+ *    in a container TLE with the assumption that all CPU are identical
+ *    with the same polarity and entitlement because we have maximum 256
+ *    CPUs and each TLE can hold up to 64 identical CPUs.
+ *  - the bit in the 64 bit CPU TLE core mask
+ */
+static void s390_topology_add_cpu(S390Topology *topo, S390CPU *cpu)
+{
+    int core_id = cpu->env.core_id;
+    int bit, origin;
+    int socket_id;
+
+    cpu->machine_data = topo;
+    socket_id = core_id / topo->num_cores;
+    /*
+     * At the core level, each CPU is represented by a bit in a 64bit
+     * uint64_t 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->socket[socket_id].mask[origin]);
+}
+
+/*
+ * s390_prepare_topology:
+ * @s390ms : pointer to the S390CcwMachite State
+ *
+ * Calls s390_topology_add_cpu to organize the topology
+ * inside the topology device before writing the SYSIB.
+ *
+ * The topology is currently fixed on boot and do not change
+ * even on migration.
+ */
+static void s390_prepare_topology(S390CcwMachineState *s390ms)
+{
+    const MachineState *ms = MACHINE(s390ms);
+    static bool done;
+    int i;
+
+    if (done) {
+        return;
+    }
+
+    for (i = 0; i < ms->possible_cpus->len; i++) {
+        if (ms->possible_cpus->cpus[i].cpu) {
+            s390_topology_add_cpu(S390_CPU_TOPOLOGY(s390ms->topology),
+                                  S390_CPU(ms->possible_cpus->cpus[i].cpu));
+        }
+    }
+
+    done = true;
+}
+
+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_HORIZONTAL;
+    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)
+{
+    int i, origin;
+
+    for (i = 0; i < topo->num_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->socket[i].mask[origin];
+            if (mask) {
+                p = fill_tle_cpu(p, mask, origin);
+            }
+        }
+    }
+    return p;
+}
+
+static int setup_stsi(S390CPU *cpu, SysIB_151x *sysib, int level)
+{
+    S390Topology *topo = (S390Topology *)cpu->machine_data;
+    char *p = sysib->tle;
+
+    sysib->mnest = level;
+    switch (level) {
+    case 2:
+        sysib->mag[S390_TOPOLOGY_MAG2] = topo->num_sockets;
+        sysib->mag[S390_TOPOLOGY_MAG1] = topo->num_cores;
+        p = s390_top_set_level2(topo, p);
+        break;
+    }
+
+    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)
+{
+    union {
+        char place_holder[S390_TOPOLOGY_SYSIB_SIZE];
+        SysIB_151x sysib;
+    } buffer QEMU_ALIGNED(8);
+    int len;
+
+    if (s390_is_pv() || !s390_has_topology() ||
+        sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
+        setcc(cpu, 3);
+        return;
+    }
+
+    s390_prepare_topology(S390_CCW_MACHINE(cpu->machine_data));
+
+    len = setup_stsi(cpu, &buffer.sysib, sel2);
+
+    if (len > 4096) {
+        setcc(cpu, 3);
+        return;
+    }
+
+    buffer.sysib.length = cpu_to_be16(len);
+    s390_cpu_virt_mem_write(cpu, addr, ar, &buffer.sysib, len);
+    setcc(cpu, 0);
+}
+
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 3ac7ec9acf..7dc96f3663 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
@@ -1919,9 +1920,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 v12 3/7] 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    | 13 +++++++++++++
 target/s390x/kvm/kvm.c       | 17 +++++++++++++++++
 6 files changed, 45 insertions(+)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index dd878ac916..2bd3d417e4 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -854,6 +854,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 f9785564d0..649dae5948 100644
--- a/target/s390x/kvm/kvm_s390x.h
+++ b/target/s390x/kvm/kvm_s390x.h
@@ -47,5 +47,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 bbf97cd66a..b1082a4b88 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -56,6 +56,17 @@ static Property s390_topology_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+/**
+ * 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
@@ -71,6 +82,7 @@ static void topology_class_init(ObjectClass *oc, void *data)
     dc->unrealize = s390_topology_unrealize;
     device_class_set_props(dc, s390_topology_properties);
     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 4be07959fd..f29e383d23 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -125,6 +125,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..e27864c5f5 100644
--- a/target/s390x/cpu-sysemu.c
+++ b/target/s390x/cpu-sysemu.c
@@ -306,3 +306,16 @@ 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)
+{
+    int ret;
+
+    if (kvm_enabled()) {
+        ret = kvm_s390_topology_set_mtcr(0);
+        if (ret) {
+            error_report("Failed to set Modified Topology Change Report: %s",
+                         strerror(-ret));
+        }
+    }
+}
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 7dc96f3663..5b6383eab0 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -2593,6 +2593,23 @@ 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,
+    };
+
+    if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
+        return 0;
+    }
+    if (!kvm_vm_check_attr(kvm_state, KVM_S390_VM_CPU_TOPOLOGY, attr)) {
+        return -ENOTSUP;
+    }
+
+    return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attribute);
+}
+
 void kvm_arch_accel_class_init(ObjectClass *oc)
 {
 }
-- 
2.31.1

[PATCH v12 4/7] 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>
---
 target/s390x/cpu.h        |  1 +
 hw/s390x/cpu-topology.c   | 49 +++++++++++++++++++++++++++++++++++++++
 target/s390x/cpu-sysemu.c |  8 +++++++
 3 files changed, 58 insertions(+)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 2bd3d417e4..d8c00de980 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -855,6 +855,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 b1082a4b88..32908d13bb 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"
 
 /**
  * s390_topology_realize:
@@ -67,6 +68,53 @@ 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)
+{
+    int ret;
+
+    /* 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_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 s390_has_topology();
+}
+
+const VMStateDescription vmstate_cpu_topology = {
+    .name = "cpu_topology",
+    .version_id = 1,
+    .post_load = cpu_topology_postload,
+    .minimum_version_id = 1,
+    .needed = cpu_topology_needed,
+};
+
 /**
  * topology_class_init:
  * @oc: Object class
@@ -83,6 +131,7 @@ static void topology_class_init(ObjectClass *oc, void *data)
     device_class_set_props(dc, s390_topology_properties);
     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 e27864c5f5..a8e3e6219d 100644
--- a/target/s390x/cpu-sysemu.c
+++ b/target/s390x/cpu-sysemu.c
@@ -319,3 +319,11 @@ void s390_cpu_topology_reset(void)
         }
     }
 }
+
+int s390_cpu_topology_mtcr_set(void)
+{
+    if (kvm_enabled()) {
+        return kvm_s390_topology_set_mtcr(1);
+    }
+    return -ENOENT;
+}
-- 
2.31.1

[PATCH v12 5/7] s390x/cpu_topology: 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 47ce0aa6fa..d78ea8843c 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -31,6 +31,12 @@ struct S390CcwMachineState {
     DeviceState *topology;
 };
 
+#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 32908d13bb..12fcd041a3 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);
+    }
+}
 
 /**
  * s390_topology_realize:
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 5b6383eab0..a79fdf1c79 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
@@ -1465,6 +1466,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;
@@ -1482,6 +1490,9 @@ static int handle_b9(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
     case PRIV_B9_RPCIT:
         r = kvm_rpcit_service_call(cpu, run);
         break;
+    case PRIV_B9_PTF:
+        kvm_handle_ptf(cpu, run);
+        break;
     case PRIV_B9_EQBS:
         /* just inject exception */
         r = -1;
-- 
2.31.1

[PATCH v12 6/7] 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.

The feature is fenced for SE (secure execution).

To allow smooth migration with old QEMU the feature is disabled by
default using the CPU flag -disable-topology.

Making the S390_FEAT_CONFIGURATION_TOPOLOGY belonging to the
default features makes the -ctop CPU flag is no more necessary,
turning the topology feature on is done with -disable-topology
only.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 include/hw/s390x/cpu-topology.h     |  5 +----
 target/s390x/cpu_features_def.h.inc |  1 +
 target/s390x/cpu_models.c           | 17 +++++++++++++++++
 target/s390x/cpu_topology.c         |  5 +++++
 target/s390x/gen-features.c         |  3 +++
 target/s390x/kvm/kvm.c              | 14 ++++++++++++++
 6 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
index e88059ccec..b2fa24ba93 100644
--- a/include/hw/s390x/cpu-topology.h
+++ b/include/hw/s390x/cpu-topology.h
@@ -36,9 +36,6 @@ struct S390Topology {
 #define TYPE_S390_CPU_TOPOLOGY "s390-topology"
 OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY)
 
-static inline bool s390_has_topology(void)
-{
-    return false;
-}
+bool s390_has_topology(void);
 
 #endif
diff --git a/target/s390x/cpu_features_def.h.inc b/target/s390x/cpu_features_def.h.inc
index e3cfe63735..016a720e38 100644
--- a/target/s390x/cpu_features_def.h.inc
+++ b/target/s390x/cpu_features_def.h.inc
@@ -147,6 +147,7 @@ DEF_FEAT(SIE_CEI, "cei", SCLP_CPU, 43, "SIE: Conditional-external-interception f
 DEF_FEAT(DAT_ENH_2, "dateh2", MISC, 0, "DAT-enhancement facility 2")
 DEF_FEAT(CMM, "cmm", MISC, 0, "Collaborative-memory-management facility")
 DEF_FEAT(AP, "ap", MISC, 0, "AP instructions installed")
+DEF_FEAT(DISABLE_CPU_TOPOLOGY, "disable-topology", MISC, 0, "Disable CPU Topology")
 
 /* Features exposed via the PLO instruction. */
 DEF_FEAT(PLO_CL, "plo-cl", PLO, 0, "PLO Compare and load (32 bit in general registers)")
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index c3a4f80633..1f5348d6a3 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -253,6 +253,7 @@ bool s390_has_feat(S390Feat feat)
         case S390_FEAT_SIE_CMMA:
         case S390_FEAT_SIE_PFMFI:
         case S390_FEAT_SIE_IBS:
+        case S390_FEAT_CONFIGURATION_TOPOLOGY:
             return false;
             break;
         default:
@@ -422,6 +423,21 @@ void s390_cpu_list(void)
     }
 }
 
+static void check_incompatibility(S390CPUModel *model, Error **errp)
+{
+    static int dep[][2] = {
+        { S390_FEAT_CONFIGURATION_TOPOLOGY, S390_FEAT_DISABLE_CPU_TOPOLOGY },
+    };
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(dep); i++) {
+        if (test_bit(dep[i][0], model->features) &&
+            test_bit(dep[i][1], model->features)) {
+            clear_bit(dep[i][0], model->features);
+        }
+    }
+}
+
 static void check_consistency(const S390CPUModel *model)
 {
     static int dep[][2] = {
@@ -592,6 +608,7 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
     cpu->model->cpu_id_format = max_model->cpu_id_format;
     cpu->model->cpu_ver = max_model->cpu_ver;
 
+    check_incompatibility(cpu->model, &err);
     check_consistency(cpu->model);
     check_compatibility(max_model, cpu->model, &err);
     if (err) {
diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
index b81f016ba1..8123e6ddf0 100644
--- a/target/s390x/cpu_topology.c
+++ b/target/s390x/cpu_topology.c
@@ -15,6 +15,11 @@
 #include "hw/s390x/cpu-topology.h"
 #include "hw/s390x/sclp.h"
 
+bool s390_has_topology(void)
+{
+    return s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY);
+}
+
 /*
  * s390_topology_add_cpu:
  * @topo: pointer to the topology
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 1e3b7c0dc9..f3acfdd9a5 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -488,6 +488,7 @@ static uint16_t full_GEN9_GA3[] = {
 static uint16_t full_GEN10_GA1[] = {
     S390_FEAT_EDAT,
     S390_FEAT_CONFIGURATION_TOPOLOGY,
+    S390_FEAT_DISABLE_CPU_TOPOLOGY,
     S390_FEAT_GROUP_MSA_EXT_2,
     S390_FEAT_ESOP,
     S390_FEAT_SIE_PFMFI,
@@ -605,6 +606,8 @@ static uint16_t default_GEN9_GA1[] = {
 static uint16_t default_GEN10_GA1[] = {
     S390_FEAT_EDAT,
     S390_FEAT_GROUP_MSA_EXT_2,
+    S390_FEAT_DISABLE_CPU_TOPOLOGY,
+    S390_FEAT_CONFIGURATION_TOPOLOGY,
 };
 
 #define default_GEN10_GA2 EmptyFeat
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index a79fdf1c79..ec2c9fd8fa 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -2471,6 +2471,20 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
         set_bit(S390_FEAT_UNPACK, model->features);
     }
 
+    /*
+     * If we have support for CPU Topology prevent overrule
+     * S390_FEAT_CONFIGURATION_TOPOLOGY with S390_FEAT_DISABLE_CPU_TOPOLOGY
+     * implemented in KVM, activate the CPU TOPOLOGY feature.
+     */
+    if (kvm_check_extension(kvm_state, KVM_CAP_S390_CPU_TOPOLOGY)) {
+        if (kvm_vm_enable_cap(kvm_state, KVM_CAP_S390_CPU_TOPOLOGY, 0) < 0) {
+            error_setg(errp, "KVM: Error enabling KVM_CAP_S390_CPU_TOPOLOGY");
+            return;
+        }
+        set_bit(S390_FEAT_CONFIGURATION_TOPOLOGY, model->features);
+        set_bit(S390_FEAT_DISABLE_CPU_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 v12 7/7] 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..2fad28453c
--- /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)
+
+Enabling CPU topology
+---------------------
+
+Currently, CPU topology is disabled by default.
+
+Enabling CPU topology can be done by setting the feature flag
+``disable-topology`` to ``off`` like in:
+
+.. code-block:: sh
+   -cpu gen16b,disable-topology=off
+
+Having the topology disabled by default allows migration between
+old and new QEMU without adding new flags.
-- 
2.31.1

Re: [PATCH v12 0/7] s390x: CPU Topology

[Cédric Le Goater]

Hello Pierre

On 11/29/22 18:41, Pierre Morel wrote:
> Hi,
> 
> The implementation of the CPU Topology in QEMU has been modified
> since the last patch series.
> 
> - The two preliminary patches have been accepted and are no longer
>    part of this series.
> 
> - The topology machine property has been abandoned
> 
> - the topology_capable QEMU capability has been abandoned
> 
> - both where replaced with a new CPU feature, topology-disable
>    to fence per default the ctop topology information feature.
> 
> 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.
Please make sure the patchset compiles on non-s390x platforms and check
that the documentation generates correctly. You will need to install :

   python3-sphinx python3-sphinx_rtd_theme

'configure' should then enable doc generation.

Thanks,

C.

Re: [PATCH v12 1/7] s390x/cpu topology: Creating CPU topology device

[Thomas Huth]

On 29/11/2022 18.42, Pierre Morel wrote:
> We will need a Topology device to transfer the topology
> during migration and to implement machine reset.
> 
> The device creation is fenced by s390_has_topology().
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
...
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 2e64ffab45..973bbdd36e 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -44,6 +44,7 @@
>   #include "hw/s390x/pv.h"
>   #include "migration/blocker.h"
>   #include "qapi/visitor.h"
> +#include "hw/s390x/cpu-topology.h"
>   
>   static Error *pv_mig_blocker;
>   
> @@ -102,6 +103,24 @@ static void s390_init_cpus(MachineState *machine)
>       }
>   }
>   
> +static DeviceState *s390_init_topology(MachineState *machine, Error **errp)
> +{
> +    DeviceState *dev;
> +
> +    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
> +
> +    object_property_add_child(&machine->parent_obj,
> +                              TYPE_S390_CPU_TOPOLOGY, OBJECT(dev));
> +    object_property_set_int(OBJECT(dev), "num-cores",
> +                            machine->smp.cores * machine->smp.threads, errp);

I wonder what will happen if we ever support multithreading on s390x later? 
... won't this cause some oddities when migrating older machines types with 
smp.threads > 1 later? Maybe we should prohibit to enable the CPU topology 
instead if a user tried to use threads > 1 with an older machine type?

  Thomas


> +    object_property_set_int(OBJECT(dev), "num-sockets",
> +                            machine->smp.sockets, errp);
> +
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);
> +
> +    return dev;
> +}

Re: [PATCH v12 1/7] s390x/cpu topology: Creating CPU topology device

[Pierre Morel]

On 12/1/22 10:08, Thomas Huth wrote:
> On 29/11/2022 18.42, Pierre Morel wrote:
>> We will need a Topology device to transfer the topology
>> during migration and to implement machine reset.
>>
>> The device creation is fenced by s390_has_topology().
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
> ...
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 2e64ffab45..973bbdd36e 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -44,6 +44,7 @@
>>   #include "hw/s390x/pv.h"
>>   #include "migration/blocker.h"
>>   #include "qapi/visitor.h"
>> +#include "hw/s390x/cpu-topology.h"
>>   static Error *pv_mig_blocker;
>> @@ -102,6 +103,24 @@ static void s390_init_cpus(MachineState *machine)
>>       }
>>   }
>> +static DeviceState *s390_init_topology(MachineState *machine, Error 
>> **errp)
>> +{
>> +    DeviceState *dev;
>> +
>> +    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
>> +
>> +    object_property_add_child(&machine->parent_obj,
>> +                              TYPE_S390_CPU_TOPOLOGY, OBJECT(dev));
>> +    object_property_set_int(OBJECT(dev), "num-cores",
>> +                            machine->smp.cores * 
>> machine->smp.threads, errp);
> 
> I wonder what will happen if we ever support multithreading on s390x 
> later? ... won't this cause some oddities when migrating older machines 
> types with smp.threads > 1 later? Maybe we should prohibit to enable the 
> CPU topology instead if a user tried to use threads > 1 with an older 
> machine type?

Yes, right, I forgot to change this back.
Anyway it has no sens for new machine which prohibit smp.threads > 1 
neither.

I change this by returning an error in case we have smp.threads > 1

Thanks.

Pierre

> 
>   Thomas
> 
> 
>> +    object_property_set_int(OBJECT(dev), "num-sockets",
>> +                            machine->smp.sockets, errp);
>> +
>> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);
>> +
>> +    return dev;
>> +}
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v12 6/7] s390x/cpu_topology: activating CPU topology

[Thomas Huth]

On 29/11/2022 18.42, Pierre Morel wrote:
> 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.
> 
> The feature is fenced for SE (secure execution).

Out of curiosity: Why does it not work yet?

> To allow smooth migration with old QEMU the feature is disabled by
> default using the CPU flag -disable-topology.

I stared at this code for a while now, but I have to admit that I don't 
quite get it. Why do we need a new "disable" feature flag here? I think it 
is pretty much impossible to set "ctop=on" with an older version of QEMU, 
since it would require the QEMU to enable KVM_CAP_S390_CPU_TOPOLOGY in the 
kernel for this feature bit - and older versions of QEMU don't set this 
capability yet.

Which scenario would fail without this disable-topology feature bit? What do 
I miss?

> Making the S390_FEAT_CONFIGURATION_TOPOLOGY belonging to the
> default features makes the -ctop CPU flag is no more necessary,

too many verbs in this sentence ;-)

> turning the topology feature on is done with -disable-topology
> only.
...

  Thomas

Re: [PATCH v12 6/7] s390x/cpu_topology: activating CPU topology

[Pierre Morel]

On 12/1/22 11:15, Thomas Huth wrote:
> On 29/11/2022 18.42, Pierre Morel wrote:
>> 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.
>>
>> The feature is fenced for SE (secure execution).
> 
> Out of curiosity: Why does it not work yet?
> 
>> To allow smooth migration with old QEMU the feature is disabled by
>> default using the CPU flag -disable-topology.
> 
> I stared at this code for a while now, but I have to admit that I don't 
> quite get it. Why do we need a new "disable" feature flag here? I think 
> it is pretty much impossible to set "ctop=on" with an older version of 
> QEMU, since it would require the QEMU to enable 
> KVM_CAP_S390_CPU_TOPOLOGY in the kernel for this feature bit - and older 
> versions of QEMU don't set this capability yet.
> 
> Which scenario would fail without this disable-topology feature bit? 
> What do I miss?

The only scenario it provides is that ctop is then disabled by default 
on newer QEMU allowing migration between old and new QEMU for older 
machine without changing the CPU flags.

Otherwise, we would need -ctop=off on newer QEMU to disable the topology.


> 
>> Making the S390_FEAT_CONFIGURATION_TOPOLOGY belonging to the
>> default features makes the -ctop CPU flag is no more necessary,
> 
> too many verbs in this sentence ;-)

definitively :)

> 
>> turning the topology feature on is done with -disable-topology
>> only.
> ...
> 
>   Thomas
> 
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v12 0/7] s390x: CPU Topology

[Pierre Morel]

On 12/1/22 09:45, Cédric Le Goater wrote:
> Hello Pierre
> 
> On 11/29/22 18:41, Pierre Morel wrote:
>> Hi,
>>
>> The implementation of the CPU Topology in QEMU has been modified
>> since the last patch series.
>>
>> - The two preliminary patches have been accepted and are no longer
>>    part of this series.
>>
>> - The topology machine property has been abandoned
>>
>> - the topology_capable QEMU capability has been abandoned
>>
>> - both where replaced with a new CPU feature, topology-disable
>>    to fence per default the ctop topology information feature.
>>
>> 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.

Hello Cedric,

> Please make sure the patchset compiles on non-s390x platforms and check

Yes,

> that the documentation generates correctly. You will need to install :
> 
>    python3-sphinx python3-sphinx_rtd_theme
> 
> 'configure' should then enable doc generation.

Yes, thanks,

Pierre

> 
> Thanks,
> 
> C.
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v12 6/7] s390x/cpu_topology: activating CPU topology

[Thomas Huth]

On 01/12/2022 12.52, Pierre Morel wrote:
> 
> 
> On 12/1/22 11:15, Thomas Huth wrote:
>> On 29/11/2022 18.42, Pierre Morel wrote:
>>> 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.
>>>
>>> The feature is fenced for SE (secure execution).
>>
>> Out of curiosity: Why does it not work yet?
>>
>>> To allow smooth migration with old QEMU the feature is disabled by
>>> default using the CPU flag -disable-topology.
>>
>> I stared at this code for a while now, but I have to admit that I don't 
>> quite get it. Why do we need a new "disable" feature flag here? I think it 
>> is pretty much impossible to set "ctop=on" with an older version of QEMU, 
>> since it would require the QEMU to enable KVM_CAP_S390_CPU_TOPOLOGY in the 
>> kernel for this feature bit - and older versions of QEMU don't set this 
>> capability yet.
>>
>> Which scenario would fail without this disable-topology feature bit? What 
>> do I miss?
> 
> The only scenario it provides is that ctop is then disabled by default on 
> newer QEMU allowing migration between old and new QEMU for older machine 
> without changing the CPU flags.
> 
> Otherwise, we would need -ctop=off on newer QEMU to disable the topology.

Ah, it's because you added S390_FEAT_CONFIGURATION_TOPOLOGY to the default 
feature set here:

  static uint16_t default_GEN10_GA1[] = {
      S390_FEAT_EDAT,
      S390_FEAT_GROUP_MSA_EXT_2,
+    S390_FEAT_DISABLE_CPU_TOPOLOGY,
+    S390_FEAT_CONFIGURATION_TOPOLOGY,
  };

?

But what sense does it make to enable it by default, just to disable it by 
default again with the S390_FEAT_DISABLE_CPU_TOPOLOGY feature? ... sorry, I 
still don't quite get it, but maybe it's because my sinuses are quite 
clogged due to a bad cold ... so if you could elaborate again, that would be 
very appreciated!

However, looking at this from a distance, I would not rather not add this to 
any default older CPU model at all (since it also depends on the kernel to 
have this feature enabled)? Enabling it in the host model is still ok, since 
the host model is not migration safe anyway.

  Thomas

Re: [PATCH v12 6/7] s390x/cpu_topology: activating CPU topology

[Pierre Morel]

On 12/2/22 10:05, Thomas Huth wrote:
> On 01/12/2022 12.52, Pierre Morel wrote:
>>
>>
>> On 12/1/22 11:15, Thomas Huth wrote:
>>> On 29/11/2022 18.42, Pierre Morel wrote:
>>>> 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.
>>>>
>>>> The feature is fenced for SE (secure execution).
>>>
>>> Out of curiosity: Why does it not work yet?
>>>
>>>> To allow smooth migration with old QEMU the feature is disabled by
>>>> default using the CPU flag -disable-topology.
>>>
>>> I stared at this code for a while now, but I have to admit that I 
>>> don't quite get it. Why do we need a new "disable" feature flag here? 
>>> I think it is pretty much impossible to set "ctop=on" with an older 
>>> version of QEMU, since it would require the QEMU to enable 
>>> KVM_CAP_S390_CPU_TOPOLOGY in the kernel for this feature bit - and 
>>> older versions of QEMU don't set this capability yet.
>>>
>>> Which scenario would fail without this disable-topology feature bit? 
>>> What do I miss?
>>
>> The only scenario it provides is that ctop is then disabled by default 
>> on newer QEMU allowing migration between old and new QEMU for older 
>> machine without changing the CPU flags.
>>
>> Otherwise, we would need -ctop=off on newer QEMU to disable the topology.
> 
> Ah, it's because you added S390_FEAT_CONFIGURATION_TOPOLOGY to the 
> default feature set here:
> 
>   static uint16_t default_GEN10_GA1[] = {
>       S390_FEAT_EDAT,
>       S390_FEAT_GROUP_MSA_EXT_2,
> +    S390_FEAT_DISABLE_CPU_TOPOLOGY,
> +    S390_FEAT_CONFIGURATION_TOPOLOGY,
>   };
> 
> ?
> 
> But what sense does it make to enable it by default, just to disable it 
> by default again with the S390_FEAT_DISABLE_CPU_TOPOLOGY feature? ... 
> sorry, I still don't quite get it, but maybe it's because my sinuses are 
> quite clogged due to a bad cold ... so if you could elaborate again, 
> that would be very appreciated!
> 
> However, looking at this from a distance, I would not rather not add 
> this to any default older CPU model at all (since it also depends on the 
> kernel to have this feature enabled)? Enabling it in the host model is 
> still ok, since the host model is not migration safe anyway.
> 
>   Thomas
> 

I think I did not understand what is exactly the request that was made 
about having a CPU flag to disable the topology when we decide to not 
have a new machine with new machine property.

Let see what we have if the only change to mainline is to activate 
S390_FEAT_CONFIGURATION_TOPOLOGY with the KVM capability:

In mainline, ctop is enabled in the full GEN10 only.

Consequently we have this feature activated by default for the host 
model only and deactivated by default if we specify the CPU.
It can be activated if we specify the CPU with the flag ctop=on.

This is what was in the patch series before the beginning of the 
discussion about having a new machine property for new machines.

If this what we want: activating the topology by the CPU flag ctop=on it 
is perfect for me and I can take the original patch.
We may later make it a default for new machines.

Otherwise or if I misunderstood something, which is greatly possible, I 
need more advises.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v12 6/7] s390x/cpu_topology: activating CPU topology

[Thomas Huth]

On 02/12/2022 15.08, Pierre Morel wrote:
> 
> 
> On 12/2/22 10:05, Thomas Huth wrote:
>> On 01/12/2022 12.52, Pierre Morel wrote:
>>>
>>>
>>> On 12/1/22 11:15, Thomas Huth wrote:
>>>> On 29/11/2022 18.42, Pierre Morel wrote:
>>>>> 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.
>>>>>
>>>>> The feature is fenced for SE (secure execution).
>>>>
>>>> Out of curiosity: Why does it not work yet?
>>>>
>>>>> To allow smooth migration with old QEMU the feature is disabled by
>>>>> default using the CPU flag -disable-topology.
>>>>
>>>> I stared at this code for a while now, but I have to admit that I don't 
>>>> quite get it. Why do we need a new "disable" feature flag here? I think 
>>>> it is pretty much impossible to set "ctop=on" with an older version of 
>>>> QEMU, since it would require the QEMU to enable 
>>>> KVM_CAP_S390_CPU_TOPOLOGY in the kernel for this feature bit - and older 
>>>> versions of QEMU don't set this capability yet.
>>>>
>>>> Which scenario would fail without this disable-topology feature bit? 
>>>> What do I miss?
>>>
>>> The only scenario it provides is that ctop is then disabled by default on 
>>> newer QEMU allowing migration between old and new QEMU for older machine 
>>> without changing the CPU flags.
>>>
>>> Otherwise, we would need -ctop=off on newer QEMU to disable the topology.
>>
>> Ah, it's because you added S390_FEAT_CONFIGURATION_TOPOLOGY to the default 
>> feature set here:
>>
>>   static uint16_t default_GEN10_GA1[] = {
>>       S390_FEAT_EDAT,
>>       S390_FEAT_GROUP_MSA_EXT_2,
>> +    S390_FEAT_DISABLE_CPU_TOPOLOGY,
>> +    S390_FEAT_CONFIGURATION_TOPOLOGY,
>>   };
>>
>> ?
>>
>> But what sense does it make to enable it by default, just to disable it by 
>> default again with the S390_FEAT_DISABLE_CPU_TOPOLOGY feature? ... sorry, 
>> I still don't quite get it, but maybe it's because my sinuses are quite 
>> clogged due to a bad cold ... so if you could elaborate again, that would 
>> be very appreciated!
>>
>> However, looking at this from a distance, I would not rather not add this 
>> to any default older CPU model at all (since it also depends on the kernel 
>> to have this feature enabled)? Enabling it in the host model is still ok, 
>> since the host model is not migration safe anyway.
>>
>>   Thomas
>>
> 
> I think I did not understand what is exactly the request that was made about 
> having a CPU flag to disable the topology when we decide to not have a new 
> machine with new machine property.
> 
> Let see what we have if the only change to mainline is to activate 
> S390_FEAT_CONFIGURATION_TOPOLOGY with the KVM capability:
> 
> In mainline, ctop is enabled in the full GEN10 only.
> 
> Consequently we have this feature activated by default for the host model 
> only and deactivated by default if we specify the CPU.
> It can be activated if we specify the CPU with the flag ctop=on.
> 
> This is what was in the patch series before the beginning of the discussion 
> about having a new machine property for new machines.

Sorry for all the mess ... I'm also not an expert when it comes to CPU model 
features paired with compatibility and migration, and I'm still in progress 
of learning ...

> If this what we want: activating the topology by the CPU flag ctop=on it is 
> perfect for me and I can take the original patch.
> We may later make it a default for new machines.

Given my current understanding, I think it's the best thing to do right now. 
Not enable it by default, except for the host model where the enablement is 
fine since migration is not supported any.

As you said, we could still decide later to change the default for new 
machines. Though, I recently learnt that features should also not be enable 
by default at all if they depend on the environment, like a Linux kernel 
that needs to have support for the feature. So maybe we should keep it off 
by default forever - or just enable it on new CPU models (>=z17?) that would 
require a new host kernel anyway.

  Thomas

Re: [PATCH v12 6/7] s390x/cpu_topology: activating CPU topology

[Pierre Morel]

On 12/2/22 15:26, Thomas Huth wrote:
> On 02/12/2022 15.08, Pierre Morel wrote:
>>
>>
>> On 12/2/22 10:05, Thomas Huth wrote:
>>> On 01/12/2022 12.52, Pierre Morel wrote:
>>>>
>>>>
>>>> On 12/1/22 11:15, Thomas Huth wrote:
>>>>> On 29/11/2022 18.42, Pierre Morel wrote:
>>>>>> 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.
>>>>>>
>>>>>> The feature is fenced for SE (secure execution).
>>>>>
>>>>> Out of curiosity: Why does it not work yet?
>>>>>
>>>>>> To allow smooth migration with old QEMU the feature is disabled by
>>>>>> default using the CPU flag -disable-topology.
>>>>>
>>>>> I stared at this code for a while now, but I have to admit that I 
>>>>> don't quite get it. Why do we need a new "disable" feature flag 
>>>>> here? I think it is pretty much impossible to set "ctop=on" with an 
>>>>> older version of QEMU, since it would require the QEMU to enable 
>>>>> KVM_CAP_S390_CPU_TOPOLOGY in the kernel for this feature bit - and 
>>>>> older versions of QEMU don't set this capability yet.
>>>>>
>>>>> Which scenario would fail without this disable-topology feature 
>>>>> bit? What do I miss?
>>>>
>>>> The only scenario it provides is that ctop is then disabled by 
>>>> default on newer QEMU allowing migration between old and new QEMU 
>>>> for older machine without changing the CPU flags.
>>>>
>>>> Otherwise, we would need -ctop=off on newer QEMU to disable the 
>>>> topology.
>>>
>>> Ah, it's because you added S390_FEAT_CONFIGURATION_TOPOLOGY to the 
>>> default feature set here:
>>>
>>>   static uint16_t default_GEN10_GA1[] = {
>>>       S390_FEAT_EDAT,
>>>       S390_FEAT_GROUP_MSA_EXT_2,
>>> +    S390_FEAT_DISABLE_CPU_TOPOLOGY,
>>> +    S390_FEAT_CONFIGURATION_TOPOLOGY,
>>>   };
>>>
>>> ?
>>>
>>> But what sense does it make to enable it by default, just to disable 
>>> it by default again with the S390_FEAT_DISABLE_CPU_TOPOLOGY feature? 
>>> ... sorry, I still don't quite get it, but maybe it's because my 
>>> sinuses are quite clogged due to a bad cold ... so if you could 
>>> elaborate again, that would be very appreciated!
>>>
>>> However, looking at this from a distance, I would not rather not add 
>>> this to any default older CPU model at all (since it also depends on 
>>> the kernel to have this feature enabled)? Enabling it in the host 
>>> model is still ok, since the host model is not migration safe anyway.
>>>
>>>   Thomas
>>>
>>
>> I think I did not understand what is exactly the request that was made 
>> about having a CPU flag to disable the topology when we decide to not 
>> have a new machine with new machine property.
>>
>> Let see what we have if the only change to mainline is to activate 
>> S390_FEAT_CONFIGURATION_TOPOLOGY with the KVM capability:
>>
>> In mainline, ctop is enabled in the full GEN10 only.
>>
>> Consequently we have this feature activated by default for the host 
>> model only and deactivated by default if we specify the CPU.
>> It can be activated if we specify the CPU with the flag ctop=on.
>>
>> This is what was in the patch series before the beginning of the 
>> discussion about having a new machine property for new machines.
> 
> Sorry for all the mess ... I'm also not an expert when it comes to CPU 
> model features paired with compatibility and migration, and I'm still in 
> progress of learning ...
> 
>> If this what we want: activating the topology by the CPU flag ctop=on 
>> it is perfect for me and I can take the original patch.
>> We may later make it a default for new machines.
> 
> Given my current understanding, I think it's the best thing to do right 
> now. Not enable it by default, except for the host model where the 
> enablement is fine since migration is not supported any.
> 
> As you said, we could still decide later to change the default for new 
> machines. Though, I recently learnt that features should also not be 
> enable by default at all if they depend on the environment, like a Linux 
> kernel that needs to have support for the feature. So maybe we should 
> keep it off by default forever - or just enable it on new CPU models 
> (>=z17?) that would require a new host kernel anyway.
> 
>   Thomas
> 

OK, thanks, so I let it with a default as off and we change that later 
in a new CPU model or a new machine as we will see what is the best fit.

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v12 1/7] s390x/cpu topology: Creating CPU topology device

[Janis Schoetterl-Glausch]

On Tue, 2022-11-29 at 18:42 +0100, Pierre Morel wrote:
> We will need a Topology device to transfer the topology
> during migration and to implement machine reset.
> 
> The device creation is fenced by s390_has_topology().
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  include/hw/s390x/cpu-topology.h    | 44 +++++++++++++++
>  include/hw/s390x/s390-virtio-ccw.h |  1 +
>  hw/s390x/cpu-topology.c            | 87 ++++++++++++++++++++++++++++++
>  hw/s390x/s390-virtio-ccw.c         | 25 +++++++++
>  hw/s390x/meson.build               |  1 +
>  5 files changed, 158 insertions(+)
>  create mode 100644 include/hw/s390x/cpu-topology.h
>  create mode 100644 hw/s390x/cpu-topology.c
> 
[...]

> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 9bba21a916..47ce0aa6fa 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;
>      uint8_t loadparm[8];
> +    DeviceState *topology;

Why is this a DeviceState, not S390Topology?
It *has* to be a S390Topology, right? Since you cast it to one in patch 2.

>  };
>  
>  struct S390CcwMachineClass {
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> new file mode 100644
> index 0000000000..bbf97cd66a
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c
> 
[...]
>  
> +static DeviceState *s390_init_topology(MachineState *machine, Error **errp)
> +{
> +    DeviceState *dev;
> +
> +    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
> +
> +    object_property_add_child(&machine->parent_obj,
> +                              TYPE_S390_CPU_TOPOLOGY, OBJECT(dev));

Why set this property, and why on the machine parent?

> +    object_property_set_int(OBJECT(dev), "num-cores",
> +                            machine->smp.cores * machine->smp.threads, errp);
> +    object_property_set_int(OBJECT(dev), "num-sockets",
> +                            machine->smp.sockets, errp);
> +
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);

I must admit that I haven't fully grokked qemu's memory management yet.
Is the topology devices now owned by the sysbus?
If so, is it fine to have a pointer to it S390CcwMachineState?
> +
> +    return dev;
> +}
> +
[...]

Re: [PATCH v12 2/7] s390x/cpu topology: reporting the CPU topology to the guest

[Janis Schoetterl-Glausch]

On Tue, 2022-11-29 at 18:42 +0100, Pierre Morel wrote:
> The guest uses the STSI instruction to get information on the
> CPU topology.
> 
> Let us implement the STSI instruction for the basis CPU topology
> level, level 2.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  target/s390x/cpu.h          |  77 +++++++++++++++
>  hw/s390x/s390-virtio-ccw.c  |  12 +--
>  target/s390x/cpu_topology.c | 186 ++++++++++++++++++++++++++++++++++++
>  target/s390x/kvm/kvm.c      |   6 +-
>  target/s390x/meson.build    |   1 +
>  5 files changed, 274 insertions(+), 8 deletions(-)
>  create mode 100644 target/s390x/cpu_topology.c
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 7d6d01325b..dd878ac916 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> 
[...]

> +/* Configuration topology */
> +typedef struct SysIB_151x {
> +    uint8_t  reserved0[2];
> +    uint16_t length;
> +    uint8_t  mag[S390_TOPOLOGY_MAG];
> +    uint8_t  reserved1;
> +    uint8_t  mnest;
> +    uint32_t reserved2;
> +    char tle[0];

AFAIK [] is preferred over [0].

> +} QEMU_PACKED QEMU_ALIGNED(8) SysIB_151x;
> +QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);

[...]
> 
> +/*
> + * s390_topology_add_cpu:
> + * @topo: pointer to the topology
> + * @cpu : pointer to the new CPU
> + *
> + * The topology pointed by S390CPU, gives us the CPU topology
> + * established by the -smp QEMU aruments.
> + * The core-id is used to calculate the position of the CPU inside
> + * the topology:
> + *  - the socket, container TLE, containing the CPU, we have one socket
> + *    for every num_cores cores.
> + *  - the CPU TLE inside the socket, we have potentionly up to 4 CPU TLE
> + *    in a container TLE with the assumption that all CPU are identical
> + *    with the same polarity and entitlement because we have maximum 256
> + *    CPUs and each TLE can hold up to 64 identical CPUs.
> + *  - the bit in the 64 bit CPU TLE core mask
> + */
> +static void s390_topology_add_cpu(S390Topology *topo, S390CPU *cpu)
> +{
> +    int core_id = cpu->env.core_id;
> +    int bit, origin;
> +    int socket_id;
> +
> +    cpu->machine_data = topo;
> +    socket_id = core_id / topo->num_cores;
> +    /*
> +     * At the core level, each CPU is represented by a bit in a 64bit
> +     * uint64_t 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.
> +     */

This comment still contains redundant sentences.
Did you have a look at my suggestion in v10 patch 1?

> +    bit = core_id;
> +    origin = bit / 64;
> +    bit %= 64;
> +    bit = 63 - bit;
> +
> +    topo->socket[socket_id].active_count++;
> +    set_bit(bit, &topo->socket[socket_id].mask[origin]);
> +}
> +
> +/*
> + * s390_prepare_topology:
> + * @s390ms : pointer to the S390CcwMachite State
> + *
> + * Calls s390_topology_add_cpu to organize the topology
> + * inside the topology device before writing the SYSIB.
> + *
> + * The topology is currently fixed on boot and do not change

does not change

> + * even on migration.
> + */
> +static void s390_prepare_topology(S390CcwMachineState *s390ms)
> +{
> +    const MachineState *ms = MACHINE(s390ms);
> +    static bool done;
> +    int i;
> +
> +    if (done) {
> +        return;
> +    }
> +
> +    for (i = 0; i < ms->possible_cpus->len; i++) {
> +        if (ms->possible_cpus->cpus[i].cpu) {
> +            s390_topology_add_cpu(S390_CPU_TOPOLOGY(s390ms->topology),
> +                                  S390_CPU(ms->possible_cpus->cpus[i].cpu));
> +        }
> +    }
> +
> +    done = true;
> +}
> +
> 
[...]

Re: [PATCH v12 3/7] s390x/cpu_topology: resetting the Topology-Change-Report

[Janis Schoetterl-Glausch]

On Tue, 2022-11-29 at 18:42 +0100, 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.
  ^ weird space

[...]

Re: [PATCH v12 1/7] s390x/cpu topology: Creating CPU topology device

[Pierre Morel]

On 12/6/22 10:31, Janis Schoetterl-Glausch wrote:
> On Tue, 2022-11-29 at 18:42 +0100, Pierre Morel wrote:
>> We will need a Topology device to transfer the topology
>> during migration and to implement machine reset.
>>
>> The device creation is fenced by s390_has_topology().
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   include/hw/s390x/cpu-topology.h    | 44 +++++++++++++++
>>   include/hw/s390x/s390-virtio-ccw.h |  1 +
>>   hw/s390x/cpu-topology.c            | 87 ++++++++++++++++++++++++++++++
>>   hw/s390x/s390-virtio-ccw.c         | 25 +++++++++
>>   hw/s390x/meson.build               |  1 +
>>   5 files changed, 158 insertions(+)
>>   create mode 100644 include/hw/s390x/cpu-topology.h
>>   create mode 100644 hw/s390x/cpu-topology.c
>>
> [...]
> 
>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
>> index 9bba21a916..47ce0aa6fa 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;
>>       uint8_t loadparm[8];
>> +    DeviceState *topology;
> 
> Why is this a DeviceState, not S390Topology?
> It *has* to be a S390Topology, right? Since you cast it to one in patch 2.

Yes, currently it is the S390Topology.
The idea of Cedric was to have something more generic for future use.

> 
>>   };
>>   
>>   struct S390CcwMachineClass {
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> new file mode 100644
>> index 0000000000..bbf97cd66a
>> --- /dev/null
>> +++ b/hw/s390x/cpu-topology.c
>>
> [...]
>>   
>> +static DeviceState *s390_init_topology(MachineState *machine, Error **errp)
>> +{
>> +    DeviceState *dev;
>> +
>> +    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
>> +
>> +    object_property_add_child(&machine->parent_obj,
>> +                              TYPE_S390_CPU_TOPOLOGY, OBJECT(dev));
> 
> Why set this property, and why on the machine parent?

For what I understood setting the num_cores and num_sockets as 
properties of the CPU Topology object allows to have them better 
integrated in the QEMU object framework.

The topology is added to the S390CcwmachineState, it is the parent of 
the machine.


> 
>> +    object_property_set_int(OBJECT(dev), "num-cores",
>> +                            machine->smp.cores * machine->smp.threads, errp);
>> +    object_property_set_int(OBJECT(dev), "num-sockets",
>> +                            machine->smp.sockets, errp);
>> +
>> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);
> 
> I must admit that I haven't fully grokked qemu's memory management yet.
> Is the topology devices now owned by the sysbus?

Yes it is so we see it on the qtree with its properties.


> If so, is it fine to have a pointer to it S390CcwMachineState?

Why not?
It seems logical to me that the sysbus belong to the virtual machine.
But sometime the way of QEMU are not very transparent for me :)
so I can be wrong.

Regards,
Pierre

>> +
>> +    return dev;
>> +}
>> +
> [...]

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v12 2/7] s390x/cpu topology: reporting the CPU topology to the guest

[Pierre Morel]

On 12/6/22 10:48, Janis Schoetterl-Glausch wrote:
> On Tue, 2022-11-29 at 18:42 +0100, Pierre Morel wrote:
>> The guest uses the STSI instruction to get information on the
>> CPU topology.
>>
>> Let us implement the STSI instruction for the basis CPU topology
>> level, level 2.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   target/s390x/cpu.h          |  77 +++++++++++++++
>>   hw/s390x/s390-virtio-ccw.c  |  12 +--
>>   target/s390x/cpu_topology.c | 186 ++++++++++++++++++++++++++++++++++++
>>   target/s390x/kvm/kvm.c      |   6 +-
>>   target/s390x/meson.build    |   1 +
>>   5 files changed, 274 insertions(+), 8 deletions(-)
>>   create mode 100644 target/s390x/cpu_topology.c
>>
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 7d6d01325b..dd878ac916 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>>
> [...]
> 
>> +/* Configuration topology */
>> +typedef struct SysIB_151x {
>> +    uint8_t  reserved0[2];
>> +    uint16_t length;
>> +    uint8_t  mag[S390_TOPOLOGY_MAG];
>> +    uint8_t  reserved1;
>> +    uint8_t  mnest;
>> +    uint32_t reserved2;
>> +    char tle[0];
> 
> AFAIK [] is preferred over [0].

grr, yes, I think I have been already told so :)


> 
>> +} QEMU_PACKED QEMU_ALIGNED(8) SysIB_151x;
>> +QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
> 
> [...]
>>
>> +/*
>> + * s390_topology_add_cpu:
>> + * @topo: pointer to the topology
>> + * @cpu : pointer to the new CPU
>> + *
>> + * The topology pointed by S390CPU, gives us the CPU topology
>> + * established by the -smp QEMU aruments.
>> + * The core-id is used to calculate the position of the CPU inside
>> + * the topology:
>> + *  - the socket, container TLE, containing the CPU, we have one socket
>> + *    for every num_cores cores.
>> + *  - the CPU TLE inside the socket, we have potentionly up to 4 CPU TLE
>> + *    in a container TLE with the assumption that all CPU are identical
>> + *    with the same polarity and entitlement because we have maximum 256
>> + *    CPUs and each TLE can hold up to 64 identical CPUs.
>> + *  - the bit in the 64 bit CPU TLE core mask
>> + */
>> +static void s390_topology_add_cpu(S390Topology *topo, S390CPU *cpu)
>> +{
>> +    int core_id = cpu->env.core_id;
>> +    int bit, origin;
>> +    int socket_id;
>> +
>> +    cpu->machine_data = topo;
>> +    socket_id = core_id / topo->num_cores;
>> +    /*
>> +     * At the core level, each CPU is represented by a bit in a 64bit
>> +     * uint64_t 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.
>> +     */
> 
> This comment still contains redundant sentences.
> Did you have a look at my suggestion in v10 patch 1?

Yes, I had, and sorry, I forgot to report here inside the patch 11.
I will take it, thanks for it.

> 
>> +    bit = core_id;
>> +    origin = bit / 64;
>> +    bit %= 64;
>> +    bit = 63 - bit;
>> +
>> +    topo->socket[socket_id].active_count++;
>> +    set_bit(bit, &topo->socket[socket_id].mask[origin]);
>> +}
>> +
>> +/*
>> + * s390_prepare_topology:
>> + * @s390ms : pointer to the S390CcwMachite State
>> + *
>> + * Calls s390_topology_add_cpu to organize the topology
>> + * inside the topology device before writing the SYSIB.
>> + *
>> + * The topology is currently fixed on boot and do not change
> 
> does not change

yes, thanks


regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v12 3/7] s390x/cpu_topology: resetting the Topology-Change-Report

[Pierre Morel]

On 12/6/22 10:50, Janis Schoetterl-Glausch wrote:
> On Tue, 2022-11-29 at 18:42 +0100, 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.
>    ^ weird space
> 
> [...]

Yes, thanks

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v12 1/7] s390x/cpu topology: Creating CPU topology device

[Janis Schoetterl-Glausch]

On Tue, 2022-12-06 at 11:32 +0100, Pierre Morel wrote:
> 
> On 12/6/22 10:31, Janis Schoetterl-Glausch wrote:
> > On Tue, 2022-11-29 at 18:42 +0100, Pierre Morel wrote:
> > > We will need a Topology device to transfer the topology
> > > during migration and to implement machine reset.
> > > 
> > > The device creation is fenced by s390_has_topology().
> > > 
> > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > > ---
> > >   include/hw/s390x/cpu-topology.h    | 44 +++++++++++++++
> > >   include/hw/s390x/s390-virtio-ccw.h |  1 +
> > >   hw/s390x/cpu-topology.c            | 87 ++++++++++++++++++++++++++++++
> > >   hw/s390x/s390-virtio-ccw.c         | 25 +++++++++
> > >   hw/s390x/meson.build               |  1 +
> > >   5 files changed, 158 insertions(+)
> > >   create mode 100644 include/hw/s390x/cpu-topology.h
> > >   create mode 100644 hw/s390x/cpu-topology.c
> > > 
> > [...]
> > 
> > > diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> > > index 9bba21a916..47ce0aa6fa 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;
> > >       uint8_t loadparm[8];
> > > +    DeviceState *topology;
> > 
> > Why is this a DeviceState, not S390Topology?
> > It *has* to be a S390Topology, right? Since you cast it to one in patch 2.
> 
> Yes, currently it is the S390Topology.
> The idea of Cedric was to have something more generic for future use.

But it still needs to be a S390Topology otherwise you cannot cast it to one, can you?
> 
> > 
> > >   };
> > >   
> > >   struct S390CcwMachineClass {
> > > diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> > > new file mode 100644
> > > index 0000000000..bbf97cd66a
> > > --- /dev/null
> > > +++ b/hw/s390x/cpu-topology.c
> > > 
> > [...]
> > >   
> > > +static DeviceState *s390_init_topology(MachineState *machine, Error **errp)
> > > +{
> > > +    DeviceState *dev;
> > > +
> > > +    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
> > > +
> > > +    object_property_add_child(&machine->parent_obj,
> > > +                              TYPE_S390_CPU_TOPOLOGY, OBJECT(dev));
> > 
> > Why set this property, and why on the machine parent?
> 
> For what I understood setting the num_cores and num_sockets as 
> properties of the CPU Topology object allows to have them better 
> integrated in the QEMU object framework.

That I understand.
> 
> The topology is added to the S390CcwmachineState, it is the parent of 
> the machine.

But why? And is it added to the S390CcwMachineState, or its parent?
> 
> 
> > 
> > > +    object_property_set_int(OBJECT(dev), "num-cores",
> > > +                            machine->smp.cores * machine->smp.threads, errp);
> > > +    object_property_set_int(OBJECT(dev), "num-sockets",
> > > +                            machine->smp.sockets, errp);
> > > +
> > > +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);
> > 
> > I must admit that I haven't fully grokked qemu's memory management yet.
> > Is the topology devices now owned by the sysbus?
> 
> Yes it is so we see it on the qtree with its properties.
> 
> 
> > If so, is it fine to have a pointer to it S390CcwMachineState?
> 
> Why not?

If it's owned by the sysbus and the object is not explicitly referenced
for the pointer, it might be deallocated and then you'd have a dangling pointer.

> It seems logical to me that the sysbus belong to the virtual machine.
> But sometime the way of QEMU are not very transparent for me :)
> so I can be wrong.
> 
> Regards,
> Pierre
> 
> > > +
> > > +    return dev;
> > > +}
> > > +
> > [...]
> 

Re: [PATCH v12 1/7] s390x/cpu topology: Creating CPU topology device

[Pierre Morel]

On 12/6/22 14:35, Janis Schoetterl-Glausch wrote:
> On Tue, 2022-12-06 at 11:32 +0100, Pierre Morel wrote:
>>
>> On 12/6/22 10:31, Janis Schoetterl-Glausch wrote:
>>> On Tue, 2022-11-29 at 18:42 +0100, Pierre Morel wrote:
>>>> We will need a Topology device to transfer the topology
>>>> during migration and to implement machine reset.
>>>>
>>>> The device creation is fenced by s390_has_topology().
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> ---
>>>>    include/hw/s390x/cpu-topology.h    | 44 +++++++++++++++
>>>>    include/hw/s390x/s390-virtio-ccw.h |  1 +
>>>>    hw/s390x/cpu-topology.c            | 87 ++++++++++++++++++++++++++++++
>>>>    hw/s390x/s390-virtio-ccw.c         | 25 +++++++++
>>>>    hw/s390x/meson.build               |  1 +
>>>>    5 files changed, 158 insertions(+)
>>>>    create mode 100644 include/hw/s390x/cpu-topology.h
>>>>    create mode 100644 hw/s390x/cpu-topology.c
>>>>
>>> [...]
>>>
>>>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
>>>> index 9bba21a916..47ce0aa6fa 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;
>>>>        uint8_t loadparm[8];
>>>> +    DeviceState *topology;
>>>
>>> Why is this a DeviceState, not S390Topology?
>>> It *has* to be a S390Topology, right? Since you cast it to one in patch 2.
>>
>> Yes, currently it is the S390Topology.
>> The idea of Cedric was to have something more generic for future use.
> 
> But it still needs to be a S390Topology otherwise you cannot cast it to one, can you?

May be I did not understand correctly what Cedric wants.
For my part I agree with you I do not see the point to have something 
different than a S390Topology pointer.

Also doing that is more secure as we do not need cast... which reveals a 
bug I have in setup_stsi().... !!!!

Let's do that and see what Cedric says.

>>
>>>
>>>>    };
>>>>    
>>>>    struct S390CcwMachineClass {
>>>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>>>> new file mode 100644
>>>> index 0000000000..bbf97cd66a
>>>> --- /dev/null
>>>> +++ b/hw/s390x/cpu-topology.c
>>>>
>>> [...]
>>>>    
>>>> +static DeviceState *s390_init_topology(MachineState *machine, Error **errp)
>>>> +{
>>>> +    DeviceState *dev;
>>>> +
>>>> +    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
>>>> +
>>>> +    object_property_add_child(&machine->parent_obj,
>>>> +                              TYPE_S390_CPU_TOPOLOGY, OBJECT(dev));
>>>
>>> Why set this property, and why on the machine parent?
>>
>> For what I understood setting the num_cores and num_sockets as
>> properties of the CPU Topology object allows to have them better
>> integrated in the QEMU object framework.
> 
> That I understand.
>>
>> The topology is added to the S390CcwmachineState, it is the parent of
>> the machine.
> 
> But why? And is it added to the S390CcwMachineState, or its parent?

it is added to the S390CcwMachineState.
We receive the MachineState as the "machine" parameter here and it is 
added to the "machine->parent_obj" which is the S390CcwMachineState.



>>
>>
>>>
>>>> +    object_property_set_int(OBJECT(dev), "num-cores",
>>>> +                            machine->smp.cores * machine->smp.threads, errp);
>>>> +    object_property_set_int(OBJECT(dev), "num-sockets",
>>>> +                            machine->smp.sockets, errp);
>>>> +
>>>> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);
>>>
>>> I must admit that I haven't fully grokked qemu's memory management yet.
>>> Is the topology devices now owned by the sysbus?
>>
>> Yes it is so we see it on the qtree with its properties.
>>
>>
>>> If so, is it fine to have a pointer to it S390CcwMachineState?
>>
>> Why not?
> 
> If it's owned by the sysbus and the object is not explicitly referenced
> for the pointer, it might be deallocated and then you'd have a dangling pointer.

Why would it be deallocated ?
as long it is not unrealized it belongs to the sysbus doesn't it?

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v12 2/7] s390x/cpu topology: reporting the CPU topology to the guest

[Pierre Morel]

On 11/29/22 18:42, Pierre Morel wrote:
> The guest uses the STSI instruction to get information on the
> CPU topology.
> 
> Let us implement the STSI instruction for the basis CPU topology
> level, level 2.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   target/s390x/cpu.h          |  77 +++++++++++++++
>   hw/s390x/s390-virtio-ccw.c  |  12 +--
>   target/s390x/cpu_topology.c | 186 ++++++++++++++++++++++++++++++++++++
>   target/s390x/kvm/kvm.c      |   6 +-
>   target/s390x/meson.build    |   1 +
>   5 files changed, 274 insertions(+), 8 deletions(-)
>   create mode 100644 target/s390x/cpu_topology.c
> 

> + */
> +static void s390_topology_add_cpu(S390Topology *topo, S390CPU *cpu)
> +{
> +    int core_id = cpu->env.core_id;
> +    int bit, origin;
> +    int socket_id;
> +
> +    cpu->machine_data = topo;

Sorry this wrong machine_data is already used as a pointer to the 
S390CcwMachineState machine.



> +    socket_id = core_id / topo->num_cores;
> +    /*

...snip...

> +
> +static int setup_stsi(S390CPU *cpu, SysIB_151x *sysib, int level)
> +{
> +    S390Topology *topo = (S390Topology *)cpu->machine_data;

Sorry, wrong too this must be:

     S390CcwMachineState *s390ms = cpu->machine_data;
     S390Topology *topo = S390_CPU_TOPOLOGY(s390ms->topology);

> +    char *p = sysib->tle;
> +
> +    sysib->mnest = level;
> +    switch (level) {
> +    case 2:
> +        sysib->mag[S390_TOPOLOGY_MAG2] = topo->num_sockets;
> +        sysib->mag[S390_TOPOLOGY_MAG1] = topo->num_cores;
> +        p = s390_top_set_level2(topo, p);
> +        break;
> +    }
> +
> +    return p - (char *)sysib;
> +}
> +


Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v12 1/7] s390x/cpu topology: Creating CPU topology device

[Janis Schoetterl-Glausch]

On Tue, 2022-12-06 at 15:35 +0100, Pierre Morel wrote:
> 
> On 12/6/22 14:35, Janis Schoetterl-Glausch wrote:
> > On Tue, 2022-12-06 at 11:32 +0100, Pierre Morel wrote:
> > > 
> > > On 12/6/22 10:31, Janis Schoetterl-Glausch wrote:
> > > > On Tue, 2022-11-29 at 18:42 +0100, Pierre Morel wrote:
> > > > > We will need a Topology device to transfer the topology
> > > > > during migration and to implement machine reset.
> > > > > 
> > > > > The device creation is fenced by s390_has_topology().
> > > > > 
> > > > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > > > > ---
> > > > >    include/hw/s390x/cpu-topology.h    | 44 +++++++++++++++
> > > > >    include/hw/s390x/s390-virtio-ccw.h |  1 +
> > > > >    hw/s390x/cpu-topology.c            | 87 ++++++++++++++++++++++++++++++
> > > > >    hw/s390x/s390-virtio-ccw.c         | 25 +++++++++
> > > > >    hw/s390x/meson.build               |  1 +
> > > > >    5 files changed, 158 insertions(+)
> > > > >    create mode 100644 include/hw/s390x/cpu-topology.h
> > > > >    create mode 100644 hw/s390x/cpu-topology.c
> > > > 
[...]
> > > > >    
> > > > > +static DeviceState *s390_init_topology(MachineState *machine, Error **errp)
> > > > > +{
> > > > > +    DeviceState *dev;
> > > > > +
> > > > > +    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
> > > > > +
> > > > > +    object_property_add_child(&machine->parent_obj,
> > > > > +                              TYPE_S390_CPU_TOPOLOGY, OBJECT(dev));
> > > > 
> > > > Why set this property, and why on the machine parent?
> > > 
> > > For what I understood setting the num_cores and num_sockets as
> > > properties of the CPU Topology object allows to have them better
> > > integrated in the QEMU object framework.
> > 
> > That I understand.
> > > 
> > > The topology is added to the S390CcwmachineState, it is the parent of
> > > the machine.
> > 
> > But why? And is it added to the S390CcwMachineState, or its parent?
> 
> it is added to the S390CcwMachineState.
> We receive the MachineState as the "machine" parameter here and it is 
> added to the "machine->parent_obj" which is the S390CcwMachineState.

Oh, I was confused. &machine->parent_obj is just a cast of MachineState* to Object*.
It's the very same object.
And what is the reason to add the topology as child property?
Just so it shows up in the qtree? Wouldn't it anyway under the sysbus?
> 
> 
> 
> > > 
> > > 
> > > > 
> > > > > +    object_property_set_int(OBJECT(dev), "num-cores",
> > > > > +                            machine->smp.cores * machine->smp.threads, errp);
> > > > > +    object_property_set_int(OBJECT(dev), "num-sockets",
> > > > > +                            machine->smp.sockets, errp);
> > > > > +
> > > > > +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);
> > > > 
> > > > I must admit that I haven't fully grokked qemu's memory management yet.
> > > > Is the topology devices now owned by the sysbus?
> > > 
> > > Yes it is so we see it on the qtree with its properties.
> > > 
> > > 
> > > > If so, is it fine to have a pointer to it S390CcwMachineState?
> > > 
> > > Why not?
> > 
> > If it's owned by the sysbus and the object is not explicitly referenced
> > for the pointer, it might be deallocated and then you'd have a dangling pointer.
> 
> Why would it be deallocated ?

That's beside the point, if you transfer ownership, you have no control over when
the deallocation happens.
It's going to be fine in practice, but I don't think you should rely on it.
I think you could just do sysbus_realize instead of ..._and_unref,
but like I said, I haven't fully understood qemu memory management.
(It would also leak in a sense, but since the machine exists forever that should be fine)

> as long it is not unrealized it belongs to the sysbus doesn't it?
> 
> Regards,
> Pierre
> 

Re: [PATCH v12 2/7] s390x/cpu topology: reporting the CPU topology to the guest

[Cédric Le Goater]

On 11/29/22 18:42, Pierre Morel wrote:
> The guest uses the STSI instruction to get information on the
> CPU topology.
> 
> Let us implement the STSI instruction for the basis CPU topology
> level, level 2.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   target/s390x/cpu.h          |  77 +++++++++++++++
>   hw/s390x/s390-virtio-ccw.c  |  12 +--
>   target/s390x/cpu_topology.c | 186 ++++++++++++++++++++++++++++++++++++
>   target/s390x/kvm/kvm.c      |   6 +-
>   target/s390x/meson.build    |   1 +
>   5 files changed, 274 insertions(+), 8 deletions(-)
>   create mode 100644 target/s390x/cpu_topology.c
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 7d6d01325b..dd878ac916 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -175,6 +175,7 @@ struct ArchCPU {
>       /* needed for live migration */
>       void *irqstate;
>       uint32_t irqstate_saved_size;
> +    void *machine_data;
>   };
>   
>   
> @@ -565,6 +566,80 @@ typedef union SysIB {
>   } SysIB;
>   QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
>   
> +/*
> + * CPU Topology List provided by STSI with fc=15 provides a list
> + * of two different Topology List Entries (TLE) types to specify
> + * the topology hierarchy.
> + *
> + * - Container Topology List Entry
> + *   Defines a container to contain other Topology List Entries
> + *   of any type, nested containers or CPU.
> + * - CPU Topology List Entry
> + *   Specifies the CPUs position, type, entitlement and polarization
> + *   of the CPUs contained in the last Container TLE.
> + *
> + * There can be theoretically up to five levels of containers, QEMU
> + * uses only one level, the socket level.
> + *
> + * A container of with a nesting level (NL) greater than 1 can only
> + * contain another container of nesting level NL-1.
> + *
> + * A container of nesting level 1 (socket), contains as many CPU TLE
> + * as needed to describe the position and qualities of all CPUs inside
> + * the container.
> + * The qualities of a CPU are polarization, entitlement and type.
> + *
> + * The CPU TLE defines the position of the CPUs of identical qualities
> + * using a 64bits mask which first bit has its offset defined by
> + * the CPU address orgin field of the CPU TLE like in:
> + * CPU address = origin * 64 + bit position within the mask
> + *
> + */
> +/* 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);
> +
> +/* 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);
> +
> +#define S390_TOPOLOGY_MAG  6
> +#define S390_TOPOLOGY_MAG6 0
> +#define S390_TOPOLOGY_MAG5 1
> +#define S390_TOPOLOGY_MAG4 2
> +#define S390_TOPOLOGY_MAG3 3
> +#define S390_TOPOLOGY_MAG2 4
> +#define S390_TOPOLOGY_MAG1 5
> +/* Configuration topology */
> +typedef struct SysIB_151x {
> +    uint8_t  reserved0[2];
> +    uint16_t length;
> +    uint8_t  mag[S390_TOPOLOGY_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);
> +
> +/* Max 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 +918,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/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 973bbdd36e..4be07959fd 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -64,11 +64,10 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>       return S390_CPU(ms->possible_cpus->cpus[cpu_addr].cpu);
>   }
>   
> -static S390CPU *s390x_new_cpu(const char *typename, uint32_t core_id,
> -                              Error **errp)
> +static void s390x_new_cpu(MachineState *ms, uint32_t core_id, Error **errp)
>   {
> -    S390CPU *cpu = S390_CPU(object_new(typename));
> -    S390CPU *ret = NULL;
> +    S390CcwMachineState *s390ms = S390_CCW_MACHINE(ms);
> +    S390CPU *cpu = S390_CPU(object_new(ms->cpu_type));
>   
>       if (!object_property_set_int(OBJECT(cpu), "core-id", core_id, errp)) {
>           goto out;
> @@ -76,11 +75,10 @@ static S390CPU *s390x_new_cpu(const char *typename, uint32_t core_id,
>       if (!qdev_realize(DEVICE(cpu), NULL, errp)) {
>           goto out;
>       }
> -    ret = cpu;
> +    cpu->machine_data = s390ms;
>   
>   out:
>       object_unref(OBJECT(cpu));
> -    return ret;
>   }
>   
>   static void s390_init_cpus(MachineState *machine)
> @@ -99,7 +97,7 @@ static void s390_init_cpus(MachineState *machine)
>       mc->possible_cpu_arch_ids(machine);
>   
>       for (i = 0; i < machine->smp.cpus; i++) {
> -        s390x_new_cpu(machine->cpu_type, i, &error_fatal);
> +        s390x_new_cpu(machine, i, &error_fatal);
>       }
>   }
>   
> diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
> new file mode 100644
> index 0000000000..b81f016ba1
> --- /dev/null
> +++ b/target/s390x/cpu_topology.c
> @@ -0,0 +1,186 @@
> +/*
> + * 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"
> +
> +/*
> + * s390_topology_add_cpu:
> + * @topo: pointer to the topology
> + * @cpu : pointer to the new CPU
> + *
> + * The topology pointed by S390CPU, gives us the CPU topology
> + * established by the -smp QEMU aruments.
> + * The core-id is used to calculate the position of the CPU inside
> + * the topology:
> + *  - the socket, container TLE, containing the CPU, we have one socket
> + *    for every num_cores cores.
> + *  - the CPU TLE inside the socket, we have potentionly up to 4 CPU TLE
> + *    in a container TLE with the assumption that all CPU are identical
> + *    with the same polarity and entitlement because we have maximum 256
> + *    CPUs and each TLE can hold up to 64 identical CPUs.
> + *  - the bit in the 64 bit CPU TLE core mask
> + */
> +static void s390_topology_add_cpu(S390Topology *topo, S390CPU *cpu)
> +{
> +    int core_id = cpu->env.core_id;
> +    int bit, origin;
> +    int socket_id;
> +
> +    cpu->machine_data = topo;
> +    socket_id = core_id / topo->num_cores;
> +    /*
> +     * At the core level, each CPU is represented by a bit in a 64bit
> +     * uint64_t 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->socket[socket_id].mask[origin]);
> +}
> +
> +/*
> + * s390_prepare_topology:
> + * @s390ms : pointer to the S390CcwMachite State
> + *
> + * Calls s390_topology_add_cpu to organize the topology
> + * inside the topology device before writing the SYSIB.
> + *
> + * The topology is currently fixed on boot and do not change
> + * even on migration.
> + */
> +static void s390_prepare_topology(S390CcwMachineState *s390ms)
> +{
> +    const MachineState *ms = MACHINE(s390ms);
> +    static bool done;
> +    int i;
> +
> +    if (done) {
> +        return;
> +    }
> +
> +    for (i = 0; i < ms->possible_cpus->len; i++) {
> +        if (ms->possible_cpus->cpus[i].cpu) {
> +            s390_topology_add_cpu(S390_CPU_TOPOLOGY(s390ms->topology),
> +                                  S390_CPU(ms->possible_cpus->cpus[i].cpu));
> +        }
> +    }
> +
> +    done = true;
> +}
> +
> +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_HORIZONTAL;
> +    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)
> +{
> +    int i, origin;
> +
> +    for (i = 0; i < topo->num_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->socket[i].mask[origin];
> +            if (mask) {
> +                p = fill_tle_cpu(p, mask, origin);
> +            }
> +        }
> +    }
> +    return p;
> +}
> +
> +static int setup_stsi(S390CPU *cpu, SysIB_151x *sysib, int level)
> +{
> +    S390Topology *topo = (S390Topology *)cpu->machine_data;
> +    char *p = sysib->tle;
> +
> +    sysib->mnest = level;
> +    switch (level) {
> +    case 2:
> +        sysib->mag[S390_TOPOLOGY_MAG2] = topo->num_sockets;
> +        sysib->mag[S390_TOPOLOGY_MAG1] = topo->num_cores;
> +        p = s390_top_set_level2(topo, p);
> +        break;
> +    }
> +
> +    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)
> +{
> +    union {
> +        char place_holder[S390_TOPOLOGY_SYSIB_SIZE];
> +        SysIB_151x sysib;
> +    } buffer QEMU_ALIGNED(8);
> +    int len;
> +
> +    if (s390_is_pv() || !s390_has_topology() ||
> +        sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
> +        setcc(cpu, 3);
> +        return;
> +    }
>
> +    s390_prepare_topology(S390_CCW_MACHINE(cpu->machine_data));
> +
> +    len = setup_stsi(cpu, &buffer.sysib, sel2);


The S390_CPU_TOPOLOGY object is created by the machine at init time
and the two above routines are the only users of this object.

The first loops on all possible CPUs to populate the bitmask array
'socket' under S390_CPU_TOPOLOGY and the second uses the result to
populate the buffer returned to the guest OS.

I don't understand why the S390_CPU_TOPOLOGY object is needed at all.
AFAICT, this is just adding extra complexity. Is the pachset preparing
ground for some more features ? If so, it should be explained in the
commit log.

As for now, I see no good justification for S390_CPU_TOPOLOGY and we
could add support with a simple routine called from insert_stsi_15_1_x().

Thanks,

C.

> +
> +    if (len > 4096) {
> +        setcc(cpu, 3);
> +        return;
> +    }
> +
> +    buffer.sysib.length = cpu_to_be16(len);
> +    s390_cpu_virt_mem_write(cpu, addr, ar, &buffer.sysib, len);
> +    setcc(cpu, 0);
> +}
> +
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 3ac7ec9acf..7dc96f3663 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
> @@ -1919,9 +1920,12 @@ static int handle_stsi(S390CPU *cpu)
>           if (run->s390_stsi.sel1 != 2 || run->s390_stsi.sel2 != 2) {
>               return 0;
>           }
> -        /* Only sysib 3.2.2 needs post-handling for now. */
>           insert_stsi_3_2_2(cpu, run->s390_stsi.addr, run->s390_stsi.ar);
>           return 0;
> +    case 15:
> +        insert_stsi_15_1_x(cpu, run->s390_stsi.sel2, run->s390_stsi.addr,
> +                           run->s390_stsi.ar);
> +        return 0;
>       default:
>           return 0;
>       }
> diff --git a/target/s390x/meson.build b/target/s390x/meson.build
> index 84c1402a6a..890ccfa789 100644
> --- a/target/s390x/meson.build
> +++ b/target/s390x/meson.build
> @@ -29,6 +29,7 @@ s390x_softmmu_ss.add(files(
>     'sigp.c',
>     'cpu-sysemu.c',
>     'cpu_models_sysemu.c',
> +  'cpu_topology.c',
>   ))
>   
>   s390x_user_ss = ss.source_set()

Re: [PATCH v12 2/7] s390x/cpu topology: reporting the CPU topology to the guest

[Pierre Morel]

On 12/7/22 10:12, Cédric Le Goater wrote:
> On 11/29/22 18:42, Pierre Morel wrote:
>> The guest uses the STSI instruction to get information on the
>> CPU topology.
>>
>> Let us implement the STSI instruction for the basis CPU topology
>> level, level 2.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---

...snip...

>> +
>> +#define S390_TOPOLOGY_MAX_MNEST 2
>> +
>> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
>> +{
>> +    union {
>> +        char place_holder[S390_TOPOLOGY_SYSIB_SIZE];
>> +        SysIB_151x sysib;
>> +    } buffer QEMU_ALIGNED(8);
>> +    int len;
>> +
>> +    if (s390_is_pv() || !s390_has_topology() ||
>> +        sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
>> +        setcc(cpu, 3);
>> +        return;
>> +    }
>>
>> +    s390_prepare_topology(S390_CCW_MACHINE(cpu->machine_data));
>> +
>> +    len = setup_stsi(cpu, &buffer.sysib, sel2);
> 
> 
> The S390_CPU_TOPOLOGY object is created by the machine at init time
> and the two above routines are the only users of this object.

This is right at this moment but the object will be used in the next 
patches for implementing reset, patch 3, and migration, patch 4.


> 
> The first loops on all possible CPUs to populate the bitmask array
> 'socket' under S390_CPU_TOPOLOGY and the second uses the result to
> populate the buffer returned to the guest OS.
> 
> I don't understand why the S390_CPU_TOPOLOGY object is needed at all.
> AFAICT, this is just adding extra complexity.

I used an object because I thought it could be cleaner for the 
implementation of reset and migration.

> Is the pachset preparing
> ground for some more features ? 

Yes it is, I removed the books and drawers topology containers from this 
patch series in the version 10 of the patch series to postpone their 
implementation.

The next series on topology implementation will also add, beside the 
implementation of drawers and books, the possibility to modify the 
topology during the life of a guest.

These, book, drawer and the topology modification will need to be migrated.

Is there a good alternative to facilitate the implementation of the 
migration ?

Of course we can put all together inside the CcwMachineState but 
wouldn't the use of a dedicated object make it all cleaner?

Regards,
Pierre

If so, it should be explained in the
> commit log.
> 
> As for now, I see no good justification for S390_CPU_TOPOLOGY and we
> could add support with a simple routine called from insert_stsi_15_1_x().
> 
> Thanks,
> 
> C.

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v12 1/7] s390x/cpu topology: Creating CPU topology device

[Pierre Morel]

On 12/6/22 22:06, Janis Schoetterl-Glausch wrote:
> On Tue, 2022-12-06 at 15:35 +0100, Pierre Morel wrote:
>>
>> On 12/6/22 14:35, Janis Schoetterl-Glausch wrote:
>>> On Tue, 2022-12-06 at 11:32 +0100, Pierre Morel wrote:
>>>>
>>>> On 12/6/22 10:31, Janis Schoetterl-Glausch wrote:
>>>>> On Tue, 2022-11-29 at 18:42 +0100, Pierre Morel wrote:
>>>>>> We will need a Topology device to transfer the topology
>>>>>> during migration and to implement machine reset.
>>>>>>
>>>>>> The device creation is fenced by s390_has_topology().
>>>>>>
>>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>>>> ---
>>>>>>     include/hw/s390x/cpu-topology.h    | 44 +++++++++++++++
>>>>>>     include/hw/s390x/s390-virtio-ccw.h |  1 +
>>>>>>     hw/s390x/cpu-topology.c            | 87 ++++++++++++++++++++++++++++++
>>>>>>     hw/s390x/s390-virtio-ccw.c         | 25 +++++++++
>>>>>>     hw/s390x/meson.build               |  1 +
>>>>>>     5 files changed, 158 insertions(+)
>>>>>>     create mode 100644 include/hw/s390x/cpu-topology.h
>>>>>>     create mode 100644 hw/s390x/cpu-topology.c
>>>>>
> [...]
>>>>>>     
>>>>>> +static DeviceState *s390_init_topology(MachineState *machine, Error **errp)
>>>>>> +{
>>>>>> +    DeviceState *dev;
>>>>>> +
>>>>>> +    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
>>>>>> +
>>>>>> +    object_property_add_child(&machine->parent_obj,
>>>>>> +                              TYPE_S390_CPU_TOPOLOGY, OBJECT(dev));
>>>>>
>>>>> Why set this property, and why on the machine parent?
>>>>
>>>> For what I understood setting the num_cores and num_sockets as
>>>> properties of the CPU Topology object allows to have them better
>>>> integrated in the QEMU object framework.
>>>
>>> That I understand.
>>>>
>>>> The topology is added to the S390CcwmachineState, it is the parent of
>>>> the machine.
>>>
>>> But why? And is it added to the S390CcwMachineState, or its parent?
>>
>> it is added to the S390CcwMachineState.
>> We receive the MachineState as the "machine" parameter here and it is
>> added to the "machine->parent_obj" which is the S390CcwMachineState.
> 
> Oh, I was confused. &machine->parent_obj is just a cast of MachineState* to Object*.
> It's the very same object.
> And what is the reason to add the topology as child property?
> Just so it shows up in the qtree? Wouldn't it anyway under the sysbus?

Yes it would appear on the info qtree but not in the qom-tree


>>
>>
>>
>>>>
>>>>
>>>>>
>>>>>> +    object_property_set_int(OBJECT(dev), "num-cores",
>>>>>> +                            machine->smp.cores * machine->smp.threads, errp);
>>>>>> +    object_property_set_int(OBJECT(dev), "num-sockets",
>>>>>> +                            machine->smp.sockets, errp);
>>>>>> +
>>>>>> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);
>>>>>
>>>>> I must admit that I haven't fully grokked qemu's memory management yet.
>>>>> Is the topology devices now owned by the sysbus?
>>>>
>>>> Yes it is so we see it on the qtree with its properties.
>>>>
>>>>
>>>>> If so, is it fine to have a pointer to it S390CcwMachineState?
>>>>
>>>> Why not?
>>>
>>> If it's owned by the sysbus and the object is not explicitly referenced
>>> for the pointer, it might be deallocated and then you'd have a dangling pointer.
>>
>> Why would it be deallocated ?
> 
> That's beside the point, if you transfer ownership, you have no control over when
> the deallocation happens.
> It's going to be fine in practice, but I don't think you should rely on it.
> I think you could just do sysbus_realize instead of ..._and_unref,
> but like I said, I haven't fully understood qemu memory management.
> (It would also leak in a sense, but since the machine exists forever that should be fine)

If I understand correctly:

- qdev_new adds a reference count to the new created object, dev.

- object_property_add_child adds a reference count to the child also 
here the new created device dev so the ref count of dev is 2 .

after the unref on dev, the ref count of dev get down to 1

then it seems OK. Did I miss something?

Regards,
Pierre

> 
>> as long it is not unrealized it belongs to the sysbus doesn't it?
>>
>> Regards,
>> Pierre
>>
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v12 1/7] s390x/cpu topology: Creating CPU topology device

[Janis Schoetterl-Glausch]

 * On Wed, 2022-12-07 at 11:00 +0100, Pierre Morel wrote:
> 
> On 12/6/22 22:06, Janis Schoetterl-Glausch wrote:
> > On Tue, 2022-12-06 at 15:35 +0100, Pierre Morel wrote:
> > > 
> > > On 12/6/22 14:35, Janis Schoetterl-Glausch wrote:
> > > > On Tue, 2022-12-06 at 11:32 +0100, Pierre Morel wrote:
> > > > > 
> > > > > On 12/6/22 10:31, Janis Schoetterl-Glausch wrote:
> > > > > > On Tue, 2022-11-29 at 18:42 +0100, Pierre Morel wrote:
> > > > > > > We will need a Topology device to transfer the topology
> > > > > > > during migration and to implement machine reset.
> > > > > > > 
> > > > > > > The device creation is fenced by s390_has_topology().
> > > > > > > 
> > > > > > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > > > > > > ---
> > > > > > >  include/hw/s390x/cpu-topology.h | 44 +++++++++++++++
> > > > > > >  include/hw/s390x/s390-virtio-ccw.h | 1 +
> > > > > > >  hw/s390x/cpu-topology.c | 87 ++++++++++++++++++++++++++++++
> > > > > > >  hw/s390x/s390-virtio-ccw.c | 25 +++++++++
> > > > > > >  hw/s390x/meson.build | 1 +
> > > > > > >  5 files changed, 158 insertions(+)
> > > > > > >  create mode 100644 include/hw/s390x/cpu-topology.h
> > > > > > >  create mode 100644 hw/s390x/cpu-topology.c
> > > > > > 
[...]

> > > > > > > + object_property_set_int(OBJECT(dev), "num-cores",
> > > > > > > + machine->smp.cores * machine->smp.threads, errp);
> > > > > > > + object_property_set_int(OBJECT(dev), "num-sockets",
> > > > > > > + machine->smp.sockets, errp);
> > > > > > > +
> > > > > > > + sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);
> > > > > > 
> > > > > > I must admit that I haven't fully grokked qemu's memory management yet.
> > > > > > Is the topology devices now owned by the sysbus?
> > > > > 
> > > > > Yes it is so we see it on the qtree with its properties.
> > > > > 
> > > > > 
> > > > > > If so, is it fine to have a pointer to it S390CcwMachineState?
> > > > > 
> > > > > Why not?
> > > > 
> > > > If it's owned by the sysbus and the object is not explicitly referenced
> > > > for the pointer, it might be deallocated and then you'd have a dangling pointer.
> > > 
> > > Why would it be deallocated ?
> > 
> > That's beside the point, if you transfer ownership, you have no control over when
> > the deallocation happens.
> > It's going to be fine in practice, but I don't think you should rely on it.
> > I think you could just do sysbus_realize instead of ..._and_unref,
> > but like I said, I haven't fully understood qemu memory management.
> > (It would also leak in a sense, but since the machine exists forever that should be fine)
> 
> If I understand correctly:
> 
> - qdev_new adds a reference count to the new created object, dev.
> 
> - object_property_add_child adds a reference count to the child also 
> here the new created device dev so the ref count of dev is 2 .
> 
> after the unref on dev, the ref count of dev get down to 1
> 
> then it seems OK. Did I miss something?

The properties ref belongs to the property, if the property were removed,
it would be unref'ed. There is no extra ref for the pointer in S390CcwMachineState.
I'm coming from a clean code perspective, I don't think we'd run into this problem in practice.
> 
> Regards,
> Pierre
> 
> > 
> > > as long it is not unrealized it belongs to the sysbus doesn't it?
> > > 
> > > Regards,
> > > Pierre
> > > 
> > 
> 

Re: [PATCH v12 1/7] s390x/cpu topology: Creating CPU topology device

[Pierre Morel]

On 12/7/22 12:38, Janis Schoetterl-Glausch wrote:
>   * On Wed, 2022-12-07 at 11:00 +0100, Pierre Morel wrote:
>>
>> On 12/6/22 22:06, Janis Schoetterl-Glausch wrote:
>>> On Tue, 2022-12-06 at 15:35 +0100, Pierre Morel wrote:
>>>>
>>>> On 12/6/22 14:35, Janis Schoetterl-Glausch wrote:
>>>>> On Tue, 2022-12-06 at 11:32 +0100, Pierre Morel wrote:
>>>>>>
>>>>>> On 12/6/22 10:31, Janis Schoetterl-Glausch wrote:
>>>>>>> On Tue, 2022-11-29 at 18:42 +0100, Pierre Morel wrote:
>>>>>>>> We will need a Topology device to transfer the topology
>>>>>>>> during migration and to implement machine reset.
>>>>>>>>
>>>>>>>> The device creation is fenced by s390_has_topology().
>>>>>>>>
>>>>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>>>>>> ---
>>>>>>>>   include/hw/s390x/cpu-topology.h | 44 +++++++++++++++
>>>>>>>>   include/hw/s390x/s390-virtio-ccw.h | 1 +
>>>>>>>>   hw/s390x/cpu-topology.c | 87 ++++++++++++++++++++++++++++++
>>>>>>>>   hw/s390x/s390-virtio-ccw.c | 25 +++++++++
>>>>>>>>   hw/s390x/meson.build | 1 +
>>>>>>>>   5 files changed, 158 insertions(+)
>>>>>>>>   create mode 100644 include/hw/s390x/cpu-topology.h
>>>>>>>>   create mode 100644 hw/s390x/cpu-topology.c
>>>>>>>
> [...]
> 
>>>>>>>> + object_property_set_int(OBJECT(dev), "num-cores",
>>>>>>>> + machine->smp.cores * machine->smp.threads, errp);
>>>>>>>> + object_property_set_int(OBJECT(dev), "num-sockets",
>>>>>>>> + machine->smp.sockets, errp);
>>>>>>>> +
>>>>>>>> + sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);
>>>>>>>
>>>>>>> I must admit that I haven't fully grokked qemu's memory management yet.
>>>>>>> Is the topology devices now owned by the sysbus?
>>>>>>
>>>>>> Yes it is so we see it on the qtree with its properties.
>>>>>>
>>>>>>
>>>>>>> If so, is it fine to have a pointer to it S390CcwMachineState?
>>>>>>
>>>>>> Why not?
>>>>>
>>>>> If it's owned by the sysbus and the object is not explicitly referenced
>>>>> for the pointer, it might be deallocated and then you'd have a dangling pointer.
>>>>
>>>> Why would it be deallocated ?
>>>
>>> That's beside the point, if you transfer ownership, you have no control over when
>>> the deallocation happens.
>>> It's going to be fine in practice, but I don't think you should rely on it.
>>> I think you could just do sysbus_realize instead of ..._and_unref,
>>> but like I said, I haven't fully understood qemu memory management.
>>> (It would also leak in a sense, but since the machine exists forever that should be fine)
>>
>> If I understand correctly:
>>
>> - qdev_new adds a reference count to the new created object, dev.
>>
>> - object_property_add_child adds a reference count to the child also
>> here the new created device dev so the ref count of dev is 2 .
>>
>> after the unref on dev, the ref count of dev get down to 1
>>
>> then it seems OK. Did I miss something?
> 
> The properties ref belongs to the property, if the property were removed,
> it would be unref'ed. There is no extra ref for the pointer in S390CcwMachineState.
> I'm coming from a clean code perspective, I don't think we'd run into this problem in practice.

OK, I understand, you are right.
My original code used object_resolve_path() to retrieve the object what 
made things cleaner I think.

For performance reason, Cedric proposed during the review of V10 to add 
the pointer to the machine state instead.

I must say that I am not very comfortable to argument on this.
@Cedric what do you think?


Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen