[PATCH v2 0/5] s390x: CPU Topology

[PATCH v2 0/5] s390x: CPU Topology

[Pierre Morel]

Hi,

This series is a first part of the implementation of CPU topology
for S390 greatly reduced from the first spin.

In particular, we reduced the scope to the S390x specificities, removing
all code touching to SMP or NUMA, with the goal to:
- facilitate review and acceptance
- let for later the SMP part currently actively discussed in mainline
- be able despite the reduction of code to handle CPU topology for S390
  using the current S390 topology provided by QEMU with cores and sockets
  only.

To use these patches you will need the Linux series.
You find it there:
https://marc.info/?l=kvm&m=162697338719109&w=3

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.

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

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

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

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

S390 Topology is a 6 levels hierarchical topology with up to 5 level
    of containers. The last topology level, specifying the CPU cores.

    This patch series only uses the two lower levels sockets and cores.
    
    To get the information on the topology, S390 provides the STSI
    instruction, which stores a structures providing the list of the
    containers used in the Machine topology: the SYSIB.
    A selector within the STSI instruction allow to chose how many topology
    levels will be provide in the SYSIB.

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

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

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

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

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

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


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

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

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

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


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

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

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

- Effect of -smp parsing on the topology with an example:
    -smp 9,sockets=4,cores=4,maxcpus=16

    We have 4 socket each holding 4 cores so that we have a maximum 
    of 16 CPU, 9 of them are active on boot. (Should be obvious)

# lscpu -e
CPU NODE DRAWER BOOK SOCKET CORE L1d:L1i:L2d:L2i ONLINE CONFIGURED POLARIZATION ADDRESS
  0    0      0    0      0    0 0:0:0:0            yes yes        horizontal   0
  1    0      0    0      0    1 1:1:1:1            yes yes        horizontal   1
  2    0      0    0      0    2 2:2:2:2            yes yes        horizontal   2
  3    0      0    0      0    3 3:3:3:3            yes yes        horizontal   3
  4    0      0    0      1    4 4:4:4:4            yes yes        horizontal   4
  5    0      0    0      1    5 5:5:5:5            yes yes        horizontal   5
  6    0      0    0      1    6 6:6:6:6            yes yes        horizontal   6
  7    0      0    0      1    7 7:7:7:7            yes yes        horizontal   7
  8    0      0    0      2    8 8:8:8:8            yes yes        horizontal   8
# 


- To plug a new CPU inside the topology one can simply use the CPU
    address like in:
  
(qemu) device_add host-s390x-cpu,core-id=12
# lscpu -e
CPU NODE DRAWER BOOK SOCKET CORE L1d:L1i:L2d:L2i ONLINE CONFIGURED POLARIZATION ADDRESS
  0    0      0    0      0    0 0:0:0:0            yes yes        horizontal   0
  1    0      0    0      0    1 1:1:1:1            yes yes        horizontal   1
  2    0      0    0      0    2 2:2:2:2            yes yes        horizontal   2
  3    0      0    0      0    3 3:3:3:3            yes yes        horizontal   3
  4    0      0    0      1    4 4:4:4:4            yes yes        horizontal   4
  5    0      0    0      1    5 5:5:5:5            yes yes        horizontal   5
  6    0      0    0      1    6 6:6:6:6            yes yes        horizontal   6
  7    0      0    0      1    7 7:7:7:7            yes yes        horizontal   7
  8    0      0    0      2    8 8:8:8:8            yes yes        horizontal   8
  9    -      -    -      -    - :::                 no yes        horizontal   12
# chcpu -e 9
CPU 9 enabled
# lscpu -e
CPU NODE DRAWER BOOK SOCKET CORE L1d:L1i:L2d:L2i ONLINE CONFIGURED POLARIZATION ADDRESS
  0    0      0    0      0    0 0:0:0:0            yes yes        horizontal   0
  1    0      0    0      0    1 1:1:1:1            yes yes        horizontal   1
  2    0      0    0      0    2 2:2:2:2            yes yes        horizontal   2
  3    0      0    0      0    3 3:3:3:3            yes yes        horizontal   3
  4    0      0    0      1    4 4:4:4:4            yes yes        horizontal   4
  5    0      0    0      1    5 5:5:5:5            yes yes        horizontal   5
  6    0      0    0      1    6 6:6:6:6            yes yes        horizontal   6
  7    0      0    0      1    7 7:7:7:7            yes yes        horizontal   7
  8    0      0    0      2    8 8:8:8:8            yes yes        horizontal   8
  9    0      0    0      3    9 9:9:9:9            yes yes        horizontal   12
#

It is up to the admin level, Libvirt for example, to pin the righ CPU to the right
vCPU, but as we can see without NUMA, chosing separate sockets for CPUs is not easy
without hotplug because without information the code will assign the vCPU and fill
the sockets one after the other.
Note that this is also the default behavior on the LPAR.


Regards,
Pierre

Pierre Morel (5):
  s390x: kvm: topology: Linux header update
  s390x: kvm: topology: interception of PTF instruction
  s390x: topology: CPU topology objects and structures
  s390x: topology: Topology list entries and SYSIB 15.x.x
  s390x: topology: implementating Store Topology System Information

 hw/s390x/cpu-topology.c            | 334 +++++++++++++++++++++++++++++
 hw/s390x/meson.build               |   1 +
 hw/s390x/s390-virtio-ccw.c         |  49 +++++
 include/hw/s390x/cpu-topology.h    |  67 ++++++
 include/hw/s390x/s390-virtio-ccw.h |   7 +
 linux-headers/linux/kvm.h          |   1 +
 target/s390x/cpu.h                 |  44 ++++
 target/s390x/kvm/kvm.c             | 122 +++++++++++
 8 files changed, 625 insertions(+)
 create mode 100644 hw/s390x/cpu-topology.c
 create mode 100644 include/hw/s390x/cpu-topology.h

-- 
2.25.1

[PATCH v2 1/5] s390x: kvm: topology: Linux header update

[Pierre Morel]

Just as information, the linux header update patch is inside the
Linux patch series.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 linux-headers/linux/kvm.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index bcaf66cc4d..38e96ea6f7 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1112,6 +1112,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_BINARY_STATS_FD 203
 #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
 #define KVM_CAP_ARM_MTE 205
+#define KVM_CAP_S390_CPU_TOPOLOGY 206
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.25.1

[PATCH v2 2/5] s390x: kvm: topology: interception of PTF instruction

[Pierre Morel]

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

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c         | 45 ++++++++++++++++++++++++++++++
 include/hw/s390x/s390-virtio-ccw.h |  7 +++++
 target/s390x/kvm/kvm.c             | 21 ++++++++++++++
 3 files changed, 73 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index e4b18aef49..500e856974 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -404,6 +404,49 @@ static void s390_pv_prepare_reset(S390CcwMachineState *ms)
     s390_pv_prep_reset();
 }
 
+int s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra)
+{
+    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
+    CPUS390XState *env = &cpu->env;
+    uint64_t reg = env->regs[r1];
+    uint8_t fc = reg & S390_TOPO_FC_MASK;
+
+    if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
+        s390_program_interrupt(env, PGM_OPERAND, ra);
+        return 0;
+    }
+
+    if (env->psw.mask & PSW_MASK_PSTATE) {
+        s390_program_interrupt(env, PGM_PRIVILEGED, ra);
+        return 0;
+    }
+
+    if (reg & ~S390_TOPO_FC_MASK) {
+        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+        return 0;
+    }
+
+    switch (fc) {
+    case 0:    /* Horizontal polarization is already set */
+        env->regs[r1] = S390_PTF_REASON_DONE;
+        return 2;
+    case 1:    /* Vertical polarization is not supported */
+        env->regs[r1] = S390_PTF_REASON_NONE;
+        return 2;
+    case 2:    /* Report if a topology change report is pending */
+        if (ms->topology_change_report_pending) {
+            ms->topology_change_report_pending = false;
+            return 1;
+        }
+        return 0;
+    default:
+        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+        break;
+    }
+
+    return 0;
+}
+
 static void s390_machine_reset(MachineState *machine)
 {
     S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
@@ -433,6 +476,8 @@ static void s390_machine_reset(MachineState *machine)
         run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
         break;
     case S390_RESET_MODIFIED_CLEAR:
+        /* clear topology_change_report pending condition on subsystem reset */
+        ms->topology_change_report_pending = false;
         /*
          * Susbsystem reset needs to be done before we unshare memory
          * and lose access to VIRTIO structures in guest memory.
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 3331990e02..fbde357332 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -27,9 +27,16 @@ struct S390CcwMachineState {
     bool aes_key_wrap;
     bool dea_key_wrap;
     bool pv;
+    bool topology_change_report_pending;
     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
+int s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra);
+
 struct S390CcwMachineClass {
     /*< private >*/
     MachineClass parent_class;
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 5b1fdb55c4..9a0c13d4ac 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
@@ -1452,6 +1453,16 @@ static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
     }
 }
 
+static int kvm_handle_ptf(S390CPU *cpu, struct kvm_run *run)
+{
+    uint8_t r1 = (run->s390_sieic.ipb >> 20) & 0x0f;
+    uint8_t ret;
+
+    ret = s390_handle_ptf(cpu, r1, RA_IGNORED);
+    setcc(cpu, ret);
+    return 0;
+}
+
 static int handle_b9(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
 {
     int r = 0;
@@ -1469,6 +1480,9 @@ static int handle_b9(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
     case PRIV_B9_RPCIT:
         r = kvm_rpcit_service_call(cpu, run);
         break;
+    case PRIV_B9_PTF:
+        r = kvm_handle_ptf(cpu, run);
+        break;
     case PRIV_B9_EQBS:
         /* just inject exception */
         r = -1;
@@ -2470,6 +2484,13 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
         set_bit(S390_FEAT_DIAG_318, model->features);
     }
 
+    /*
+     * Configuration topology is partially handled in KVM
+     */
+    if (kvm_check_extension(kvm_state, KVM_CAP_S390_CPU_TOPOLOGY)) {
+        set_bit(S390_FEAT_CONFIGURATION_TOPOLOGY, model->features);
+    }
+
     /* strip of features that are not part of the maximum model */
     bitmap_and(model->features, model->features, model->def->full_feat,
                S390_FEAT_MAX);
-- 
2.25.1

[PATCH v2 3/5] s390x: topology: CPU topology objects and structures

[Pierre Morel]

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

Every object under this single book will be build dynamically
immediately after a CPU has be realized if it is needed.
The CPU will fill the sockets once after the other, according to the
number of core per socket defined during the smp parsing.

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

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

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

[PATCH v2 4/5] s390x: topology: Topology list entries and SYSIB 15.x.x

[Pierre Morel]

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

This patch will be squatched with the next patch.

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

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

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

[Pierre Morel]

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

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

With this patch the maximum MNEST level is 2, this is also
the only level allowed and only SYSIB 15_1_2 will be built.

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

diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 9a0c13d4ac..0194756e6a 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -52,6 +52,7 @@
 #include "hw/s390x/s390-virtio-ccw.h"
 #include "hw/s390x/s390-virtio-hcall.h"
 #include "hw/s390x/pv.h"
+#include "hw/s390x/cpu-topology.h"
 
 #ifndef DEBUG_KVM
 #define DEBUG_KVM  0
@@ -1907,6 +1908,102 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64 addr, uint8_t ar)
     }
 }
 
+static int stsi_15_container(void *p, int nl, int id)
+{
+    SysIBTl_container *tle = (SysIBTl_container *)p;
+
+    tle->nl = nl;
+    tle->id = id;
+
+    return sizeof(*tle);
+}
+
+static int stsi_15_cpus(void *p, S390TopologyCores *cd)
+{
+    SysIBTl_cpu *tle = (SysIBTl_cpu *)p;
+
+    tle->nl = 0;
+    tle->dedicated = cd->dedicated;
+    tle->polarity = cd->polarity;
+    tle->type = cd->cputype;
+    tle->origin = cd->origin;
+    tle->mask = cd->mask;
+
+    return sizeof(*tle);
+}
+
+static int set_socket(const MachineState *ms, void *p,
+                      S390TopologySocket *socket)
+{
+    BusChild *kid;
+    int l, len = 0;
+
+    len += stsi_15_container(p, 1, socket->socket_id);
+    p += len;
+
+    QTAILQ_FOREACH_REVERSE(kid, &socket->bus->children, sibling) {
+        l = stsi_15_cpus(p, S390_TOPOLOGY_CORES(kid->child));
+        p += l;
+        len += l;
+    }
+    return len;
+}
+
+static void insert_stsi_15_1_2(const MachineState *ms, void *p)
+{
+    S390TopologyBook *book;
+    SysIB_151x *sysib;
+    BusChild *kid;
+    int level = 2;
+    int len, l;
+
+    sysib = (SysIB_151x *)p;
+    sysib->mnest = level;
+    sysib->mag[TOPOLOGY_NR_MAG2] = ms->smp.sockets;
+    sysib->mag[TOPOLOGY_NR_MAG1] = ms->smp.cores;
+
+    book = s390_get_topology();
+    len = sizeof(SysIB_151x);
+    p += len;
+
+    QTAILQ_FOREACH_REVERSE(kid, &book->bus->children, sibling) {
+        l = set_socket(ms, p, S390_TOPOLOGY_SOCKET(kid->child));
+        p += l;
+        len += l;
+    }
+
+    sysib->length = len;
+}
+
+static void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
+{
+    const MachineState *machine = MACHINE(qdev_get_machine());
+    void *p;
+    int ret, cc;
+
+    /*
+     * Until the SCLP STSI Facility reporting the MNEST value is used,
+     * a sel2 value of 2 is the only value allowed in STSI 15.1.x.
+     */
+    if (sel2 != 2) {
+        setcc(cpu, 3);
+        return;
+    }
+
+    p = g_malloc0(4096);
+
+   insert_stsi_15_1_2(machine, p);
+
+    if (s390_is_pv()) {
+        ret = s390_cpu_pv_mem_write(cpu, 0, p, 4096);
+    } else {
+        ret = s390_cpu_virt_mem_write(cpu, addr, ar, p, 4096);
+    }
+    cc = ret ? 3 : 0;
+    setcc(cpu, cc);
+    g_free(p);
+}
+
 static int handle_stsi(S390CPU *cpu)
 {
     CPUState *cs = CPU(cpu);
@@ -1920,6 +2017,10 @@ static int handle_stsi(S390CPU *cpu)
         /* Only sysib 3.2.2 needs post-handling for now. */
         insert_stsi_3_2_2(cpu, run->s390_stsi.addr, run->s390_stsi.ar);
         return 0;
+    case 15:
+        insert_stsi_15_1_x(cpu, run->s390_stsi.sel2, run->s390_stsi.addr,
+                           run->s390_stsi.ar);
+        return 0;
     default:
         return 0;
     }
-- 
2.25.1

Re: [PATCH v2 2/5] s390x: kvm: topology: interception of PTF instruction

[Pierre Morel]

On 7/22/21 7:42 PM, Pierre Morel wrote:
> Interception of the PTF instruction depending on the new
> KVM_CAP_S390_CPU_TOPOLOGY KVM extension.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   hw/s390x/s390-virtio-ccw.c         | 45 ++++++++++++++++++++++++++++++
>   include/hw/s390x/s390-virtio-ccw.h |  7 +++++
>   target/s390x/kvm/kvm.c             | 21 ++++++++++++++
>   3 files changed, 73 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index e4b18aef49..500e856974 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -404,6 +404,49 @@ static void s390_pv_prepare_reset(S390CcwMachineState *ms)
>       s390_pv_prep_reset();
>   }
>   
> +int s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra)
> +{
> +    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
> +    CPUS390XState *env = &cpu->env;
> +    uint64_t reg = env->regs[r1];
> +    uint8_t fc = reg & S390_TOPO_FC_MASK;
> +
> +    if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
> +        s390_program_interrupt(env, PGM_OPERAND, ra);
> +        return 0;
> +    }
> +
> +    if (env->psw.mask & PSW_MASK_PSTATE) {
> +        s390_program_interrupt(env, PGM_PRIVILEGED, ra);
> +        return 0;
> +    }
> +
> +    if (reg & ~S390_TOPO_FC_MASK) {
> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> +        return 0;
> +    }
> +
> +    switch (fc) {
> +    case 0:    /* Horizontal polarization is already set */
> +        env->regs[r1] = S390_PTF_REASON_DONE;
> +        return 2;
> +    case 1:    /* Vertical polarization is not supported */
> +        env->regs[r1] = S390_PTF_REASON_NONE;
> +        return 2;
> +    case 2:    /* Report if a topology change report is pending */
> +        if (ms->topology_change_report_pending) {
> +            ms->topology_change_report_pending = false;
> +            return 1;
> +        }
> +        return 0;
> +    default:
> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> +        break;
> +    }
> +
> +    return 0;
> +}
> +

Hi all,

it seems the part where the topology change is made pending on CPU 
creation disappeared from this patch...:

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index e02b2a8299..a9eeb11d1f 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -302,12 +302,14 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev,
                          DeviceState *dev, Error **errp)
  {
      MachineState *ms = MACHINE(hotplug_dev);
+    S390CcwMachineState *s390ms = S390_CCW_MACHINE(ms);
      S390CPU *cpu = S390_CPU(dev);

      g_assert(!ms->possible_cpus->cpus[cpu->env.core_id].cpu);
      ms->possible_cpus->cpus[cpu->env.core_id].cpu = OBJECT(dev);

      s390_topology_new_cpu(cpu->env.core_id);
+    s390ms->topology_change_report_pending = true;

      if (dev->hotplugged) {
          raise_irq_cpu_hotplug();


I will add this on the next round.

Otherwise, the changes in the Linux side to implement interpretation do 
not affect the QEMU implementation.

so... a gentle ping?

Pierre


>   static void s390_machine_reset(MachineState *machine)
>   {
>       S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
> @@ -433,6 +476,8 @@ static void s390_machine_reset(MachineState *machine)
>           run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
>           break;
>       case S390_RESET_MODIFIED_CLEAR:
> +        /* clear topology_change_report pending condition on subsystem reset */
> +        ms->topology_change_report_pending = false;
>           /*
>            * Susbsystem reset needs to be done before we unshare memory
>            * and lose access to VIRTIO structures in guest memory.
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 3331990e02..fbde357332 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -27,9 +27,16 @@ struct S390CcwMachineState {
>       bool aes_key_wrap;
>       bool dea_key_wrap;
>       bool pv;
> +    bool topology_change_report_pending;
>       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
> +int s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra);
> +
>   struct S390CcwMachineClass {
>       /*< private >*/
>       MachineClass parent_class;
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 5b1fdb55c4..9a0c13d4ac 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
> @@ -1452,6 +1453,16 @@ static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
>       }
>   }
>   
> +static int kvm_handle_ptf(S390CPU *cpu, struct kvm_run *run)
> +{
> +    uint8_t r1 = (run->s390_sieic.ipb >> 20) & 0x0f;
> +    uint8_t ret;
> +
> +    ret = s390_handle_ptf(cpu, r1, RA_IGNORED);
> +    setcc(cpu, ret);
> +    return 0;
> +}
> +
>   static int handle_b9(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
>   {
>       int r = 0;
> @@ -1469,6 +1480,9 @@ static int handle_b9(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
>       case PRIV_B9_RPCIT:
>           r = kvm_rpcit_service_call(cpu, run);
>           break;
> +    case PRIV_B9_PTF:
> +        r = kvm_handle_ptf(cpu, run);
> +        break;
>       case PRIV_B9_EQBS:
>           /* just inject exception */
>           r = -1;
> @@ -2470,6 +2484,13 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>           set_bit(S390_FEAT_DIAG_318, model->features);
>       }
>   
> +    /*
> +     * Configuration topology is partially handled in KVM
> +     */
> +    if (kvm_check_extension(kvm_state, KVM_CAP_S390_CPU_TOPOLOGY)) {
> +        set_bit(S390_FEAT_CONFIGURATION_TOPOLOGY, model->features);
> +    }
> +
>       /* strip of features that are not part of the maximum model */
>       bitmap_and(model->features, model->features, model->def->full_feat,
>                  S390_FEAT_MAX);
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v2 0/5] s390x: CPU Topology

[Pierre Morel]

a gentle ping :)

I would like if you have time, comments on the architecture I propose,
if the handling is done at the right level, KVM vs QEMU.

Therefor I added Viktor as CC.

This series is independent of the changes on -smp from Yanan Wang 
currently reviewed in main line.

Thanks,

Regards,
Pierre


On 7/22/21 7:42 PM, Pierre Morel wrote:
> Hi,
> 
> This series is a first part of the implementation of CPU topology
> for S390 greatly reduced from the first spin.
> 
> In particular, we reduced the scope to the S390x specificities, removing
> all code touching to SMP or NUMA, with the goal to:
> - facilitate review and acceptance
> - let for later the SMP part currently actively discussed in mainline
> - be able despite the reduction of code to handle CPU topology for S390
>    using the current S390 topology provided by QEMU with cores and sockets
>    only.
> 
> To use these patches you will need the Linux series.
> You find it there:
> https://marc.info/?l=kvm&m=162697338719109&w=3
> 
> 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.
> 
> ====================
> A short introduction
> ====================
> 
> CPU Topology is described in the S390 POP with essentially the description
> of two instructions:
> 
> PTF Perform Topology function used to poll for topology change
>      and used to set the polarization but this part is not part of this item.
> 
> STSI Store System Information and the SYSIB 15.1.x providing the Topology
>      configuration.
> 
> S390 Topology is a 6 levels hierarchical topology with up to 5 level
>      of containers. The last topology level, specifying the CPU cores.
> 
>      This patch series only uses the two lower levels sockets and cores.
>      
>      To get the information on the topology, S390 provides the STSI
>      instruction, which stores a structures providing the list of the
>      containers used in the Machine topology: the SYSIB.
>      A selector within the STSI instruction allow to chose how many topology
>      levels will be provide in the SYSIB.
> 
>      Using the Topology List Entries (TLE) provided inside the SYSIB we
>      the Linux kernel is able to compute the information about the cache
>      distance between two cores and can use this information to take
>      scheduling decisions.
> 
> Note:
> -----
>       Z15 reports 3 levels of containers, drawers, book, sockets as
>       Container-TLEs above the core description inside CPU-TLEs.
> 
> The Topology can be seen at several places inside zLinux:
>      - sysfs: /sys/devices/system/cpu/cpuX/topology
>      - procfs: /proc/sysinfo and /proc/cpuinfo
>      - lscpu -e : gives toplogy information
> 
> The different Topology levels have names:
>      - Node - Drawer - Book - sockets or physical package - core
> 
> Threads:
>      Multithreading, is not part of the topology as described by the
>      SYSIB 15.1.x
> 
> The interest of the guest to know the CPU topology is obviously to be
> able to optimise the load balancing and the migration of threads.
> KVM will have the same interest concerning vCPUs scheduling and cache
> optimisation.
> 
> 
> ==========
> The design
> ==========
> 
> 1) To be ready for hotplug, I chose an Object oriented design
> of the topology containers:
> - A node is a bridge on the SYSBUS and defines a "node bus"
> - A drawer is hotplug on the "node bus"
> - A book on the "drawer bus"
> - A socket on the "book bus"
> - And the CPU Topology List Entry (CPU-TLE)sits on the socket bus.
> These objects will be enhanced with the cache information when
> NUMA is implemented.
> 
> This also allows for easy retrieval when building the different SYSIB
> for Store Topology System Information (STSI)
> 
> 2) Perform Topology Function (PTF) instruction is made available to the
> guest with a new KVM capability and intercepted in QEMU, allowing the
> guest to pool for topology changes.
> 
> 
> =====================
> Features and TBD list
> =====================
> 
> - There is no direct match between IDs shown by:
>      - lscpu (unrelated numbered list),
>      - SYSIB 15.1.x (topology ID)
> 
> - The CPU number, left column of lscpu, is used to reference a CPU
>      by Linux tools
>      While the CPU address is used by QEMU for hotplug.
> 
> - Effect of -smp parsing on the topology with an example:
>      -smp 9,sockets=4,cores=4,maxcpus=16
> 
>      We have 4 socket each holding 4 cores so that we have a maximum
>      of 16 CPU, 9 of them are active on boot. (Should be obvious)
> 
> # lscpu -e
> CPU NODE DRAWER BOOK SOCKET CORE L1d:L1i:L2d:L2i ONLINE CONFIGURED POLARIZATION ADDRESS
>    0    0      0    0      0    0 0:0:0:0            yes yes        horizontal   0
>    1    0      0    0      0    1 1:1:1:1            yes yes        horizontal   1
>    2    0      0    0      0    2 2:2:2:2            yes yes        horizontal   2
>    3    0      0    0      0    3 3:3:3:3            yes yes        horizontal   3
>    4    0      0    0      1    4 4:4:4:4            yes yes        horizontal   4
>    5    0      0    0      1    5 5:5:5:5            yes yes        horizontal   5
>    6    0      0    0      1    6 6:6:6:6            yes yes        horizontal   6
>    7    0      0    0      1    7 7:7:7:7            yes yes        horizontal   7
>    8    0      0    0      2    8 8:8:8:8            yes yes        horizontal   8
> #
> 
> 
> - To plug a new CPU inside the topology one can simply use the CPU
>      address like in:
>    
> (qemu) device_add host-s390x-cpu,core-id=12
> # lscpu -e
> CPU NODE DRAWER BOOK SOCKET CORE L1d:L1i:L2d:L2i ONLINE CONFIGURED POLARIZATION ADDRESS
>    0    0      0    0      0    0 0:0:0:0            yes yes        horizontal   0
>    1    0      0    0      0    1 1:1:1:1            yes yes        horizontal   1
>    2    0      0    0      0    2 2:2:2:2            yes yes        horizontal   2
>    3    0      0    0      0    3 3:3:3:3            yes yes        horizontal   3
>    4    0      0    0      1    4 4:4:4:4            yes yes        horizontal   4
>    5    0      0    0      1    5 5:5:5:5            yes yes        horizontal   5
>    6    0      0    0      1    6 6:6:6:6            yes yes        horizontal   6
>    7    0      0    0      1    7 7:7:7:7            yes yes        horizontal   7
>    8    0      0    0      2    8 8:8:8:8            yes yes        horizontal   8
>    9    -      -    -      -    - :::                 no yes        horizontal   12
> # chcpu -e 9
> CPU 9 enabled
> # lscpu -e
> CPU NODE DRAWER BOOK SOCKET CORE L1d:L1i:L2d:L2i ONLINE CONFIGURED POLARIZATION ADDRESS
>    0    0      0    0      0    0 0:0:0:0            yes yes        horizontal   0
>    1    0      0    0      0    1 1:1:1:1            yes yes        horizontal   1
>    2    0      0    0      0    2 2:2:2:2            yes yes        horizontal   2
>    3    0      0    0      0    3 3:3:3:3            yes yes        horizontal   3
>    4    0      0    0      1    4 4:4:4:4            yes yes        horizontal   4
>    5    0      0    0      1    5 5:5:5:5            yes yes        horizontal   5
>    6    0      0    0      1    6 6:6:6:6            yes yes        horizontal   6
>    7    0      0    0      1    7 7:7:7:7            yes yes        horizontal   7
>    8    0      0    0      2    8 8:8:8:8            yes yes        horizontal   8
>    9    0      0    0      3    9 9:9:9:9            yes yes        horizontal   12
> #
> 
> It is up to the admin level, Libvirt for example, to pin the righ CPU to the right
> vCPU, but as we can see without NUMA, chosing separate sockets for CPUs is not easy
> without hotplug because without information the code will assign the vCPU and fill
> the sockets one after the other.
> Note that this is also the default behavior on the LPAR.
> 
> 
> Regards,
> Pierre
> 
> Pierre Morel (5):
>    s390x: kvm: topology: Linux header update
>    s390x: kvm: topology: interception of PTF instruction
>    s390x: topology: CPU topology objects and structures
>    s390x: topology: Topology list entries and SYSIB 15.x.x
>    s390x: topology: implementating Store Topology System Information
> 
>   hw/s390x/cpu-topology.c            | 334 +++++++++++++++++++++++++++++
>   hw/s390x/meson.build               |   1 +
>   hw/s390x/s390-virtio-ccw.c         |  49 +++++
>   include/hw/s390x/cpu-topology.h    |  67 ++++++
>   include/hw/s390x/s390-virtio-ccw.h |   7 +
>   linux-headers/linux/kvm.h          |   1 +
>   target/s390x/cpu.h                 |  44 ++++
>   target/s390x/kvm/kvm.c             | 122 +++++++++++
>   8 files changed, 625 insertions(+)
>   create mode 100644 hw/s390x/cpu-topology.c
>   create mode 100644 include/hw/s390x/cpu-topology.h
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v2 0/5] s390x: CPU Topology

[Christian Borntraeger]

On 26.08.21 11:22, Pierre Morel wrote:
> 
> a gentle ping :)
> 
> I would like if you have time, comments on the architecture I propose,
> if the handling is done at the right level, KVM vs QEMU.

Do we expect changes in this series due to the discussed changes of PTF interpretion?

> 
> Therefor I added Viktor as CC.
> 
> This series is independent of the changes on -smp from Yanan Wang currently reviewed in main line.
> 
> Thanks,
> 
> Regards,
> Pierre
> 
> 
> On 7/22/21 7:42 PM, Pierre Morel wrote:
>> Hi,
>>
>> This series is a first part of the implementation of CPU topology
>> for S390 greatly reduced from the first spin.
>>
>> In particular, we reduced the scope to the S390x specificities, removing
>> all code touching to SMP or NUMA, with the goal to:
>> - facilitate review and acceptance
>> - let for later the SMP part currently actively discussed in mainline
>> - be able despite the reduction of code to handle CPU topology for S390
>>    using the current S390 topology provided by QEMU with cores and sockets
>>    only.
>>
>> To use these patches you will need the Linux series.
>> You find it there:
>> https://marc.info/?l=kvm&m=162697338719109&w=3
>>
>> 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.
>>
>> ====================
>> A short introduction
>> ====================
>>
>> CPU Topology is described in the S390 POP with essentially the description
>> of two instructions:
>>
>> PTF Perform Topology function used to poll for topology change
>>      and used to set the polarization but this part is not part of this item.
>>
>> STSI Store System Information and the SYSIB 15.1.x providing the Topology
>>      configuration.
>>
>> S390 Topology is a 6 levels hierarchical topology with up to 5 level
>>      of containers. The last topology level, specifying the CPU cores.
>>
>>      This patch series only uses the two lower levels sockets and cores.
>>      To get the information on the topology, S390 provides the STSI
>>      instruction, which stores a structures providing the list of the
>>      containers used in the Machine topology: the SYSIB.
>>      A selector within the STSI instruction allow to chose how many topology
>>      levels will be provide in the SYSIB.
>>
>>      Using the Topology List Entries (TLE) provided inside the SYSIB we
>>      the Linux kernel is able to compute the information about the cache
>>      distance between two cores and can use this information to take
>>      scheduling decisions.
>>
>> Note:
>> -----
>>       Z15 reports 3 levels of containers, drawers, book, sockets as
>>       Container-TLEs above the core description inside CPU-TLEs.
>>
>> The Topology can be seen at several places inside zLinux:
>>      - sysfs: /sys/devices/system/cpu/cpuX/topology
>>      - procfs: /proc/sysinfo and /proc/cpuinfo
>>      - lscpu -e : gives toplogy information
>>
>> The different Topology levels have names:
>>      - Node - Drawer - Book - sockets or physical package - core
>>
>> Threads:
>>      Multithreading, is not part of the topology as described by the
>>      SYSIB 15.1.x
>>
>> The interest of the guest to know the CPU topology is obviously to be
>> able to optimise the load balancing and the migration of threads.
>> KVM will have the same interest concerning vCPUs scheduling and cache
>> optimisation.
>>
>>
>> ==========
>> The design
>> ==========
>>
>> 1) To be ready for hotplug, I chose an Object oriented design
>> of the topology containers:
>> - A node is a bridge on the SYSBUS and defines a "node bus"
>> - A drawer is hotplug on the "node bus"
>> - A book on the "drawer bus"
>> - A socket on the "book bus"
>> - And the CPU Topology List Entry (CPU-TLE)sits on the socket bus.
>> These objects will be enhanced with the cache information when
>> NUMA is implemented.
>>
>> This also allows for easy retrieval when building the different SYSIB
>> for Store Topology System Information (STSI)
>>
>> 2) Perform Topology Function (PTF) instruction is made available to the
>> guest with a new KVM capability and intercepted in QEMU, allowing the
>> guest to pool for topology changes.
>>
>>
>> =====================
>> Features and TBD list
>> =====================
>>
>> - There is no direct match between IDs shown by:
>>      - lscpu (unrelated numbered list),
>>      - SYSIB 15.1.x (topology ID)
>>
>> - The CPU number, left column of lscpu, is used to reference a CPU
>>      by Linux tools
>>      While the CPU address is used by QEMU for hotplug.
>>
>> - Effect of -smp parsing on the topology with an example:
>>      -smp 9,sockets=4,cores=4,maxcpus=16
>>
>>      We have 4 socket each holding 4 cores so that we have a maximum
>>      of 16 CPU, 9 of them are active on boot. (Should be obvious)
>>
>> # lscpu -e
>> CPU NODE DRAWER BOOK SOCKET CORE L1d:L1i:L2d:L2i ONLINE CONFIGURED POLARIZATION ADDRESS
>>    0    0      0    0      0    0 0:0:0:0            yes yes        horizontal   0
>>    1    0      0    0      0    1 1:1:1:1            yes yes        horizontal   1
>>    2    0      0    0      0    2 2:2:2:2            yes yes        horizontal   2
>>    3    0      0    0      0    3 3:3:3:3            yes yes        horizontal   3
>>    4    0      0    0      1    4 4:4:4:4            yes yes        horizontal   4
>>    5    0      0    0      1    5 5:5:5:5            yes yes        horizontal   5
>>    6    0      0    0      1    6 6:6:6:6            yes yes        horizontal   6
>>    7    0      0    0      1    7 7:7:7:7            yes yes        horizontal   7
>>    8    0      0    0      2    8 8:8:8:8            yes yes        horizontal   8
>> #
>>
>>
>> - To plug a new CPU inside the topology one can simply use the CPU
>>      address like in:
>> (qemu) device_add host-s390x-cpu,core-id=12
>> # lscpu -e
>> CPU NODE DRAWER BOOK SOCKET CORE L1d:L1i:L2d:L2i ONLINE CONFIGURED POLARIZATION ADDRESS
>>    0    0      0    0      0    0 0:0:0:0            yes yes        horizontal   0
>>    1    0      0    0      0    1 1:1:1:1            yes yes        horizontal   1
>>    2    0      0    0      0    2 2:2:2:2            yes yes        horizontal   2
>>    3    0      0    0      0    3 3:3:3:3            yes yes        horizontal   3
>>    4    0      0    0      1    4 4:4:4:4            yes yes        horizontal   4
>>    5    0      0    0      1    5 5:5:5:5            yes yes        horizontal   5
>>    6    0      0    0      1    6 6:6:6:6            yes yes        horizontal   6
>>    7    0      0    0      1    7 7:7:7:7            yes yes        horizontal   7
>>    8    0      0    0      2    8 8:8:8:8            yes yes        horizontal   8
>>    9    -      -    -      -    - :::                 no yes        horizontal   12
>> # chcpu -e 9
>> CPU 9 enabled
>> # lscpu -e
>> CPU NODE DRAWER BOOK SOCKET CORE L1d:L1i:L2d:L2i ONLINE CONFIGURED POLARIZATION ADDRESS
>>    0    0      0    0      0    0 0:0:0:0            yes yes        horizontal   0
>>    1    0      0    0      0    1 1:1:1:1            yes yes        horizontal   1
>>    2    0      0    0      0    2 2:2:2:2            yes yes        horizontal   2
>>    3    0      0    0      0    3 3:3:3:3            yes yes        horizontal   3
>>    4    0      0    0      1    4 4:4:4:4            yes yes        horizontal   4
>>    5    0      0    0      1    5 5:5:5:5            yes yes        horizontal   5
>>    6    0      0    0      1    6 6:6:6:6            yes yes        horizontal   6
>>    7    0      0    0      1    7 7:7:7:7            yes yes        horizontal   7
>>    8    0      0    0      2    8 8:8:8:8            yes yes        horizontal   8
>>    9    0      0    0      3    9 9:9:9:9            yes yes        horizontal   12
>> #
>>
>> It is up to the admin level, Libvirt for example, to pin the righ CPU to the right
>> vCPU, but as we can see without NUMA, chosing separate sockets for CPUs is not easy
>> without hotplug because without information the code will assign the vCPU and fill
>> the sockets one after the other.
>> Note that this is also the default behavior on the LPAR.
>>
>>
>> Regards,
>> Pierre
>>
>> Pierre Morel (5):
>>    s390x: kvm: topology: Linux header update
>>    s390x: kvm: topology: interception of PTF instruction
>>    s390x: topology: CPU topology objects and structures
>>    s390x: topology: Topology list entries and SYSIB 15.x.x
>>    s390x: topology: implementating Store Topology System Information
>>
>>   hw/s390x/cpu-topology.c            | 334 +++++++++++++++++++++++++++++
>>   hw/s390x/meson.build               |   1 +
>>   hw/s390x/s390-virtio-ccw.c         |  49 +++++
>>   include/hw/s390x/cpu-topology.h    |  67 ++++++
>>   include/hw/s390x/s390-virtio-ccw.h |   7 +
>>   linux-headers/linux/kvm.h          |   1 +
>>   target/s390x/cpu.h                 |  44 ++++
>>   target/s390x/kvm/kvm.c             | 122 +++++++++++
>>   8 files changed, 625 insertions(+)
>>   create mode 100644 hw/s390x/cpu-topology.c
>>   create mode 100644 include/hw/s390x/cpu-topology.h
>>
> 

Re: [PATCH v2 0/5] s390x: CPU Topology

[Pierre Morel]

On 8/30/21 11:54 AM, Christian Borntraeger wrote:
> 
> 
> On 26.08.21 11:22, Pierre Morel wrote:
>>
>> a gentle ping :)
>>
>> I would like if you have time, comments on the architecture I propose,
>> if the handling is done at the right level, KVM vs QEMU.
> 
> Do we expect changes in this series due to the discussed changes of PTF 
> interpretion?

No we do not expect any change.
The configuration topology feature is enabled in QEMU if KVM provides 
the KVM_CAP_S390_CPU_TOPOLOGY.

Interpretation is set in KVM if QEMU activated the feature and if the 
host supports the configuration topology feature.

If the host does not support the feature, interception is done and QEMU 
emulates the PTF instruction.

The feature can be fenced with qemu -cpu XX,ctop=off for CPU already 
having the feature activated as default in QEMU (newer GEN10_GA1)


Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v2 2/5] s390x: kvm: topology: interception of PTF instruction

[Thomas Huth]

On 22/07/2021 19.42, Pierre Morel wrote:
> Interception of the PTF instruction depending on the new
> KVM_CAP_S390_CPU_TOPOLOGY KVM extension.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   hw/s390x/s390-virtio-ccw.c         | 45 ++++++++++++++++++++++++++++++
>   include/hw/s390x/s390-virtio-ccw.h |  7 +++++
>   target/s390x/kvm/kvm.c             | 21 ++++++++++++++
>   3 files changed, 73 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index e4b18aef49..500e856974 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -404,6 +404,49 @@ static void s390_pv_prepare_reset(S390CcwMachineState *ms)
>       s390_pv_prep_reset();
>   }
>   
> +int s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra)
> +{
> +    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
> +    CPUS390XState *env = &cpu->env;
> +    uint64_t reg = env->regs[r1];
> +    uint8_t fc = reg & S390_TOPO_FC_MASK;
> +
> +    if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
> +        s390_program_interrupt(env, PGM_OPERAND, ra);

I think that should be PGM_OPERATION instead?

> +        return 0;
> +    }
> +
> +    if (env->psw.mask & PSW_MASK_PSTATE) {
> +        s390_program_interrupt(env, PGM_PRIVILEGED, ra);
> +        return 0;
> +    }
> +
> +    if (reg & ~S390_TOPO_FC_MASK) {
> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> +        return 0;
> +    }
> +
> +    switch (fc) {
> +    case 0:    /* Horizontal polarization is already set */
> +        env->regs[r1] = S390_PTF_REASON_DONE; > +        return 2;
> +    case 1:    /* Vertical polarization is not supported */
> +        env->regs[r1] = S390_PTF_REASON_NONE;


This way, you're clearing the bits in the FC field. Is this intended by the 
architecture? If I get the PoP right, it just sets the bits in the RC field, 
but likely it should not clear the 1 in the FC field? Did you try on LPAR or 
z/VM to see what happens there?

> +        return 2;
> +    case 2:    /* Report if a topology change report is pending */
> +        if (ms->topology_change_report_pending) {
> +            ms->topology_change_report_pending = false;
> +            return 1;
> +        }
> +        return 0;
> +    default:
> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> +        break;

Just a matter of taste - but you could drop the break here.

> +    }
> +
> +    return 0;
> +}
> +
>   static void s390_machine_reset(MachineState *machine)
>   {
>       S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
> @@ -433,6 +476,8 @@ static void s390_machine_reset(MachineState *machine)
>           run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
>           break;
>       case S390_RESET_MODIFIED_CLEAR:
> +        /* clear topology_change_report pending condition on subsystem reset */
> +        ms->topology_change_report_pending = false;
>           /*
>            * Susbsystem reset needs to be done before we unshare memory
>            * and lose access to VIRTIO structures in guest memory.
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 3331990e02..fbde357332 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -27,9 +27,16 @@ struct S390CcwMachineState {
>       bool aes_key_wrap;
>       bool dea_key_wrap;
>       bool pv;
> +    bool topology_change_report_pending;
>       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
> +int s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra);
> +
>   struct S390CcwMachineClass {
>       /*< private >*/
>       MachineClass parent_class;
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 5b1fdb55c4..9a0c13d4ac 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
> @@ -1452,6 +1453,16 @@ static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
>       }
>   }
>   
> +static int kvm_handle_ptf(S390CPU *cpu, struct kvm_run *run)
> +{
> +    uint8_t r1 = (run->s390_sieic.ipb >> 20) & 0x0f;
> +    uint8_t ret;

Why is ret an uint8_t ? s390_handle_ptf() returns an "int".

> +    ret = s390_handle_ptf(cpu, r1, RA_IGNORED);
> +    setcc(cpu, ret);
> +    return 0; > +}

  Thomas

Re: [PATCH v2 3/5] s390x: topology: CPU topology objects and structures

[Thomas Huth]

On 22/07/2021 19.42, Pierre Morel wrote:
> We use new objects to have a dynamic administration of the CPU topology.
> The highier level object is the S390 book. In a first implementation

s/highier/higher/ ... or highest ?

> we will have only a single S390 book.
> The book is built as a SYSBUS bridge during the CPU initialisation. > Every object under this single book will be build dynamically
> immediately after a CPU has be realized if it is needed.
> The CPU will fill the sockets once after the other, according to the
> number of core per socket defined during the smp parsing.
> 
> Each CPU inside a socket will be represented by a bit in a 64bit
> unsigned long. Set on plug and clear on unplug of a CPU.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   hw/s390x/cpu-topology.c         | 334 ++++++++++++++++++++++++++++++++
>   hw/s390x/meson.build            |   1 +
>   hw/s390x/s390-virtio-ccw.c      |   4 +
>   include/hw/s390x/cpu-topology.h |  67 +++++++
>   4 files changed, 406 insertions(+)
>   create mode 100644 hw/s390x/cpu-topology.c
>   create mode 100644 include/hw/s390x/cpu-topology.h
> 
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> new file mode 100644
> index 0000000000..73f91d5334
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c
> @@ -0,0 +1,334 @@
> +/*
> + * CPU Topology
> + *
> + * Copyright 2021 IBM Corp.
> + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
> +
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/error-report.h"
> +#include "hw/sysbus.h"
> +#include "hw/s390x/cpu-topology.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/boards.h"
> +#include "qemu/typedefs.h"
> +#include "target/s390x/cpu.h"
> +#include "hw/s390x/s390-virtio-ccw.h"
> +
> +static S390TopologyCores *s390_create_cores(S390TopologySocket *socket,
> +                                            int origin)
> +{
> +    DeviceState *dev;
> +    S390TopologyCores *cores;
> +    const MachineState *ms = MACHINE(qdev_get_machine());
> +
> +    if (socket->bus->num_children >= ms->smp.cores) {
> +        return NULL;
> +    }
> +
> +    dev = qdev_new(TYPE_S390_TOPOLOGY_CORES);
> +    qdev_realize_and_unref(dev, socket->bus, &error_fatal);
> +
> +    cores = S390_TOPOLOGY_CORES(dev);
> +    cores->origin = origin;
> +    socket->cnt += 1;
> +
> +    return cores;
> +}
> +
> +static S390TopologySocket *s390_create_socket(S390TopologyBook *book, int id)
> +{
> +    DeviceState *dev;
> +    S390TopologySocket *socket;
> +    const MachineState *ms = MACHINE(qdev_get_machine());
> +
> +    if (book->bus->num_children >= ms->smp.sockets) {
> +        return NULL;
> +    }
> +
> +    dev = qdev_new(TYPE_S390_TOPOLOGY_SOCKET);
> +    qdev_realize_and_unref(dev, book->bus, &error_fatal);
> +
> +    socket = S390_TOPOLOGY_SOCKET(dev);
> +    socket->socket_id = id;
> +    book->cnt++;
> +
> +    return socket;
> +}
> +

Could you add a short comment in front of s390_get_cores, describing what it 
does? From the name, I would not expect that this function creates something 
automatically, so a comment explaining this would be nice.

> +static S390TopologyCores *s390_get_cores(S390TopologySocket *socket, int origin)
> +{
> +    S390TopologyCores *cores;
> +    BusChild *kid;
> +
> +    QTAILQ_FOREACH(kid, &socket->bus->children, sibling) {
> +        cores = S390_TOPOLOGY_CORES(kid->child);
> +        if (cores->origin == origin) {
> +            return cores;
> +        }
> +    }
> +    return s390_create_cores(socket, origin);
> +}

dito.

> +static S390TopologySocket *s390_get_socket(S390TopologyBook *book,
> +                                           int socket_id)
> +{
> +    S390TopologySocket *socket;
> +    BusChild *kid;
> +
> +    QTAILQ_FOREACH(kid, &book->bus->children, sibling) {
> +        socket = S390_TOPOLOGY_SOCKET(kid->child);
> +        if (socket->socket_id == socket_id) {
> +            return socket;
> +        }
> +    }
> +    return s390_create_socket(book, socket_id);
> +}
> +
> +/*
> + * s390_topology_new_cpu:
> + * @core_id: the core ID is machine wide
> + *
> + * We have a single book returned by s390_get_topology(),
> + * then we build the hierarchy on demand.
> + * Note that we do not destroy the hierarchy on error creating
> + * an entry in the topology, we just keept it empty.

s/keept/keep/

> + * We do not need to worry about not finding a topology level
> + * entry this would have been catched during smp parsing.

s/catched/caught/

> + */
> +void s390_topology_new_cpu(int core_id)
> +{
> +    const MachineState *ms = MACHINE(qdev_get_machine());
> +    S390TopologyBook *book;
> +    S390TopologySocket *socket;
> +    S390TopologyCores *cores;
> +    int cores_per_socket, sock_idx;
> +    int origin, bit;
> +
> +    book = s390_get_topology();
> +
> +    cores_per_socket = ms->smp.max_cpus / ms->smp.sockets;
> +
> +    sock_idx = (core_id / cores_per_socket);
> +    socket = s390_get_socket(book, sock_idx);
> +
> +    /*
> +     * We assert that all CPU are identical IFL, shared and

assert? Or did you mean assume? When reading something like "assert" I'd 
expect some error checking here...?

> +     * horizontal topology, the only reason to have several
> +     * S390TopologyCores is to have more than 64 CPUs.
> +     */
> +    origin = 64 * (core_id / 64);
> +
> +    cores = s390_get_cores(socket, origin);
> +
> +    bit = 63 - (core_id - origin);
> +    set_bit(bit, &cores->mask);
> +    cores->origin = origin;
> +}
> +
> +/*
> + * Setting the first topology: 1 book, 1 socket
> + * This is enough for 64 cores if the topology is flat (single socket)
> + */
> +void s390_topology_setup(MachineState *ms)
> +{
> +    DeviceState *dev;
> +
> +    /* Create BOOK bridge device */
> +    dev = qdev_new(TYPE_S390_TOPOLOGY_BOOK);
> +    object_property_add_child(qdev_get_machine(),
> +                              TYPE_S390_TOPOLOGY_BOOK, OBJECT(dev));
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +}
> +
> +S390TopologyBook *s390_get_topology(void)
> +{
> +    static S390TopologyBook *book;
> +
> +    if (!book) {
> +        book = S390_TOPOLOGY_BOOK(
> +            object_resolve_path(TYPE_S390_TOPOLOGY_BOOK, NULL));
> +        assert(book != NULL);
> +    }
> +
> +    return book;
> +}
> +
> +/* --- CORES Definitions --- */
> +
> +static Property s390_topology_cores_properties[] = {
> +    DEFINE_PROP_BOOL("dedicated", S390TopologyCores, dedicated, false),
> +    DEFINE_PROP_UINT8("polarity", S390TopologyCores, polarity,
> +                      S390_TOPOLOGY_POLARITY_H),
> +    DEFINE_PROP_UINT8("cputype", S390TopologyCores, cputype,
> +                      S390_TOPOLOGY_CPU_TYPE),
> +    DEFINE_PROP_UINT16("origin", S390TopologyCores, origin, 0),
> +    DEFINE_PROP_UINT64("mask", S390TopologyCores, mask, 0),
> +    DEFINE_PROP_UINT8("id", S390TopologyCores, id, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void cpu_cores_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> +
> +    device_class_set_props(dc, s390_topology_cores_properties);
> +    hc->unplug = qdev_simple_device_unplug_cb;
> +    dc->bus_type = TYPE_S390_TOPOLOGY_SOCKET_BUS;
> +    dc->desc = "topology cpu entry";
> +}
> +
> +static const TypeInfo cpu_cores_info = {
> +    .name          = TYPE_S390_TOPOLOGY_CORES,
> +    .parent        = TYPE_DEVICE,
> +    .instance_size = sizeof(S390TopologyCores),
> +    .class_init    = cpu_cores_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_HOTPLUG_HANDLER },
> +        { }
> +    }
> +};
> +
> +/* --- SOCKETS Definitions --- */
> +static Property s390_topology_socket_properties[] = {
> +    DEFINE_PROP_UINT8("socket_id", S390TopologySocket, socket_id, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static char *socket_bus_get_dev_path(DeviceState *dev)
> +{
> +    S390TopologySocket *socket = S390_TOPOLOGY_SOCKET(dev);
> +    DeviceState *book = dev->parent_bus->parent;
> +    char *id = qdev_get_dev_path(book);
> +    char *ret;
> +
> +    if (id) {
> +        ret = g_strdup_printf("%s:%02d", id, socket->socket_id);
> +        g_free(id);
> +    } else {
> +        ret = g_malloc(6);
> +        snprintf(ret, 6, "_:%02d", socket->socket_id);

Why don't you use g_strdup_printf() here?

> +    }
> +
> +    return ret;
> +}
> +
> +static void socket_bus_class_init(ObjectClass *oc, void *data)
> +{
> +    BusClass *k = BUS_CLASS(oc);
> +
> +    k->get_dev_path = socket_bus_get_dev_path;
> +    k->max_dev = S390_MAX_SOCKETS;
> +}
> +
> +static const TypeInfo socket_bus_info = {
> +    .name = TYPE_S390_TOPOLOGY_SOCKET_BUS,
> +    .parent = TYPE_BUS,
> +    .instance_size = 0,
> +    .class_init = socket_bus_class_init,
> +};
> +
> +static void s390_socket_device_realize(DeviceState *dev, Error **errp)
> +{
> +    S390TopologySocket *socket = S390_TOPOLOGY_SOCKET(dev);
> +    BusState *bus;
> +
> +    bus = qbus_create(TYPE_S390_TOPOLOGY_SOCKET_BUS, dev,
> +                      TYPE_S390_TOPOLOGY_SOCKET_BUS);
> +    qbus_set_hotplug_handler(bus, OBJECT(dev));
> +    socket->bus = bus;
> +}
> +
> +static void socket_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> +
> +    hc->unplug = qdev_simple_device_unplug_cb;
> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +    dc->bus_type = TYPE_S390_TOPOLOGY_BOOK_BUS;
> +    dc->realize = s390_socket_device_realize;
> +    device_class_set_props(dc, s390_topology_socket_properties);
> +    dc->desc = "topology socket";
> +}
> +
> +static const TypeInfo socket_info = {
> +    .name          = TYPE_S390_TOPOLOGY_SOCKET,
> +    .parent        = TYPE_DEVICE,
> +    .instance_size = sizeof(S390TopologySocket),
> +    .class_init    = socket_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_HOTPLUG_HANDLER },
> +        { }
> +    }
> +};
> +
> +static char *book_bus_get_dev_path(DeviceState *dev)
> +{
> +    return g_strdup_printf("00");
> +}
> +
> +static void book_bus_class_init(ObjectClass *oc, void *data)
> +{
> +    BusClass *k = BUS_CLASS(oc);
> +
> +    k->get_dev_path = book_bus_get_dev_path;
> +    k->max_dev = S390_MAX_BOOKS;
> +}
> +
> +static const TypeInfo book_bus_info = {
> +    .name = TYPE_S390_TOPOLOGY_BOOK_BUS,
> +    .parent = TYPE_BUS,
> +    .instance_size = 0,
> +    .class_init = book_bus_class_init,
> +};
> +
> +static void s390_book_device_realize(DeviceState *dev, Error **errp)
> +{
> +    S390TopologyBook *book = S390_TOPOLOGY_BOOK(dev);
> +    BusState *bus;
> +
> +    bus = qbus_create(TYPE_S390_TOPOLOGY_BOOK_BUS, dev,
> +                      TYPE_S390_TOPOLOGY_BOOK_BUS);
> +    qbus_set_hotplug_handler(bus, OBJECT(dev));
> +    book->bus = bus;
> +}
> +
> +static void book_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> +
> +    hc->unplug = qdev_simple_device_unplug_cb;
> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +    dc->realize = s390_book_device_realize;
> +    dc->desc = "topology book";
> +}
> +
> +static const TypeInfo book_info = {
> +    .name          = TYPE_S390_TOPOLOGY_BOOK,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(S390TopologyBook),
> +    .class_init    = book_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_HOTPLUG_HANDLER },
> +        { }
> +    }
> +};

I didn't spot any migration related code in here ... is this already 
migration-safe?

  Thomas

Re: [PATCH v2 4/5] s390x: topology: Topology list entries and SYSIB 15.x.x

[Thomas Huth]

On 22/07/2021 19.42, Pierre Morel wrote:
> We define the CPU type Topology List Entry and the
> Container type Topology List Entry to implement SYSIB 15.1.x
> 
> This patch will be squatched with the next patch.

s/squatched/squashed/

... anyway, why did you sent it separately?

  Thomas

Re: [PATCH v2 4/5] s390x: topology: Topology list entries and SYSIB 15.x.x

[Thomas Huth]

On 22/07/2021 19.42, Pierre Morel wrote:
> We define the CPU type Topology List Entry and the
> Container type Topology List Entry to implement SYSIB 15.1.x
> 
> This patch will be squatched with the next patch.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   target/s390x/cpu.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 44 insertions(+)
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index b26ae8fff2..d573ba205e 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -564,6 +564,50 @@ typedef union SysIB {
>   } SysIB;
>   QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
>   
> +/* CPU type Topology List Entry */
> +typedef struct SysIBTl_cpu {
> +        uint8_t nl;
> +        uint8_t reserved0[3];
> +        uint8_t reserved1:5;
> +        uint8_t dedicated:1;
> +        uint8_t polarity:2;
> +        uint8_t type;
> +        uint16_t origin;
> +        uint64_t mask;
> +} QEMU_PACKED SysIBTl_cpu;
> +
> +/* Container type Topology List Entry */
> +typedef struct SysIBTl_container {
> +        uint8_t nl;
> +        uint8_t reserved[6];
> +        uint8_t id;
> +} QEMU_PACKED SysIBTl_container;

Any chance that you could drop the PACKED from th above two structs and use 
QEMU_BUILD_BUG_ON to check the size instead?
... PACKED was causing some build issues on other architectures in the past 
already, so we should try to avoid it if possible.

  Thomas

Re: [PATCH v2 5/5] s390x: topology: implementating Store Topology System Information

[Thomas Huth]

On 22/07/2021 19.42, Pierre Morel wrote:
> The handling of STSI is enhenced with the interception of the

s/enhenced/enhanced/

> function code 15 for storing CPU topology.
> 
> Using the objects built during the pluging of CPU, we build the
> SYSIB 15_1_x structures.
> 
> With this patch the maximum MNEST level is 2, this is also
> the only level allowed and only SYSIB 15_1_2 will be built.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   target/s390x/kvm/kvm.c | 101 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 101 insertions(+)
> 
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 9a0c13d4ac..0194756e6a 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -52,6 +52,7 @@
>   #include "hw/s390x/s390-virtio-ccw.h"
>   #include "hw/s390x/s390-virtio-hcall.h"
>   #include "hw/s390x/pv.h"
> +#include "hw/s390x/cpu-topology.h"
>   
>   #ifndef DEBUG_KVM
>   #define DEBUG_KVM  0
> @@ -1907,6 +1908,102 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64 addr, uint8_t ar)
>       }
>   }
>   
> +static int stsi_15_container(void *p, int nl, int id)
> +{
> +    SysIBTl_container *tle = (SysIBTl_container *)p;
> +
> +    tle->nl = nl;
> +    tle->id = id;
> +
> +    return sizeof(*tle);
> +}
> +
> +static int stsi_15_cpus(void *p, S390TopologyCores *cd)
> +{
> +    SysIBTl_cpu *tle = (SysIBTl_cpu *)p;
> +
> +    tle->nl = 0;
> +    tle->dedicated = cd->dedicated;
> +    tle->polarity = cd->polarity;
> +    tle->type = cd->cputype;
> +    tle->origin = cd->origin;
> +    tle->mask = cd->mask;
> +
> +    return sizeof(*tle);
> +}
> +
> +static int set_socket(const MachineState *ms, void *p,
> +                      S390TopologySocket *socket)
> +{
> +    BusChild *kid;
> +    int l, len = 0;
> +
> +    len += stsi_15_container(p, 1, socket->socket_id);
> +    p += len;
> +
> +    QTAILQ_FOREACH_REVERSE(kid, &socket->bus->children, sibling) {
> +        l = stsi_15_cpus(p, S390_TOPOLOGY_CORES(kid->child));
> +        p += l;
> +        len += l;
> +    }
> +    return len;
> +}
> +
> +static void insert_stsi_15_1_2(const MachineState *ms, void *p)
> +{
> +    S390TopologyBook *book;
> +    SysIB_151x *sysib;
> +    BusChild *kid;
> +    int level = 2;
> +    int len, l;
> +
> +    sysib = (SysIB_151x *)p;
> +    sysib->mnest = level;
> +    sysib->mag[TOPOLOGY_NR_MAG2] = ms->smp.sockets;
> +    sysib->mag[TOPOLOGY_NR_MAG1] = ms->smp.cores;
> +
> +    book = s390_get_topology();
> +    len = sizeof(SysIB_151x);
> +    p += len;
> +
> +    QTAILQ_FOREACH_REVERSE(kid, &book->bus->children, sibling) {
> +        l = set_socket(ms, p, S390_TOPOLOGY_SOCKET(kid->child));
> +        p += l;
> +        len += l;
> +    }
> +
> +    sysib->length = len;
> +}
> +
> +static void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
> +{
> +    const MachineState *machine = MACHINE(qdev_get_machine());
> +    void *p;
> +    int ret, cc;
> +
> +    /*
> +     * Until the SCLP STSI Facility reporting the MNEST value is used,
> +     * a sel2 value of 2 is the only value allowed in STSI 15.1.x.
> +     */
> +    if (sel2 != 2) {
> +        setcc(cpu, 3);
> +        return;
> +    }
> +
> +    p = g_malloc0(4096);

Use TARGET_PAGE_SIZE instead of magic value 4096?

> +   insert_stsi_15_1_2(machine, p);

Wrong indentation (3 instead of 4 spaces).

> +    if (s390_is_pv()) {
> +        ret = s390_cpu_pv_mem_write(cpu, 0, p, 4096);
> +    } else {
> +        ret = s390_cpu_virt_mem_write(cpu, addr, ar, p, 4096);

TARGET_PAGE_SIZE?

> +    }
> +    cc = ret ? 3 : 0;
> +    setcc(cpu, cc);
> +    g_free(p);
> +}
> +
>   static int handle_stsi(S390CPU *cpu)
>   {
>       CPUState *cs = CPU(cpu);
> @@ -1920,6 +2017,10 @@ static int handle_stsi(S390CPU *cpu)
>           /* Only sysib 3.2.2 needs post-handling for now. */
>           insert_stsi_3_2_2(cpu, run->s390_stsi.addr, run->s390_stsi.ar);
>           return 0;
> +    case 15:
> +        insert_stsi_15_1_x(cpu, run->s390_stsi.sel2, run->s390_stsi.addr,
> +                           run->s390_stsi.ar);
> +        return 0;
>       default:
>           return 0;
>       }
> 

  Thomas

Re: [PATCH v2 2/5] s390x: kvm: topology: interception of PTF instruction

[Pierre Morel]

On 9/6/21 7:21 PM, Thomas Huth wrote:
> On 22/07/2021 19.42, Pierre Morel wrote:
>> Interception of the PTF instruction depending on the new
>> KVM_CAP_S390_CPU_TOPOLOGY KVM extension.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/s390-virtio-ccw.c         | 45 ++++++++++++++++++++++++++++++
>>   include/hw/s390x/s390-virtio-ccw.h |  7 +++++
>>   target/s390x/kvm/kvm.c             | 21 ++++++++++++++
>>   3 files changed, 73 insertions(+)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index e4b18aef49..500e856974 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -404,6 +404,49 @@ static void 
>> s390_pv_prepare_reset(S390CcwMachineState *ms)
>>       s390_pv_prep_reset();
>>   }
>> +int s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra)
>> +{
>> +    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
>> +    CPUS390XState *env = &cpu->env;
>> +    uint64_t reg = env->regs[r1];
>> +    uint8_t fc = reg & S390_TOPO_FC_MASK;
>> +
>> +    if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
>> +        s390_program_interrupt(env, PGM_OPERAND, ra);
> 
> I think that should be PGM_OPERATION instead?

Right, I thought I did do the modification since v1.
Seems I forgot or it get lost :(
I will take care of this for the next time.

> 
>> +        return 0;
>> +    }
>> +
>> +    if (env->psw.mask & PSW_MASK_PSTATE) {
>> +        s390_program_interrupt(env, PGM_PRIVILEGED, ra);
>> +        return 0;
>> +    }
>> +
>> +    if (reg & ~S390_TOPO_FC_MASK) {
>> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> +        return 0;
>> +    }
>> +
>> +    switch (fc) {
>> +    case 0:    /* Horizontal polarization is already set */
>> +        env->regs[r1] = S390_PTF_REASON_DONE; > +        return 2;
>> +    case 1:    /* Vertical polarization is not supported */
>> +        env->regs[r1] = S390_PTF_REASON_NONE;
> 
> 
> This way, you're clearing the bits in the FC field. Is this intended by 
> the architecture? If I get the PoP right, it just sets the bits in the 
> RC field, but likely it should not clear the 1 in the FC field? Did you 
> try on LPAR or z/VM to see what happens there?

You are right, the FC field is not changed on LPAR.

> 
>> +        return 2;
>> +    case 2:    /* Report if a topology change report is pending */
>> +        if (ms->topology_change_report_pending) {
>> +            ms->topology_change_report_pending = false;
>> +            return 1;
>> +        }
>> +        return 0;
>> +    default:
>> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> +        break;
> 
> Just a matter of taste - but you could drop the break here.

ok

> 
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static void s390_machine_reset(MachineState *machine)
>>   {
>>       S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
>> @@ -433,6 +476,8 @@ static void s390_machine_reset(MachineState *machine)
>>           run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
>>           break;
>>       case S390_RESET_MODIFIED_CLEAR:
>> +        /* clear topology_change_report pending condition on 
>> subsystem reset */
>> +        ms->topology_change_report_pending = false;
>>           /*
>>            * Susbsystem reset needs to be done before we unshare memory
>>            * and lose access to VIRTIO structures in guest memory.
>> diff --git a/include/hw/s390x/s390-virtio-ccw.h 
>> b/include/hw/s390x/s390-virtio-ccw.h
>> index 3331990e02..fbde357332 100644
>> --- a/include/hw/s390x/s390-virtio-ccw.h
>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>> @@ -27,9 +27,16 @@ struct S390CcwMachineState {
>>       bool aes_key_wrap;
>>       bool dea_key_wrap;
>>       bool pv;
>> +    bool topology_change_report_pending;
>>       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
>> +int s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra);
>> +
>>   struct S390CcwMachineClass {
>>       /*< private >*/
>>       MachineClass parent_class;
>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>> index 5b1fdb55c4..9a0c13d4ac 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
>> @@ -1452,6 +1453,16 @@ static int kvm_mpcifc_service_call(S390CPU 
>> *cpu, struct kvm_run *run)
>>       }
>>   }
>> +static int kvm_handle_ptf(S390CPU *cpu, struct kvm_run *run)
>> +{
>> +    uint8_t r1 = (run->s390_sieic.ipb >> 20) & 0x0f;
>> +    uint8_t ret;
> 
> Why is ret an uint8_t ? s390_handle_ptf() returns an "int".

No reason, I must have use the same type as the line before.
I change to int.

> 
>> +    ret = s390_handle_ptf(cpu, r1, RA_IGNORED);
>> +    setcc(cpu, ret);
>> +    return 0; > +}
> 
>   Thomas
> 

Thanks for the comments,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v2 3/5] s390x: topology: CPU topology objects and structures

[Pierre Morel]

On 9/7/21 9:32 AM, Thomas Huth wrote:
> On 22/07/2021 19.42, Pierre Morel wrote:
>> We use new objects to have a dynamic administration of the CPU topology.
>> The highier level object is the S390 book. In a first implementation
> 
> s/highier/higher/ ... or highest ?

thx I chose highest

And I modify to
"
The highest level object in this implementation is the s390 book and in 
this first implementation of CPU topology for S390 we have a single book.
"

> 
>> we will have only a single S390 book.
>> The book is built as a SYSBUS bridge during the CPU initialisation. > 
>> Every object under this single book will be build dynamically
>> immediately after a CPU has be realized if it is needed.
>> The CPU will fill the sockets once after the other, according to the
>> number of core per socket defined during the smp parsing.
>>
>> Each CPU inside a socket will be represented by a bit in a 64bit
>> unsigned long. Set on plug and clear on unplug of a CPU.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/cpu-topology.c         | 334 ++++++++++++++++++++++++++++++++
>>   hw/s390x/meson.build            |   1 +
>>   hw/s390x/s390-virtio-ccw.c      |   4 +
>>   include/hw/s390x/cpu-topology.h |  67 +++++++
>>   4 files changed, 406 insertions(+)
>>   create mode 100644 hw/s390x/cpu-topology.c
>>   create mode 100644 include/hw/s390x/cpu-topology.h
>>
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> new file mode 100644
>> index 0000000000..73f91d5334
>> --- /dev/null
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -0,0 +1,334 @@
>> +/*
>> + * CPU Topology
>> + *
>> + * Copyright 2021 IBM Corp.
>> + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
>> +
>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
>> (at
>> + * your option) any later version. See the COPYING file in the top-level
>> + * directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qemu/error-report.h"
>> +#include "hw/sysbus.h"
>> +#include "hw/s390x/cpu-topology.h"
>> +#include "hw/qdev-properties.h"
>> +#include "hw/boards.h"
>> +#include "qemu/typedefs.h"
>> +#include "target/s390x/cpu.h"
>> +#include "hw/s390x/s390-virtio-ccw.h"
>> +
>> +static S390TopologyCores *s390_create_cores(S390TopologySocket *socket,
>> +                                            int origin)
>> +{
>> +    DeviceState *dev;
>> +    S390TopologyCores *cores;
>> +    const MachineState *ms = MACHINE(qdev_get_machine());
>> +
>> +    if (socket->bus->num_children >= ms->smp.cores) {
>> +        return NULL;
>> +    }
>> +
>> +    dev = qdev_new(TYPE_S390_TOPOLOGY_CORES);
>> +    qdev_realize_and_unref(dev, socket->bus, &error_fatal);
>> +
>> +    cores = S390_TOPOLOGY_CORES(dev);
>> +    cores->origin = origin;
>> +    socket->cnt += 1;
>> +
>> +    return cores;
>> +}
>> +
>> +static S390TopologySocket *s390_create_socket(S390TopologyBook *book, 
>> int id)
>> +{
>> +    DeviceState *dev;
>> +    S390TopologySocket *socket;
>> +    const MachineState *ms = MACHINE(qdev_get_machine());
>> +
>> +    if (book->bus->num_children >= ms->smp.sockets) {
>> +        return NULL;
>> +    }
>> +
>> +    dev = qdev_new(TYPE_S390_TOPOLOGY_SOCKET);
>> +    qdev_realize_and_unref(dev, book->bus, &error_fatal);
>> +
>> +    socket = S390_TOPOLOGY_SOCKET(dev);
>> +    socket->socket_id = id;
>> +    book->cnt++;
>> +
>> +    return socket;
>> +}
>> +
> 
> Could you add a short comment in front of s390_get_cores, describing 
> what it does? From the name, I would not expect that this function 
> creates something automatically, so a comment explaining this would be 
> nice.

Yes, I will do.


> 
>> +static S390TopologyCores *s390_get_cores(S390TopologySocket *socket, 
>> int origin)
>> +{
>> +    S390TopologyCores *cores;
>> +    BusChild *kid;
>> +
>> +    QTAILQ_FOREACH(kid, &socket->bus->children, sibling) {
>> +        cores = S390_TOPOLOGY_CORES(kid->child);
>> +        if (cores->origin == origin) {
>> +            return cores;
>> +        }
>> +    }
>> +    return s390_create_cores(socket, origin);
>> +}
> 
> dito.

too

> 
>> +static S390TopologySocket *s390_get_socket(S390TopologyBook *book,
>> +                                           int socket_id)
>> +{
>> +    S390TopologySocket *socket;
>> +    BusChild *kid;
>> +
>> +    QTAILQ_FOREACH(kid, &book->bus->children, sibling) {
>> +        socket = S390_TOPOLOGY_SOCKET(kid->child);
>> +        if (socket->socket_id == socket_id) {
>> +            return socket;
>> +        }
>> +    }
>> +    return s390_create_socket(book, socket_id);
>> +}
>> +
>> +/*
>> + * s390_topology_new_cpu:
>> + * @core_id: the core ID is machine wide
>> + *
>> + * We have a single book returned by s390_get_topology(),
>> + * then we build the hierarchy on demand.
>> + * Note that we do not destroy the hierarchy on error creating
>> + * an entry in the topology, we just keept it empty.
> 
> s/keept/keep/
yes.

> 
>> + * We do not need to worry about not finding a topology level
>> + * entry this would have been catched during smp parsing.
> 
> s/catched/caught/
hum, yes

> 
>> + */
>> +void s390_topology_new_cpu(int core_id)
>> +{
>> +    const MachineState *ms = MACHINE(qdev_get_machine());
>> +    S390TopologyBook *book;
>> +    S390TopologySocket *socket;
>> +    S390TopologyCores *cores;
>> +    int cores_per_socket, sock_idx;
>> +    int origin, bit;
>> +
>> +    book = s390_get_topology();
>> +
>> +    cores_per_socket = ms->smp.max_cpus / ms->smp.sockets;
>> +
>> +    sock_idx = (core_id / cores_per_socket);
>> +    socket = s390_get_socket(book, sock_idx);
>> +
>> +    /*
>> +     * We assert that all CPU are identical IFL, shared and
> 
> assert? Or did you mean assume? When reading something like "assert" I'd 
> expect some error checking here...?

no the right word is "assume"

> 
>> +     * horizontal topology, the only reason to have several
>> +     * S390TopologyCores is to have more than 64 CPUs.
>> +     */
>> +    origin = 64 * (core_id / 64);
>> +
>> +    cores = s390_get_cores(socket, origin);
>> +

...snip...

>> +static char *socket_bus_get_dev_path(DeviceState *dev)
>> +{
>> +    S390TopologySocket *socket = S390_TOPOLOGY_SOCKET(dev);
>> +    DeviceState *book = dev->parent_bus->parent;
>> +    char *id = qdev_get_dev_path(book);
>> +    char *ret;
>> +
>> +    if (id) {
>> +        ret = g_strdup_printf("%s:%02d", id, socket->socket_id);
>> +        g_free(id);
>> +    } else {
>> +        ret = g_malloc(6);
>> +        snprintf(ret, 6, "_:%02d", socket->socket_id);
> 
> Why don't you use g_strdup_printf() here?

I wonder too...


> 
>> +    }
>> +
>> +    return ret;
>> +}
...snip...
>> +    }
>> +};
> 
> I didn't spot any migration related code in here ... is this already 
> migration-safe?

You are right, I must think about this,
I think we must keep the same topology, even false, during the migration 
and set the topology_change_report_pending when the migration is done.

I will add this in the next round.

Thanks a lot for the reviewing
Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v2 4/5] s390x: topology: Topology list entries and SYSIB 15.x.x

[Pierre Morel]

On 9/7/21 9:46 AM, Thomas Huth wrote:
> On 22/07/2021 19.42, Pierre Morel wrote:
>> We define the CPU type Topology List Entry and the
>> Container type Topology List Entry to implement SYSIB 15.1.x
>>
>> This patch will be squatched with the next patch.
> 
> s/squatched/squashed/
> 
> ... anyway, why did you sent it separately?
> 

I thought it would be easier to review.
I will squash it next.

thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v2 4/5] s390x: topology: Topology list entries and SYSIB 15.x.x

[Pierre Morel]

On 9/7/21 9:54 AM, Thomas Huth wrote:
> On 22/07/2021 19.42, Pierre Morel wrote:
>> We define the CPU type Topology List Entry and the
>> Container type Topology List Entry to implement SYSIB 15.1.x
>>
>> This patch will be squatched with the next patch.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   target/s390x/cpu.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 44 insertions(+)
>>
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index b26ae8fff2..d573ba205e 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -564,6 +564,50 @@ typedef union SysIB {
>>   } SysIB;
>>   QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
>> +/* CPU type Topology List Entry */
>> +typedef struct SysIBTl_cpu {
>> +        uint8_t nl;
>> +        uint8_t reserved0[3];
>> +        uint8_t reserved1:5;
>> +        uint8_t dedicated:1;
>> +        uint8_t polarity:2;
>> +        uint8_t type;
>> +        uint16_t origin;
>> +        uint64_t mask;
>> +} QEMU_PACKED SysIBTl_cpu;
>> +
>> +/* Container type Topology List Entry */
>> +typedef struct SysIBTl_container {
>> +        uint8_t nl;
>> +        uint8_t reserved[6];
>> +        uint8_t id;
>> +} QEMU_PACKED SysIBTl_container;
> 
> Any chance that you could drop the PACKED from th above two structs and 
> use QEMU_BUILD_BUG_ON to check the size instead?
> ... PACKED was causing some build issues on other architectures in the 
> past already, so we should try to avoid it if possible.

I can do this.

Thanks
Pierre

> 
>   Thomas
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v2 5/5] s390x: topology: implementating Store Topology System Information

[Pierre Morel]

On 9/7/21 10:00 AM, Thomas Huth wrote:
> On 22/07/2021 19.42, Pierre Morel wrote:
>> The handling of STSI is enhenced with the interception of the
> 
> s/enhenced/enhanced/

yes, thanks

> 
>> function code 15 for storing CPU topology.

...

>> +static void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, 
>> uint8_t ar)
>> +{
>> +    const MachineState *machine = MACHINE(qdev_get_machine());
>> +    void *p;
>> +    int ret, cc;
>> +
>> +    /*
>> +     * Until the SCLP STSI Facility reporting the MNEST value is used,
>> +     * a sel2 value of 2 is the only value allowed in STSI 15.1.x.
>> +     */
>> +    if (sel2 != 2) {
>> +        setcc(cpu, 3);
>> +        return;
>> +    }
>> +
>> +    p = g_malloc0(4096);
> 
> Use TARGET_PAGE_SIZE instead of magic value 4096?
yes
> 
>> +   insert_stsi_15_1_2(machine, p);
> 
> Wrong indentation (3 instead of 4 spaces).

oh, yes, thanks

> 
>> +    if (s390_is_pv()) {
>> +        ret = s390_cpu_pv_mem_write(cpu, 0, p, 4096);
>> +    } else {
>> +        ret = s390_cpu_virt_mem_write(cpu, addr, ar, p, 4096);
> 
> TARGET_PAGE_SIZE?

yes, of course.

> 
>> +    }
>> +    cc = ret ? 3 : 0;
>> +    setcc(cpu, cc);
>> +    g_free(p);
>> +}
>> +
>>   static int handle_stsi(S390CPU *cpu)
>>   {
>>       CPUState *cs = CPU(cpu);
>> @@ -1920,6 +2017,10 @@ static int handle_stsi(S390CPU *cpu)
>>           /* Only sysib 3.2.2 needs post-handling for now. */
>>           insert_stsi_3_2_2(cpu, run->s390_stsi.addr, run->s390_stsi.ar);
>>           return 0;
>> +    case 15:
>> +        insert_stsi_15_1_x(cpu, run->s390_stsi.sel2, 
>> run->s390_stsi.addr,
>> +                           run->s390_stsi.ar);
>> +        return 0;
>>       default:
>>           return 0;
>>       }
>>
> 
>   Thomas
> 

Thanks for reviewing,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v2 3/5] s390x: topology: CPU topology objects and structures

[Pierre Morel]

On 9/7/21 9:32 AM, Thomas Huth wrote:
> On 22/07/2021 19.42, Pierre Morel wrote:
>> We use new objects to have a dynamic administration of the CPU topology.
>> The highier level object is the S390 book. In a first implementation
> 

> 
> I didn't spot any migration related code in here ... is this already 
> migration-safe?
> 

Not sure at all.

The topology may change at any moment and we interpret PTF, the 
instruction which tell us if the topology changed.
Obviously the topology on the target may not be the same as on the source.

So what I propose is to disable topology change during the migration:
- on migration start, disable PTF interpretation and block the 
topology_change _report in the emulation.
- on migration end set back PTF interpretation and unblock the emulation

In the case, in discussion with David on KVM, that we do not emulate PTF 
for hosts without the stfl(11) we can even make it simpler in QEMU by 
always reporting "no change" for PTF 2 in the emulation.

Note that the Linux kernel, even if the topology can change at any 
moment use a polling every minute to check the topology changes, so I 
guess we can ignore the optimization during the migration.

What do you think?


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v2 3/5] s390x: topology: CPU topology objects and structures

[Thomas Huth]

On 07/09/2021 14.45, Pierre Morel wrote:
> 
> 
> On 9/7/21 9:32 AM, Thomas Huth wrote:
>> On 22/07/2021 19.42, Pierre Morel wrote:
>>> We use new objects to have a dynamic administration of the CPU topology.
>>> The highier level object is the S390 book. In a first implementation
>>
> 
>>
>> I didn't spot any migration related code in here ... is this already 
>> migration-safe?
>>
> 
> Not sure at all.
> 
> The topology may change at any moment and we interpret PTF, the instruction 
> which tell us if the topology changed.
> Obviously the topology on the target may not be the same as on the source.
> 
> So what I propose is to disable topology change during the migration:
> - on migration start, disable PTF interpretation and block the 
> topology_change _report in the emulation.
> - on migration end set back PTF interpretation and unblock the emulation
> 
> In the case, in discussion with David on KVM, that we do not emulate PTF for 
> hosts without the stfl(11) we can even make it simpler in QEMU by always 
> reporting "no change" for PTF 2 in the emulation.
> 
> Note that the Linux kernel, even if the topology can change at any moment 
> use a polling every minute to check the topology changes, so I guess we can 
> ignore the optimization during the migration.
> 
> What do you think?

I don't have much clue, this topology stuff is still mostly a black box to 
me - so there is no interrupt or something similar presented to the guest 
when the topology changes? The guest really has to poll for changes? ... 
that sounds like a weird design to me... if the guest polls too frequently, 
it wastes cycles due to the polling - but if it polls not often enough, it 
could run for a while with wrong topology information?

  Thomas

Re: [PATCH v2 3/5] s390x: topology: CPU topology objects and structures

[Pierre Morel]

On 9/29/21 10:12 AM, Thomas Huth wrote:
> On 07/09/2021 14.45, Pierre Morel wrote:
>>
>>
>> On 9/7/21 9:32 AM, Thomas Huth wrote:
>>> On 22/07/2021 19.42, Pierre Morel wrote:
>>>> We use new objects to have a dynamic administration of the CPU 
>>>> topology.
>>>> The highier level object is the S390 book. In a first implementation
>>>
>>
>>>
>>> I didn't spot any migration related code in here ... is this already 
>>> migration-safe?
>>>
>>
>> Not sure at all.
>>
>> The topology may change at any moment and we interpret PTF, the 
>> instruction which tell us if the topology changed.
>> Obviously the topology on the target may not be the same as on the 
>> source.
>>
>> So what I propose is to disable topology change during the migration:
>> - on migration start, disable PTF interpretation and block the 
>> topology_change _report in the emulation.
>> - on migration end set back PTF interpretation and unblock the emulation
>>
>> In the case, in discussion with David on KVM, that we do not emulate 
>> PTF for hosts without the stfl(11) we can even make it simpler in QEMU 
>> by always reporting "no change" for PTF 2 in the emulation.
>>
>> Note that the Linux kernel, even if the topology can change at any 
>> moment use a polling every minute to check the topology changes, so I 
>> guess we can ignore the optimization during the migration.
>>
>> What do you think?
> 
> I don't have much clue, this topology stuff is still mostly a black box 
> to me - so there is no interrupt or something similar presented to the 
> guest when the topology changes? The guest really has to poll for 
> changes? ... that sounds like a weird design to me... if the guest polls 
> too frequently, it wastes cycles due to the polling - but if it polls 
> not often enough, it could run for a while with wrong topology information?

Yes, it is so.
There are no interrupt for topology change, no event or any other 
notification, I guess the overhead has been considered too high.

I guess that a change to the topology is done when (1) a CPU is not 
running, exists in the configuration and used to be running but then is 
being moved in the stopped state, so its environment can be safely 
migrated to another CPU for the next scheduling slice or (2) when the 
CPU is added to or removed from the configuration.

I also guess that what would be nice would be to get the information in 
the guest when it needs to get scheduling decisions.
I had a try on this but it was not done right and I must think a little 
bit more on this. Currently the Linux kernel does a poling every minute 
using PTF(2) which is speed up to 100ms in case the admin voluntary 
changes the topology.

Pierre

> 
>   Thomas
> 
> 

-- 
Pierre Morel
IBM Lab Boeblingen