[PATCH v13 0/7] s390x: CPU Topology

[PATCH v13 0/7] s390x: CPU Topology

[Pierre Morel]

Hi,

Implementation discussions
==========================

CPU models
----------

Since the S390_FEAT_CONFIGURATION_TOPOLOGY is already in the CPU model
for old QEMU we could not activate it as usual from KVM but needed
a KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
Checking and enabling this capability enables
S390_FEAT_CONFIGURATION_TOPOLOGY.

Migration
---------

Once the S390_FEAT_CONFIGURATION_TOPOLOGY is enabled in the source
host the STFL(11) is provided to the guest.
Since the feature is already in the CPU model of older QEMU,
a migration from a new QEMU enabling the topology to an old QEMU
will keep STFL(11) enabled making the guest get an exception for
illegal operation as soon as it uses the PTF instruction.

A VMState keeping track of the S390_FEAT_CONFIGURATION_TOPOLOGY
allows to forbid the migration in such a case.

Note that the VMState will be used to hold information on the
topology once we implement topology change for a running guest. 

Topology
--------

Until we introduce bookss and drawers, polarization and dedication
the topology is kept very simple and is specified uniquely by
the core_id of the vCPU which is also the vCPU address.

Testing
=======

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.

Documentation
=============

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.

Future developments
===================

Two series are actively prepared:
- Adding drawers, book, polarization and dedication to the vCPU.
- changing the topology with a running guest

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 |  87 ++++++++++
 docs/system/target-s390x.rst       |   1 +
 include/hw/s390x/cpu-topology.h    |  52 ++++++
 include/hw/s390x/s390-virtio-ccw.h |   6 +
 target/s390x/cpu.h                 |  78 +++++++++
 target/s390x/kvm/kvm_s390x.h       |   1 +
 hw/s390x/cpu-topology.c            | 261 +++++++++++++++++++++++++++++
 hw/s390x/s390-virtio-ccw.c         |   7 +
 target/s390x/cpu-sysemu.c          |  21 +++
 target/s390x/cpu_models.c          |   1 +
 target/s390x/kvm/cpu_topology.c    | 186 ++++++++++++++++++++
 target/s390x/kvm/kvm.c             |  46 ++++-
 hw/s390x/meson.build               |   1 +
 target/s390x/kvm/meson.build       |   3 +-
 14 files changed, 749 insertions(+), 2 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/kvm/cpu_topology.c

-- 
2.31.1

- since v12

- suppress new CPU flag "disable-topology" just use ctop

- no use of special fields in CCW machine or in CPU

- modifications in documentation

- insert documentation in tree
  (Cedric)

- moved cpu-topology.c from target/s390 to target/s390/kvm
  to compile smoothly (without topology) for TCG
  (Cedric)

- 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 v13 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 ++++++++++
 hw/s390x/cpu-topology.c         | 149 ++++++++++++++++++++++++++++++++
 hw/s390x/s390-virtio-ccw.c      |   6 ++
 hw/s390x/meson.build            |   1 +
 4 files changed, 200 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..6c3d2d080f
--- /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/sysbus.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)
+
+void s390_init_topology(MachineState *machine, Error **errp);
+bool s390_has_topology(void);
+S390Topology *s390_get_topology(void);
+
+#endif
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
new file mode 100644
index 0000000000..b3e59873f6
--- /dev/null
+++ b/hw/s390x/cpu-topology.c
@@ -0,0 +1,149 @@
+/*
+ * 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/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_has_topology
+ *
+ * Return false until the commit activating the topology is
+ * commited.
+ */
+bool s390_has_topology(void)
+{
+    return false;
+}
+
+/**
+ * s390_get_topology
+ *
+ * Returns a pointer to the topology.
+ *
+ * This function is called when we know the topology exist.
+ * Testing if the topology exist is done with s390_has_topology()
+ */
+S390Topology *s390_get_topology(void)
+{
+    static S390Topology *s390Topology;
+
+    if (!s390Topology) {
+        s390Topology = S390_CPU_TOPOLOGY(
+            object_resolve_path(TYPE_S390_CPU_TOPOLOGY, NULL));
+    }
+
+    assert(s390Topology);
+
+    return s390Topology;
+}
+
+/**
+ * s390_init_topology
+ * @machine: The Machine state, used to retrieve the SMP parameters
+ * @errp: the error pointer in case of problem
+ *
+ * This function creates and initialize the S390Topology with
+ * the QEMU -smp parameters we will use during adding cores to the
+ * topology.
+ */
+void s390_init_topology(MachineState *machine, Error **errp)
+{
+    DeviceState *dev;
+
+    if (machine->smp.threads > 1) {
+        error_setg(errp, "CPU Topology do not support multithreading");
+        return;
+    }
+
+    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, errp);
+    object_property_set_int(OBJECT(dev), "num-sockets",
+                            machine->smp.sockets, errp);
+
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);
+}
+
+/**
+ * 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..8971ffb871 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;
 
@@ -255,6 +256,11 @@ 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_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..58dfbdff4f 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -24,6 +24,7 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
   's390-stattrib-kvm.c',
   'pv.c',
   's390-pci-kvm.c',
+  'cpu-topology.c',
 ))
 s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
   'tod-tcg.c',
-- 
2.31.1

[PATCH v13 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              |  76 +++++++++++++
 target/s390x/kvm/cpu_topology.c | 186 ++++++++++++++++++++++++++++++++
 target/s390x/kvm/kvm.c          |   6 +-
 target/s390x/kvm/meson.build    |   3 +-
 4 files changed, 269 insertions(+), 2 deletions(-)
 create mode 100644 target/s390x/kvm/cpu_topology.c

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7d6d01325b..729ace321a 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -565,6 +565,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[];
+} 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 +917,6 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
 
 #include "exec/cpu-all.h"
 
+void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar);
+
 #endif
diff --git a/target/s390x/kvm/cpu_topology.c b/target/s390x/kvm/cpu_topology.c
new file mode 100644
index 0000000000..1d6fd4394b
--- /dev/null
+++ b/target/s390x/kvm/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 arguments.
+ * 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;
+
+    socket_id = core_id / topo->num_cores;
+/*
+ * At the core level, each CPU is represented by a bit in a 64bit
+ * unsigned long.
+ * The architecture specifies that all CPU in a CPU TLE have the same
+ * type, polarization and are all dedicated or shared.
+ * In the case that a socket contains CPUs with different type, polarization
+ * or entitlement then they will be defined in different CPU containers.
+ * Currently we assume all CPU are identical IFL CPUs and that they are
+ * all dedicated CPUs with horizontal polarization.
+ * Therefore, the only reason to have several CPU TLE inside a socket is
+ * to support CPU id differences > 64.
+ * In that case, the origin field in a container represents the offset of
+ * the first CPU in that CPU container, thereby allowing representation
+ * of all CPUs via multiple containers.
+ */
+    bit = core_id;
+    origin = bit / 64;
+    bit %= 64;
+    bit = 63 - bit;
+
+    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 does not change
+ * even on migration.
+ */
+static void s390_prepare_topology(S390CcwMachineState *s390ms)
+{
+    const MachineState *ms = MACHINE(s390ms);
+    S390Topology *topo = s390_get_topology();
+    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(topo,
+                                  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 = s390_get_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;
+}
+
+#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(current_machine));
+
+    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/kvm/meson.build b/target/s390x/kvm/meson.build
index aef52b6686..5daa5c6033 100644
--- a/target/s390x/kvm/meson.build
+++ b/target/s390x/kvm/meson.build
@@ -1,6 +1,7 @@
 
 s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
-  'kvm.c'
+  'kvm.c',
+  'cpu_topology.c'
 ), if_false: files(
   'stubs.c'
 ))
-- 
2.31.1

[PATCH v13 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 729ace321a..bc1a7de932 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -853,6 +853,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 b3e59873f6..f54afcf550 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -118,6 +118,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
@@ -133,6 +144,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 8971ffb871..f03a5aaeea 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -109,6 +109,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 v13 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 bc1a7de932..284c708a6c 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -854,6 +854,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 f54afcf550..8a2fe041d4 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -18,6 +18,7 @@
 #include "target/s390x/cpu.h"
 #include "hw/s390x/s390-virtio-ccw.h"
 #include "hw/s390x/cpu-topology.h"
+#include "migration/vmstate.h"
 
 /**
  * s390_has_topology
@@ -129,6 +130,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
@@ -145,6 +193,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 v13 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 9bba21a916..c1d46e78af 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -30,6 +30,12 @@ struct S390CcwMachineState {
     uint8_t loadparm[8];
 };
 
+#define S390_PTF_REASON_NONE (0x00 << 8)
+#define S390_PTF_REASON_DONE (0x01 << 8)
+#define S390_PTF_REASON_BUSY (0x02 << 8)
+#define S390_TOPO_FC_MASK 0xffUL
+void s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra);
+
 struct S390CcwMachineClass {
     /*< private >*/
     MachineClass parent_class;
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index 8a2fe041d4..a2a553e362 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -19,6 +19,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_has_topology
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 v13 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 disabled by default and fenced for SE
(secure execution).

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 include/hw/s390x/cpu-topology.h | 10 +++++++++-
 hw/s390x/cpu-topology.c         |  5 ++---
 target/s390x/cpu_models.c       |  1 +
 target/s390x/kvm/kvm.c          | 12 ++++++++++++
 4 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
index 6c3d2d080f..fe25a3bf6b 100644
--- a/include/hw/s390x/cpu-topology.h
+++ b/include/hw/s390x/cpu-topology.h
@@ -38,7 +38,15 @@ struct S390Topology {
 OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY)
 
 void s390_init_topology(MachineState *machine, Error **errp);
-bool s390_has_topology(void);
 S390Topology *s390_get_topology(void);
 
+#ifdef CONFIG_KVM
+bool s390_has_topology(void);
+#else
+static inline bool s390_has_topology(void)
+{
+    return false;
+}
+#endif
+
 #endif
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index a2a553e362..bf125bb4c0 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -75,12 +75,11 @@ void s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra)
 /**
  * s390_has_topology
  *
- * Return false until the commit activating the topology is
- * commited.
+ * checks the S390_FEAT_CONFIGURATION_TOPOLOGY availability.
  */
 bool s390_has_topology(void)
 {
-    return false;
+    return s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY);
 }
 
 /**
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index c3a4f80633..3f05e05fd3 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:
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index a79fdf1c79..abf1202c28 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -2471,6 +2471,18 @@ 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 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);
+    }
+
     /* 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 v13 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 | 87 ++++++++++++++++++++++++++++++
 docs/system/target-s390x.rst       |  1 +
 2 files changed, 88 insertions(+)
 create mode 100644 docs/system/s390x/cpu-topology.rst

diff --git a/docs/system/s390x/cpu-topology.rst b/docs/system/s390x/cpu-topology.rst
new file mode 100644
index 0000000000..12f974cc54
--- /dev/null
+++ b/docs/system/s390x/cpu-topology.rst
@@ -0,0 +1,87 @@
+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.
+
+Enabling CPU topology
+---------------------
+
+Currently, CPU topology is only enabled in the host model.
+
+Enabling CPU topology in a CPU model is done by setting the CPU flag
+``ctop`` to ``on`` like in:
+
+.. code-block:: bash
+
+   -cpu gen16b,ctop=on
+
+Having the topology disabled by default allows migration between
+old and new QEMU without adding new flags.
+
+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.
+
+In the following machine we define 8 sockets with 4 cores each.
+Note that S390 QEMU machines do not implement multithreading.
+
+.. code-block:: bash
+
+  $ qemu-system-s390x -m 2G \
+    -cpu gen16b,ctop=on \
+    -smp cpus=5,sockets=8,cores=4,maxcpus=32 \
+    -device host-s390x-cpu,core-id=14 \
+
+New CPUs can be plugged using the device_add hmp command like in:
+
+.. code-block:: bash
+
+  (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 4 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 writen in a big endian mask:
+
+* in socket 0: 0xf0000000 (core id 0,1,2,3)
+* in socket 1: 0x08000000 (core id 4)
+* in socket 2: 0x00400000 (core id 9)
+* in socket 3: 0x00020000 (core id 14)
diff --git a/docs/system/target-s390x.rst b/docs/system/target-s390x.rst
index c636f64113..ff0ffe04f3 100644
--- a/docs/system/target-s390x.rst
+++ b/docs/system/target-s390x.rst
@@ -33,3 +33,4 @@ Architectural features
 .. toctree::
    s390x/bootdevices
    s390x/protvirt
+   s390x/cpu-topology
-- 
2.31.1

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

[Thomas Huth]

On 08/12/2022 10.44, Pierre Morel wrote:
> Hi,
> 
> Implementation discussions
> ==========================
> 
> CPU models
> ----------
> 
> Since the S390_FEAT_CONFIGURATION_TOPOLOGY is already in the CPU model
> for old QEMU we could not activate it as usual from KVM but needed
> a KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
> Checking and enabling this capability enables
> S390_FEAT_CONFIGURATION_TOPOLOGY.
> 
> Migration
> ---------
> 
> Once the S390_FEAT_CONFIGURATION_TOPOLOGY is enabled in the source
> host the STFL(11) is provided to the guest.
> Since the feature is already in the CPU model of older QEMU,
> a migration from a new QEMU enabling the topology to an old QEMU
> will keep STFL(11) enabled making the guest get an exception for
> illegal operation as soon as it uses the PTF instruction.

I now thought that it is not possible to enable "ctop" on older QEMUs since 
the don't enable the KVM capability? ... or is it still somehow possible? 
What did I miss?

  Thomas


> A VMState keeping track of the S390_FEAT_CONFIGURATION_TOPOLOGY
> allows to forbid the migration in such a case.
> 
> Note that the VMState will be used to hold information on the
> topology once we implement topology change for a running guest.

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

[Thomas Huth]

On 08/12/2022 10.44, 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/cpu-topology.c b/hw/s390x/cpu-topology.c
> new file mode 100644
> index 0000000000..b3e59873f6
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c
> @@ -0,0 +1,149 @@
> +/*
> + * 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/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_has_topology
> + *
> + * Return false until the commit activating the topology is
> + * commited.
> + */
> +bool s390_has_topology(void)
> +{
> +    return false;
> +}
> +
> +/**
> + * s390_get_topology
> + *
> + * Returns a pointer to the topology.
> + *
> + * This function is called when we know the topology exist.
> + * Testing if the topology exist is done with s390_has_topology()
> + */
> +S390Topology *s390_get_topology(void)
> +{
> +    static S390Topology *s390Topology;
> +
> +    if (!s390Topology) {
> +        s390Topology = S390_CPU_TOPOLOGY(
> +            object_resolve_path(TYPE_S390_CPU_TOPOLOGY, NULL));
> +    }
> +
> +    assert(s390Topology);

I think you can move the assert() into the body of the if-statement.

> +    return s390Topology;
> +}
> +
> +/**
> + * s390_init_topology
> + * @machine: The Machine state, used to retrieve the SMP parameters
> + * @errp: the error pointer in case of problem
> + *
> + * This function creates and initialize the S390Topology with
> + * the QEMU -smp parameters we will use during adding cores to the
> + * topology.
> + */
> +void s390_init_topology(MachineState *machine, Error **errp)
> +{
> +    DeviceState *dev;
> +
> +    if (machine->smp.threads > 1) {
> +        error_setg(errp, "CPU Topology do not support multithreading");

s/CPU Toplogy do/CPU topology does/

> +        return;
> +    }
> +
> +    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, errp);
> +    object_property_set_int(OBJECT(dev), "num-sockets",
> +                            machine->smp.sockets, errp);
> +
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);
> +}

  Thomas

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

[Cédric Le Goater]

On 12/8/22 10:44, Pierre Morel wrote:
> Hi,
> 
> Implementation discussions
> ==========================
> 
> CPU models
> ----------
> 
> Since the S390_FEAT_CONFIGURATION_TOPOLOGY is already in the CPU model
> for old QEMU we could not activate it as usual from KVM but needed
> a KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
> Checking and enabling this capability enables
> S390_FEAT_CONFIGURATION_TOPOLOGY.
> 
> Migration
> ---------
> 
> Once the S390_FEAT_CONFIGURATION_TOPOLOGY is enabled in the source
> host the STFL(11) is provided to the guest.
> Since the feature is already in the CPU model of older QEMU,
> a migration from a new QEMU enabling the topology to an old QEMU
> will keep STFL(11) enabled making the guest get an exception for
> illegal operation as soon as it uses the PTF instruction.
> 
> A VMState keeping track of the S390_FEAT_CONFIGURATION_TOPOLOGY
> allows to forbid the migration in such a case.
> 
> Note that the VMState will be used to hold information on the
> topology once we implement topology change for a running guest.
> 
> Topology
> --------
> 
> Until we introduce bookss and drawers, polarization and dedication
> the topology is kept very simple and is specified uniquely by
> the core_id of the vCPU which is also the vCPU address.
> 
> Testing
> =======
> 
> 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.
> 
> Documentation
> =============
> 
> 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.
> 
> Future developments
> ===================
> 
> Two series are actively prepared:
> - Adding drawers, book, polarization and dedication to the vCPU.
> - changing the topology with a running guest

Since we have time before the next QEMU 8.0 release, could you please
send the whole patchset ? Having the full picture would help in taking
decisions for downstream also.

I am still uncertain about the usefulness of S390Topology object because,
as for now, the state can be computed on the fly, the reset can be
handled at the machine level directly under s390_machine_reset() and so
could migration if the machine had a vmstate (not the case today but
quite easy to add). So before merging anything that could be difficult
to maintain and/or backport, I would prefer to see it all !

Thanks,

C.


> 
> 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 |  87 ++++++++++
>   docs/system/target-s390x.rst       |   1 +
>   include/hw/s390x/cpu-topology.h    |  52 ++++++
>   include/hw/s390x/s390-virtio-ccw.h |   6 +
>   target/s390x/cpu.h                 |  78 +++++++++
>   target/s390x/kvm/kvm_s390x.h       |   1 +
>   hw/s390x/cpu-topology.c            | 261 +++++++++++++++++++++++++++++
>   hw/s390x/s390-virtio-ccw.c         |   7 +
>   target/s390x/cpu-sysemu.c          |  21 +++
>   target/s390x/cpu_models.c          |   1 +
>   target/s390x/kvm/cpu_topology.c    | 186 ++++++++++++++++++++
>   target/s390x/kvm/kvm.c             |  46 ++++-
>   hw/s390x/meson.build               |   1 +
>   target/s390x/kvm/meson.build       |   3 +-
>   14 files changed, 749 insertions(+), 2 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/kvm/cpu_topology.c
> 

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

[Cédric Le Goater]

On 12/8/22 10:44, 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().

Some of the info you gave in the cover letter would help the reader
of this commit log.


> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   include/hw/s390x/cpu-topology.h |  44 ++++++++++
>   hw/s390x/cpu-topology.c         | 149 ++++++++++++++++++++++++++++++++
>   hw/s390x/s390-virtio-ccw.c      |   6 ++
>   hw/s390x/meson.build            |   1 +
>   4 files changed, 200 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..6c3d2d080f
> --- /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/sysbus.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;

hmm, I think a Device should be enough. There are no interrupts or memory
regions associated to it and the reset is handled independently of any bus.
This object is simply a place older for computed state.

> +    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)
> +
> +void s390_init_topology(MachineState *machine, Error **errp);
> +bool s390_has_topology(void);
> +S390Topology *s390_get_topology(void);
> +
> +#endif
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> new file mode 100644
> index 0000000000..b3e59873f6
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c
> @@ -0,0 +1,149 @@
> +/*
> + * 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/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_has_topology
> + *
> + * Return false until the commit activating the topology is
> + * commited.
> + */
> +bool s390_has_topology(void)
> +{
> +    return false;
> +}
> +
> +/**
> + * s390_get_topology
> + *
> + * Returns a pointer to the topology.
> + *
> + * This function is called when we know the topology exist.
> + * Testing if the topology exist is done with s390_has_topology()
> + */
> +S390Topology *s390_get_topology(void)
> +{
> +    static S390Topology *s390Topology;
> +
> +    if (!s390Topology) {
> +        s390Topology = S390_CPU_TOPOLOGY(
> +            object_resolve_path(TYPE_S390_CPU_TOPOLOGY, NULL));
> +    }

This is back. Too bad. I guess the compilation issues on all platforms
made it difficult to avoid.

> +    assert(s390Topology);
> +
> +    return s390Topology;
> +}
> +
> +/**
> + * s390_init_topology
> + * @machine: The Machine state, used to retrieve the SMP parameters
> + * @errp: the error pointer in case of problem
> + *
> + * This function creates and initialize the S390Topology with
> + * the QEMU -smp parameters we will use during adding cores to the
> + * topology.
> + */
> +void s390_init_topology(MachineState *machine, Error **errp)
> +{
> +    DeviceState *dev;
> +
> +    if (machine->smp.threads > 1) {
> +        error_setg(errp, "CPU Topology do not support multithreading");
> +        return;
> +    }

Isn't SMT already tested at the machine level ?

> +
> +    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, errp);
> +    object_property_set_int(OBJECT(dev), "num-sockets",
> +                            machine->smp.sockets, errp);
> +
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);
> +}
> +
> +/**
> + * 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..8971ffb871 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;
>   
> @@ -255,6 +256,11 @@ 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_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..58dfbdff4f 100644
> --- a/hw/s390x/meson.build
> +++ b/hw/s390x/meson.build
> @@ -24,6 +24,7 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
>     's390-stattrib-kvm.c',
>     'pv.c',
>     's390-pci-kvm.c',
> +  'cpu-topology.c',
>   ))
>   s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
>     'tod-tcg.c',

Re: [PATCH v13 4/7] s390x/cpu_topology: CPU topology migration

[Cédric Le Goater]

On 12/8/22 10:44, Pierre Morel wrote:
> The migration can only take place if both source and destination
> of the migration both use or both do not use the CPU topology
> facility.
> 
> We indicate a change in topology during migration postload for the
> case the topology changed between source and destination.
> 
> 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 bc1a7de932..284c708a6c 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -854,6 +854,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 f54afcf550..8a2fe041d4 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -18,6 +18,7 @@
>   #include "target/s390x/cpu.h"
>   #include "hw/s390x/s390-virtio-ccw.h"
>   #include "hw/s390x/cpu-topology.h"
> +#include "migration/vmstate.h"
>   
>   /**
>    * s390_has_topology
> @@ -129,6 +130,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,

Please use 'cpu_topology_post_load', it is easier to catch with a grep.

Thanks,

C.

> +    .minimum_version_id = 1,
> +    .needed = cpu_topology_needed,
> +};
> +
>   /**
>    * topology_class_init:
>    * @oc: Object class
> @@ -145,6 +193,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;
> +}

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

[Cédric Le Goater]

On 12/8/22 10:44, 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              |  76 +++++++++++++
>   target/s390x/kvm/cpu_topology.c | 186 ++++++++++++++++++++++++++++++++
>   target/s390x/kvm/kvm.c          |   6 +-
>   target/s390x/kvm/meson.build    |   3 +-
>   4 files changed, 269 insertions(+), 2 deletions(-)
>   create mode 100644 target/s390x/kvm/cpu_topology.c
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 7d6d01325b..729ace321a 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -565,6 +565,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[];
> +} 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 +917,6 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
>   
>   #include "exec/cpu-all.h"
>   
> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar);
> +
>   #endif
> diff --git a/target/s390x/kvm/cpu_topology.c b/target/s390x/kvm/cpu_topology.c
> new file mode 100644
> index 0000000000..1d6fd4394b
> --- /dev/null
> +++ b/target/s390x/kvm/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 arguments.
> + * 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;
> +
> +    socket_id = core_id / topo->num_cores;
> +/*
> + * At the core level, each CPU is represented by a bit in a 64bit
> + * unsigned long.
> + * The architecture specifies that all CPU in a CPU TLE have the same
> + * type, polarization and are all dedicated or shared.
> + * In the case that a socket contains CPUs with different type, polarization
> + * or entitlement then they will be defined in different CPU containers.
> + * Currently we assume all CPU are identical IFL CPUs and that they are
> + * all dedicated CPUs with horizontal polarization.
> + * Therefore, the only reason to have several CPU TLE inside a socket is
> + * to support CPU id differences > 64.
> + * In that case, the origin field in a container represents the offset of
> + * the first CPU in that CPU container, thereby allowing representation
> + * of all CPUs via multiple containers.
> + */
> +    bit = core_id;
> +    origin = bit / 64;
> +    bit %= 64;
> +    bit = 63 - bit;
> +
> +    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 does not change
> + * even on migration.
> + */
> +static void s390_prepare_topology(S390CcwMachineState *s390ms)
> +{
> +    const MachineState *ms = MACHINE(s390ms);
> +    S390Topology *topo = s390_get_topology();
> +    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(topo,
> +                                  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 = s390_get_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;
> +}
> +
> +#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() ||

Isn't the test s390_is_pv() redundant since patch 6 deactivates
S390_FEAT_CONFIGURATION_TOPOLOGY for PV guests ?


> +        sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
> +        setcc(cpu, 3);
> +        return;
> +    }
> +
> +    s390_prepare_topology(S390_CCW_MACHINE(current_machine));
> +
> +    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/kvm/meson.build b/target/s390x/kvm/meson.build
> index aef52b6686..5daa5c6033 100644
> --- a/target/s390x/kvm/meson.build
> +++ b/target/s390x/kvm/meson.build
> @@ -1,6 +1,7 @@
>   
>   s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
> -  'kvm.c'
> +  'kvm.c',
> +  'cpu_topology.c'
>   ), if_false: files(
>     'stubs.c'
>   ))

Re: [PATCH v13 4/7] s390x/cpu_topology: CPU topology migration

[Pierre Morel]

Sorry, I made an error in this patch that break the migration.

On 12/8/22 10:44, Pierre Morel wrote:

> +
> +const VMStateDescription vmstate_cpu_topology = {
> +    .name = "cpu_topology",
> +    .version_id = 1,
> +    .post_load = cpu_topology_postload,
> +    .minimum_version_id = 1,
> +    .needed = cpu_topology_needed,
> +};

Here having no VMStateField break the migration.
Since there will be a new spin I will change this in the next with 
something like this where I also add the saving of the mtcr before the 
migration to set it back after the migration.


+/**
+ * cpu_topology_post_load
+ * @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_post_load(void *opaque, int version_id)
+{
+    S390Topology *topo = opaque;
+    int ret;
+
+    /* Set the MTCR to the saved value */
+    ret = s390_cpu_topology_mtcr_set(topo->mtcr);
+    if (ret) {
+        error_report("Failed to set MTCR: %s", strerror(-ret));
+    }
+    return ret;
+}
+
+/**
+ * cpu_topology_pre_save:
+ * @opaque: The pointer to the S390Topology
+ *
+ * Save the usage of the CPU Topology in the VM State.
+ */
+static int cpu_topology_pre_save(void *opaque)
+{
+    S390Topology *topo = opaque;
+
+    return  s390_cpu_topology_mtcr_get(&topo->mtcr);
+}
+
+/**
+ * cpu_topology_needed:
+ * @opaque: The pointer to the S390Topology
+ *
+ * We always need to know if source and destination use the topology.
+ */
+static bool cpu_topology_needed(void *opaque)
+{
+    return true;
+}
+
+const VMStateDescription vmstate_cpu_topology = {
+    .name = "cpu_topology",
+    .version_id = 1,
+    .post_load = cpu_topology_post_load,
+    .pre_save = cpu_topology_pre_save,
+    .minimum_version_id = 1,
+    .needed = cpu_topology_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(mtcr, S390Topology),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 12/9/22 14:32, Thomas Huth wrote:
> On 08/12/2022 10.44, Pierre Morel wrote:
>> Hi,
>>
>> Implementation discussions
>> ==========================
>>
>> CPU models
>> ----------
>>
>> Since the S390_FEAT_CONFIGURATION_TOPOLOGY is already in the CPU model
>> for old QEMU we could not activate it as usual from KVM but needed
>> a KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
>> Checking and enabling this capability enables
>> S390_FEAT_CONFIGURATION_TOPOLOGY.
>>
>> Migration
>> ---------
>>
>> Once the S390_FEAT_CONFIGURATION_TOPOLOGY is enabled in the source
>> host the STFL(11) is provided to the guest.
>> Since the feature is already in the CPU model of older QEMU,
>> a migration from a new QEMU enabling the topology to an old QEMU
>> will keep STFL(11) enabled making the guest get an exception for
>> illegal operation as soon as it uses the PTF instruction.
> 
> I now thought that it is not possible to enable "ctop" on older QEMUs 
> since the don't enable the KVM capability? ... or is it still somehow 
> possible? What did I miss?
> 
>   Thomas

Enabling ctop with ctop=on on old QEMU is not possible, this is right.
But, if STFL(11) is enable in the source KVM by a new QEMU, I can see 
that even with -ctop=off the STFL(11) is migrated to the destination.

It is highly possible that I missed something in the cpu model.

A solution proposed by Cedric was to add a new machine but we did not 
want this because we decided that we do not want to wait for a new machine.

Another solution could be to have a we can have a new CPU feature 
overruling ctop like S390_FEAT_CPU_TOPOLOGY in the last series version 12.
I am not sure it must be linked with the creation of a new machine.

The solution here in this series is to add a VMState which will block 
the migration with older QEMU if the topology is activated with ctop on 
a new QEMU.

Regards,
Pierre

> 
> 
>> A VMState keeping track of the S390_FEAT_CONFIGURATION_TOPOLOGY
>> allows to forbid the migration in such a case.
>>
>> Note that the VMState will be used to hold information on the
>> topology once we implement topology change for a running guest.
> 

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 12/9/22 14:50, Thomas Huth wrote:
> On 08/12/2022 10.44, 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/cpu-topology.c b/hw/s390x/cpu-topology.c
>> new file mode 100644
>> index 0000000000..b3e59873f6
>> --- /dev/null
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -0,0 +1,149 @@
>> +/*
>> + * 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/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_has_topology
>> + *
>> + * Return false until the commit activating the topology is
>> + * commited.
>> + */
>> +bool s390_has_topology(void)
>> +{
>> +    return false;
>> +}
>> +
>> +/**
>> + * s390_get_topology
>> + *
>> + * Returns a pointer to the topology.
>> + *
>> + * This function is called when we know the topology exist.
>> + * Testing if the topology exist is done with s390_has_topology()
>> + */
>> +S390Topology *s390_get_topology(void)
>> +{
>> +    static S390Topology *s390Topology;
>> +
>> +    if (!s390Topology) {
>> +        s390Topology = S390_CPU_TOPOLOGY(
>> +            object_resolve_path(TYPE_S390_CPU_TOPOLOGY, NULL));
>> +    }
>> +
>> +    assert(s390Topology);
> 
> I think you can move the assert() into the body of the if-statement.

Yes, clearly.

> 
>> +    return s390Topology;
>> +}
>> +
>> +/**
>> + * s390_init_topology
>> + * @machine: The Machine state, used to retrieve the SMP parameters
>> + * @errp: the error pointer in case of problem
>> + *
>> + * This function creates and initialize the S390Topology with
>> + * the QEMU -smp parameters we will use during adding cores to the
>> + * topology.
>> + */
>> +void s390_init_topology(MachineState *machine, Error **errp)
>> +{
>> +    DeviceState *dev;
>> +
>> +    if (machine->smp.threads > 1) {
>> +        error_setg(errp, "CPU Topology do not support multithreading");
> 
> s/CPU Toplogy do/CPU topology does/

Yes, thanks.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Thomas Huth]

On 12/12/2022 09.51, Pierre Morel wrote:
> 
> 
> On 12/9/22 14:32, Thomas Huth wrote:
>> On 08/12/2022 10.44, Pierre Morel wrote:
>>> Hi,
>>>
>>> Implementation discussions
>>> ==========================
>>>
>>> CPU models
>>> ----------
>>>
>>> Since the S390_FEAT_CONFIGURATION_TOPOLOGY is already in the CPU model
>>> for old QEMU we could not activate it as usual from KVM but needed
>>> a KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
>>> Checking and enabling this capability enables
>>> S390_FEAT_CONFIGURATION_TOPOLOGY.
>>>
>>> Migration
>>> ---------
>>>
>>> Once the S390_FEAT_CONFIGURATION_TOPOLOGY is enabled in the source
>>> host the STFL(11) is provided to the guest.
>>> Since the feature is already in the CPU model of older QEMU,
>>> a migration from a new QEMU enabling the topology to an old QEMU
>>> will keep STFL(11) enabled making the guest get an exception for
>>> illegal operation as soon as it uses the PTF instruction.
>>
>> I now thought that it is not possible to enable "ctop" on older QEMUs 
>> since the don't enable the KVM capability? ... or is it still somehow 
>> possible? What did I miss?
>>
>>   Thomas
> 
> Enabling ctop with ctop=on on old QEMU is not possible, this is right.
> But, if STFL(11) is enable in the source KVM by a new QEMU, I can see that 
> even with -ctop=off the STFL(11) is migrated to the destination.

Is this with the "host" CPU model or another one? And did you explicitly 
specify "ctop=off" at the command line, or are you just using the default 
setting by not specifying it?

  Thomas

Re: [PATCH v13 4/7] s390x/cpu_topology: CPU topology migration

[Pierre Morel]

On 12/9/22 15:56, Cédric Le Goater wrote:
> On 12/8/22 10:44, Pierre Morel wrote:
>> The migration can only take place if both source and destination
>> of the migration both use or both do not use the CPU topology
>> facility.
>>
>> We indicate a change in topology during migration postload for the
>> case the topology changed between source and destination.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>


>> +
>> +const VMStateDescription vmstate_cpu_topology = {
>> +    .name = "cpu_topology",
>> +    .version_id = 1,
>> +    .post_load = cpu_topology_postload,
> 
> Please use 'cpu_topology_post_load', it is easier to catch with a grep.
> 

OK

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 12/9/22 16:43, Cédric Le Goater wrote:
> On 12/8/22 10:44, 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>
>> ---

...

>> +#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() ||
> 
> Isn't the test s390_is_pv() redundant since patch 6 deactivates
> S390_FEAT_CONFIGURATION_TOPOLOGY for PV guests ?

Yes you are right it is.
I remove it.

Regards,
Pierre



-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 12/9/22 15:45, Cédric Le Goater wrote:
> On 12/8/22 10:44, Pierre Morel wrote:
>> Hi,
>>
>> Implementation discussions
>> ==========================
>>
>> CPU models
>> ----------
>>
>> Since the S390_FEAT_CONFIGURATION_TOPOLOGY is already in the CPU model
>> for old QEMU we could not activate it as usual from KVM but needed
>> a KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
>> Checking and enabling this capability enables
>> S390_FEAT_CONFIGURATION_TOPOLOGY.
>>
>> Migration
>> ---------
>>
>> Once the S390_FEAT_CONFIGURATION_TOPOLOGY is enabled in the source
>> host the STFL(11) is provided to the guest.
>> Since the feature is already in the CPU model of older QEMU,
>> a migration from a new QEMU enabling the topology to an old QEMU
>> will keep STFL(11) enabled making the guest get an exception for
>> illegal operation as soon as it uses the PTF instruction.
>>
>> A VMState keeping track of the S390_FEAT_CONFIGURATION_TOPOLOGY
>> allows to forbid the migration in such a case.
>>
>> Note that the VMState will be used to hold information on the
>> topology once we implement topology change for a running guest.
>>
>> Topology
>> --------
>>
>> Until we introduce bookss and drawers, polarization and dedication
>> the topology is kept very simple and is specified uniquely by
>> the core_id of the vCPU which is also the vCPU address.
>>
>> Testing
>> =======
>>
>> 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.
>>
>> Documentation
>> =============
>>
>> 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.
>>
>> Future developments
>> ===================
>>
>> Two series are actively prepared:
>> - Adding drawers, book, polarization and dedication to the vCPU.
>> - changing the topology with a running guest
> 
> Since we have time before the next QEMU 8.0 release, could you please
> send the whole patchset ? Having the full picture would help in taking
> decisions for downstream also.
> 
> I am still uncertain about the usefulness of S390Topology object because,
> as for now, the state can be computed on the fly, the reset can be
> handled at the machine level directly under s390_machine_reset() and so
> could migration if the machine had a vmstate (not the case today but
> quite easy to add). So before merging anything that could be difficult
> to maintain and/or backport, I would prefer to see it all !
> 

The goal of dedicating an object for topology was to ease the 
maintenance and portability by using the QEMU object framework.

If on contrary it is a problem for maintenance or backport we surely 
better not use it.

Any other opinion?

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 12/12/22 10:07, Thomas Huth wrote:
> On 12/12/2022 09.51, Pierre Morel wrote:
>>
>>
>> On 12/9/22 14:32, Thomas Huth wrote:
>>> On 08/12/2022 10.44, Pierre Morel wrote:
>>>> Hi,
>>>>
>>>> Implementation discussions
>>>> ==========================
>>>>
>>>> CPU models
>>>> ----------
>>>>
>>>> Since the S390_FEAT_CONFIGURATION_TOPOLOGY is already in the CPU model
>>>> for old QEMU we could not activate it as usual from KVM but needed
>>>> a KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
>>>> Checking and enabling this capability enables
>>>> S390_FEAT_CONFIGURATION_TOPOLOGY.
>>>>
>>>> Migration
>>>> ---------
>>>>
>>>> Once the S390_FEAT_CONFIGURATION_TOPOLOGY is enabled in the source
>>>> host the STFL(11) is provided to the guest.
>>>> Since the feature is already in the CPU model of older QEMU,
>>>> a migration from a new QEMU enabling the topology to an old QEMU
>>>> will keep STFL(11) enabled making the guest get an exception for
>>>> illegal operation as soon as it uses the PTF instruction.
>>>
>>> I now thought that it is not possible to enable "ctop" on older QEMUs 
>>> since the don't enable the KVM capability? ... or is it still somehow 
>>> possible? What did I miss?
>>>
>>>   Thomas
>>
>> Enabling ctop with ctop=on on old QEMU is not possible, this is right.
>> But, if STFL(11) is enable in the source KVM by a new QEMU, I can see 
>> that even with -ctop=off the STFL(11) is migrated to the destination.
> 
> Is this with the "host" CPU model or another one? And did you explicitly 
> specify "ctop=off" at the command line, or are you just using the 
> default setting by not specifying it?

With explicit cpumodel and using ctop=off like in

sudo /usr/local/bin/qemu-system-s390x_master \
      -m 512M \
      -enable-kvm -smp 4,sockets=4,cores=2,maxcpus=8 \
      -cpu z14,ctop=off \
      -machine s390-ccw-virtio-7.2,accel=kvm \
      ...


regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Thomas Huth]

On 12/12/2022 11.10, Pierre Morel wrote:
> 
> 
> On 12/12/22 10:07, Thomas Huth wrote:
>> On 12/12/2022 09.51, Pierre Morel wrote:
>>>
>>>
>>> On 12/9/22 14:32, Thomas Huth wrote:
>>>> On 08/12/2022 10.44, Pierre Morel wrote:
>>>>> Hi,
>>>>>
>>>>> Implementation discussions
>>>>> ==========================
>>>>>
>>>>> CPU models
>>>>> ----------
>>>>>
>>>>> Since the S390_FEAT_CONFIGURATION_TOPOLOGY is already in the CPU model
>>>>> for old QEMU we could not activate it as usual from KVM but needed
>>>>> a KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
>>>>> Checking and enabling this capability enables
>>>>> S390_FEAT_CONFIGURATION_TOPOLOGY.
>>>>>
>>>>> Migration
>>>>> ---------
>>>>>
>>>>> Once the S390_FEAT_CONFIGURATION_TOPOLOGY is enabled in the source
>>>>> host the STFL(11) is provided to the guest.
>>>>> Since the feature is already in the CPU model of older QEMU,
>>>>> a migration from a new QEMU enabling the topology to an old QEMU
>>>>> will keep STFL(11) enabled making the guest get an exception for
>>>>> illegal operation as soon as it uses the PTF instruction.
>>>>
>>>> I now thought that it is not possible to enable "ctop" on older QEMUs 
>>>> since the don't enable the KVM capability? ... or is it still somehow 
>>>> possible? What did I miss?
>>>>
>>>>   Thomas
>>>
>>> Enabling ctop with ctop=on on old QEMU is not possible, this is right.
>>> But, if STFL(11) is enable in the source KVM by a new QEMU, I can see 
>>> that even with -ctop=off the STFL(11) is migrated to the destination.
>>
>> Is this with the "host" CPU model or another one? And did you explicitly 
>> specify "ctop=off" at the command line, or are you just using the default 
>> setting by not specifying it?
> 
> With explicit cpumodel and using ctop=off like in
> 
> sudo /usr/local/bin/qemu-system-s390x_master \
>       -m 512M \
>       -enable-kvm -smp 4,sockets=4,cores=2,maxcpus=8 \
>       -cpu z14,ctop=off \
>       -machine s390-ccw-virtio-7.2,accel=kvm \
>       ...

Ok ... that sounds like a bug somewhere in your patches or in the kernel 
code ... the guest should never see STFL bit 11 if ctop=off, should it?

  Thomas

Re: [PATCH v13 4/7] s390x/cpu_topology: CPU topology migration

[Christian Borntraeger]

Am 08.12.22 um 10:44 schrieb 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.

I dont get why we need this? If the target QEMU has topology it should
already create this according to the configuration. WHy do we need a
trigger?

> 
> 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 bc1a7de932..284c708a6c 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -854,6 +854,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 f54afcf550..8a2fe041d4 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -18,6 +18,7 @@
>   #include "target/s390x/cpu.h"
>   #include "hw/s390x/s390-virtio-ccw.h"
>   #include "hw/s390x/cpu-topology.h"
> +#include "migration/vmstate.h"
>   
>   /**
>    * s390_has_topology
> @@ -129,6 +130,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
> @@ -145,6 +193,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;
> +}

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

[Christian Borntraeger]

Am 12.12.22 um 11:17 schrieb Thomas Huth:
> On 12/12/2022 11.10, Pierre Morel wrote:
>>
>>
>> On 12/12/22 10:07, Thomas Huth wrote:
>>> On 12/12/2022 09.51, Pierre Morel wrote:
>>>>
>>>>
>>>> On 12/9/22 14:32, Thomas Huth wrote:
>>>>> On 08/12/2022 10.44, Pierre Morel wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Implementation discussions
>>>>>> ==========================
>>>>>>
>>>>>> CPU models
>>>>>> ----------
>>>>>>
>>>>>> Since the S390_FEAT_CONFIGURATION_TOPOLOGY is already in the CPU model
>>>>>> for old QEMU we could not activate it as usual from KVM but needed
>>>>>> a KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
>>>>>> Checking and enabling this capability enables
>>>>>> S390_FEAT_CONFIGURATION_TOPOLOGY.
>>>>>>
>>>>>> Migration
>>>>>> ---------
>>>>>>
>>>>>> Once the S390_FEAT_CONFIGURATION_TOPOLOGY is enabled in the source
>>>>>> host the STFL(11) is provided to the guest.
>>>>>> Since the feature is already in the CPU model of older QEMU,
>>>>>> a migration from a new QEMU enabling the topology to an old QEMU
>>>>>> will keep STFL(11) enabled making the guest get an exception for
>>>>>> illegal operation as soon as it uses the PTF instruction.
>>>>>
>>>>> I now thought that it is not possible to enable "ctop" on older QEMUs since the don't enable the KVM capability? ... or is it still somehow possible? What did I miss?
>>>>>
>>>>>   Thomas
>>>>
>>>> Enabling ctop with ctop=on on old QEMU is not possible, this is right.
>>>> But, if STFL(11) is enable in the source KVM by a new QEMU, I can see that even with -ctop=off the STFL(11) is migrated to the destination.

This does not make sense. the cpu model and stfle values are not migrated. This is re-created during startup depending on the command line parameters of -cpu.
Thats why source and host have the same command lines for -cpu. And STFLE.11 must not be set on the SOURCE of ctop is off.


>>>
>>> Is this with the "host" CPU model or another one? And did you explicitly specify "ctop=off" at the command line, or are you just using the default setting by not specifying it?
>>
>> With explicit cpumodel and using ctop=off like in
>>
>> sudo /usr/local/bin/qemu-system-s390x_master \
>>       -m 512M \
>>       -enable-kvm -smp 4,sockets=4,cores=2,maxcpus=8 \
>>       -cpu z14,ctop=off \
>>       -machine s390-ccw-virtio-7.2,accel=kvm \
>>       ...
> 
> Ok ... that sounds like a bug somewhere in your patches or in the kernel code ... the guest should never see STFL bit 11 if ctop=off, should it?

Correct. If ctop=off then QEMU should disable STFLE.11 for the CPU model.

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

[Christian Borntraeger]

Am 12.12.22 um 11:01 schrieb Pierre Morel:
> 
> 
> On 12/9/22 15:45, Cédric Le Goater wrote:
>> On 12/8/22 10:44, Pierre Morel wrote:
>>> Hi,
>>>
>>> Implementation discussions
>>> ==========================
>>>
>>> CPU models
>>> ----------
>>>
>>> Since the S390_FEAT_CONFIGURATION_TOPOLOGY is already in the CPU model
>>> for old QEMU we could not activate it as usual from KVM but needed
>>> a KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
>>> Checking and enabling this capability enables
>>> S390_FEAT_CONFIGURATION_TOPOLOGY.
>>>
>>> Migration
>>> ---------
>>>
>>> Once the S390_FEAT_CONFIGURATION_TOPOLOGY is enabled in the source
>>> host the STFL(11) is provided to the guest.
>>> Since the feature is already in the CPU model of older QEMU,
>>> a migration from a new QEMU enabling the topology to an old QEMU
>>> will keep STFL(11) enabled making the guest get an exception for
>>> illegal operation as soon as it uses the PTF instruction.
>>>
>>> A VMState keeping track of the S390_FEAT_CONFIGURATION_TOPOLOGY
>>> allows to forbid the migration in such a case.
>>>
>>> Note that the VMState will be used to hold information on the
>>> topology once we implement topology change for a running guest.
>>>
>>> Topology
>>> --------
>>>
>>> Until we introduce bookss and drawers, polarization and dedication
>>> the topology is kept very simple and is specified uniquely by
>>> the core_id of the vCPU which is also the vCPU address.
>>>
>>> Testing
>>> =======
>>>
>>> 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.
>>>
>>> Documentation
>>> =============
>>>
>>> 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.
>>>
>>> Future developments
>>> ===================
>>>
>>> Two series are actively prepared:
>>> - Adding drawers, book, polarization and dedication to the vCPU.
>>> - changing the topology with a running guest
>>
>> Since we have time before the next QEMU 8.0 release, could you please
>> send the whole patchset ? Having the full picture would help in taking
>> decisions for downstream also.
>>
>> I am still uncertain about the usefulness of S390Topology object because,
>> as for now, the state can be computed on the fly, the reset can be
>> handled at the machine level directly under s390_machine_reset() and so
>> could migration if the machine had a vmstate (not the case today but
>> quite easy to add). So before merging anything that could be difficult
>> to maintain and/or backport, I would prefer to see it all !
>>
> 
> The goal of dedicating an object for topology was to ease the maintenance and portability by using the QEMU object framework.
> 
> If on contrary it is a problem for maintenance or backport we surely better not use it.
> 
> Any other opinion?

I agree with Cedric. There is no point in a topology object.
The state is calculated on the fly depending on the command line. This
would change if we ever implement the PTF horizontal/vertical state. But this
can then be another CPU state.

So I think we could go forward with this patch as a simple patch set that allows to
sets a static topology. This makes sense on its own, e.g. if you plan to pin your
vCPUs to given host CPUs.

Dynamic changes do come with CPU hotplug, not sure what libvirt does with new CPUs
in that case during migration. I assume those are re-generated on the target with
whatever topology was created on the source. So I guess this will just work, but
it would be good if we could test that.

A more fine-grained topology (drawer, book) could be added later or upfront. It
does require common code and libvirt enhancements, though.

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

[Janis Schoetterl-Glausch]

On Tue, 2022-12-13 at 14:41 +0100, Christian Borntraeger wrote:
> 
> Am 12.12.22 um 11:17 schrieb Thomas Huth:
> > On 12/12/2022 11.10, Pierre Morel wrote:
> > > 
> > > 
> > > On 12/12/22 10:07, Thomas Huth wrote:
> > > > On 12/12/2022 09.51, Pierre Morel wrote:
> > > > > 
> > > > > 
> > > > > On 12/9/22 14:32, Thomas Huth wrote:
> > > > > > On 08/12/2022 10.44, Pierre Morel wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > Implementation discussions
> > > > > > > ==========================
> > > > > > > 
> > > > > > > CPU models
> > > > > > > ----------
> > > > > > > 
> > > > > > > Since the S390_FEAT_CONFIGURATION_TOPOLOGY is already in the CPU model
> > > > > > > for old QEMU we could not activate it as usual from KVM but needed
> > > > > > > a KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
> > > > > > > Checking and enabling this capability enables
> > > > > > > S390_FEAT_CONFIGURATION_TOPOLOGY.
> > > > > > > 
> > > > > > > Migration
> > > > > > > ---------
> > > > > > > 
> > > > > > > Once the S390_FEAT_CONFIGURATION_TOPOLOGY is enabled in the source
> > > > > > > host the STFL(11) is provided to the guest.
> > > > > > > Since the feature is already in the CPU model of older QEMU,
> > > > > > > a migration from a new QEMU enabling the topology to an old QEMU
> > > > > > > will keep STFL(11) enabled making the guest get an exception for
> > > > > > > illegal operation as soon as it uses the PTF instruction.
> > > > > > 
> > > > > > I now thought that it is not possible to enable "ctop" on older QEMUs since the don't enable the KVM capability? ... or is it still somehow possible? What did I miss?
> > > > > > 
> > > > > >   Thomas
> > > > > 
> > > > > Enabling ctop with ctop=on on old QEMU is not possible, this is right.
> > > > > But, if STFL(11) is enable in the source KVM by a new QEMU, I can see that even with -ctop=off the STFL(11) is migrated to the destination.
> 
> This does not make sense. the cpu model and stfle values are not migrated. This is re-created during startup depending on the command line parameters of -cpu.
> Thats why source and host have the same command lines for -cpu. And STFLE.11 must not be set on the SOURCE of ctop is off.
> 
What about linux? I didn't look to thoroughly at it, but it caches the stfle bits, doesn't it?
So if the migration succeeds, even tho it should not, it will look to the guest like the facility is enabled.
> 
> > > > 
> > > > Is this with the "host" CPU model or another one? And did you explicitly specify "ctop=off" at the command line, or are you just using the default setting by not specifying it?
> > > 
> > > With explicit cpumodel and using ctop=off like in
> > > 
> > > sudo /usr/local/bin/qemu-system-s390x_master \
> > >       -m 512M \
> > >       -enable-kvm -smp 4,sockets=4,cores=2,maxcpus=8 \
> > >       -cpu z14,ctop=off \
> > >       -machine s390-ccw-virtio-7.2,accel=kvm \
> > >       ...
> > 
> > Ok ... that sounds like a bug somewhere in your patches or in the kernel code ... the guest should never see STFL bit 11 if ctop=off, should it?
> 
> Correct. If ctop=off then QEMU should disable STFLE.11 for the CPU model.

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

[Christian Borntraeger]

Am 13.12.22 um 14:57 schrieb Janis Schoetterl-Glausch:
> On Tue, 2022-12-13 at 14:41 +0100, Christian Borntraeger wrote:
>>
>> Am 12.12.22 um 11:17 schrieb Thomas Huth:
>>> On 12/12/2022 11.10, Pierre Morel wrote:
>>>>
>>>>
>>>> On 12/12/22 10:07, Thomas Huth wrote:
>>>>> On 12/12/2022 09.51, Pierre Morel wrote:
>>>>>>
>>>>>>
>>>>>> On 12/9/22 14:32, Thomas Huth wrote:
>>>>>>> On 08/12/2022 10.44, Pierre Morel wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Implementation discussions
>>>>>>>> ==========================
>>>>>>>>
>>>>>>>> CPU models
>>>>>>>> ----------
>>>>>>>>
>>>>>>>> Since the S390_FEAT_CONFIGURATION_TOPOLOGY is already in the CPU model
>>>>>>>> for old QEMU we could not activate it as usual from KVM but needed
>>>>>>>> a KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
>>>>>>>> Checking and enabling this capability enables
>>>>>>>> S390_FEAT_CONFIGURATION_TOPOLOGY.
>>>>>>>>
>>>>>>>> Migration
>>>>>>>> ---------
>>>>>>>>
>>>>>>>> Once the S390_FEAT_CONFIGURATION_TOPOLOGY is enabled in the source
>>>>>>>> host the STFL(11) is provided to the guest.
>>>>>>>> Since the feature is already in the CPU model of older QEMU,
>>>>>>>> a migration from a new QEMU enabling the topology to an old QEMU
>>>>>>>> will keep STFL(11) enabled making the guest get an exception for
>>>>>>>> illegal operation as soon as it uses the PTF instruction.
>>>>>>>
>>>>>>> I now thought that it is not possible to enable "ctop" on older QEMUs since the don't enable the KVM capability? ... or is it still somehow possible? What did I miss?
>>>>>>>
>>>>>>>    Thomas
>>>>>>
>>>>>> Enabling ctop with ctop=on on old QEMU is not possible, this is right.
>>>>>> But, if STFL(11) is enable in the source KVM by a new QEMU, I can see that even with -ctop=off the STFL(11) is migrated to the destination.
>>
>> This does not make sense. the cpu model and stfle values are not migrated. This is re-created during startup depending on the command line parameters of -cpu.
>> Thats why source and host have the same command lines for -cpu. And STFLE.11 must not be set on the SOURCE of ctop is off.
>>
> What about linux? I didn't look to thoroughly at it, but it caches the stfle bits, doesn't it?
> So if the migration succeeds, even tho it should not, it will look to the guest like the facility is enabled.

That is true, but the migration should not succeed in that case unless you use an unsafe way of migrating. And the cpu model was exactly added to block those unsafe way.
One thing:
-cpu host is unsafe (host-passthrough in libvirt speak). Either use host-model or a fully specified model like z14.2,ctop=on....

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

[Christian Borntraeger]

Am 13.12.22 um 14:50 schrieb Christian Borntraeger:
> 
> 
> Am 12.12.22 um 11:01 schrieb Pierre Morel:
>>
>>
>> On 12/9/22 15:45, Cédric Le Goater wrote:
>>> On 12/8/22 10:44, Pierre Morel wrote:
>>>> Hi,
>>>>
>>>> Implementation discussions
>>>> ==========================
>>>>
>>>> CPU models
>>>> ----------
>>>>
>>>> Since the S390_FEAT_CONFIGURATION_TOPOLOGY is already in the CPU model
>>>> for old QEMU we could not activate it as usual from KVM but needed
>>>> a KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
>>>> Checking and enabling this capability enables
>>>> S390_FEAT_CONFIGURATION_TOPOLOGY.
>>>>
>>>> Migration
>>>> ---------
>>>>
>>>> Once the S390_FEAT_CONFIGURATION_TOPOLOGY is enabled in the source
>>>> host the STFL(11) is provided to the guest.
>>>> Since the feature is already in the CPU model of older QEMU,
>>>> a migration from a new QEMU enabling the topology to an old QEMU
>>>> will keep STFL(11) enabled making the guest get an exception for
>>>> illegal operation as soon as it uses the PTF instruction.
>>>>
>>>> A VMState keeping track of the S390_FEAT_CONFIGURATION_TOPOLOGY
>>>> allows to forbid the migration in such a case.
>>>>
>>>> Note that the VMState will be used to hold information on the
>>>> topology once we implement topology change for a running guest.
>>>>
>>>> Topology
>>>> --------
>>>>
>>>> Until we introduce bookss and drawers, polarization and dedication
>>>> the topology is kept very simple and is specified uniquely by
>>>> the core_id of the vCPU which is also the vCPU address.
>>>>
>>>> Testing
>>>> =======
>>>>
>>>> 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.
>>>>
>>>> Documentation
>>>> =============
>>>>
>>>> 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.
>>>>
>>>> Future developments
>>>> ===================
>>>>
>>>> Two series are actively prepared:
>>>> - Adding drawers, book, polarization and dedication to the vCPU.
>>>> - changing the topology with a running guest
>>>
>>> Since we have time before the next QEMU 8.0 release, could you please
>>> send the whole patchset ? Having the full picture would help in taking
>>> decisions for downstream also.
>>>
>>> I am still uncertain about the usefulness of S390Topology object because,
>>> as for now, the state can be computed on the fly, the reset can be
>>> handled at the machine level directly under s390_machine_reset() and so
>>> could migration if the machine had a vmstate (not the case today but
>>> quite easy to add). So before merging anything that could be difficult
>>> to maintain and/or backport, I would prefer to see it all !
>>>
>>
>> The goal of dedicating an object for topology was to ease the maintenance and portability by using the QEMU object framework.
>>
>> If on contrary it is a problem for maintenance or backport we surely better not use it.
>>
>> Any other opinion?
> 
> I agree with Cedric. There is no point in a topology object.
> The state is calculated on the fly depending on the command line. This
> would change if we ever implement the PTF horizontal/vertical state. But this
> can then be another CPU state.
> 
> So I think we could go forward with this patch as a simple patch set that allows to
> sets a static topology. This makes sense on its own, e.g. if you plan to pin your
> vCPUs to given host CPUs.
> 
> Dynamic changes do come with CPU hotplug, not sure what libvirt does with new CPUs
> in that case during migration. I assume those are re-generated on the target with
> whatever topology was created on the source. So I guess this will just work, but
> it would be good if we could test that.
> 
> A more fine-grained topology (drawer, book) could be added later or upfront. It
> does require common code and libvirt enhancements, though.

Now I have discussed with Pierre and there IS a state that we want to migrate.
The topology change state is a guest wide bit that might be still set when
topology is changed->bit is set
guest is not yet started
migration

2 options:
1. a vmstate with that bit and migrate the state
2. always set the topology change bit after migration

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

[Janis Schoetterl-Glausch]

On Tue, 2022-12-13 at 16:12 +0100, Christian Borntraeger wrote:
> 
> Am 13.12.22 um 14:50 schrieb Christian Borntraeger:
> > 
> > 
> > Am 12.12.22 um 11:01 schrieb Pierre Morel:
> > > 
> > > 
> > > On 12/9/22 15:45, Cédric Le Goater wrote:
> > > > On 12/8/22 10:44, Pierre Morel wrote:
> > > > > Hi,
> > > > > 
> > > > > Implementation discussions
> > > > > ==========================
> > > > > 
> > > > > CPU models
> > > > > ----------
> > > > > 
> > > > > Since the S390_FEAT_CONFIGURATION_TOPOLOGY is already in the CPU model
> > > > > for old QEMU we could not activate it as usual from KVM but needed
> > > > > a KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
> > > > > Checking and enabling this capability enables
> > > > > S390_FEAT_CONFIGURATION_TOPOLOGY.
> > > > > 
> > > > > Migration
> > > > > ---------
> > > > > 
> > > > > Once the S390_FEAT_CONFIGURATION_TOPOLOGY is enabled in the source
> > > > > host the STFL(11) is provided to the guest.
> > > > > Since the feature is already in the CPU model of older QEMU,
> > > > > a migration from a new QEMU enabling the topology to an old QEMU
> > > > > will keep STFL(11) enabled making the guest get an exception for
> > > > > illegal operation as soon as it uses the PTF instruction.
> > > > > 
> > > > > A VMState keeping track of the S390_FEAT_CONFIGURATION_TOPOLOGY
> > > > > allows to forbid the migration in such a case.
> > > > > 
> > > > > Note that the VMState will be used to hold information on the
> > > > > topology once we implement topology change for a running guest.
> > > > > 
> > > > > Topology
> > > > > --------
> > > > > 
> > > > > Until we introduce bookss and drawers, polarization and dedication
> > > > > the topology is kept very simple and is specified uniquely by
> > > > > the core_id of the vCPU which is also the vCPU address.
> > > > > 
> > > > > Testing
> > > > > =======
> > > > > 
> > > > > 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.
> > > > > 
> > > > > Documentation
> > > > > =============
> > > > > 
> > > > > 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.
> > > > > 
> > > > > Future developments
> > > > > ===================
> > > > > 
> > > > > Two series are actively prepared:
> > > > > - Adding drawers, book, polarization and dedication to the vCPU.
> > > > > - changing the topology with a running guest
> > > > 
> > > > Since we have time before the next QEMU 8.0 release, could you please
> > > > send the whole patchset ? Having the full picture would help in taking
> > > > decisions for downstream also.
> > > > 
> > > > I am still uncertain about the usefulness of S390Topology object because,
> > > > as for now, the state can be computed on the fly, the reset can be
> > > > handled at the machine level directly under s390_machine_reset() and so
> > > > could migration if the machine had a vmstate (not the case today but
> > > > quite easy to add). So before merging anything that could be difficult
> > > > to maintain and/or backport, I would prefer to see it all !
> > > > 
> > > 
> > > The goal of dedicating an object for topology was to ease the maintenance and portability by using the QEMU object framework.
> > > 
> > > If on contrary it is a problem for maintenance or backport we surely better not use it.
> > > 
> > > Any other opinion?
> > 
> > I agree with Cedric. There is no point in a topology object.
> > The state is calculated on the fly depending on the command line. This
> > would change if we ever implement the PTF horizontal/vertical state. But this
> > can then be another CPU state.
> > 
> > So I think we could go forward with this patch as a simple patch set that allows to
> > sets a static topology. This makes sense on its own, e.g. if you plan to pin your
> > vCPUs to given host CPUs.
> > 
> > Dynamic changes do come with CPU hotplug, not sure what libvirt does with new CPUs
> > in that case during migration. I assume those are re-generated on the target with
> > whatever topology was created on the source. So I guess this will just work, but
> > it would be good if we could test that.
> > 
> > A more fine-grained topology (drawer, book) could be added later or upfront. It
> > does require common code and libvirt enhancements, though.
> 
> Now I have discussed with Pierre and there IS a state that we want to migrate.
> The topology change state is a guest wide bit that might be still set when
> topology is changed->bit is set
> guest is not yet started
> migration
> 
> 2 options:
> 1. a vmstate with that bit and migrate the state
> 2. always set the topology change bit after migration

2. is the default behavior if you do nothing. VCPU creation on the target sets
the change bit to 1.
So 1. is only to prevent spurious topology change indication.

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

[Pierre Morel]

On 12/13/22 14:41, Christian Borntraeger wrote:
> 
> 
> Am 12.12.22 um 11:17 schrieb Thomas Huth:
>> On 12/12/2022 11.10, Pierre Morel wrote:
>>>
>>>
>>> On 12/12/22 10:07, Thomas Huth wrote:
>>>> On 12/12/2022 09.51, Pierre Morel wrote:
>>>>>
>>>>>
>>>>> On 12/9/22 14:32, Thomas Huth wrote:
>>>>>> On 08/12/2022 10.44, Pierre Morel wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Implementation discussions
>>>>>>> ==========================
>>>>>>>
>>>>>>> CPU models
>>>>>>> ----------
>>>>>>>
>>>>>>> Since the S390_FEAT_CONFIGURATION_TOPOLOGY is already in the CPU 
>>>>>>> model
>>>>>>> for old QEMU we could not activate it as usual from KVM but needed
>>>>>>> a KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
>>>>>>> Checking and enabling this capability enables
>>>>>>> S390_FEAT_CONFIGURATION_TOPOLOGY.
>>>>>>>
>>>>>>> Migration
>>>>>>> ---------
>>>>>>>
>>>>>>> Once the S390_FEAT_CONFIGURATION_TOPOLOGY is enabled in the source
>>>>>>> host the STFL(11) is provided to the guest.
>>>>>>> Since the feature is already in the CPU model of older QEMU,
>>>>>>> a migration from a new QEMU enabling the topology to an old QEMU
>>>>>>> will keep STFL(11) enabled making the guest get an exception for
>>>>>>> illegal operation as soon as it uses the PTF instruction.
>>>>>>
>>>>>> I now thought that it is not possible to enable "ctop" on older 
>>>>>> QEMUs since the don't enable the KVM capability? ... or is it 
>>>>>> still somehow possible? What did I miss?
>>>>>>
>>>>>>   Thomas
>>>>>
>>>>> Enabling ctop with ctop=on on old QEMU is not possible, this is right.
>>>>> But, if STFL(11) is enable in the source KVM by a new QEMU, I can 
>>>>> see that even with -ctop=off the STFL(11) is migrated to the 
>>>>> destination.
> 
> This does not make sense. the cpu model and stfle values are not 
> migrated. This is re-created during startup depending on the command 
> line parameters of -cpu.
> Thats why source and host have the same command lines for -cpu. And 
> STFLE.11 must not be set on the SOURCE of ctop is off.

OK, so it is a feature

> 
> 
>>>>
>>>> Is this with the "host" CPU model or another one? And did you 
>>>> explicitly specify "ctop=off" at the command line, or are you just 
>>>> using the default setting by not specifying it?
>>>
>>> With explicit cpumodel and using ctop=off like in
>>>
>>> sudo /usr/local/bin/qemu-system-s390x_master \
>>>       -m 512M \
>>>       -enable-kvm -smp 4,sockets=4,cores=2,maxcpus=8 \
>>>       -cpu z14,ctop=off \
>>>       -machine s390-ccw-virtio-7.2,accel=kvm \
>>>       ...
>>
>> Ok ... that sounds like a bug somewhere in your patches or in the 
>> kernel code ... the guest should never see STFL bit 11 if ctop=off, 
>> should it?
> 
> Correct. If ctop=off then QEMU should disable STFLE.11 for the CPU model.

Sorry but not completely correct in the case of migration.

After a migration if the source host specifies ctop=on and target host 
specifies ctop=off it does see the STFL bit 11.

The admin should not, but can, specify ctop=off on target if the source 
set ctop=on. Then the target will start and run in a degraded mode.

Admin should specify the same flags on both ends, as you said above the 
STFL bits are not migrated and target QEMU can not verify what the 
original flags were.

However, isn't it a bug?
Is there a reason to not prevent QEMU to start with a wrong cpu model 
like specifying different flags on both ends or even different cpu?

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 12/13/22 16:31, Janis Schoetterl-Glausch wrote:
> On Tue, 2022-12-13 at 16:12 +0100, Christian Borntraeger wrote:
>>
>> Am 13.12.22 um 14:50 schrieb Christian Borntraeger:
>>>
>>>
>>> Am 12.12.22 um 11:01 schrieb Pierre Morel:
>>>>
>>>>
>>>> On 12/9/22 15:45, Cédric Le Goater wrote:
>>>>> On 12/8/22 10:44, Pierre Morel wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Implementation discussions
>>>>>> ==========================
>>>>>>
>>>>>> CPU models
>>>>>> ----------
>>>>>>
>>>>>> Since the S390_FEAT_CONFIGURATION_TOPOLOGY is already in the CPU model
>>>>>> for old QEMU we could not activate it as usual from KVM but needed
>>>>>> a KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
>>>>>> Checking and enabling this capability enables
>>>>>> S390_FEAT_CONFIGURATION_TOPOLOGY.
>>>>>>
>>>>>> Migration
>>>>>> ---------
>>>>>>
>>>>>> Once the S390_FEAT_CONFIGURATION_TOPOLOGY is enabled in the source
>>>>>> host the STFL(11) is provided to the guest.
>>>>>> Since the feature is already in the CPU model of older QEMU,
>>>>>> a migration from a new QEMU enabling the topology to an old QEMU
>>>>>> will keep STFL(11) enabled making the guest get an exception for
>>>>>> illegal operation as soon as it uses the PTF instruction.
>>>>>>
>>>>>> A VMState keeping track of the S390_FEAT_CONFIGURATION_TOPOLOGY
>>>>>> allows to forbid the migration in such a case.
>>>>>>
>>>>>> Note that the VMState will be used to hold information on the
>>>>>> topology once we implement topology change for a running guest.
>>>>>>
>>>>>> Topology
>>>>>> --------
>>>>>>
>>>>>> Until we introduce bookss and drawers, polarization and dedication
>>>>>> the topology is kept very simple and is specified uniquely by
>>>>>> the core_id of the vCPU which is also the vCPU address.
>>>>>>
>>>>>> Testing
>>>>>> =======
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> Documentation
>>>>>> =============
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> Future developments
>>>>>> ===================
>>>>>>
>>>>>> Two series are actively prepared:
>>>>>> - Adding drawers, book, polarization and dedication to the vCPU.
>>>>>> - changing the topology with a running guest
>>>>>
>>>>> Since we have time before the next QEMU 8.0 release, could you please
>>>>> send the whole patchset ? Having the full picture would help in taking
>>>>> decisions for downstream also.
>>>>>
>>>>> I am still uncertain about the usefulness of S390Topology object because,
>>>>> as for now, the state can be computed on the fly, the reset can be
>>>>> handled at the machine level directly under s390_machine_reset() and so
>>>>> could migration if the machine had a vmstate (not the case today but
>>>>> quite easy to add). So before merging anything that could be difficult
>>>>> to maintain and/or backport, I would prefer to see it all !
>>>>>
>>>>
>>>> The goal of dedicating an object for topology was to ease the maintenance and portability by using the QEMU object framework.
>>>>
>>>> If on contrary it is a problem for maintenance or backport we surely better not use it.
>>>>
>>>> Any other opinion?
>>>
>>> I agree with Cedric. There is no point in a topology object.
>>> The state is calculated on the fly depending on the command line. This
>>> would change if we ever implement the PTF horizontal/vertical state. But this
>>> can then be another CPU state.
>>>
>>> So I think we could go forward with this patch as a simple patch set that allows to
>>> sets a static topology. This makes sense on its own, e.g. if you plan to pin your
>>> vCPUs to given host CPUs.
>>>
>>> Dynamic changes do come with CPU hotplug, not sure what libvirt does with new CPUs
>>> in that case during migration. I assume those are re-generated on the target with
>>> whatever topology was created on the source. So I guess this will just work, but
>>> it would be good if we could test that.
>>>
>>> A more fine-grained topology (drawer, book) could be added later or upfront. It
>>> does require common code and libvirt enhancements, though.
>>
>> Now I have discussed with Pierre and there IS a state that we want to migrate.
>> The topology change state is a guest wide bit that might be still set when
>> topology is changed->bit is set
>> guest is not yet started
>> migration
>>
>> 2 options:
>> 1. a vmstate with that bit and migrate the state
>> 2. always set the topology change bit after migration
> 
> 2. is the default behavior if you do nothing. VCPU creation on the target sets
> the change bit to 1.
> So 1. is only to prevent spurious topology change indication.
> 

OK, then I go the easy way and suppress the object.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v13 4/7] s390x/cpu_topology: CPU topology migration

[Pierre Morel]

On 12/13/22 14:26, Christian Borntraeger wrote:
> Am 08.12.22 um 10:44 schrieb 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.
> 
> I dont get why we need this? If the target QEMU has topology it should
> already create this according to the configuration. WHy do we need a
> trigger?

We do not.
The first idea was to migrate the topology with a dedicated object but 
after today discussion, it will be recreated by the admin on the target 
and the MTCR will not be migrated but simply set to 1 on target start.

So next spin will not get migration included.

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

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

[Thomas Huth]

On 13/12/2022 18.24, Pierre Morel wrote:
> 
> 
> On 12/13/22 14:41, Christian Borntraeger wrote:
>>
>>
>> Am 12.12.22 um 11:17 schrieb Thomas Huth:
>>> On 12/12/2022 11.10, Pierre Morel wrote:
>>>>
>>>>
>>>> On 12/12/22 10:07, Thomas Huth wrote:
>>>>> On 12/12/2022 09.51, Pierre Morel wrote:
>>>>>>
>>>>>>
>>>>>> On 12/9/22 14:32, Thomas Huth wrote:
>>>>>>> On 08/12/2022 10.44, Pierre Morel wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Implementation discussions
>>>>>>>> ==========================
>>>>>>>>
>>>>>>>> CPU models
>>>>>>>> ----------
>>>>>>>>
>>>>>>>> Since the S390_FEAT_CONFIGURATION_TOPOLOGY is already in the CPU model
>>>>>>>> for old QEMU we could not activate it as usual from KVM but needed
>>>>>>>> a KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
>>>>>>>> Checking and enabling this capability enables
>>>>>>>> S390_FEAT_CONFIGURATION_TOPOLOGY.
>>>>>>>>
>>>>>>>> Migration
>>>>>>>> ---------
>>>>>>>>
>>>>>>>> Once the S390_FEAT_CONFIGURATION_TOPOLOGY is enabled in the source
>>>>>>>> host the STFL(11) is provided to the guest.
>>>>>>>> Since the feature is already in the CPU model of older QEMU,
>>>>>>>> a migration from a new QEMU enabling the topology to an old QEMU
>>>>>>>> will keep STFL(11) enabled making the guest get an exception for
>>>>>>>> illegal operation as soon as it uses the PTF instruction.
>>>>>>>
>>>>>>> I now thought that it is not possible to enable "ctop" on older QEMUs 
>>>>>>> since the don't enable the KVM capability? ... or is it still somehow 
>>>>>>> possible? What did I miss?
>>>>>>>
>>>>>>>   Thomas
>>>>>>
>>>>>> Enabling ctop with ctop=on on old QEMU is not possible, this is right.
>>>>>> But, if STFL(11) is enable in the source KVM by a new QEMU, I can see 
>>>>>> that even with -ctop=off the STFL(11) is migrated to the destination.
>>
>> This does not make sense. the cpu model and stfle values are not migrated. 
>> This is re-created during startup depending on the command line parameters 
>> of -cpu.
>> Thats why source and host have the same command lines for -cpu. And 
>> STFLE.11 must not be set on the SOURCE of ctop is off.
> 
> OK, so it is a feature
> 
>>
>>
>>>>>
>>>>> Is this with the "host" CPU model or another one? And did you 
>>>>> explicitly specify "ctop=off" at the command line, or are you just 
>>>>> using the default setting by not specifying it?
>>>>
>>>> With explicit cpumodel and using ctop=off like in
>>>>
>>>> sudo /usr/local/bin/qemu-system-s390x_master \
>>>>       -m 512M \
>>>>       -enable-kvm -smp 4,sockets=4,cores=2,maxcpus=8 \
>>>>       -cpu z14,ctop=off \
>>>>       -machine s390-ccw-virtio-7.2,accel=kvm \
>>>>       ...
>>>
>>> Ok ... that sounds like a bug somewhere in your patches or in the kernel 
>>> code ... the guest should never see STFL bit 11 if ctop=off, should it?
>>
>> Correct. If ctop=off then QEMU should disable STFLE.11 for the CPU model.
> 
> Sorry but not completely correct in the case of migration.
> 
> After a migration if the source host specifies ctop=on and target host 
> specifies ctop=off it does see the STFL bit 11.
>
> The admin should not, but can, specify ctop=off on target if the source set 
> ctop=on. Then the target will start and run in a degraded mode.
> 
> Admin should specify the same flags on both ends, as you said above the STFL 
> bits are not migrated and target QEMU can not verify what the original flags 
> were.
> 
> However, isn't it a bug?
> Is there a reason to not prevent QEMU to start with a wrong cpu model like 
> specifying different flags on both ends or even different cpu?

It's clearly an user error if the two QEMUs are started with different flags 
on the source and destination ends. But it would be great if there was a 
generic way to check for this error condition and bail out early instead of 
doing the migration and let the user run into weird problems later... Does 
anybody have an idea whether there is a good and easy way to implement such 
a check?

  Thomas