All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v11 00/11] s390x: CPU Topology
@ 2022-11-03 17:01 Pierre Morel
  2022-11-03 17:01 ` [PATCH v11 01/11] s390x: Register TYPE_S390_CCW_MACHINE properties as class properties Pierre Morel
                   ` (10 more replies)
  0 siblings, 11 replies; 36+ messages in thread
From: Pierre Morel @ 2022-11-03 17:01 UTC (permalink / raw)
  To: qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange, clg

Hi,

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

0) Two new patches in front of the series:
   - A preliminary patch to move the machine properties to the 
     class_init routine
   - The max thread machine class attribute patch.

  Both patches could be taken upstream separatly from each other and
  from the rest of the series.

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

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

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

Also a documentation has been added to the series.


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

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

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

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

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

Regards,
Pierre

Pierre Morel (11):
  s390x: Register TYPE_S390_CCW_MACHINE properties as class properties
  s390x/cpu topology: add max_threads machine class attribute
  s390x/cpu topology: core_id sets s390x CPU topology
  s390x/cpu topology: reporting the CPU topology to the guest
  s390x/cpu_topology: resetting the Topology-Change-Report
  s390x/cpu_topology: CPU topology migration
  target/s390x: interception of PTF instruction
  s390x/cpu topology: add topology_capable QEMU capability
  s390x/cpu topology: add topology machine property
  s390x/cpu_topology: activating CPU topology
  docs/s390x: document s390x cpu topology

 docs/system/s390x/cpu-topology.rst |  80 ++++++++
 include/hw/boards.h                |   3 +
 include/hw/s390x/cpu-topology.h    |  45 +++++
 include/hw/s390x/s390-virtio-ccw.h |  10 +
 target/s390x/cpu.h                 |  81 ++++++++
 target/s390x/kvm/kvm_s390x.h       |   1 +
 hw/core/machine.c                  |   3 +
 hw/s390x/cpu-topology.c            | 286 +++++++++++++++++++++++++++++
 hw/s390x/s390-virtio-ccw.c         | 192 +++++++++++++------
 target/s390x/cpu-sysemu.c          |  21 +++
 target/s390x/cpu_topology.c        | 100 ++++++++++
 target/s390x/kvm/kvm.c             |  50 ++++-
 util/qemu-config.c                 |   4 +
 hw/s390x/meson.build               |   1 +
 qemu-options.hx                    |   6 +-
 target/s390x/meson.build           |   1 +
 16 files changed, 827 insertions(+), 57 deletions(-)
 create mode 100644 docs/system/s390x/cpu-topology.rst
 create mode 100644 include/hw/s390x/cpu-topology.h
 create mode 100644 hw/s390x/cpu-topology.c
 create mode 100644 target/s390x/cpu_topology.c

-- 
2.31.1

Changelog:

- since v10

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

- since v9

- remove books and drawers

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

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

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

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

- Early check for topology support
  (Cedric)

- since v8

- Linux patches are now mainline

- simplification of the implementation
  (Janis)

- Migration, new machine definition
  (Thomas)

- Documentation

- since v7

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

- check return values during new CPU creation
  (Thomas)

- Improving codding style and argument usages
  (Thomas)

- since v6

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

- since v5

- rebasing on newer QEMU version

- reworked most lines above 80 characters.

- since v4

- Added drawer and books to topology

- Added numa topology

- Added documentation

- since v3

- Added migration
  (Thomas)

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

- Take care of endianess to prepare TCG
  (Thomas)

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

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v11 01/11] s390x: Register TYPE_S390_CCW_MACHINE properties as class properties
  2022-11-03 17:01 [PATCH v11 00/11] s390x: CPU Topology Pierre Morel
@ 2022-11-03 17:01 ` Pierre Morel
  2022-11-04  6:32   ` Thomas Huth
  2022-11-03 17:01 ` [PATCH v11 02/11] s390x/cpu topology: add max_threads machine class attribute Pierre Morel
                   ` (9 subsequent siblings)
  10 siblings, 1 reply; 36+ messages in thread
From: Pierre Morel @ 2022-11-03 17:01 UTC (permalink / raw)
  To: qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange, clg

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

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 1cc20d8717..567498e780 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -43,6 +43,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/s390x/pv.h"
 #include "migration/blocker.h"
+#include "qapi/visitor.h"
 
 static Error *pv_mig_blocker;
 
@@ -589,38 +590,6 @@ static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
     return newsz;
 }
 
-static void ccw_machine_class_init(ObjectClass *oc, void *data)
-{
-    MachineClass *mc = MACHINE_CLASS(oc);
-    NMIClass *nc = NMI_CLASS(oc);
-    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
-    S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
-
-    s390mc->ri_allowed = true;
-    s390mc->cpu_model_allowed = true;
-    s390mc->css_migration_enabled = true;
-    s390mc->hpage_1m_allowed = true;
-    mc->init = ccw_init;
-    mc->reset = s390_machine_reset;
-    mc->block_default_type = IF_VIRTIO;
-    mc->no_cdrom = 1;
-    mc->no_floppy = 1;
-    mc->no_parallel = 1;
-    mc->no_sdcard = 1;
-    mc->max_cpus = S390_MAX_CPUS;
-    mc->has_hotpluggable_cpus = true;
-    assert(!mc->get_hotplug_handler);
-    mc->get_hotplug_handler = s390_get_hotplug_handler;
-    mc->cpu_index_to_instance_props = s390_cpu_index_to_props;
-    mc->possible_cpu_arch_ids = s390_possible_cpu_arch_ids;
-    /* it is overridden with 'host' cpu *in kvm_arch_init* */
-    mc->default_cpu_type = S390_CPU_TYPE_NAME("qemu");
-    hc->plug = s390_machine_device_plug;
-    hc->unplug_request = s390_machine_device_unplug_request;
-    nc->nmi_monitor_handler = s390_nmi;
-    mc->default_ram_id = "s390.ram";
-}
-
 static inline bool machine_get_aes_key_wrap(Object *obj, Error **errp)
 {
     S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
@@ -710,19 +679,29 @@ bool hpage_1m_allowed(void)
     return get_machine_class()->hpage_1m_allowed;
 }
 
-static char *machine_get_loadparm(Object *obj, Error **errp)
+static void machine_get_loadparm(Object *obj, Visitor *v,
+                                 const char *name, void *opaque,
+                                 Error **errp)
 {
     S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
+    char *str = g_strndup((char *) ms->loadparm, sizeof(ms->loadparm));
 
-    /* make a NUL-terminated string */
-    return g_strndup((char *) ms->loadparm, sizeof(ms->loadparm));
+    visit_type_str(v, name, &str, errp);
+    g_free(str);
 }
 
-static void machine_set_loadparm(Object *obj, const char *val, Error **errp)
+static void machine_set_loadparm(Object *obj, Visitor *v,
+                                 const char *name, void *opaque,
+                                 Error **errp)
 {
     S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
+    char *val;
     int i;
 
+    if (!visit_type_str(v, name, &val, errp)) {
+        return;
+    }
+
     for (i = 0; i < sizeof(ms->loadparm) && val[i]; i++) {
         uint8_t c = qemu_toupper(val[i]); /* mimic HMC */
 
@@ -740,34 +719,72 @@ static void machine_set_loadparm(Object *obj, const char *val, Error **errp)
         ms->loadparm[i] = ' '; /* pad right with spaces */
     }
 }
-static inline void s390_machine_initfn(Object *obj)
+
+static void ccw_machine_class_init(ObjectClass *oc, void *data)
 {
-    object_property_add_bool(obj, "aes-key-wrap",
-                             machine_get_aes_key_wrap,
-                             machine_set_aes_key_wrap);
-    object_property_set_description(obj, "aes-key-wrap",
+    MachineClass *mc = MACHINE_CLASS(oc);
+    NMIClass *nc = NMI_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
+    S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
+
+    s390mc->ri_allowed = true;
+    s390mc->cpu_model_allowed = true;
+    s390mc->css_migration_enabled = true;
+    s390mc->hpage_1m_allowed = true;
+    mc->init = ccw_init;
+    mc->reset = s390_machine_reset;
+    mc->block_default_type = IF_VIRTIO;
+    mc->no_cdrom = 1;
+    mc->no_floppy = 1;
+    mc->no_parallel = 1;
+    mc->no_sdcard = 1;
+    mc->max_cpus = S390_MAX_CPUS;
+    mc->has_hotpluggable_cpus = true;
+    assert(!mc->get_hotplug_handler);
+    mc->get_hotplug_handler = s390_get_hotplug_handler;
+    mc->cpu_index_to_instance_props = s390_cpu_index_to_props;
+    mc->possible_cpu_arch_ids = s390_possible_cpu_arch_ids;
+    /* it is overridden with 'host' cpu *in kvm_arch_init* */
+    mc->default_cpu_type = S390_CPU_TYPE_NAME("qemu");
+    hc->plug = s390_machine_device_plug;
+    hc->unplug_request = s390_machine_device_unplug_request;
+    nc->nmi_monitor_handler = s390_nmi;
+    mc->default_ram_id = "s390.ram";
+
+    object_class_property_add_bool(oc, "aes-key-wrap",
+                                   machine_get_aes_key_wrap,
+                                   machine_set_aes_key_wrap);
+    object_class_property_set_description(oc, "aes-key-wrap",
             "enable/disable AES key wrapping using the CPACF wrapping key");
-    object_property_set_bool(obj, "aes-key-wrap", true, NULL);
 
-    object_property_add_bool(obj, "dea-key-wrap",
-                             machine_get_dea_key_wrap,
-                             machine_set_dea_key_wrap);
-    object_property_set_description(obj, "dea-key-wrap",
+    object_class_property_add_bool(oc, "dea-key-wrap",
+                                   machine_get_dea_key_wrap,
+                                   machine_set_dea_key_wrap);
+    object_class_property_set_description(oc, "dea-key-wrap",
             "enable/disable DEA key wrapping using the CPACF wrapping key");
-    object_property_set_bool(obj, "dea-key-wrap", true, NULL);
-    object_property_add_str(obj, "loadparm",
-            machine_get_loadparm, machine_set_loadparm);
-    object_property_set_description(obj, "loadparm",
+
+    object_class_property_add(oc, "loadparm", "loadparm",
+                              machine_get_loadparm, machine_set_loadparm,
+                              NULL, NULL);
+    object_class_property_set_description(oc, "loadparm",
             "Up to 8 chars in set of [A-Za-z0-9. ] (lower case chars converted"
             " to upper case) to pass to machine loader, boot manager,"
             " and guest kernel");
 
-    object_property_add_bool(obj, "zpcii-disable",
-                             machine_get_zpcii_disable,
-                             machine_set_zpcii_disable);
-    object_property_set_description(obj, "zpcii-disable",
+    object_class_property_add_bool(oc, "zpcii-disable",
+                                   machine_get_zpcii_disable,
+                                   machine_set_zpcii_disable);
+    object_class_property_set_description(oc, "zpcii-disable",
             "disable zPCI interpretation facilties");
-    object_property_set_bool(obj, "zpcii-disable", false, NULL);
+}
+
+static inline void s390_machine_initfn(Object *obj)
+{
+    S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
+
+    ms->aes_key_wrap = true;
+    ms->dea_key_wrap = true;
+    ms->zpcii_disable = false;
 }
 
 static const TypeInfo ccw_machine_info = {
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v11 02/11] s390x/cpu topology: add max_threads machine class attribute
  2022-11-03 17:01 [PATCH v11 00/11] s390x: CPU Topology Pierre Morel
  2022-11-03 17:01 ` [PATCH v11 01/11] s390x: Register TYPE_S390_CCW_MACHINE properties as class properties Pierre Morel
@ 2022-11-03 17:01 ` Pierre Morel
  2022-11-03 17:01 ` [PATCH v11 03/11] s390x/cpu topology: core_id sets s390x CPU topology Pierre Morel
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 36+ messages in thread
From: Pierre Morel @ 2022-11-03 17:01 UTC (permalink / raw)
  To: qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange, clg

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

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

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

diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 8a0090a071..4f8a39abda 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -40,6 +40,7 @@ struct S390CcwMachineClass {
     bool cpu_model_allowed;
     bool css_migration_enabled;
     bool hpage_1m_allowed;
+    int max_threads;
 };
 
 /* runtime-instrumentation allowed by the machine */
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 567498e780..9ab91df5b1 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -85,8 +85,15 @@ out:
 static void s390_init_cpus(MachineState *machine)
 {
     MachineClass *mc = MACHINE_GET_CLASS(machine);
+    S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
     int i;
 
+    if (machine->smp.threads > s390mc->max_threads) {
+        error_report("S390 does not support more than %d threads.",
+                     s390mc->max_threads);
+        exit(1);
+    }
+
     /* initialize possible_cpus */
     mc->possible_cpu_arch_ids(machine);
 
@@ -731,6 +738,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     s390mc->cpu_model_allowed = true;
     s390mc->css_migration_enabled = true;
     s390mc->hpage_1m_allowed = true;
+    s390mc->max_threads = 1;
     mc->init = ccw_init;
     mc->reset = s390_machine_reset;
     mc->block_default_type = IF_VIRTIO;
@@ -859,8 +867,11 @@ static void ccw_machine_7_1_instance_options(MachineState *machine)
 
 static void ccw_machine_7_1_class_options(MachineClass *mc)
 {
+    S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
+
     ccw_machine_7_2_class_options(mc);
     compat_props_add(mc->compat_props, hw_compat_7_1, hw_compat_7_1_len);
+    s390mc->max_threads = S390_MAX_CPUS;
 }
 DEFINE_CCW_MACHINE(7_1, "7.1", false);
 
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v11 03/11] s390x/cpu topology: core_id sets s390x CPU topology
  2022-11-03 17:01 [PATCH v11 00/11] s390x: CPU Topology Pierre Morel
  2022-11-03 17:01 ` [PATCH v11 01/11] s390x: Register TYPE_S390_CCW_MACHINE properties as class properties Pierre Morel
  2022-11-03 17:01 ` [PATCH v11 02/11] s390x/cpu topology: add max_threads machine class attribute Pierre Morel
@ 2022-11-03 17:01 ` Pierre Morel
  2022-11-15 11:15   ` Cédric Le Goater
  2022-11-03 17:01 ` [PATCH v11 04/11] s390x/cpu topology: reporting the CPU topology to the guest Pierre Morel
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 36+ messages in thread
From: Pierre Morel @ 2022-11-03 17:01 UTC (permalink / raw)
  To: qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange, clg

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

Let's build the topology based on the core_id.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 include/hw/s390x/cpu-topology.h    |  41 ++++++++++
 include/hw/s390x/s390-virtio-ccw.h |   1 +
 target/s390x/cpu.h                 |   2 +
 hw/s390x/cpu-topology.c            | 125 +++++++++++++++++++++++++++++
 hw/s390x/s390-virtio-ccw.c         |  23 ++++++
 hw/s390x/meson.build               |   1 +
 6 files changed, 193 insertions(+)
 create mode 100644 include/hw/s390x/cpu-topology.h
 create mode 100644 hw/s390x/cpu-topology.c

diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
new file mode 100644
index 0000000000..4e16a2153d
--- /dev/null
+++ b/include/hw/s390x/cpu-topology.h
@@ -0,0 +1,41 @@
+/*
+ * CPU Topology
+ *
+ * Copyright IBM Corp. 2022
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#ifndef HW_S390X_CPU_TOPOLOGY_H
+#define HW_S390X_CPU_TOPOLOGY_H
+
+#include "hw/qdev-core.h"
+#include "qom/object.h"
+
+#define S390_TOPOLOGY_CPU_IFL 0x03
+#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
+
+typedef struct S390TopoSocket {
+    int active_count;
+    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
+} S390TopoSocket;
+
+struct S390Topology {
+    SysBusDevice parent_obj;
+    uint32_t nr_cpus;
+    uint32_t nr_sockets;
+    S390TopoSocket *socket;
+};
+
+#define TYPE_S390_CPU_TOPOLOGY "s390-topology"
+OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY)
+
+void s390_topology_new_cpu(S390CPU *cpu);
+
+static inline bool s390_has_topology(void)
+{
+    return false;
+}
+
+#endif
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 4f8a39abda..23b472708d 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -29,6 +29,7 @@ struct S390CcwMachineState {
     bool pv;
     bool zpcii_disable;
     uint8_t loadparm[8];
+    void *topology;
 };
 
 struct S390CcwMachineClass {
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7d6d01325b..c9066b2496 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -175,6 +175,8 @@ struct ArchCPU {
     /* needed for live migration */
     void *irqstate;
     uint32_t irqstate_saved_size;
+    /* Topology this CPU belongs too */
+    void *topology;
 };
 
 
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
new file mode 100644
index 0000000000..6af41d3d7b
--- /dev/null
+++ b/hw/s390x/cpu-topology.c
@@ -0,0 +1,125 @@
+/*
+ * CPU Topology
+ *
+ * Copyright IBM Corp. 2022
+ * Author(s): Pierre Morel <pmorel@linux.ibm.com>
+
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "hw/sysbus.h"
+#include "hw/qdev-properties.h"
+#include "hw/boards.h"
+#include "qemu/typedefs.h"
+#include "target/s390x/cpu.h"
+#include "hw/s390x/s390-virtio-ccw.h"
+#include "hw/s390x/cpu-topology.h"
+
+/*
+ * s390_topology_new_cpu:
+ * @cpu: a pointer to the new CPU
+ *
+ * The topology pointed by S390CPU, gives us the CPU topology
+ * established by the -smp QEMU aruments.
+ * The core-id is used to calculate the position of the CPU inside
+ * the topology:
+ *  - the socket, container TLE, containing the CPU, we have one socket
+ *    for every nr_cpus (nr_cpus = smp.cores * smp.threads)
+ *  - the CPU TLE inside the socket, we have potentionly up to 4 CPU TLE
+ *    in a container TLE with the assumption that all CPU are identical
+ *    with the same polarity and entitlement because we have maximum 256
+ *    CPUs and each TLE can hold up to 64 identical CPUs.
+ *  - the bit in the 64 bit CPU TLE core mask
+ */
+void s390_topology_new_cpu(S390CPU *cpu)
+{
+    S390Topology *topo = (S390Topology *)cpu->topology;
+    int core_id = cpu->env.core_id;
+    int bit, origin;
+    int socket_id;
+
+    socket_id = core_id / topo->nr_cpus;
+
+    /*
+     * At the core level, each CPU is represented by a bit in a 64bit
+     * uint64_t which represent the presence of a CPU.
+     * The firmware assume that all CPU in a CPU TLE have the same
+     * type, polarization and are all dedicated or shared.
+     * In that case the origin variable represents the offset of the first
+     * CPU in the CPU container.
+     * More than 64 CPUs per socket are represented in several CPU containers
+     * inside the socket container.
+     * The only reason to have several S390TopologyCores inside a socket is
+     * to have more than 64 CPUs.
+     * In that case the origin variable represents the offset of the first CPU
+     * in the CPU container. More than 64 CPUs per socket are represented in
+     * several CPU containers inside the socket container.
+     */
+    bit = core_id;
+    origin = bit / 64;
+    bit %= 64;
+    bit = 63 - bit;
+
+    topo->socket[socket_id].active_count++;
+    set_bit(bit, &topo->socket[socket_id].mask[origin]);
+}
+
+/**
+ * s390_topology_realize:
+ * @dev: the device state
+ * @errp: the error pointer (not used)
+ *
+ * During realize the machine CPU topology is initialized with the
+ * QEMU -smp parameters.
+ * The maximum count of CPU TLE in the all Topology can not be greater
+ * than the maximum CPUs.
+ */
+static void s390_topology_realize(DeviceState *dev, Error **errp)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+    S390Topology *topo = S390_CPU_TOPOLOGY(dev);
+
+    topo->nr_cpus = ms->smp.cores * ms->smp.threads;
+    topo->nr_sockets = ms->smp.sockets;
+    topo->socket = g_new0(S390TopoSocket, topo->nr_sockets);
+}
+
+static Property s390_topology_properties[] = {
+    DEFINE_PROP_UINT32("nr_cpus", S390Topology, nr_cpus, 1),
+    DEFINE_PROP_UINT32("nr_sockets", S390Topology, nr_sockets, 1),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+/**
+ * topology_class_init:
+ * @oc: Object class
+ * @data: (not used)
+ *
+ * A very simple object we will need for reset and migration.
+ */
+static void topology_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = s390_topology_realize;
+    device_class_set_props(dc, s390_topology_properties);
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+}
+
+static const TypeInfo cpu_topology_info = {
+    .name          = TYPE_S390_CPU_TOPOLOGY,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(S390Topology),
+    .class_init    = topology_class_init,
+};
+
+static void topology_register(void)
+{
+    type_register_static(&cpu_topology_info);
+}
+type_init(topology_register);
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 9ab91df5b1..5776d3e58f 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -44,6 +44,7 @@
 #include "hw/s390x/pv.h"
 #include "migration/blocker.h"
 #include "qapi/visitor.h"
+#include "hw/s390x/cpu-topology.h"
 
 static Error *pv_mig_blocker;
 
@@ -102,6 +103,19 @@ static void s390_init_cpus(MachineState *machine)
     }
 }
 
+static void s390_init_topology(MachineState *machine)
+{
+    DeviceState *dev;
+
+    if (s390_has_topology()) {
+        dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
+        object_property_add_child(&machine->parent_obj,
+                                  TYPE_S390_CPU_TOPOLOGY, OBJECT(dev));
+        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+        S390_CCW_MACHINE(machine)->topology = dev;
+    }
+}
+
 static const char *const reset_dev_types[] = {
     TYPE_VIRTUAL_CSS_BRIDGE,
     "s390-sclp-event-facility",
@@ -252,6 +266,9 @@ static void ccw_init(MachineState *machine)
     /* init memory + setup max page size. Required for the CPU model */
     s390_memory_init(machine->ram);
 
+    /* Adding the topology must be done before CPU initialization */
+    s390_init_topology(machine);
+
     /* init CPUs (incl. CPU model) early so s390_has_feature() works */
     s390_init_cpus(machine);
 
@@ -314,6 +331,12 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev,
     g_assert(!ms->possible_cpus->cpus[cpu->env.core_id].cpu);
     ms->possible_cpus->cpus[cpu->env.core_id].cpu = OBJECT(dev);
 
+    /* Inserting the CPU in the Topology can not fail */
+    if (S390_CCW_MACHINE(ms)->topology) {
+        cpu->topology = S390_CCW_MACHINE(ms)->topology;
+        s390_topology_new_cpu(cpu);
+    }
+
     if (dev->hotplugged) {
         raise_irq_cpu_hotplug();
     }
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index f291016fee..653f6ab488 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -2,6 +2,7 @@ s390x_ss = ss.source_set()
 s390x_ss.add(files(
   'ap-bridge.c',
   'ap-device.c',
+  'cpu-topology.c',
   'ccw-device.c',
   'css-bridge.c',
   'css.c',
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v11 04/11] s390x/cpu topology: reporting the CPU topology to the guest
  2022-11-03 17:01 [PATCH v11 00/11] s390x: CPU Topology Pierre Morel
                   ` (2 preceding siblings ...)
  2022-11-03 17:01 ` [PATCH v11 03/11] s390x/cpu topology: core_id sets s390x CPU topology Pierre Morel
@ 2022-11-03 17:01 ` Pierre Morel
  2022-11-15 11:21   ` Cédric Le Goater
  2022-11-17  8:40   ` Cédric Le Goater
  2022-11-03 17:01 ` [PATCH v11 05/11] s390x/cpu_topology: resetting the Topology-Change-Report Pierre Morel
                   ` (6 subsequent siblings)
  10 siblings, 2 replies; 36+ messages in thread
From: Pierre Morel @ 2022-11-03 17:01 UTC (permalink / raw)
  To: qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange, clg

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

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

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 include/hw/s390x/cpu-topology.h |   6 ++
 target/s390x/cpu.h              |  77 ++++++++++++++++++++++++
 hw/s390x/cpu-topology.c         |   1 -
 target/s390x/cpu_topology.c     | 100 ++++++++++++++++++++++++++++++++
 target/s390x/kvm/kvm.c          |   6 +-
 target/s390x/meson.build        |   1 +
 6 files changed, 189 insertions(+), 2 deletions(-)
 create mode 100644 target/s390x/cpu_topology.c

diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
index 4e16a2153d..6fec10e032 100644
--- a/include/hw/s390x/cpu-topology.h
+++ b/include/hw/s390x/cpu-topology.h
@@ -16,6 +16,11 @@
 #define S390_TOPOLOGY_CPU_IFL 0x03
 #define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
 
+#define S390_TOPOLOGY_POLARITY_HORIZONTAL      0x00
+#define S390_TOPOLOGY_POLARITY_VERTICAL_LOW    0x01
+#define S390_TOPOLOGY_POLARITY_VERTICAL_MEDIUM 0x02
+#define S390_TOPOLOGY_POLARITY_VERTICAL_HIGH   0x03
+
 typedef struct S390TopoSocket {
     int active_count;
     uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
@@ -26,6 +31,7 @@ struct S390Topology {
     uint32_t nr_cpus;
     uint32_t nr_sockets;
     S390TopoSocket *socket;
+    bool topology_needed;
 };
 
 #define TYPE_S390_CPU_TOPOLOGY "s390-topology"
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index c9066b2496..69a7523146 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -567,6 +567,81 @@ typedef union SysIB {
 } SysIB;
 QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
 
+/*
+ * CPU Topology List provided by STSI with fc=15 provides a list
+ * of two different Topology List Entries (TLE) types to specify
+ * the topology hierarchy.
+ *
+ * - Container Topology List Entry
+ *   Defines a container to contain other Topology List Entries
+ *   of any type, nested containers or CPU.
+ * - CPU Topology List Entry
+ *   Specifies the CPUs position, type, entitlement and polarization
+ *   of the CPUs contained in the last Container TLE.
+ *
+ * There can be theoretically up to five levels of containers, QEMU
+ * uses only one level, the socket level.
+ *
+ * A container of with a nesting level (NL) greater than 1 can only
+ * contain another container of nesting level NL-1.
+ *
+ * A container of nesting level 1 (socket), contains as many CPU TLE
+ * as needed to describe the position and qualities of all CPUs inside
+ * the container.
+ * The qualities of a CPU are polarization, entitlement and type.
+ *
+ * The CPU TLE defines the position of the CPUs of identical qualities
+ * using a 64bits mask which first bit has its offset defined by
+ * the CPU address orgin field of the CPU TLE like in:
+ * CPU address = origin * 64 + bit position within the mask
+ *
+ */
+/* Container type Topology List Entry */
+typedef struct SysIBTl_container {
+        uint8_t nl;
+        uint8_t reserved[6];
+        uint8_t id;
+} QEMU_PACKED QEMU_ALIGNED(8) SysIBTl_container;
+QEMU_BUILD_BUG_ON(sizeof(SysIBTl_container) != 8);
+
+/* CPU type Topology List Entry */
+typedef struct SysIBTl_cpu {
+        uint8_t nl;
+        uint8_t reserved0[3];
+        uint8_t reserved1:5;
+        uint8_t dedicated:1;
+        uint8_t polarity:2;
+        uint8_t type;
+        uint16_t origin;
+        uint64_t mask;
+} QEMU_PACKED QEMU_ALIGNED(8) SysIBTl_cpu;
+QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16);
+
+#define S390_TOPOLOGY_MAG  6
+#define S390_TOPOLOGY_MAG6 0
+#define S390_TOPOLOGY_MAG5 1
+#define S390_TOPOLOGY_MAG4 2
+#define S390_TOPOLOGY_MAG3 3
+#define S390_TOPOLOGY_MAG2 4
+#define S390_TOPOLOGY_MAG1 5
+/* Configuration topology */
+typedef struct SysIB_151x {
+    uint8_t  reserved0[2];
+    uint16_t length;
+    uint8_t  mag[S390_TOPOLOGY_MAG];
+    uint8_t  reserved1;
+    uint8_t  mnest;
+    uint32_t reserved2;
+    char tle[0];
+} QEMU_PACKED QEMU_ALIGNED(8) SysIB_151x;
+QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
+
+/* Max size of a SYSIB structure is when all CPU are alone in a container */
+#define S390_TOPOLOGY_SYSIB_SIZE (sizeof(SysIB_151x) +                         \
+                                  S390_MAX_CPUS * (sizeof(SysIBTl_container) + \
+                                                   sizeof(SysIBTl_cpu)))
+
+
 /* MMU defines */
 #define ASCE_ORIGIN           (~0xfffULL) /* segment table origin             */
 #define ASCE_SUBSPACE         0x200       /* subspace group control           */
@@ -845,4 +920,6 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
 
 #include "exec/cpu-all.h"
 
+void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar);
+
 #endif
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index 6af41d3d7b..9fa8ca1261 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -44,7 +44,6 @@ void s390_topology_new_cpu(S390CPU *cpu)
     int socket_id;
 
     socket_id = core_id / topo->nr_cpus;
-
     /*
      * At the core level, each CPU is represented by a bit in a 64bit
      * uint64_t which represent the presence of a CPU.
diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
new file mode 100644
index 0000000000..a1179d8e95
--- /dev/null
+++ b/target/s390x/cpu_topology.c
@@ -0,0 +1,100 @@
+/*
+ * QEMU S390x CPU Topology
+ *
+ * Copyright IBM Corp. 2022
+ * Author(s): Pierre Morel <pmorel@linux.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "hw/s390x/pv.h"
+#include "hw/sysbus.h"
+#include "hw/s390x/cpu-topology.h"
+#include "hw/s390x/sclp.h"
+
+static char *fill_container(char *p, int level, int id)
+{
+    SysIBTl_container *tle = (SysIBTl_container *)p;
+
+    tle->nl = level;
+    tle->id = id;
+    return p + sizeof(*tle);
+}
+
+static char *fill_tle_cpu(char *p, uint64_t mask, int origin)
+{
+    SysIBTl_cpu *tle = (SysIBTl_cpu *)p;
+
+    tle->nl = 0;
+    tle->dedicated = 1;
+    tle->polarity = S390_TOPOLOGY_POLARITY_HORIZONTAL;
+    tle->type = S390_TOPOLOGY_CPU_IFL;
+    tle->origin = cpu_to_be64(origin * 64);
+    tle->mask = cpu_to_be64(mask);
+    return p + sizeof(*tle);
+}
+
+static char *s390_top_set_level2(S390Topology *topo, char *p)
+{
+    int i, origin;
+
+    for (i = 0; i < topo->nr_sockets; i++) {
+        if (!topo->socket[i].active_count) {
+            continue;
+        }
+        p = fill_container(p, 1, i);
+        for (origin = 0; origin < S390_TOPOLOGY_MAX_ORIGIN; origin++) {
+            uint64_t mask = 0L;
+
+            mask = topo->socket[i].mask[origin];
+            if (mask) {
+                p = fill_tle_cpu(p, mask, origin);
+            }
+        }
+    }
+    return p;
+}
+
+static int setup_stsi(S390CPU *cpu, SysIB_151x *sysib, int level)
+{
+    S390Topology *topo = (S390Topology *)cpu->topology;
+    char *p = sysib->tle;
+
+    sysib->mnest = level;
+    switch (level) {
+    case 2:
+        sysib->mag[S390_TOPOLOGY_MAG2] = topo->nr_sockets;
+        sysib->mag[S390_TOPOLOGY_MAG1] = topo->nr_cpus;
+        p = s390_top_set_level2(topo, p);
+        break;
+    }
+
+    return p - (char *)sysib;
+}
+
+#define S390_TOPOLOGY_MAX_MNEST 2
+
+void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
+{
+    union {
+        char place_holder[S390_TOPOLOGY_SYSIB_SIZE];
+        SysIB_151x sysib;
+    } buffer QEMU_ALIGNED(8);
+    int len;
+
+    if (s390_is_pv() || !s390_has_topology() ||
+        sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
+        setcc(cpu, 3);
+        return;
+    }
+
+    len = setup_stsi(cpu, &buffer.sysib, sel2);
+
+    buffer.sysib.length = cpu_to_be16(len);
+    s390_cpu_virt_mem_write(cpu, addr, ar, &buffer.sysib, len);
+    setcc(cpu, 0);
+}
+
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 3ac7ec9acf..7dc96f3663 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -51,6 +51,7 @@
 #include "hw/s390x/s390-virtio-ccw.h"
 #include "hw/s390x/s390-virtio-hcall.h"
 #include "hw/s390x/pv.h"
+#include "hw/s390x/cpu-topology.h"
 
 #ifndef DEBUG_KVM
 #define DEBUG_KVM  0
@@ -1919,9 +1920,12 @@ static int handle_stsi(S390CPU *cpu)
         if (run->s390_stsi.sel1 != 2 || run->s390_stsi.sel2 != 2) {
             return 0;
         }
-        /* Only sysib 3.2.2 needs post-handling for now. */
         insert_stsi_3_2_2(cpu, run->s390_stsi.addr, run->s390_stsi.ar);
         return 0;
+    case 15:
+        insert_stsi_15_1_x(cpu, run->s390_stsi.sel2, run->s390_stsi.addr,
+                           run->s390_stsi.ar);
+        return 0;
     default:
         return 0;
     }
diff --git a/target/s390x/meson.build b/target/s390x/meson.build
index 84c1402a6a..890ccfa789 100644
--- a/target/s390x/meson.build
+++ b/target/s390x/meson.build
@@ -29,6 +29,7 @@ s390x_softmmu_ss.add(files(
   'sigp.c',
   'cpu-sysemu.c',
   'cpu_models_sysemu.c',
+  'cpu_topology.c',
 ))
 
 s390x_user_ss = ss.source_set()
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v11 05/11] s390x/cpu_topology: resetting the Topology-Change-Report
  2022-11-03 17:01 [PATCH v11 00/11] s390x: CPU Topology Pierre Morel
                   ` (3 preceding siblings ...)
  2022-11-03 17:01 ` [PATCH v11 04/11] s390x/cpu topology: reporting the CPU topology to the guest Pierre Morel
@ 2022-11-03 17:01 ` Pierre Morel
  2022-11-03 17:01 ` [PATCH v11 06/11] s390x/cpu_topology: CPU topology migration Pierre Morel
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 36+ messages in thread
From: Pierre Morel @ 2022-11-03 17:01 UTC (permalink / raw)
  To: qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange, clg

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

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 target/s390x/cpu.h           |  1 +
 target/s390x/kvm/kvm_s390x.h |  1 +
 hw/s390x/cpu-topology.c      | 12 ++++++++++++
 hw/s390x/s390-virtio-ccw.c   |  1 +
 target/s390x/cpu-sysemu.c    | 13 +++++++++++++
 target/s390x/kvm/kvm.c       | 17 +++++++++++++++++
 6 files changed, 45 insertions(+)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 69a7523146..70de251b07 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -856,6 +856,7 @@ void s390_enable_css_support(S390CPU *cpu);
 void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg);
 int s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch_id,
                                 int vq, bool assign);
+void s390_cpu_topology_reset(void);
 #ifndef CONFIG_USER_ONLY
 unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu);
 #else
diff --git a/target/s390x/kvm/kvm_s390x.h b/target/s390x/kvm/kvm_s390x.h
index f9785564d0..649dae5948 100644
--- a/target/s390x/kvm/kvm_s390x.h
+++ b/target/s390x/kvm/kvm_s390x.h
@@ -47,5 +47,6 @@ void kvm_s390_crypto_reset(void);
 void kvm_s390_restart_interrupt(S390CPU *cpu);
 void kvm_s390_stop_interrupt(S390CPU *cpu);
 void kvm_s390_set_diag318(CPUState *cs, uint64_t diag318_info);
+int kvm_s390_topology_set_mtcr(uint64_t attr);
 
 #endif /* KVM_S390X_H */
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index 9fa8ca1261..21d2785b37 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -94,6 +94,17 @@ static Property s390_topology_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+/**
+ * s390_topology_reset:
+ * @dev: the device
+ *
+ * Calls the sysemu topology reset
+ */
+static void s390_topology_reset(DeviceState *dev)
+{
+    s390_cpu_topology_reset();
+}
+
 /**
  * topology_class_init:
  * @oc: Object class
@@ -108,6 +119,7 @@ static void topology_class_init(ObjectClass *oc, void *data)
     dc->realize = s390_topology_realize;
     device_class_set_props(dc, s390_topology_properties);
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+    dc->reset = s390_topology_reset;
 }
 
 static const TypeInfo cpu_topology_info = {
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 5776d3e58f..4de2622f99 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -122,6 +122,7 @@ static const char *const reset_dev_types[] = {
     "s390-flic",
     "diag288",
     TYPE_S390_PCI_HOST_BRIDGE,
+    TYPE_S390_CPU_TOPOLOGY,
 };
 
 static void subsystem_reset(void)
diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
index 948e4bd3e0..e27864c5f5 100644
--- a/target/s390x/cpu-sysemu.c
+++ b/target/s390x/cpu-sysemu.c
@@ -306,3 +306,16 @@ void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg)
         kvm_s390_set_diag318(cs, arg.host_ulong);
     }
 }
+
+void s390_cpu_topology_reset(void)
+{
+    int ret;
+
+    if (kvm_enabled()) {
+        ret = kvm_s390_topology_set_mtcr(0);
+        if (ret) {
+            error_report("Failed to set Modified Topology Change Report: %s",
+                         strerror(-ret));
+        }
+    }
+}
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 7dc96f3663..5b6383eab0 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -2593,6 +2593,23 @@ int kvm_s390_get_zpci_op(void)
     return cap_zpci_op;
 }
 
+int kvm_s390_topology_set_mtcr(uint64_t attr)
+{
+    struct kvm_device_attr attribute = {
+        .group = KVM_S390_VM_CPU_TOPOLOGY,
+        .attr  = attr,
+    };
+
+    if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
+        return 0;
+    }
+    if (!kvm_vm_check_attr(kvm_state, KVM_S390_VM_CPU_TOPOLOGY, attr)) {
+        return -ENOTSUP;
+    }
+
+    return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attribute);
+}
+
 void kvm_arch_accel_class_init(ObjectClass *oc)
 {
 }
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v11 06/11] s390x/cpu_topology: CPU topology migration
  2022-11-03 17:01 [PATCH v11 00/11] s390x: CPU Topology Pierre Morel
                   ` (4 preceding siblings ...)
  2022-11-03 17:01 ` [PATCH v11 05/11] s390x/cpu_topology: resetting the Topology-Change-Report Pierre Morel
@ 2022-11-03 17:01 ` Pierre Morel
  2022-11-03 17:01 ` [PATCH v11 07/11] target/s390x: interception of PTF instruction Pierre Morel
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 36+ messages in thread
From: Pierre Morel @ 2022-11-03 17:01 UTC (permalink / raw)
  To: qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange, clg

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

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

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

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 70de251b07..53127c249c 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -857,6 +857,7 @@ void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg);
 int s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch_id,
                                 int vq, bool assign);
 void s390_cpu_topology_reset(void);
+int s390_cpu_topology_mtcr_set(void);
 #ifndef CONFIG_USER_ONLY
 unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu);
 #else
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index 21d2785b37..df4470d2b4 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -19,6 +19,7 @@
 #include "target/s390x/cpu.h"
 #include "hw/s390x/s390-virtio-ccw.h"
 #include "hw/s390x/cpu-topology.h"
+#include "migration/vmstate.h"
 
 /*
  * s390_topology_new_cpu:
@@ -105,6 +106,83 @@ static void s390_topology_reset(DeviceState *dev)
     s390_cpu_topology_reset();
 }
 
+/**
+ * cpu_topology_postload
+ * @opaque: a pointer to the S390Topology
+ * @version_id: version identifier
+ *
+ * We check that the topology is used or is not used
+ * on both side identically.
+ *
+ * If the topology is in use we set the Modified Topology Change Report
+ * on the destination host.
+ */
+static int cpu_topology_postload(void *opaque, int version_id)
+{
+    S390Topology *topo = opaque;
+    int ret;
+
+    if (topo->topology_needed != s390_has_topology()) {
+        if (topo->topology_needed) {
+            error_report("Topology facility is needed in destination");
+        } else {
+            error_report("Topology facility can not be used in destination");
+        }
+        return -EINVAL;
+    }
+
+    /* We do not support CPU Topology, all is good */
+    if (!s390_has_topology()) {
+        return 0;
+    }
+
+    /* We support CPU Topology, set the MTCR unconditionally */
+    ret = s390_cpu_topology_mtcr_set();
+    if (ret) {
+        error_report("Failed to set MTCR: %s", strerror(-ret));
+    }
+    return ret;
+}
+
+/**
+ * cpu_topology_presave:
+ * @opaque: The pointer to the S390Topology
+ *
+ * Save the usage of the CPU Topology in the VM State.
+ */
+static int cpu_topology_presave(void *opaque)
+{
+    S390Topology *topo = opaque;
+
+    topo->topology_needed = s390_has_topology();
+    return 0;
+}
+
+/**
+ * cpu_topology_needed:
+ * @opaque: The pointer to the S390Topology
+ *
+ * We always need to know if source and destination use the topology.
+ */
+static bool cpu_topology_needed(void *opaque)
+{
+    return true;
+}
+
+
+const VMStateDescription vmstate_cpu_topology = {
+    .name = "cpu_topology",
+    .version_id = 1,
+    .post_load = cpu_topology_postload,
+    .pre_save = cpu_topology_presave,
+    .minimum_version_id = 1,
+    .needed = cpu_topology_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(topology_needed, S390Topology),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 /**
  * topology_class_init:
  * @oc: Object class
@@ -120,6 +198,7 @@ static void topology_class_init(ObjectClass *oc, void *data)
     device_class_set_props(dc, s390_topology_properties);
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     dc->reset = s390_topology_reset;
+    dc->vmsd = &vmstate_cpu_topology;
 }
 
 static const TypeInfo cpu_topology_info = {
diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
index e27864c5f5..a8e3e6219d 100644
--- a/target/s390x/cpu-sysemu.c
+++ b/target/s390x/cpu-sysemu.c
@@ -319,3 +319,11 @@ void s390_cpu_topology_reset(void)
         }
     }
 }
+
+int s390_cpu_topology_mtcr_set(void)
+{
+    if (kvm_enabled()) {
+        return kvm_s390_topology_set_mtcr(1);
+    }
+    return -ENOENT;
+}
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v11 07/11] target/s390x: interception of PTF instruction
  2022-11-03 17:01 [PATCH v11 00/11] s390x: CPU Topology Pierre Morel
                   ` (5 preceding siblings ...)
  2022-11-03 17:01 ` [PATCH v11 06/11] s390x/cpu_topology: CPU topology migration Pierre Morel
@ 2022-11-03 17:01 ` Pierre Morel
  2022-11-03 17:01 ` [PATCH v11 08/11] s390x/cpu topology: add topology_capable QEMU capability Pierre Morel
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 36+ messages in thread
From: Pierre Morel @ 2022-11-03 17:01 UTC (permalink / raw)
  To: qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange, clg

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

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

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

diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 23b472708d..6488279690 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -32,6 +32,12 @@ struct S390CcwMachineState {
     void *topology;
 };
 
+#define S390_PTF_REASON_NONE (0x00 << 8)
+#define S390_PTF_REASON_DONE (0x01 << 8)
+#define S390_PTF_REASON_BUSY (0x02 << 8)
+#define S390_TOPO_FC_MASK 0xffUL
+void s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra);
+
 struct S390CcwMachineClass {
     /*< private >*/
     MachineClass parent_class;
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index df4470d2b4..fc220bd8ac 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -20,6 +20,58 @@
 #include "hw/s390x/s390-virtio-ccw.h"
 #include "hw/s390x/cpu-topology.h"
 #include "migration/vmstate.h"
+#include "target/s390x/cpu.h"
+#include "hw/s390x/s390-virtio-ccw.h"
+
+/*
+ * s390_handle_ptf:
+ *
+ * @register 1: contains the function code
+ *
+ * Function codes 0 and 1 handle the CPU polarization.
+ * We assume an horizontal topology, the only one supported currently
+ * by Linux, consequently we answer to function code 0, requesting
+ * horizontal polarization that it is already the current polarization
+ * and reject vertical polarization request without further explanation.
+ *
+ * Function code 2 is handling topology changes and is interpreted
+ * by the SIE.
+ */
+void s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra)
+{
+    CPUS390XState *env = &cpu->env;
+    uint64_t reg = env->regs[r1];
+    uint8_t fc = reg & S390_TOPO_FC_MASK;
+
+    if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
+        s390_program_interrupt(env, PGM_OPERATION, ra);
+        return;
+    }
+
+    if (env->psw.mask & PSW_MASK_PSTATE) {
+        s390_program_interrupt(env, PGM_PRIVILEGED, ra);
+        return;
+    }
+
+    if (reg & ~S390_TOPO_FC_MASK) {
+        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+        return;
+    }
+
+    switch (fc) {
+    case 0:    /* Horizontal polarization is already set */
+        env->regs[r1] |= S390_PTF_REASON_DONE;
+        setcc(cpu, 2);
+        break;
+    case 1:    /* Vertical polarization is not supported */
+        env->regs[r1] |= S390_PTF_REASON_NONE;
+        setcc(cpu, 2);
+        break;
+    default:
+        /* Note that fc == 2 is interpreted by the SIE */
+        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+    }
+}
 
 /*
  * s390_topology_new_cpu:
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 5b6383eab0..a79fdf1c79 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -97,6 +97,7 @@
 
 #define PRIV_B9_EQBS                    0x9c
 #define PRIV_B9_CLP                     0xa0
+#define PRIV_B9_PTF                     0xa2
 #define PRIV_B9_PCISTG                  0xd0
 #define PRIV_B9_PCILG                   0xd2
 #define PRIV_B9_RPCIT                   0xd3
@@ -1465,6 +1466,13 @@ static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
     }
 }
 
+static void kvm_handle_ptf(S390CPU *cpu, struct kvm_run *run)
+{
+    uint8_t r1 = (run->s390_sieic.ipb >> 20) & 0x0f;
+
+    s390_handle_ptf(cpu, r1, RA_IGNORED);
+}
+
 static int handle_b9(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
 {
     int r = 0;
@@ -1482,6 +1490,9 @@ static int handle_b9(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
     case PRIV_B9_RPCIT:
         r = kvm_rpcit_service_call(cpu, run);
         break;
+    case PRIV_B9_PTF:
+        kvm_handle_ptf(cpu, run);
+        break;
     case PRIV_B9_EQBS:
         /* just inject exception */
         r = -1;
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v11 08/11] s390x/cpu topology: add topology_capable QEMU capability
  2022-11-03 17:01 [PATCH v11 00/11] s390x: CPU Topology Pierre Morel
                   ` (6 preceding siblings ...)
  2022-11-03 17:01 ` [PATCH v11 07/11] target/s390x: interception of PTF instruction Pierre Morel
@ 2022-11-03 17:01 ` Pierre Morel
  2022-11-15 13:27   ` Cédric Le Goater
  2022-11-03 17:01 ` [PATCH v11 09/11] s390x/cpu topology: add topology machine property Pierre Morel
                   ` (2 subsequent siblings)
  10 siblings, 1 reply; 36+ messages in thread
From: Pierre Morel @ 2022-11-03 17:01 UTC (permalink / raw)
  To: qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange, clg

S390 CPU topology is only allowed for s390-virtio-ccw-7.2 and
newer S390 machines.

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

diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 6488279690..89fca3f79f 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -48,6 +48,7 @@ struct S390CcwMachineClass {
     bool css_migration_enabled;
     bool hpage_1m_allowed;
     int max_threads;
+    bool topology_capable;
 };
 
 /* runtime-instrumentation allowed by the machine */
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 4de2622f99..f1a9d6e793 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -763,6 +763,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     s390mc->css_migration_enabled = true;
     s390mc->hpage_1m_allowed = true;
     s390mc->max_threads = 1;
+    s390mc->topology_capable = true;
     mc->init = ccw_init;
     mc->reset = s390_machine_reset;
     mc->block_default_type = IF_VIRTIO;
@@ -896,6 +897,7 @@ static void ccw_machine_7_1_class_options(MachineClass *mc)
     ccw_machine_7_2_class_options(mc);
     compat_props_add(mc->compat_props, hw_compat_7_1, hw_compat_7_1_len);
     s390mc->max_threads = S390_MAX_CPUS;
+    s390mc->topology_capable = false;
 }
 DEFINE_CCW_MACHINE(7_1, "7.1", false);
 
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v11 09/11] s390x/cpu topology: add topology machine property
  2022-11-03 17:01 [PATCH v11 00/11] s390x: CPU Topology Pierre Morel
                   ` (7 preceding siblings ...)
  2022-11-03 17:01 ` [PATCH v11 08/11] s390x/cpu topology: add topology_capable QEMU capability Pierre Morel
@ 2022-11-03 17:01 ` Pierre Morel
  2022-11-03 17:20   ` Cornelia Huck
  2022-11-15 13:48   ` Cédric Le Goater
  2022-11-03 17:01 ` [PATCH v11 10/11] s390x/cpu_topology: activating CPU topology Pierre Morel
  2022-11-03 17:01 ` [PATCH v11 11/11] docs/s390x: document s390x cpu topology Pierre Morel
  10 siblings, 2 replies; 36+ messages in thread
From: Pierre Morel @ 2022-11-03 17:01 UTC (permalink / raw)
  To: qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange, clg

We keep the possibility to switch on/off the topology on newer
machines with the property topology=[on|off].

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

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 311ed17e18..67147c47bf 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -379,6 +379,9 @@ struct MachineState {
     } \
     type_init(machine_initfn##_register_types)
 
+extern GlobalProperty hw_compat_7_2[];
+extern const size_t hw_compat_7_2_len;
+
 extern GlobalProperty hw_compat_7_1[];
 extern const size_t hw_compat_7_1_len;
 
diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
index 6fec10e032..f566394302 100644
--- a/include/hw/s390x/cpu-topology.h
+++ b/include/hw/s390x/cpu-topology.h
@@ -12,6 +12,8 @@
 
 #include "hw/qdev-core.h"
 #include "qom/object.h"
+#include "cpu.h"
+#include "hw/s390x/s390-virtio-ccw.h"
 
 #define S390_TOPOLOGY_CPU_IFL 0x03
 #define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
@@ -38,10 +40,6 @@ struct S390Topology {
 OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY)
 
 void s390_topology_new_cpu(S390CPU *cpu);
-
-static inline bool s390_has_topology(void)
-{
-    return false;
-}
+bool s390_has_topology(void);
 
 #endif
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 89fca3f79f..d7602aedda 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -28,6 +28,7 @@ struct S390CcwMachineState {
     bool dea_key_wrap;
     bool pv;
     bool zpcii_disable;
+    bool cpu_topology;
     uint8_t loadparm[8];
     void *topology;
 };
diff --git a/hw/core/machine.c b/hw/core/machine.c
index aa520e74a8..4f46d4ef23 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -40,6 +40,9 @@
 #include "hw/virtio/virtio-pci.h"
 #include "qom/object_interfaces.h"
 
+GlobalProperty hw_compat_7_2[] = {};
+const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
+
 GlobalProperty hw_compat_7_1[] = {};
 const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
 
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index fc220bd8ac..c1550cc1e8 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -73,6 +73,25 @@ void s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra)
     }
 }
 
+bool s390_has_topology(void)
+{
+    static S390CcwMachineState *ccw;
+    Object *obj;
+
+    if (ccw) {
+        return ccw->cpu_topology;
+    }
+
+    /* we have to bail out for the "none" machine */
+    obj = object_dynamic_cast(qdev_get_machine(),
+                              TYPE_S390_CCW_MACHINE);
+    if (!obj) {
+        return false;
+    }
+    ccw = S390_CCW_MACHINE(obj);
+    return ccw->cpu_topology;
+}
+
 /*
  * s390_topology_new_cpu:
  * @cpu: a pointer to the new CPU
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index f1a9d6e793..ebb5615337 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -710,6 +710,26 @@ bool hpage_1m_allowed(void)
     return get_machine_class()->hpage_1m_allowed;
 }
 
+static inline bool machine_get_topology(Object *obj, Error **errp)
+{
+    S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
+
+    return ms->cpu_topology;
+}
+
+static inline void machine_set_topology(Object *obj, bool value, Error **errp)
+{
+    S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
+
+    if (!get_machine_class()->topology_capable) {
+        error_setg(errp, "Property cpu-topology not available on machine %s",
+                   get_machine_class()->parent_class.name);
+        return;
+    }
+
+    ms->cpu_topology = value;
+}
+
 static void machine_get_loadparm(Object *obj, Visitor *v,
                                  const char *name, void *opaque,
                                  Error **errp)
@@ -809,6 +829,12 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
                                    machine_set_zpcii_disable);
     object_class_property_set_description(oc, "zpcii-disable",
             "disable zPCI interpretation facilties");
+
+    object_class_property_add_bool(oc, "topology",
+                                   machine_get_topology,
+                                   machine_set_topology);
+    object_class_property_set_description(oc, "topology",
+            "enable CPU topology");
 }
 
 static inline void s390_machine_initfn(Object *obj)
@@ -818,6 +844,7 @@ static inline void s390_machine_initfn(Object *obj)
     ms->aes_key_wrap = true;
     ms->dea_key_wrap = true;
     ms->zpcii_disable = false;
+    ms->cpu_topology = true;
 }
 
 static const TypeInfo ccw_machine_info = {
@@ -888,6 +915,7 @@ static void ccw_machine_7_1_instance_options(MachineState *machine)
     s390_cpudef_featoff_greater(16, 1, S390_FEAT_PAIE);
     s390_set_qemu_cpu_model(0x8561, 15, 1, qemu_cpu_feat);
     ms->zpcii_disable = true;
+    ms->cpu_topology = true;
 }
 
 static void ccw_machine_7_1_class_options(MachineClass *mc)
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 5325f6bf80..0a040552bd 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -240,6 +240,10 @@ static QemuOptsList machine_opts = {
             .name = "zpcii-disable",
             .type = QEMU_OPT_BOOL,
             .help = "disable zPCI interpretation facilities",
+        },{
+            .name = "topology",
+            .type = QEMU_OPT_BOOL,
+            .help = "disable CPU topology",
         },
         { /* End of list */ }
     }
diff --git a/qemu-options.hx b/qemu-options.hx
index eb38e5dc40..ef59b28a03 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -38,7 +38,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
     "                hmat=on|off controls ACPI HMAT support (default=off)\n"
     "                memory-backend='backend-id' specifies explicitly provided backend for main RAM (default=none)\n"
     "                cxl-fmw.0.targets.0=firsttarget,cxl-fmw.0.targets.1=secondtarget,cxl-fmw.0.size=size[,cxl-fmw.0.interleave-granularity=granularity]\n"
-    "                zpcii-disable=on|off disables zPCI interpretation facilities (default=off)\n",
+    "                zpcii-disable=on|off disables zPCI interpretation facilities (default=off)\n"
+    "                topology=on|off disables CPU topology (default=off)\n",
     QEMU_ARCH_ALL)
 SRST
 ``-machine [type=]name[,prop=value[,...]]``
@@ -163,6 +164,9 @@ SRST
         Disables zPCI interpretation facilties on s390-ccw hosts.
         This feature can be used to disable hardware virtual assists
         related to zPCI devices. The default is off.
+
+    ``topology=on|off``
+        Disables CPU topology on for S390 machines starting with s390-ccw-virtio-7.3.
 ERST
 
 DEF("M", HAS_ARG, QEMU_OPTION_M,
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v11 10/11] s390x/cpu_topology: activating CPU topology
  2022-11-03 17:01 [PATCH v11 00/11] s390x: CPU Topology Pierre Morel
                   ` (8 preceding siblings ...)
  2022-11-03 17:01 ` [PATCH v11 09/11] s390x/cpu topology: add topology machine property Pierre Morel
@ 2022-11-03 17:01 ` Pierre Morel
  2022-11-03 17:01 ` [PATCH v11 11/11] docs/s390x: document s390x cpu topology Pierre Morel
  10 siblings, 0 replies; 36+ messages in thread
From: Pierre Morel @ 2022-11-03 17:01 UTC (permalink / raw)
  To: qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange, clg

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

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

diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index a79fdf1c79..724758680b 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -2471,6 +2471,22 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
         set_bit(S390_FEAT_UNPACK, model->features);
     }
 
+    /*
+     * If we have the CPU Topology allowed in QEMU and
+     * implemented in KVM, activate the CPU TOPOLOGY feature.
+     */
+    if (s390_has_topology()) {
+        if (!kvm_check_extension(kvm_state, KVM_CAP_S390_CPU_TOPOLOGY)) {
+            error_setg(errp, "KVM: S390 topology not present in kernel");
+            return;
+        }
+        if (kvm_vm_enable_cap(kvm_state, KVM_CAP_S390_CPU_TOPOLOGY, 0) < 0) {
+            error_setg(errp, "KVM: Error enabling KVM_CAP_S390_CPU_TOPOLOGY");
+            return;
+        }
+        set_bit(S390_FEAT_CONFIGURATION_TOPOLOGY, model->features);
+    }
+
     /* We emulate a zPCI bus and AEN, therefore we don't need HW support */
     set_bit(S390_FEAT_ZPCI, model->features);
     set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v11 11/11] docs/s390x: document s390x cpu topology
  2022-11-03 17:01 [PATCH v11 00/11] s390x: CPU Topology Pierre Morel
                   ` (9 preceding siblings ...)
  2022-11-03 17:01 ` [PATCH v11 10/11] s390x/cpu_topology: activating CPU topology Pierre Morel
@ 2022-11-03 17:01 ` Pierre Morel
  10 siblings, 0 replies; 36+ messages in thread
From: Pierre Morel @ 2022-11-03 17:01 UTC (permalink / raw)
  To: qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange, clg

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

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

diff --git a/docs/system/s390x/cpu-topology.rst b/docs/system/s390x/cpu-topology.rst
new file mode 100644
index 0000000000..a3bb0efadf
--- /dev/null
+++ b/docs/system/s390x/cpu-topology.rst
@@ -0,0 +1,80 @@
+CPU Topology on s390x
+=====================
+
+CPU Topology on S390x provides up to 5 levels of topology containers:
+nodes, drawers, books, sockets and CPUs.
+While the higher level containers, Containers Topology List Entries,
+(Containers TLE) define a tree hierarchy, the lowest level of topology
+definition, the CPU Topology List Entry (CPU TLE), provides the placement
+of the CPUs inside the parent container.
+
+Currently QEMU CPU topology uses a single level of container: the sockets.
+
+For backward compatibility, threads can be declared on the ``-smp`` command
+line. They will be seen as CPUs by the guest as long as multithreading
+is not really supported by QEMU for S390.
+
+Prerequisites
+-------------
+
+To use CPU Topology a Linux QEMU/KVM machine providing the CPU Topology facility
+(STFLE bit 11) is required.
+
+However, since this facility has been enabled by default in an early version
+of QEMU, we use a capability, ``KVM_CAP_S390_CPU_TOPOLOGY``, to notify KVM
+QEMU use of the CPU Topology.
+
+Indicating the CPU topology to the Virtual Machine
+--------------------------------------------------
+
+The CPU Topology, can be specified on the QEMU command line
+with the ``-smp`` or the ``-device`` QEMU command arguments.
+
+Like in :
+
+.. code-block:: sh
+    -smp cpus=5,sockets=8,cores=2,threads=2,maxcpus=32
+    -device host-s390x-cpu,core-id=14
+
+New CPUs can be plugged using the device_add hmp command like in:
+
+.. code-block:: sh
+   (qemu) device_add host-s390x-cpu,core-id=9
+
+The core-id defines the placement of the core in the topology by
+starting with core 0 in socket 0 up to maxcpus.
+
+In the example above:
+
+* There are 5 CPUs provided to the guest with the ``-smp`` command line
+  They will take the core-ids 0,1,2,3,4
+  As we have 2 threads in 2 cores in a socket, we have 4 CPUs provided
+  to the guest in socket 0, with core-ids 0,1,2,3.
+  The last cpu, with core-id 4, will be on socket 1.
+
+* the core with ID 14 provided by the ``-device`` command line will
+  be placed in socket 3, with core-id 14
+
+* the core with ID 9 provided by the ``device_add`` qmp command will
+  be placed in socket 2, with core-id 9
+
+Note that the core ID is machine wide and the CPU TLE masks provided
+by the STSI instruction will be:
+
+* in socket 0: 0xf0000000 (core id 0,1,2,3)
+* in socket 1: 0x00400000 (core id 9)
+* in socket 1: 0x00020000 (core id 14)
+
+Migration
+---------
+
+For virtio-ccw machines older than s390-virtio-ccw-7.2, CPU Topoogy is
+unavailable.
+
+CPU topology is by default enabled for s390-virtio-ccw-7.2 and newer machines.
+
+Disabling CPU topology can be done by setting the global option
+``topology`` to ``off`` like in:
+
+.. code-block:: sh
+   -machine s390-ccw-virtio-7.2,accel=kvm,topology=off
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 09/11] s390x/cpu topology: add topology machine property
  2022-11-03 17:01 ` [PATCH v11 09/11] s390x/cpu topology: add topology machine property Pierre Morel
@ 2022-11-03 17:20   ` Cornelia Huck
  2022-11-04 10:09     ` Pierre Morel
  2022-11-15 13:48   ` Cédric Le Goater
  1 sibling, 1 reply; 36+ messages in thread
From: Cornelia Huck @ 2022-11-03 17:20 UTC (permalink / raw)
  To: Pierre Morel, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake, armbru,
	seiden, nrb, scgl, frankja, berrange, clg

On Thu, Nov 03 2022, Pierre Morel <pmorel@linux.ibm.com> wrote:

> We keep the possibility to switch on/off the topology on newer
> machines with the property topology=[on|off].
>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  include/hw/boards.h                |  3 +++
>  include/hw/s390x/cpu-topology.h    |  8 +++-----
>  include/hw/s390x/s390-virtio-ccw.h |  1 +
>  hw/core/machine.c                  |  3 +++
>  hw/s390x/cpu-topology.c            | 19 +++++++++++++++++++
>  hw/s390x/s390-virtio-ccw.c         | 28 ++++++++++++++++++++++++++++
>  util/qemu-config.c                 |  4 ++++
>  qemu-options.hx                    |  6 +++++-
>  8 files changed, 66 insertions(+), 6 deletions(-)
>
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 311ed17e18..67147c47bf 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -379,6 +379,9 @@ struct MachineState {
>      } \
>      type_init(machine_initfn##_register_types)
>  
> +extern GlobalProperty hw_compat_7_2[];
> +extern const size_t hw_compat_7_2_len;

This still needs to go into a separate patch that introduces the 8.0
machine types for the relevant machines... I'll probably write that
patch soon (next week or so), you can pick it then into this series.

> +
>  extern GlobalProperty hw_compat_7_1[];
>  extern const size_t hw_compat_7_1_len;
>  


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 01/11] s390x: Register TYPE_S390_CCW_MACHINE properties as class properties
  2022-11-03 17:01 ` [PATCH v11 01/11] s390x: Register TYPE_S390_CCW_MACHINE properties as class properties Pierre Morel
@ 2022-11-04  6:32   ` Thomas Huth
  2022-11-04 10:16     ` Pierre Morel
  0 siblings, 1 reply; 36+ messages in thread
From: Thomas Huth @ 2022-11-04  6:32 UTC (permalink / raw)
  To: Pierre Morel, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, cohuck,
	mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake, armbru,
	seiden, nrb, scgl, frankja, berrange, clg

On 03/11/2022 18.01, Pierre Morel wrote:
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   hw/s390x/s390-virtio-ccw.c | 127 +++++++++++++++++++++----------------
>   1 file changed, 72 insertions(+), 55 deletions(-)

-EMISSINGPATCHDESCRIPTION

... please add some words *why* this is a good idea / necessary.

  Thanks,
   Thomas



^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 09/11] s390x/cpu topology: add topology machine property
  2022-11-03 17:20   ` Cornelia Huck
@ 2022-11-04 10:09     ` Pierre Morel
  0 siblings, 0 replies; 36+ messages in thread
From: Pierre Morel @ 2022-11-04 10:09 UTC (permalink / raw)
  To: Cornelia Huck, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake, armbru,
	seiden, nrb, scgl, frankja, berrange, clg



On 11/3/22 18:20, Cornelia Huck wrote:
> On Thu, Nov 03 2022, Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> We keep the possibility to switch on/off the topology on newer
>> machines with the property topology=[on|off].
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   include/hw/boards.h                |  3 +++
>>   include/hw/s390x/cpu-topology.h    |  8 +++-----
>>   include/hw/s390x/s390-virtio-ccw.h |  1 +
>>   hw/core/machine.c                  |  3 +++
>>   hw/s390x/cpu-topology.c            | 19 +++++++++++++++++++
>>   hw/s390x/s390-virtio-ccw.c         | 28 ++++++++++++++++++++++++++++
>>   util/qemu-config.c                 |  4 ++++
>>   qemu-options.hx                    |  6 +++++-
>>   8 files changed, 66 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 311ed17e18..67147c47bf 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -379,6 +379,9 @@ struct MachineState {
>>       } \
>>       type_init(machine_initfn##_register_types)
>>   
>> +extern GlobalProperty hw_compat_7_2[];
>> +extern const size_t hw_compat_7_2_len;
> 
> This still needs to go into a separate patch that introduces the 8.0
> machine types for the relevant machines... I'll probably write that
> patch soon (next week or so), you can pick it then into this series.
> 

Oh sorry, I forgot to suppress these two definitions for this series.
I do not need this for now.
I will probably need your patch introducing the 8.0 for the next spin.

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 01/11] s390x: Register TYPE_S390_CCW_MACHINE properties as class properties
  2022-11-04  6:32   ` Thomas Huth
@ 2022-11-04 10:16     ` Pierre Morel
  2022-11-04 10:53       ` Cédric Le Goater
  0 siblings, 1 reply; 36+ messages in thread
From: Pierre Morel @ 2022-11-04 10:16 UTC (permalink / raw)
  To: Thomas Huth, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, cohuck,
	mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake, armbru,
	seiden, nrb, scgl, frankja, berrange, clg



On 11/4/22 07:32, Thomas Huth wrote:
> On 03/11/2022 18.01, Pierre Morel wrote:
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/s390-virtio-ccw.c | 127 +++++++++++++++++++++----------------
>>   1 file changed, 72 insertions(+), 55 deletions(-)
> 
> -EMISSINGPATCHDESCRIPTION
> 
> ... please add some words *why* this is a good idea / necessary.

I saw that the i386 patch had no description for the same patch so...

To be honest I do not know why it is necessary.
The only reason I see is to be in sync with the PC implementation.

So what about:
"
Register TYPE_S390_CCW_MACHINE properties as class properties
to be conform with the X architectures
"
?

@Cédric , any official recommendation for doing that?

> 
>   Thanks,
>    Thomas
> 
> 

-- 
Pierre Morel
IBM Lab Boeblingen

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 01/11] s390x: Register TYPE_S390_CCW_MACHINE properties as class properties
  2022-11-04 10:16     ` Pierre Morel
@ 2022-11-04 10:53       ` Cédric Le Goater
  2022-11-04 13:58         ` Pierre Morel
  2022-11-04 14:29         ` Thomas Huth
  0 siblings, 2 replies; 36+ messages in thread
From: Cédric Le Goater @ 2022-11-04 10:53 UTC (permalink / raw)
  To: Pierre Morel, Thomas Huth, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, cohuck,
	mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake, armbru,
	seiden, nrb, scgl, frankja, berrange

On 11/4/22 11:16, Pierre Morel wrote:
> 
> 
> On 11/4/22 07:32, Thomas Huth wrote:
>> On 03/11/2022 18.01, Pierre Morel wrote:
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   hw/s390x/s390-virtio-ccw.c | 127 +++++++++++++++++++++----------------
>>>   1 file changed, 72 insertions(+), 55 deletions(-)
>>
>> -EMISSINGPATCHDESCRIPTION
>>
>> ... please add some words *why* this is a good idea / necessary.
> 
> I saw that the i386 patch had no description for the same patch so...
> 
> To be honest I do not know why it is necessary.
> The only reason I see is to be in sync with the PC implementation.
> 
> So what about:
> "
> Register TYPE_S390_CCW_MACHINE properties as class properties
> to be conform with the X architectures
> "
> ?
> 
> @Cédric , any official recommendation for doing that?

There was a bunch of commits related to QOM in this series :

   91def7b83 arm/virt: Register most properties as class properties
   f5730c69f0 i386: Register feature bit properties as class properties

which moved property definitions at the class level.

Then,

   commit d8fb7d0969 ("vl: switch -M parsing to keyval")

changed machine_help_func() to use a machine class and not machine
instance anymore.

I would use the same kind of commit log and add a Fixes tag to get it
merged in 7.2

With that,

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.
	


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 01/11] s390x: Register TYPE_S390_CCW_MACHINE properties as class properties
  2022-11-04 10:53       ` Cédric Le Goater
@ 2022-11-04 13:58         ` Pierre Morel
  2022-11-04 14:29         ` Thomas Huth
  1 sibling, 0 replies; 36+ messages in thread
From: Pierre Morel @ 2022-11-04 13:58 UTC (permalink / raw)
  To: Cédric Le Goater, Thomas Huth, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, cohuck,
	mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake, armbru,
	seiden, nrb, scgl, frankja, berrange



On 11/4/22 11:53, Cédric Le Goater wrote:
> On 11/4/22 11:16, Pierre Morel wrote:
>>
>>
>> On 11/4/22 07:32, Thomas Huth wrote:
>>> On 03/11/2022 18.01, Pierre Morel wrote:
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> ---
>>>>   hw/s390x/s390-virtio-ccw.c | 127 
>>>> +++++++++++++++++++++----------------
>>>>   1 file changed, 72 insertions(+), 55 deletions(-)
>>>
>>> -EMISSINGPATCHDESCRIPTION
>>>
>>> ... please add some words *why* this is a good idea / necessary.
>>
>> I saw that the i386 patch had no description for the same patch so...
>>
>> To be honest I do not know why it is necessary.
>> The only reason I see is to be in sync with the PC implementation.
>>
>> So what about:
>> "
>> Register TYPE_S390_CCW_MACHINE properties as class properties
>> to be conform with the X architectures
>> "
>> ?
>>
>> @Cédric , any official recommendation for doing that?
> 
> There was a bunch of commits related to QOM in this series :
> 
>    91def7b83 arm/virt: Register most properties as class properties
>    f5730c69f0 i386: Register feature bit properties as class properties
> 
> which moved property definitions at the class level.
> 
> Then,
> 
>    commit d8fb7d0969 ("vl: switch -M parsing to keyval")
> 
> changed machine_help_func() to use a machine class and not machine
> instance anymore.
> 
> I would use the same kind of commit log and add a Fixes tag to get it
> merged in 7.2
> 
> With that,
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C.
> 
> 

OK,
Thanks,

Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 01/11] s390x: Register TYPE_S390_CCW_MACHINE properties as class properties
  2022-11-04 10:53       ` Cédric Le Goater
  2022-11-04 13:58         ` Pierre Morel
@ 2022-11-04 14:29         ` Thomas Huth
  2022-11-04 14:57           ` Pierre Morel
  1 sibling, 1 reply; 36+ messages in thread
From: Thomas Huth @ 2022-11-04 14:29 UTC (permalink / raw)
  To: Cédric Le Goater, Pierre Morel, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, cohuck,
	mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake, armbru,
	seiden, nrb, scgl, frankja, berrange

On 04/11/2022 11.53, Cédric Le Goater wrote:
> On 11/4/22 11:16, Pierre Morel wrote:
>>
>>
>> On 11/4/22 07:32, Thomas Huth wrote:
>>> On 03/11/2022 18.01, Pierre Morel wrote:
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> ---
>>>>   hw/s390x/s390-virtio-ccw.c | 127 +++++++++++++++++++++----------------
>>>>   1 file changed, 72 insertions(+), 55 deletions(-)
>>>
>>> -EMISSINGPATCHDESCRIPTION
>>>
>>> ... please add some words *why* this is a good idea / necessary.
>>
>> I saw that the i386 patch had no description for the same patch so...
>>
>> To be honest I do not know why it is necessary.
>> The only reason I see is to be in sync with the PC implementation.
>>
>> So what about:
>> "
>> Register TYPE_S390_CCW_MACHINE properties as class properties
>> to be conform with the X architectures
>> "
>> ?
>>
>> @Cédric , any official recommendation for doing that?
> 
> There was a bunch of commits related to QOM in this series :
> 
>    91def7b83 arm/virt: Register most properties as class properties
>    f5730c69f0 i386: Register feature bit properties as class properties
> 
> which moved property definitions at the class level.
> 
> Then,
> 
>    commit d8fb7d0969 ("vl: switch -M parsing to keyval")
> 
> changed machine_help_func() to use a machine class and not machine
> instance anymore.
> 
> I would use the same kind of commit log and add a Fixes tag to get it
> merged in 7.2

Ah, so this fixes the problem that running QEMU with " -M 
s390-ccw-virtio,help" does not show the s390x-specific properties anymore? 
... that's certainly somethings that should be mentioned in the commit 
message! What about something like this:

"Currently, when running 'qemu-system-s390x -M -M s390-ccw-virtio,help' the 
s390x-specific properties are not listed anymore. This happens because since 
commit d8fb7d0969 ("vl: switch -M parsing to keyval") the properties have to 
be defined at the class level and not at the instance level anymore. Fix it 
on s390x now, too, by moving the registration of the properties to the class 
level"

Fixes: d8fb7d0969 ("vl: switch -M parsing to keyval")

?

  Thomas


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 01/11] s390x: Register TYPE_S390_CCW_MACHINE properties as class properties
  2022-11-04 14:29         ` Thomas Huth
@ 2022-11-04 14:57           ` Pierre Morel
  2022-11-06 11:37             ` Thomas Huth
  0 siblings, 1 reply; 36+ messages in thread
From: Pierre Morel @ 2022-11-04 14:57 UTC (permalink / raw)
  To: Thomas Huth, Cédric Le Goater, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, cohuck,
	mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake, armbru,
	seiden, nrb, scgl, frankja, berrange



On 11/4/22 15:29, Thomas Huth wrote:
> On 04/11/2022 11.53, Cédric Le Goater wrote:
>> On 11/4/22 11:16, Pierre Morel wrote:
>>>
>>>
>>> On 11/4/22 07:32, Thomas Huth wrote:
>>>> On 03/11/2022 18.01, Pierre Morel wrote:
>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>>> ---
>>>>>   hw/s390x/s390-virtio-ccw.c | 127 
>>>>> +++++++++++++++++++++----------------
>>>>>   1 file changed, 72 insertions(+), 55 deletions(-)
>>>>
>>>> -EMISSINGPATCHDESCRIPTION
>>>>
>>>> ... please add some words *why* this is a good idea / necessary.
>>>
>>> I saw that the i386 patch had no description for the same patch so...
>>>
>>> To be honest I do not know why it is necessary.
>>> The only reason I see is to be in sync with the PC implementation.
>>>
>>> So what about:
>>> "
>>> Register TYPE_S390_CCW_MACHINE properties as class properties
>>> to be conform with the X architectures
>>> "
>>> ?
>>>
>>> @Cédric , any official recommendation for doing that?
>>
>> There was a bunch of commits related to QOM in this series :
>>
>>    91def7b83 arm/virt: Register most properties as class properties
>>    f5730c69f0 i386: Register feature bit properties as class properties
>>
>> which moved property definitions at the class level.
>>
>> Then,
>>
>>    commit d8fb7d0969 ("vl: switch -M parsing to keyval")
>>
>> changed machine_help_func() to use a machine class and not machine
>> instance anymore.
>>
>> I would use the same kind of commit log and add a Fixes tag to get it
>> merged in 7.2
> 
> Ah, so this fixes the problem that running QEMU with " -M 
> s390-ccw-virtio,help" does not show the s390x-specific properties 
> anymore? ... that's certainly somethings that should be mentioned in the 
> commit message! What about something like this:
> 
> "Currently, when running 'qemu-system-s390x -M -M s390-ccw-virtio,help' 
> the s390x-specific properties are not listed anymore. This happens 
> because since commit d8fb7d0969 ("vl: switch -M parsing to keyval") the 
> properties have to be defined at the class level and not at the instance 
> level anymore. Fix it on s390x now, too, by moving the registration of 
> the properties to the class level"
> 
> Fixes: d8fb7d0969 ("vl: switch -M parsing to keyval")
> 
> ?
> 
>   Thomas
> 

That seems really good :)

Thank you Thomas!

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 01/11] s390x: Register TYPE_S390_CCW_MACHINE properties as class properties
  2022-11-04 14:57           ` Pierre Morel
@ 2022-11-06 11:37             ` Thomas Huth
  2022-11-07  9:52               ` Pierre Morel
  0 siblings, 1 reply; 36+ messages in thread
From: Thomas Huth @ 2022-11-06 11:37 UTC (permalink / raw)
  To: Pierre Morel, Cédric Le Goater, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, cohuck,
	mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake, armbru,
	seiden, nrb, scgl, frankja, berrange

On 04/11/2022 15.57, Pierre Morel wrote:
> 
> 
> On 11/4/22 15:29, Thomas Huth wrote:
>> On 04/11/2022 11.53, Cédric Le Goater wrote:
>>> On 11/4/22 11:16, Pierre Morel wrote:
>>>>
>>>>
>>>> On 11/4/22 07:32, Thomas Huth wrote:
>>>>> On 03/11/2022 18.01, Pierre Morel wrote:
>>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>>>> ---
>>>>>>   hw/s390x/s390-virtio-ccw.c | 127 +++++++++++++++++++++----------------
>>>>>>   1 file changed, 72 insertions(+), 55 deletions(-)
>>>>>
>>>>> -EMISSINGPATCHDESCRIPTION
>>>>>
>>>>> ... please add some words *why* this is a good idea / necessary.
>>>>
>>>> I saw that the i386 patch had no description for the same patch so...
>>>>
>>>> To be honest I do not know why it is necessary.
>>>> The only reason I see is to be in sync with the PC implementation.
>>>>
>>>> So what about:
>>>> "
>>>> Register TYPE_S390_CCW_MACHINE properties as class properties
>>>> to be conform with the X architectures
>>>> "
>>>> ?
>>>>
>>>> @Cédric , any official recommendation for doing that?
>>>
>>> There was a bunch of commits related to QOM in this series :
>>>
>>>    91def7b83 arm/virt: Register most properties as class properties
>>>    f5730c69f0 i386: Register feature bit properties as class properties
>>>
>>> which moved property definitions at the class level.
>>>
>>> Then,
>>>
>>>    commit d8fb7d0969 ("vl: switch -M parsing to keyval")
>>>
>>> changed machine_help_func() to use a machine class and not machine
>>> instance anymore.
>>>
>>> I would use the same kind of commit log and add a Fixes tag to get it
>>> merged in 7.2
>>
>> Ah, so this fixes the problem that running QEMU with " -M 
>> s390-ccw-virtio,help" does not show the s390x-specific properties anymore? 
>> ... that's certainly somethings that should be mentioned in the commit 
>> message! What about something like this:
>>
>> "Currently, when running 'qemu-system-s390x -M -M s390-ccw-virtio,help' 
>> the s390x-specific properties are not listed anymore. This happens because 
>> since commit d8fb7d0969 ("vl: switch -M parsing to keyval") the properties 
>> have to be defined at the class level and not at the instance level 
>> anymore. Fix it on s390x now, too, by moving the registration of the 
>> properties to the class level"
>>
>> Fixes: d8fb7d0969 ("vl: switch -M parsing to keyval")
>>
>> ?
>>
>>   Thomas
>>
> 
> That seems really good :)

All right, I've queued this patch (with the updated commit description) and 
the next one on my s390x-branch for QEMU 7.2:

  https://gitlab.com/thuth/qemu/-/commits/s390x-next/

  Thomas


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 01/11] s390x: Register TYPE_S390_CCW_MACHINE properties as class properties
  2022-11-06 11:37             ` Thomas Huth
@ 2022-11-07  9:52               ` Pierre Morel
  0 siblings, 0 replies; 36+ messages in thread
From: Pierre Morel @ 2022-11-07  9:52 UTC (permalink / raw)
  To: Thomas Huth, Cédric Le Goater, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, cohuck,
	mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake, armbru,
	seiden, nrb, scgl, frankja, berrange



On 11/6/22 12:37, Thomas Huth wrote:
> On 04/11/2022 15.57, Pierre Morel wrote:
>>
>>
>> On 11/4/22 15:29, Thomas Huth wrote:
>>> On 04/11/2022 11.53, Cédric Le Goater wrote:
>>>> On 11/4/22 11:16, Pierre Morel wrote:
>>>>>
>>>>>
>>>>> On 11/4/22 07:32, Thomas Huth wrote:
>>>>>> On 03/11/2022 18.01, Pierre Morel wrote:
>>>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>>>>> ---
>>>>>>>   hw/s390x/s390-virtio-ccw.c | 127 
>>>>>>> +++++++++++++++++++++----------------
>>>>>>>   1 file changed, 72 insertions(+), 55 deletions(-)
>>>>>>
>>>>>> -EMISSINGPATCHDESCRIPTION
>>>>>>
>>>>>> ... please add some words *why* this is a good idea / necessary.
>>>>>
>>>>> I saw that the i386 patch had no description for the same patch so...
>>>>>
>>>>> To be honest I do not know why it is necessary.
>>>>> The only reason I see is to be in sync with the PC implementation.
>>>>>
>>>>> So what about:
>>>>> "
>>>>> Register TYPE_S390_CCW_MACHINE properties as class properties
>>>>> to be conform with the X architectures
>>>>> "
>>>>> ?
>>>>>
>>>>> @Cédric , any official recommendation for doing that?
>>>>
>>>> There was a bunch of commits related to QOM in this series :
>>>>
>>>>    91def7b83 arm/virt: Register most properties as class properties
>>>>    f5730c69f0 i386: Register feature bit properties as class properties
>>>>
>>>> which moved property definitions at the class level.
>>>>
>>>> Then,
>>>>
>>>>    commit d8fb7d0969 ("vl: switch -M parsing to keyval")
>>>>
>>>> changed machine_help_func() to use a machine class and not machine
>>>> instance anymore.
>>>>
>>>> I would use the same kind of commit log and add a Fixes tag to get it
>>>> merged in 7.2
>>>
>>> Ah, so this fixes the problem that running QEMU with " -M 
>>> s390-ccw-virtio,help" does not show the s390x-specific properties 
>>> anymore? ... that's certainly somethings that should be mentioned in 
>>> the commit message! What about something like this:
>>>
>>> "Currently, when running 'qemu-system-s390x -M -M 
>>> s390-ccw-virtio,help' the s390x-specific properties are not listed 
>>> anymore. This happens because since commit d8fb7d0969 ("vl: switch -M 
>>> parsing to keyval") the properties have to be defined at the class 
>>> level and not at the instance level anymore. Fix it on s390x now, 
>>> too, by moving the registration of the properties to the class level"
>>>
>>> Fixes: d8fb7d0969 ("vl: switch -M parsing to keyval")
>>>
>>> ?
>>>
>>>   Thomas
>>>
>>
>> That seems really good :)
> 
> All right, I've queued this patch (with the updated commit description) 
> and the next one on my s390x-branch for QEMU 7.2:
> 
>   https://gitlab.com/thuth/qemu/-/commits/s390x-next/
> 
>   Thomas
> 
> 

Thank you!

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 03/11] s390x/cpu topology: core_id sets s390x CPU topology
  2022-11-03 17:01 ` [PATCH v11 03/11] s390x/cpu topology: core_id sets s390x CPU topology Pierre Morel
@ 2022-11-15 11:15   ` Cédric Le Goater
  2022-11-16 10:17     ` Pierre Morel
  0 siblings, 1 reply; 36+ messages in thread
From: Cédric Le Goater @ 2022-11-15 11:15 UTC (permalink / raw)
  To: Pierre Morel, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange

Hello Pierre,

On 11/3/22 18:01, Pierre Morel wrote:
> In the S390x CPU topology the core_id specifies the CPU address
> and the position of the core withing the topology.
> 
> Let's build the topology based on the core_id.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   include/hw/s390x/cpu-topology.h    |  41 ++++++++++
>   include/hw/s390x/s390-virtio-ccw.h |   1 +
>   target/s390x/cpu.h                 |   2 +
>   hw/s390x/cpu-topology.c            | 125 +++++++++++++++++++++++++++++
>   hw/s390x/s390-virtio-ccw.c         |  23 ++++++
>   hw/s390x/meson.build               |   1 +
>   6 files changed, 193 insertions(+)
>   create mode 100644 include/hw/s390x/cpu-topology.h
>   create mode 100644 hw/s390x/cpu-topology.c
> 
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> new file mode 100644
> index 0000000000..4e16a2153d
> --- /dev/null
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -0,0 +1,41 @@
> +/*
> + * CPU Topology
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +#ifndef HW_S390X_CPU_TOPOLOGY_H
> +#define HW_S390X_CPU_TOPOLOGY_H
> +
> +#include "hw/qdev-core.h"
> +#include "qom/object.h"
> +
> +#define S390_TOPOLOGY_CPU_IFL 0x03
> +#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
> +
> +typedef struct S390TopoSocket {
> +    int active_count;
> +    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
> +} S390TopoSocket;
> +
> +struct S390Topology {
> +    SysBusDevice parent_obj;
> +    uint32_t nr_cpus;
> +    uint32_t nr_sockets;
> +    S390TopoSocket *socket;
> +};
> +
> +#define TYPE_S390_CPU_TOPOLOGY "s390-topology"
> +OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY)
> +
> +void s390_topology_new_cpu(S390CPU *cpu);
> +
> +static inline bool s390_has_topology(void)
> +{
> +    return false;
> +}
> +
> +#endif
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 4f8a39abda..23b472708d 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -29,6 +29,7 @@ struct S390CcwMachineState {
>       bool pv;
>       bool zpcii_disable;
>       uint8_t loadparm[8];
> +    void *topology;

I think it is safe to use 'DeviceState *' or 'Object *' for the pointer
under machine. What is most practical.

>   };
>   
>   struct S390CcwMachineClass {
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 7d6d01325b..c9066b2496 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -175,6 +175,8 @@ struct ArchCPU {
>       /* needed for live migration */
>       void *irqstate;
>       uint32_t irqstate_saved_size;
> +    /* Topology this CPU belongs too */
> +    void *topology;

However, under the CPU, we don't know what the future changes reserve
for us and I would call the attribute 'opaque' or 'machine_data'.

For now, it only holds a reference to the S390Topology device model
but it could become a struct with time.
  

>   };
>   
>   
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> new file mode 100644
> index 0000000000..6af41d3d7b
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c
> @@ -0,0 +1,125 @@
> +/*
> + * CPU Topology
> + *
> + * Copyright IBM Corp. 2022
> + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
> +
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/error-report.h"
> +#include "hw/sysbus.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/boards.h"
> +#include "qemu/typedefs.h"
> +#include "target/s390x/cpu.h"
> +#include "hw/s390x/s390-virtio-ccw.h"
> +#include "hw/s390x/cpu-topology.h"
> +
> +/*
> + * s390_topology_new_cpu:
> + * @cpu: a pointer to the new CPU
> + *
> + * The topology pointed by S390CPU, gives us the CPU topology
> + * established by the -smp QEMU aruments.
> + * The core-id is used to calculate the position of the CPU inside
> + * the topology:
> + *  - the socket, container TLE, containing the CPU, we have one socket
> + *    for every nr_cpus (nr_cpus = smp.cores * smp.threads)
> + *  - the CPU TLE inside the socket, we have potentionly up to 4 CPU TLE
> + *    in a container TLE with the assumption that all CPU are identical
> + *    with the same polarity and entitlement because we have maximum 256
> + *    CPUs and each TLE can hold up to 64 identical CPUs.
> + *  - the bit in the 64 bit CPU TLE core mask
> + */
> +void s390_topology_new_cpu(S390CPU *cpu)
> +{
> +    S390Topology *topo = (S390Topology *)cpu->topology;

where is cpu->topology set ?

> +    int core_id = cpu->env.core_id;
> +    int bit, origin;
> +    int socket_id;
> +
> +    socket_id = core_id / topo->nr_cpus;
> +
> +    /*
> +     * At the core level, each CPU is represented by a bit in a 64bit
> +     * uint64_t which represent the presence of a CPU.
> +     * The firmware assume that all CPU in a CPU TLE have the same
> +     * type, polarization and are all dedicated or shared.
> +     * In that case the origin variable represents the offset of the first
> +     * CPU in the CPU container.
> +     * More than 64 CPUs per socket are represented in several CPU containers
> +     * inside the socket container.
> +     * The only reason to have several S390TopologyCores inside a socket is
> +     * to have more than 64 CPUs.
> +     * In that case the origin variable represents the offset of the first CPU
> +     * in the CPU container. More than 64 CPUs per socket are represented in
> +     * several CPU containers inside the socket container.
> +     */
> +    bit = core_id;
> +    origin = bit / 64;
> +    bit %= 64;
> +    bit = 63 - bit;
> +
> +    topo->socket[socket_id].active_count++;
> +    set_bit(bit, &topo->socket[socket_id].mask[origin]);
> +}
> +
> +/**
> + * s390_topology_realize:
> + * @dev: the device state
> + * @errp: the error pointer (not used)
> + *
> + * During realize the machine CPU topology is initialized with the
> + * QEMU -smp parameters.
> + * The maximum count of CPU TLE in the all Topology can not be greater
> + * than the maximum CPUs.
> + */
> +static void s390_topology_realize(DeviceState *dev, Error **errp)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());

hmm,

> +    S390Topology *topo = S390_CPU_TOPOLOGY(dev);
> +
> +    topo->nr_cpus = ms->smp.cores * ms->smp.threads;
> +    topo->nr_sockets = ms->smp.sockets;

These properties should be set in s390_init_topology() with :

   object_property_set_int(OBJECT(dev), "num-cpus",
                           ms->smp.cores * ms->smp.threads, errp);

   object_property_set_int(OBJECT(dev), "num-sockets",
                           ms->smp.sockets, errp);

before calling sysbus_realize_and_unref()


> +    topo->socket = g_new0(S390TopoSocket, topo->nr_sockets);

For consistency, you could add an unrealize handler to free the array.

> +}
> +
> +static Property s390_topology_properties[] = {
> +    DEFINE_PROP_UINT32("nr_cpus", S390Topology, nr_cpus, 1),
> +    DEFINE_PROP_UINT32("nr_sockets", S390Topology, nr_sockets, 1),

A quick audit of the property names in QEMU code shows that a "num-" prefix
is usually preferred.

> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +/**
> + * topology_class_init:
> + * @oc: Object class
> + * @data: (not used)
> + *
> + * A very simple object we will need for reset and migration.
> + */
> +static void topology_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = s390_topology_realize;
> +    device_class_set_props(dc, s390_topology_properties);
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +}
> +
> +static const TypeInfo cpu_topology_info = {
> +    .name          = TYPE_S390_CPU_TOPOLOGY,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(S390Topology),
> +    .class_init    = topology_class_init,
> +};
> +
> +static void topology_register(void)
> +{
> +    type_register_static(&cpu_topology_info);
> +}
> +type_init(topology_register);
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 9ab91df5b1..5776d3e58f 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -44,6 +44,7 @@
>   #include "hw/s390x/pv.h"
>   #include "migration/blocker.h"
>   #include "qapi/visitor.h"
> +#include "hw/s390x/cpu-topology.h"
>   
>   static Error *pv_mig_blocker;
>   
> @@ -102,6 +103,19 @@ static void s390_init_cpus(MachineState *machine)
>       }
>   }
>   
> +static void s390_init_topology(MachineState *machine)
> +{
> +    DeviceState *dev;
> +
> +    if (s390_has_topology()) {

I would move the s390_has_topology() check in the caller.

> +        dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
> +        object_property_add_child(&machine->parent_obj,
> +                                  TYPE_S390_CPU_TOPOLOGY, OBJECT(dev));
> +        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +        S390_CCW_MACHINE(machine)->topology = dev;

and I would move the assignment in the caller also.

> +    }
> +}
> +
>   static const char *const reset_dev_types[] = {
>       TYPE_VIRTUAL_CSS_BRIDGE,
>       "s390-sclp-event-facility",
> @@ -252,6 +266,9 @@ static void ccw_init(MachineState *machine)
>       /* init memory + setup max page size. Required for the CPU model */
>       s390_memory_init(machine->ram);
>   
> +    /* Adding the topology must be done before CPU initialization */
> +    s390_init_topology(machine);
> +
>       /* init CPUs (incl. CPU model) early so s390_has_feature() works */
>       s390_init_cpus(machine);
>   
> @@ -314,6 +331,12 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev,
>       g_assert(!ms->possible_cpus->cpus[cpu->env.core_id].cpu);
>       ms->possible_cpus->cpus[cpu->env.core_id].cpu = OBJECT(dev);
>   
> +    /* Inserting the CPU in the Topology can not fail */
> +    if (S390_CCW_MACHINE(ms)->topology) {
> +        cpu->topology = S390_CCW_MACHINE(ms)->topology;

Two QOM cast. One should be enough. Please introduce a local variable.

> +        s390_topology_new_cpu(cpu);

I would pass the 'topology' object as a parameter of s390_topology_new_cpu()
and do the cpu->topology assignment in the same routine.

May be rename it also to :

   void s390_topology_add_cpu(S390Topology *topo, S390CPU *cpu)


Thanks,

C.
  
> +    }
> +
>       if (dev->hotplugged) {
>           raise_irq_cpu_hotplug();
>       }
> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> index f291016fee..653f6ab488 100644
> --- a/hw/s390x/meson.build
> +++ b/hw/s390x/meson.build
> @@ -2,6 +2,7 @@ s390x_ss = ss.source_set()
>   s390x_ss.add(files(
>     'ap-bridge.c',
>     'ap-device.c',
> +  'cpu-topology.c',
>     'ccw-device.c',
>     'css-bridge.c',
>     'css.c',


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 04/11] s390x/cpu topology: reporting the CPU topology to the guest
  2022-11-03 17:01 ` [PATCH v11 04/11] s390x/cpu topology: reporting the CPU topology to the guest Pierre Morel
@ 2022-11-15 11:21   ` Cédric Le Goater
  2022-11-16 10:27     ` Pierre Morel
  2022-11-17  8:40   ` Cédric Le Goater
  1 sibling, 1 reply; 36+ messages in thread
From: Cédric Le Goater @ 2022-11-15 11:21 UTC (permalink / raw)
  To: Pierre Morel, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange

Hello Pierre,

On 11/3/22 18:01, Pierre Morel wrote:
> The guest can use the STSI instruction to get a buffer filled
> with the CPU topology description.
> 
> Let us implement the STSI instruction for the basis CPU topology
> level, level 2.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   include/hw/s390x/cpu-topology.h |   6 ++
>   target/s390x/cpu.h              |  77 ++++++++++++++++++++++++
>   hw/s390x/cpu-topology.c         |   1 -
>   target/s390x/cpu_topology.c     | 100 ++++++++++++++++++++++++++++++++
>   target/s390x/kvm/kvm.c          |   6 +-
>   target/s390x/meson.build        |   1 +
>   6 files changed, 189 insertions(+), 2 deletions(-)
>   create mode 100644 target/s390x/cpu_topology.c
> 
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> index 4e16a2153d..6fec10e032 100644
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -16,6 +16,11 @@
>   #define S390_TOPOLOGY_CPU_IFL 0x03
>   #define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
>   
> +#define S390_TOPOLOGY_POLARITY_HORIZONTAL      0x00
> +#define S390_TOPOLOGY_POLARITY_VERTICAL_LOW    0x01
> +#define S390_TOPOLOGY_POLARITY_VERTICAL_MEDIUM 0x02
> +#define S390_TOPOLOGY_POLARITY_VERTICAL_HIGH   0x03
> +
>   typedef struct S390TopoSocket {
>       int active_count;
>       uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
> @@ -26,6 +31,7 @@ struct S390Topology {
>       uint32_t nr_cpus;
>       uint32_t nr_sockets;
>       S390TopoSocket *socket;
> +    bool topology_needed;


This is unused in this patch. Introduced too soon ?

  
>   };
>   
>   #define TYPE_S390_CPU_TOPOLOGY "s390-topology"
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index c9066b2496..69a7523146 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -567,6 +567,81 @@ typedef union SysIB {
>   } SysIB;
>   QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
>   
> +/*
> + * CPU Topology List provided by STSI with fc=15 provides a list
> + * of two different Topology List Entries (TLE) types to specify
> + * the topology hierarchy.
> + *
> + * - Container Topology List Entry
> + *   Defines a container to contain other Topology List Entries
> + *   of any type, nested containers or CPU.
> + * - CPU Topology List Entry
> + *   Specifies the CPUs position, type, entitlement and polarization
> + *   of the CPUs contained in the last Container TLE.
> + *
> + * There can be theoretically up to five levels of containers, QEMU
> + * uses only one level, the socket level.
> + *
> + * A container of with a nesting level (NL) greater than 1 can only
> + * contain another container of nesting level NL-1.
> + *
> + * A container of nesting level 1 (socket), contains as many CPU TLE
> + * as needed to describe the position and qualities of all CPUs inside
> + * the container.
> + * The qualities of a CPU are polarization, entitlement and type.
> + *
> + * The CPU TLE defines the position of the CPUs of identical qualities
> + * using a 64bits mask which first bit has its offset defined by
> + * the CPU address orgin field of the CPU TLE like in:
> + * CPU address = origin * 64 + bit position within the mask
> + *
> + */
> +/* Container type Topology List Entry */
> +typedef struct SysIBTl_container {
> +        uint8_t nl;
> +        uint8_t reserved[6];
> +        uint8_t id;
> +} QEMU_PACKED QEMU_ALIGNED(8) SysIBTl_container;
> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_container) != 8);
> +
> +/* CPU type Topology List Entry */
> +typedef struct SysIBTl_cpu {
> +        uint8_t nl;
> +        uint8_t reserved0[3];
> +        uint8_t reserved1:5;
> +        uint8_t dedicated:1;
> +        uint8_t polarity:2;
> +        uint8_t type;
> +        uint16_t origin;
> +        uint64_t mask;
> +} QEMU_PACKED QEMU_ALIGNED(8) SysIBTl_cpu;
> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16);
> +
> +#define S390_TOPOLOGY_MAG  6
> +#define S390_TOPOLOGY_MAG6 0
> +#define S390_TOPOLOGY_MAG5 1
> +#define S390_TOPOLOGY_MAG4 2
> +#define S390_TOPOLOGY_MAG3 3
> +#define S390_TOPOLOGY_MAG2 4
> +#define S390_TOPOLOGY_MAG1 5
> +/* Configuration topology */
> +typedef struct SysIB_151x {
> +    uint8_t  reserved0[2];
> +    uint16_t length;
> +    uint8_t  mag[S390_TOPOLOGY_MAG];
> +    uint8_t  reserved1;
> +    uint8_t  mnest;
> +    uint32_t reserved2;
> +    char tle[0];
> +} QEMU_PACKED QEMU_ALIGNED(8) SysIB_151x;
> +QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
> +
> +/* Max size of a SYSIB structure is when all CPU are alone in a container */
> +#define S390_TOPOLOGY_SYSIB_SIZE (sizeof(SysIB_151x) +                         \
> +                                  S390_MAX_CPUS * (sizeof(SysIBTl_container) + \
> +                                                   sizeof(SysIBTl_cpu)))
> +
> +
>   /* MMU defines */
>   #define ASCE_ORIGIN           (~0xfffULL) /* segment table origin             */
>   #define ASCE_SUBSPACE         0x200       /* subspace group control           */
> @@ -845,4 +920,6 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
>   
>   #include "exec/cpu-all.h"
>   
> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar);
> +
>   #endif
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> index 6af41d3d7b..9fa8ca1261 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -44,7 +44,6 @@ void s390_topology_new_cpu(S390CPU *cpu)
>       int socket_id;
>   
>       socket_id = core_id / topo->nr_cpus;
> -

Unnecessary change.

Thanks,

C.



>       /*
>        * At the core level, each CPU is represented by a bit in a 64bit
>        * uint64_t which represent the presence of a CPU.
> diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
> new file mode 100644
> index 0000000000..a1179d8e95
> --- /dev/null
> +++ b/target/s390x/cpu_topology.c
> @@ -0,0 +1,100 @@
> +/*
> + * QEMU S390x CPU Topology
> + *
> + * Copyright IBM Corp. 2022
> + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#include "hw/s390x/pv.h"
> +#include "hw/sysbus.h"
> +#include "hw/s390x/cpu-topology.h"
> +#include "hw/s390x/sclp.h"
> +
> +static char *fill_container(char *p, int level, int id)
> +{
> +    SysIBTl_container *tle = (SysIBTl_container *)p;
> +
> +    tle->nl = level;
> +    tle->id = id;
> +    return p + sizeof(*tle);
> +}
> +
> +static char *fill_tle_cpu(char *p, uint64_t mask, int origin)
> +{
> +    SysIBTl_cpu *tle = (SysIBTl_cpu *)p;
> +
> +    tle->nl = 0;
> +    tle->dedicated = 1;
> +    tle->polarity = S390_TOPOLOGY_POLARITY_HORIZONTAL;
> +    tle->type = S390_TOPOLOGY_CPU_IFL;
> +    tle->origin = cpu_to_be64(origin * 64);
> +    tle->mask = cpu_to_be64(mask);
> +    return p + sizeof(*tle);
> +}
> +
> +static char *s390_top_set_level2(S390Topology *topo, char *p)
> +{
> +    int i, origin;
> +
> +    for (i = 0; i < topo->nr_sockets; i++) {
> +        if (!topo->socket[i].active_count) {
> +            continue;
> +        }
> +        p = fill_container(p, 1, i);
> +        for (origin = 0; origin < S390_TOPOLOGY_MAX_ORIGIN; origin++) {
> +            uint64_t mask = 0L;
> +
> +            mask = topo->socket[i].mask[origin];
> +            if (mask) {
> +                p = fill_tle_cpu(p, mask, origin);
> +            }
> +        }
> +    }
> +    return p;
> +}
> +
> +static int setup_stsi(S390CPU *cpu, SysIB_151x *sysib, int level)
> +{
> +    S390Topology *topo = (S390Topology *)cpu->topology;
> +    char *p = sysib->tle;
> +
> +    sysib->mnest = level;
> +    switch (level) {
> +    case 2:
> +        sysib->mag[S390_TOPOLOGY_MAG2] = topo->nr_sockets;
> +        sysib->mag[S390_TOPOLOGY_MAG1] = topo->nr_cpus;
> +        p = s390_top_set_level2(topo, p);
> +        break;
> +    }
> +
> +    return p - (char *)sysib;
> +}
> +
> +#define S390_TOPOLOGY_MAX_MNEST 2
> +
> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
> +{
> +    union {
> +        char place_holder[S390_TOPOLOGY_SYSIB_SIZE];
> +        SysIB_151x sysib;
> +    } buffer QEMU_ALIGNED(8);
> +    int len;
> +
> +    if (s390_is_pv() || !s390_has_topology() ||
> +        sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
> +        setcc(cpu, 3);
> +        return;
> +    }
> +
> +    len = setup_stsi(cpu, &buffer.sysib, sel2);
> +
> +    buffer.sysib.length = cpu_to_be16(len);
> +    s390_cpu_virt_mem_write(cpu, addr, ar, &buffer.sysib, len);
> +    setcc(cpu, 0);
> +}
> +
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 3ac7ec9acf..7dc96f3663 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -51,6 +51,7 @@
>   #include "hw/s390x/s390-virtio-ccw.h"
>   #include "hw/s390x/s390-virtio-hcall.h"
>   #include "hw/s390x/pv.h"
> +#include "hw/s390x/cpu-topology.h"
>   
>   #ifndef DEBUG_KVM
>   #define DEBUG_KVM  0
> @@ -1919,9 +1920,12 @@ static int handle_stsi(S390CPU *cpu)
>           if (run->s390_stsi.sel1 != 2 || run->s390_stsi.sel2 != 2) {
>               return 0;
>           }
> -        /* Only sysib 3.2.2 needs post-handling for now. */
>           insert_stsi_3_2_2(cpu, run->s390_stsi.addr, run->s390_stsi.ar);
>           return 0;
> +    case 15:
> +        insert_stsi_15_1_x(cpu, run->s390_stsi.sel2, run->s390_stsi.addr,
> +                           run->s390_stsi.ar);
> +        return 0;
>       default:
>           return 0;
>       }
> diff --git a/target/s390x/meson.build b/target/s390x/meson.build
> index 84c1402a6a..890ccfa789 100644
> --- a/target/s390x/meson.build
> +++ b/target/s390x/meson.build
> @@ -29,6 +29,7 @@ s390x_softmmu_ss.add(files(
>     'sigp.c',
>     'cpu-sysemu.c',
>     'cpu_models_sysemu.c',
> +  'cpu_topology.c',
>   ))
>   
>   s390x_user_ss = ss.source_set()


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 08/11] s390x/cpu topology: add topology_capable QEMU capability
  2022-11-03 17:01 ` [PATCH v11 08/11] s390x/cpu topology: add topology_capable QEMU capability Pierre Morel
@ 2022-11-15 13:27   ` Cédric Le Goater
  2022-11-16 11:23     ` Pierre Morel
  0 siblings, 1 reply; 36+ messages in thread
From: Cédric Le Goater @ 2022-11-15 13:27 UTC (permalink / raw)
  To: Pierre Morel, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange

On 11/3/22 18:01, Pierre Morel wrote:
> S390 CPU topology is only allowed for s390-virtio-ccw-7.2 and
> newer S390 machines.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>   include/hw/s390x/s390-virtio-ccw.h | 1 +
>   hw/s390x/s390-virtio-ccw.c         | 2 ++
>   2 files changed, 3 insertions(+)
> 
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 6488279690..89fca3f79f 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -48,6 +48,7 @@ struct S390CcwMachineClass {
>       bool css_migration_enabled;
>       bool hpage_1m_allowed;
>       int max_threads;
> +    bool topology_capable;
>   };
>   
>   /* runtime-instrumentation allowed by the machine */
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 4de2622f99..f1a9d6e793 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -763,6 +763,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>       s390mc->css_migration_enabled = true;
>       s390mc->hpage_1m_allowed = true;
>       s390mc->max_threads = 1;
> +    s390mc->topology_capable = true;
>       mc->init = ccw_init;
>       mc->reset = s390_machine_reset;
>       mc->block_default_type = IF_VIRTIO;
> @@ -896,6 +897,7 @@ static void ccw_machine_7_1_class_options(MachineClass *mc)
>       ccw_machine_7_2_class_options(mc);
>       compat_props_add(mc->compat_props, hw_compat_7_1, hw_compat_7_1_len);
>       s390mc->max_threads = S390_MAX_CPUS;
> +    s390mc->topology_capable = false;
>   }
>   DEFINE_CCW_MACHINE(7_1, "7.1", false);
>   


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 09/11] s390x/cpu topology: add topology machine property
  2022-11-03 17:01 ` [PATCH v11 09/11] s390x/cpu topology: add topology machine property Pierre Morel
  2022-11-03 17:20   ` Cornelia Huck
@ 2022-11-15 13:48   ` Cédric Le Goater
  2022-11-16 12:39     ` Pierre Morel
  1 sibling, 1 reply; 36+ messages in thread
From: Cédric Le Goater @ 2022-11-15 13:48 UTC (permalink / raw)
  To: Pierre Morel, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange

On 11/3/22 18:01, Pierre Morel wrote:
> We keep the possibility to switch on/off the topology on newer
> machines with the property topology=[on|off].

The code has changed. You will need to rebase. May be after the
8.0 machine is introduced, or include Cornelia's patch in the
respin.

https://lore.kernel.org/qemu-devel/20221111124534.129111-1-cohuck@redhat.com/

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   include/hw/boards.h                |  3 +++
>   include/hw/s390x/cpu-topology.h    |  8 +++-----
>   include/hw/s390x/s390-virtio-ccw.h |  1 +
>   hw/core/machine.c                  |  3 +++
>   hw/s390x/cpu-topology.c            | 19 +++++++++++++++++++
>   hw/s390x/s390-virtio-ccw.c         | 28 ++++++++++++++++++++++++++++
>   util/qemu-config.c                 |  4 ++++
>   qemu-options.hx                    |  6 +++++-
>   8 files changed, 66 insertions(+), 6 deletions(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 311ed17e18..67147c47bf 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -379,6 +379,9 @@ struct MachineState {
>       } \
>       type_init(machine_initfn##_register_types)
>   
> +extern GlobalProperty hw_compat_7_2[];
> +extern const size_t hw_compat_7_2_len;
> +
>   extern GlobalProperty hw_compat_7_1[];
>   extern const size_t hw_compat_7_1_len;
>   
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> index 6fec10e032..f566394302 100644
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -12,6 +12,8 @@
>   
>   #include "hw/qdev-core.h"
>   #include "qom/object.h"
> +#include "cpu.h"
> +#include "hw/s390x/s390-virtio-ccw.h"
>   
>   #define S390_TOPOLOGY_CPU_IFL 0x03
>   #define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
> @@ -38,10 +40,6 @@ struct S390Topology {
>   OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY)
>   
>   void s390_topology_new_cpu(S390CPU *cpu);
> -
> -static inline bool s390_has_topology(void)
> -{
> -    return false;
> -}
> +bool s390_has_topology(void);
>   
>   #endif
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 89fca3f79f..d7602aedda 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -28,6 +28,7 @@ struct S390CcwMachineState {
>       bool dea_key_wrap;
>       bool pv;
>       bool zpcii_disable;
> +    bool cpu_topology;
>       uint8_t loadparm[8];
>       void *topology;
>   };
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index aa520e74a8..4f46d4ef23 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -40,6 +40,9 @@
>   #include "hw/virtio/virtio-pci.h"
>   #include "qom/object_interfaces.h"
>   
> +GlobalProperty hw_compat_7_2[] = {};
> +const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
> +
>   GlobalProperty hw_compat_7_1[] = {};
>   const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
>   
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> index fc220bd8ac..c1550cc1e8 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -73,6 +73,25 @@ void s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra)
>       }
>   }
>   
> +bool s390_has_topology(void)
> +{
> +    static S390CcwMachineState *ccw;
> +    Object *obj;
> +
> +    if (ccw) {
> +        return ccw->cpu_topology;

Shouldn't we test the capability also ?

	return s390mc->topology_capable && ccw->cpu_topology;

> +    }
> +
> +    /* we have to bail out for the "none" machine */
> +    obj = object_dynamic_cast(qdev_get_machine(),
> +                              TYPE_S390_CCW_MACHINE);
> +    if (!obj) {
> +        return false;
> +    }

Should be an assert I think.

> +    ccw = S390_CCW_MACHINE(obj);
> +    return ccw->cpu_topology;
> +}
> +
>   /*
>    * s390_topology_new_cpu:
>    * @cpu: a pointer to the new CPU
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index f1a9d6e793..ebb5615337 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -710,6 +710,26 @@ bool hpage_1m_allowed(void)
>       return get_machine_class()->hpage_1m_allowed;
>   }
>   
> +static inline bool machine_get_topology(Object *obj, Error **errp)
> +{
> +    S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> +
> +    return ms->cpu_topology;
> +}
> +
> +static inline void machine_set_topology(Object *obj, bool value, Error **errp)
> +{
> +    S390CcwMachineState *ms = S390_CCW_MACHINE(obj);

You could introduce :

        S390CcwMachineClass *s390mc = S390_CCW_MACHINE_GET_CLASS(ms);


> +
> +    if (!get_machine_class()->topology_capable) {

and
             !s390mc->topology_capable

> +        error_setg(errp, "Property cpu-topology not available on machine %s",
> +                   get_machine_class()->parent_class.name);
> +        return;
> +    }
> +
> +    ms->cpu_topology = value;
> +}
> +
>   static void machine_get_loadparm(Object *obj, Visitor *v,
>                                    const char *name, void *opaque,
>                                    Error **errp)
> @@ -809,6 +829,12 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>                                      machine_set_zpcii_disable);
>       object_class_property_set_description(oc, "zpcii-disable",
>               "disable zPCI interpretation facilties");
> +
> +    object_class_property_add_bool(oc, "topology",
> +                                   machine_get_topology,
> +                                   machine_set_topology);
> +    object_class_property_set_description(oc, "topology",
> +            "enable CPU topology");
>   }
>   
>   static inline void s390_machine_initfn(Object *obj)
> @@ -818,6 +844,7 @@ static inline void s390_machine_initfn(Object *obj)
>       ms->aes_key_wrap = true;
>       ms->dea_key_wrap = true;
>       ms->zpcii_disable = false;
> +    ms->cpu_topology = true;
>   }
>   
>   static const TypeInfo ccw_machine_info = {
> @@ -888,6 +915,7 @@ static void ccw_machine_7_1_instance_options(MachineState *machine)
>       s390_cpudef_featoff_greater(16, 1, S390_FEAT_PAIE);
>       s390_set_qemu_cpu_model(0x8561, 15, 1, qemu_cpu_feat);
>       ms->zpcii_disable = true;
> +    ms->cpu_topology = true;

shouldn't this be false ?


Thanks,

C.

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


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 03/11] s390x/cpu topology: core_id sets s390x CPU topology
  2022-11-15 11:15   ` Cédric Le Goater
@ 2022-11-16 10:17     ` Pierre Morel
  0 siblings, 0 replies; 36+ messages in thread
From: Pierre Morel @ 2022-11-16 10:17 UTC (permalink / raw)
  To: Cédric Le Goater, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange



On 11/15/22 12:15, Cédric Le Goater wrote:
> Hello Pierre,
> 
> On 11/3/22 18:01, Pierre Morel wrote:
>> In the S390x CPU topology the core_id specifies the CPU address
>> and the position of the core withing the topology.
>>
>> Let's build the topology based on the core_id.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   include/hw/s390x/cpu-topology.h    |  41 ++++++++++
>>   include/hw/s390x/s390-virtio-ccw.h |   1 +
>>   target/s390x/cpu.h                 |   2 +
>>   hw/s390x/cpu-topology.c            | 125 +++++++++++++++++++++++++++++
>>   hw/s390x/s390-virtio-ccw.c         |  23 ++++++
>>   hw/s390x/meson.build               |   1 +
>>   6 files changed, 193 insertions(+)
>>   create mode 100644 include/hw/s390x/cpu-topology.h
>>   create mode 100644 hw/s390x/cpu-topology.c
>>
>> diff --git a/include/hw/s390x/cpu-topology.h 
>> b/include/hw/s390x/cpu-topology.h
>> new file mode 100644
>> index 0000000000..4e16a2153d
>> --- /dev/null
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -0,0 +1,41 @@
>> +/*
>> + * CPU Topology
>> + *
>> + * Copyright IBM Corp. 2022
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
>> (at
>> + * your option) any later version. See the COPYING file in the top-level
>> + * directory.
>> + */
>> +#ifndef HW_S390X_CPU_TOPOLOGY_H
>> +#define HW_S390X_CPU_TOPOLOGY_H
>> +
>> +#include "hw/qdev-core.h"
>> +#include "qom/object.h"
>> +
>> +#define S390_TOPOLOGY_CPU_IFL 0x03
>> +#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
>> +
>> +typedef struct S390TopoSocket {
>> +    int active_count;
>> +    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
>> +} S390TopoSocket;
>> +
>> +struct S390Topology {
>> +    SysBusDevice parent_obj;
>> +    uint32_t nr_cpus;
>> +    uint32_t nr_sockets;
>> +    S390TopoSocket *socket;
>> +};
>> +
>> +#define TYPE_S390_CPU_TOPOLOGY "s390-topology"
>> +OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY)
>> +
>> +void s390_topology_new_cpu(S390CPU *cpu);
>> +
>> +static inline bool s390_has_topology(void)
>> +{
>> +    return false;
>> +}
>> +
>> +#endif
>> diff --git a/include/hw/s390x/s390-virtio-ccw.h 
>> b/include/hw/s390x/s390-virtio-ccw.h
>> index 4f8a39abda..23b472708d 100644
>> --- a/include/hw/s390x/s390-virtio-ccw.h
>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>> @@ -29,6 +29,7 @@ struct S390CcwMachineState {
>>       bool pv;
>>       bool zpcii_disable;
>>       uint8_t loadparm[8];
>> +    void *topology;
> 
> I think it is safe to use 'DeviceState *' or 'Object *' for the pointer
> under machine. What is most practical.

OK, DeviceState bring one less cast..

> 
>>   };
>>   struct S390CcwMachineClass {
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 7d6d01325b..c9066b2496 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -175,6 +175,8 @@ struct ArchCPU {
>>       /* needed for live migration */
>>       void *irqstate;
>>       uint32_t irqstate_saved_size;
>> +    /* Topology this CPU belongs too */
>> +    void *topology;
> 
> However, under the CPU, we don't know what the future changes reserve
> for us and I would call the attribute 'opaque' or 'machine_data'.
> 
> For now, it only holds a reference to the S390Topology device model
> but it could become a struct with time.

OK, I use machine_data

> 
> 
>>   };
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> new file mode 100644
>> index 0000000000..6af41d3d7b
>> --- /dev/null
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -0,0 +1,125 @@
>> +/*
>> + * CPU Topology
>> + *
>> + * Copyright IBM Corp. 2022
>> + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
>> +
>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
>> (at
>> + * your option) any later version. See the COPYING file in the top-level
>> + * directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qemu/error-report.h"
>> +#include "hw/sysbus.h"
>> +#include "hw/qdev-properties.h"
>> +#include "hw/boards.h"
>> +#include "qemu/typedefs.h"
>> +#include "target/s390x/cpu.h"
>> +#include "hw/s390x/s390-virtio-ccw.h"
>> +#include "hw/s390x/cpu-topology.h"
>> +
>> +/*
>> + * s390_topology_new_cpu:
>> + * @cpu: a pointer to the new CPU
>> + *
>> + * The topology pointed by S390CPU, gives us the CPU topology
>> + * established by the -smp QEMU aruments.
>> + * The core-id is used to calculate the position of the CPU inside
>> + * the topology:
>> + *  - the socket, container TLE, containing the CPU, we have one socket
>> + *    for every nr_cpus (nr_cpus = smp.cores * smp.threads)
>> + *  - the CPU TLE inside the socket, we have potentionly up to 4 CPU TLE
>> + *    in a container TLE with the assumption that all CPU are identical
>> + *    with the same polarity and entitlement because we have maximum 256
>> + *    CPUs and each TLE can hold up to 64 identical CPUs.
>> + *  - the bit in the 64 bit CPU TLE core mask
>> + */
>> +void s390_topology_new_cpu(S390CPU *cpu)
>> +{
>> +    S390Topology *topo = (S390Topology *)cpu->topology;
> 
> where is cpu->topology set ?

In the caller but this is reworked anyway due to a rearangement of the 
function.

> 
>> +    int core_id = cpu->env.core_id;
>> +    int bit, origin;
>> +    int socket_id;
>> +
>> +    socket_id = core_id / topo->nr_cpus;
>> +
>> +    /*
>> +     * At the core level, each CPU is represented by a bit in a 64bit
>> +     * uint64_t which represent the presence of a CPU.
>> +     * The firmware assume that all CPU in a CPU TLE have the same
>> +     * type, polarization and are all dedicated or shared.
>> +     * In that case the origin variable represents the offset of the 
>> first
>> +     * CPU in the CPU container.
>> +     * More than 64 CPUs per socket are represented in several CPU 
>> containers
>> +     * inside the socket container.
>> +     * The only reason to have several S390TopologyCores inside a 
>> socket is
>> +     * to have more than 64 CPUs.
>> +     * In that case the origin variable represents the offset of the 
>> first CPU
>> +     * in the CPU container. More than 64 CPUs per socket are 
>> represented in
>> +     * several CPU containers inside the socket container.
>> +     */
>> +    bit = core_id;
>> +    origin = bit / 64;
>> +    bit %= 64;
>> +    bit = 63 - bit;
>> +
>> +    topo->socket[socket_id].active_count++;
>> +    set_bit(bit, &topo->socket[socket_id].mask[origin]);
>> +}
>> +
>> +/**
>> + * s390_topology_realize:
>> + * @dev: the device state
>> + * @errp: the error pointer (not used)
>> + *
>> + * During realize the machine CPU topology is initialized with the
>> + * QEMU -smp parameters.
>> + * The maximum count of CPU TLE in the all Topology can not be greater
>> + * than the maximum CPUs.
>> + */
>> +static void s390_topology_realize(DeviceState *dev, Error **errp)
>> +{
>> +    MachineState *ms = MACHINE(qdev_get_machine());
> 
> hmm,

will disappear

> 
>> +    S390Topology *topo = S390_CPU_TOPOLOGY(dev);
>> +
>> +    topo->nr_cpus = ms->smp.cores * ms->smp.threads;
>> +    topo->nr_sockets = ms->smp.sockets;
> 
> These properties should be set in s390_init_topology() with :
> 
>    object_property_set_int(OBJECT(dev), "num-cpus",
>                            ms->smp.cores * ms->smp.threads, errp);
> 
>    object_property_set_int(OBJECT(dev), "num-sockets",
>                            ms->smp.sockets, errp);
> 
> before calling sysbus_realize_and_unref()

OK, done

> 
> 
>> +    topo->socket = g_new0(S390TopoSocket, topo->nr_sockets);
> 
> For consistency, you could add an unrealize handler to free the array.

yes

> 
>> +}
>> +
>> +static Property s390_topology_properties[] = {
>> +    DEFINE_PROP_UINT32("nr_cpus", S390Topology, nr_cpus, 1),
>> +    DEFINE_PROP_UINT32("nr_sockets", S390Topology, nr_sockets, 1),
> 
> A quick audit of the property names in QEMU code shows that a "num-" prefix
> is usually preferred.

OK

> 
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +/**
>> + * topology_class_init:
>> + * @oc: Object class
>> + * @data: (not used)
>> + *
>> + * A very simple object we will need for reset and migration.
>> + */
>> +static void topology_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +
>> +    dc->realize = s390_topology_realize;
>> +    device_class_set_props(dc, s390_topology_properties);
>> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>> +}
>> +
>> +static const TypeInfo cpu_topology_info = {
>> +    .name          = TYPE_S390_CPU_TOPOLOGY,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(S390Topology),
>> +    .class_init    = topology_class_init,
>> +};
>> +
>> +static void topology_register(void)
>> +{
>> +    type_register_static(&cpu_topology_info);
>> +}
>> +type_init(topology_register);
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 9ab91df5b1..5776d3e58f 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -44,6 +44,7 @@
>>   #include "hw/s390x/pv.h"
>>   #include "migration/blocker.h"
>>   #include "qapi/visitor.h"
>> +#include "hw/s390x/cpu-topology.h"
>>   static Error *pv_mig_blocker;
>> @@ -102,6 +103,19 @@ static void s390_init_cpus(MachineState *machine)
>>       }
>>   }
>> +static void s390_init_topology(MachineState *machine)
>> +{
>> +    DeviceState *dev;
>> +
>> +    if (s390_has_topology()) {
> 
> I would move the s390_has_topology() check in the caller.

yes

> 
>> +        dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
>> +        object_property_add_child(&machine->parent_obj,
>> +                                  TYPE_S390_CPU_TOPOLOGY, OBJECT(dev));
>> +        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> +        S390_CCW_MACHINE(machine)->topology = dev;
> 
> and I would move the assignment in the caller also.
> 
>> +    }
>> +}
>> +
>>   static const char *const reset_dev_types[] = {
>>       TYPE_VIRTUAL_CSS_BRIDGE,
>>       "s390-sclp-event-facility",
>> @@ -252,6 +266,9 @@ static void ccw_init(MachineState *machine)
>>       /* init memory + setup max page size. Required for the CPU model */
>>       s390_memory_init(machine->ram);
>> +    /* Adding the topology must be done before CPU initialization */
>> +    s390_init_topology(machine);
>> +
>>       /* init CPUs (incl. CPU model) early so s390_has_feature() works */
>>       s390_init_cpus(machine);
>> @@ -314,6 +331,12 @@ static void s390_cpu_plug(HotplugHandler 
>> *hotplug_dev,
>>       g_assert(!ms->possible_cpus->cpus[cpu->env.core_id].cpu);
>>       ms->possible_cpus->cpus[cpu->env.core_id].cpu = OBJECT(dev);
>> +    /* Inserting the CPU in the Topology can not fail */
>> +    if (S390_CCW_MACHINE(ms)->topology) {
>> +        cpu->topology = S390_CCW_MACHINE(ms)->topology;
> 
> Two QOM cast. One should be enough. Please introduce a local variable.
> 
>> +        s390_topology_new_cpu(cpu);
> 
> I would pass the 'topology' object as a parameter of 
> s390_topology_new_cpu()
> and do the cpu->topology assignment in the same routine.
> 
> May be rename it also to :
> 
>    void s390_topology_add_cpu(S390Topology *topo, S390CPU *cpu)

OK, better.

> 
> 
> Thanks,
> 
> C.


Thanks a lot for your reviewing.

Regards,
Pierre



-- 
Pierre Morel
IBM Lab Boeblingen

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 04/11] s390x/cpu topology: reporting the CPU topology to the guest
  2022-11-15 11:21   ` Cédric Le Goater
@ 2022-11-16 10:27     ` Pierre Morel
  0 siblings, 0 replies; 36+ messages in thread
From: Pierre Morel @ 2022-11-16 10:27 UTC (permalink / raw)
  To: Cédric Le Goater, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange



On 11/15/22 12:21, Cédric Le Goater wrote:
> Hello Pierre,
> 
> On 11/3/22 18:01, Pierre Morel wrote:
>> The guest can use the STSI instruction to get a buffer filled
>> with the CPU topology description.
>>
>> Let us implement the STSI instruction for the basis CPU topology
>> level, level 2.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   include/hw/s390x/cpu-topology.h |   6 ++
>>   target/s390x/cpu.h              |  77 ++++++++++++++++++++++++
>>   hw/s390x/cpu-topology.c         |   1 -
>>   target/s390x/cpu_topology.c     | 100 ++++++++++++++++++++++++++++++++
>>   target/s390x/kvm/kvm.c          |   6 +-
>>   target/s390x/meson.build        |   1 +
>>   6 files changed, 189 insertions(+), 2 deletions(-)
>>   create mode 100644 target/s390x/cpu_topology.c
>>
>> diff --git a/include/hw/s390x/cpu-topology.h 
>> b/include/hw/s390x/cpu-topology.h
>> index 4e16a2153d..6fec10e032 100644
>> --- a/include/hw/s390x/cpu-topology.h
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -16,6 +16,11 @@
>>   #define S390_TOPOLOGY_CPU_IFL 0x03
>>   #define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
>> +#define S390_TOPOLOGY_POLARITY_HORIZONTAL      0x00
>> +#define S390_TOPOLOGY_POLARITY_VERTICAL_LOW    0x01
>> +#define S390_TOPOLOGY_POLARITY_VERTICAL_MEDIUM 0x02
>> +#define S390_TOPOLOGY_POLARITY_VERTICAL_HIGH   0x03
>> +
>>   typedef struct S390TopoSocket {
>>       int active_count;
>>       uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
>> @@ -26,6 +31,7 @@ struct S390Topology {
>>       uint32_t nr_cpus;
>>       uint32_t nr_sockets;
>>       S390TopoSocket *socket;
>> +    bool topology_needed;
> 
> 
> This is unused in this patch. Introduced too soon ?

right, thanks.

> 
> 
>>   };
>>   #define TYPE_S390_CPU_TOPOLOGY "s390-topology"
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index c9066b2496..69a7523146 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -567,6 +567,81 @@ typedef union SysIB {
>>   } SysIB;
>>   QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
>> +/*
>> + * CPU Topology List provided by STSI with fc=15 provides a list
>> + * of two different Topology List Entries (TLE) types to specify
>> + * the topology hierarchy.
>> + *
>> + * - Container Topology List Entry
>> + *   Defines a container to contain other Topology List Entries
>> + *   of any type, nested containers or CPU.
>> + * - CPU Topology List Entry
>> + *   Specifies the CPUs position, type, entitlement and polarization
>> + *   of the CPUs contained in the last Container TLE.
>> + *
>> + * There can be theoretically up to five levels of containers, QEMU
>> + * uses only one level, the socket level.
>> + *
>> + * A container of with a nesting level (NL) greater than 1 can only
>> + * contain another container of nesting level NL-1.
>> + *
>> + * A container of nesting level 1 (socket), contains as many CPU TLE
>> + * as needed to describe the position and qualities of all CPUs inside
>> + * the container.
>> + * The qualities of a CPU are polarization, entitlement and type.
>> + *
>> + * The CPU TLE defines the position of the CPUs of identical qualities
>> + * using a 64bits mask which first bit has its offset defined by
>> + * the CPU address orgin field of the CPU TLE like in:
>> + * CPU address = origin * 64 + bit position within the mask
>> + *
>> + */
>> +/* Container type Topology List Entry */
>> +typedef struct SysIBTl_container {
>> +        uint8_t nl;
>> +        uint8_t reserved[6];
>> +        uint8_t id;
>> +} QEMU_PACKED QEMU_ALIGNED(8) SysIBTl_container;
>> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_container) != 8);
>> +
>> +/* CPU type Topology List Entry */
>> +typedef struct SysIBTl_cpu {
>> +        uint8_t nl;
>> +        uint8_t reserved0[3];
>> +        uint8_t reserved1:5;
>> +        uint8_t dedicated:1;
>> +        uint8_t polarity:2;
>> +        uint8_t type;
>> +        uint16_t origin;
>> +        uint64_t mask;
>> +} QEMU_PACKED QEMU_ALIGNED(8) SysIBTl_cpu;
>> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16);
>> +
>> +#define S390_TOPOLOGY_MAG  6
>> +#define S390_TOPOLOGY_MAG6 0
>> +#define S390_TOPOLOGY_MAG5 1
>> +#define S390_TOPOLOGY_MAG4 2
>> +#define S390_TOPOLOGY_MAG3 3
>> +#define S390_TOPOLOGY_MAG2 4
>> +#define S390_TOPOLOGY_MAG1 5
>> +/* Configuration topology */
>> +typedef struct SysIB_151x {
>> +    uint8_t  reserved0[2];
>> +    uint16_t length;
>> +    uint8_t  mag[S390_TOPOLOGY_MAG];
>> +    uint8_t  reserved1;
>> +    uint8_t  mnest;
>> +    uint32_t reserved2;
>> +    char tle[0];
>> +} QEMU_PACKED QEMU_ALIGNED(8) SysIB_151x;
>> +QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
>> +
>> +/* Max size of a SYSIB structure is when all CPU are alone in a 
>> container */
>> +#define S390_TOPOLOGY_SYSIB_SIZE (sizeof(SysIB_151x) 
>> +                         \
>> +                                  S390_MAX_CPUS * 
>> (sizeof(SysIBTl_container) + \
>> +                                                   sizeof(SysIBTl_cpu)))
>> +
>> +
>>   /* MMU defines */
>>   #define ASCE_ORIGIN           (~0xfffULL) /* segment table 
>> origin             */
>>   #define ASCE_SUBSPACE         0x200       /* subspace group 
>> control           */
>> @@ -845,4 +920,6 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
>>   #include "exec/cpu-all.h"
>> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar);
>> +
>>   #endif
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> index 6af41d3d7b..9fa8ca1261 100644
>> --- a/hw/s390x/cpu-topology.c
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -44,7 +44,6 @@ void s390_topology_new_cpu(S390CPU *cpu)
>>       int socket_id;
>>       socket_id = core_id / topo->nr_cpus;
>> -
> 
> Unnecessary change.

Yes,

thanks,

regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 08/11] s390x/cpu topology: add topology_capable QEMU capability
  2022-11-15 13:27   ` Cédric Le Goater
@ 2022-11-16 11:23     ` Pierre Morel
  0 siblings, 0 replies; 36+ messages in thread
From: Pierre Morel @ 2022-11-16 11:23 UTC (permalink / raw)
  To: Cédric Le Goater, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange



On 11/15/22 14:27, Cédric Le Goater wrote:
> On 11/3/22 18:01, Pierre Morel wrote:
>> S390 CPU topology is only allowed for s390-virtio-ccw-7.2 and
>> newer S390 machines.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C.

Thanks,

Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 09/11] s390x/cpu topology: add topology machine property
  2022-11-15 13:48   ` Cédric Le Goater
@ 2022-11-16 12:39     ` Pierre Morel
       [not found]       ` <PH0PR22MB3210864C22AD57E5B32F626991079@PH0PR22MB3210.namprd22.prod.outlook.com>
  0 siblings, 1 reply; 36+ messages in thread
From: Pierre Morel @ 2022-11-16 12:39 UTC (permalink / raw)
  To: Cédric Le Goater, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange



On 11/15/22 14:48, Cédric Le Goater wrote:
> On 11/3/22 18:01, Pierre Morel wrote:
>> We keep the possibility to switch on/off the topology on newer
>> machines with the property topology=[on|off].
> 
> The code has changed. You will need to rebase. May be after the
> 8.0 machine is introduced, or include Cornelia's patch in the
> respin.
> 
> https://lore.kernel.org/qemu-devel/20221111124534.129111-1-cohuck@redhat.com/
> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   include/hw/boards.h                |  3 +++
>>   include/hw/s390x/cpu-topology.h    |  8 +++-----
>>   include/hw/s390x/s390-virtio-ccw.h |  1 +
>>   hw/core/machine.c                  |  3 +++
>>   hw/s390x/cpu-topology.c            | 19 +++++++++++++++++++
>>   hw/s390x/s390-virtio-ccw.c         | 28 ++++++++++++++++++++++++++++
>>   util/qemu-config.c                 |  4 ++++
>>   qemu-options.hx                    |  6 +++++-
>>   8 files changed, 66 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 311ed17e18..67147c47bf 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -379,6 +379,9 @@ struct MachineState {
>>       } \
>>       type_init(machine_initfn##_register_types)
>> +extern GlobalProperty hw_compat_7_2[];
>> +extern const size_t hw_compat_7_2_len;
>> +
>>   extern GlobalProperty hw_compat_7_1[];
>>   extern const size_t hw_compat_7_1_len;
>> diff --git a/include/hw/s390x/cpu-topology.h 
>> b/include/hw/s390x/cpu-topology.h
>> index 6fec10e032..f566394302 100644
>> --- a/include/hw/s390x/cpu-topology.h
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -12,6 +12,8 @@
>>   #include "hw/qdev-core.h"
>>   #include "qom/object.h"
>> +#include "cpu.h"
>> +#include "hw/s390x/s390-virtio-ccw.h"
>>   #define S390_TOPOLOGY_CPU_IFL 0x03
>>   #define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
>> @@ -38,10 +40,6 @@ struct S390Topology {
>>   OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY)
>>   void s390_topology_new_cpu(S390CPU *cpu);
>> -
>> -static inline bool s390_has_topology(void)
>> -{
>> -    return false;
>> -}
>> +bool s390_has_topology(void);
>>   #endif
>> diff --git a/include/hw/s390x/s390-virtio-ccw.h 
>> b/include/hw/s390x/s390-virtio-ccw.h
>> index 89fca3f79f..d7602aedda 100644
>> --- a/include/hw/s390x/s390-virtio-ccw.h
>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>> @@ -28,6 +28,7 @@ struct S390CcwMachineState {
>>       bool dea_key_wrap;
>>       bool pv;
>>       bool zpcii_disable;
>> +    bool cpu_topology;
>>       uint8_t loadparm[8];
>>       void *topology;
>>   };
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index aa520e74a8..4f46d4ef23 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -40,6 +40,9 @@
>>   #include "hw/virtio/virtio-pci.h"
>>   #include "qom/object_interfaces.h"
>> +GlobalProperty hw_compat_7_2[] = {};
>> +const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
>> +
>>   GlobalProperty hw_compat_7_1[] = {};
>>   const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> index fc220bd8ac..c1550cc1e8 100644
>> --- a/hw/s390x/cpu-topology.c
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -73,6 +73,25 @@ void s390_handle_ptf(S390CPU *cpu, uint8_t r1, 
>> uintptr_t ra)
>>       }
>>   }
>> +bool s390_has_topology(void)
>> +{
>> +    static S390CcwMachineState *ccw;
>> +    Object *obj;
>> +
>> +    if (ccw) {
>> +        return ccw->cpu_topology;
> 
> Shouldn't we test the capability also ?
> 
>      return s390mc->topology_capable && ccw->cpu_topology;

yes thanks

> 
>> +    }
>> +
>> +    /* we have to bail out for the "none" machine */
>> +    obj = object_dynamic_cast(qdev_get_machine(),
>> +                              TYPE_S390_CCW_MACHINE);
>> +    if (!obj) {
>> +        return false;
>> +    }
> 
> Should be an assert I think.

OK

> 
>> +    ccw = S390_CCW_MACHINE(obj);
>> +    return ccw->cpu_topology;
>> +}
>> +
>>   /*
>>    * s390_topology_new_cpu:
>>    * @cpu: a pointer to the new CPU
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index f1a9d6e793..ebb5615337 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -710,6 +710,26 @@ bool hpage_1m_allowed(void)
>>       return get_machine_class()->hpage_1m_allowed;
>>   }
>> +static inline bool machine_get_topology(Object *obj, Error **errp)
>> +{
>> +    S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
>> +
>> +    return ms->cpu_topology;
>> +}
>> +
>> +static inline void machine_set_topology(Object *obj, bool value, 
>> Error **errp)
>> +{
>> +    S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> 
> You could introduce :
> 
>         S390CcwMachineClass *s390mc = S390_CCW_MACHINE_GET_CLASS(ms);

Yes thanks

> 
> 
>> +
>> +    if (!get_machine_class()->topology_capable) {
> 
> and
>              !s390mc->topology_capable
> 
>> +        error_setg(errp, "Property cpu-topology not available on 
>> machine %s",
>> +                   get_machine_class()->parent_class.name);
>> +        return;
>> +    }
>> +
>> +    ms->cpu_topology = value;
>> +}
>> +
>>   static void machine_get_loadparm(Object *obj, Visitor *v,
>>                                    const char *name, void *opaque,
>>                                    Error **errp)
>> @@ -809,6 +829,12 @@ static void ccw_machine_class_init(ObjectClass 
>> *oc, void *data)
>>                                      machine_set_zpcii_disable);
>>       object_class_property_set_description(oc, "zpcii-disable",
>>               "disable zPCI interpretation facilties");
>> +
>> +    object_class_property_add_bool(oc, "topology",
>> +                                   machine_get_topology,
>> +                                   machine_set_topology);
>> +    object_class_property_set_description(oc, "topology",
>> +            "enable CPU topology");
>>   }
>>   static inline void s390_machine_initfn(Object *obj)
>> @@ -818,6 +844,7 @@ static inline void s390_machine_initfn(Object *obj)
>>       ms->aes_key_wrap = true;
>>       ms->dea_key_wrap = true;
>>       ms->zpcii_disable = false;
>> +    ms->cpu_topology = true;
>>   }
>>   static const TypeInfo ccw_machine_info = {
>> @@ -888,6 +915,7 @@ static void 
>> ccw_machine_7_1_instance_options(MachineState *machine)
>>       s390_cpudef_featoff_greater(16, 1, S390_FEAT_PAIE);
>>       s390_set_qemu_cpu_model(0x8561, 15, 1, qemu_cpu_feat);
>>       ms->zpcii_disable = true;
>> +    ms->cpu_topology = true;
> 
> shouldn't this be false ?

:) yes
I forgot to change this when I change the logic.

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Thank you! s390x/cpu topology
       [not found]       ` <PH0PR22MB3210864C22AD57E5B32F626991079@PH0PR22MB3210.namprd22.prod.outlook.com>
@ 2022-11-16 13:17         ` Jadon
  0 siblings, 0 replies; 36+ messages in thread
From: Jadon @ 2022-11-16 13:17 UTC (permalink / raw)
  To: Pierre Morel, Cédric Le Goater, qemu-s390x, qemu-devel,
	borntraeger, pasic, richard.henderson, david, thuth, cohuck, mst,
	pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake, armbru,
	seiden, nrb, scgl, frankja, berrange


[-- Attachment #1.1: Type: text/plain, Size: 1375 bytes --]

Friends,

I am but a small bystander as I watch in awe at the incredible work each of you do to advance the Kernel and Linux.
What you are working on is so important. Your ideas, and how you express them into the code you write, become the foundation for a better world.
I can tell you that your support for s390x architecture is quite meaningful to my company as we work to improve access, cost, and positive outcomes in healthcare.
We use many s390x Linux servers and VMs for critical systems.

Thank you for everything you do. Many of us at many companies deeply appreciate you.

With sincere respect,

-Jadon

Jadon McDowell | [optum%20logo%20gif%20200px]
Director & Senior Principal Engineer, TLCP
Enterprise Hosting Services: HPC Infrastructure Delivery
Mobile: 612-940-9268 | jadon.mcdowell@optum.com<mailto:jadon.mcdowell@optum.com>


This e-mail, including attachments, may include confidential and/or
proprietary information, and may be used only by the person or entity
to which it is addressed. If the reader of this e-mail is not the intended
recipient or intended recipient’s authorized agent, the reader is hereby
notified that any dissemination, distribution or copying of this e-mail is
prohibited. If you have received this e-mail in error, please notify the
sender by replying to this message and delete this e-mail immediately.

[-- Attachment #1.2: Type: text/html, Size: 5461 bytes --]

[-- Attachment #2: image001.png --]
[-- Type: image/png, Size: 3172 bytes --]

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 04/11] s390x/cpu topology: reporting the CPU topology to the guest
  2022-11-03 17:01 ` [PATCH v11 04/11] s390x/cpu topology: reporting the CPU topology to the guest Pierre Morel
  2022-11-15 11:21   ` Cédric Le Goater
@ 2022-11-17  8:40   ` Cédric Le Goater
  2022-11-17  9:32     ` Pierre Morel
  1 sibling, 1 reply; 36+ messages in thread
From: Cédric Le Goater @ 2022-11-17  8:40 UTC (permalink / raw)
  To: Pierre Morel, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange

On 11/3/22 18:01, Pierre Morel wrote:
> The guest can use the STSI instruction to get a buffer filled
> with the CPU topology description.
> 
> Let us implement the STSI instruction for the basis CPU topology
> level, level 2.
>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   include/hw/s390x/cpu-topology.h |   6 ++
>   target/s390x/cpu.h              |  77 ++++++++++++++++++++++++
>   hw/s390x/cpu-topology.c         |   1 -
>   target/s390x/cpu_topology.c     | 100 ++++++++++++++++++++++++++++++++
>   target/s390x/kvm/kvm.c          |   6 +-
>   target/s390x/meson.build        |   1 +
>   6 files changed, 189 insertions(+), 2 deletions(-)
>   create mode 100644 target/s390x/cpu_topology.c
> 
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> index 4e16a2153d..6fec10e032 100644
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -16,6 +16,11 @@
>   #define S390_TOPOLOGY_CPU_IFL 0x03
>   #define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
>   
> +#define S390_TOPOLOGY_POLARITY_HORIZONTAL      0x00
> +#define S390_TOPOLOGY_POLARITY_VERTICAL_LOW    0x01
> +#define S390_TOPOLOGY_POLARITY_VERTICAL_MEDIUM 0x02
> +#define S390_TOPOLOGY_POLARITY_VERTICAL_HIGH   0x03
> +
>   typedef struct S390TopoSocket {
>       int active_count;
>       uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
> @@ -26,6 +31,7 @@ struct S390Topology {
>       uint32_t nr_cpus;
>       uint32_t nr_sockets;
>       S390TopoSocket *socket;
> +    bool topology_needed;
>   };
>   
>   #define TYPE_S390_CPU_TOPOLOGY "s390-topology"
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index c9066b2496..69a7523146 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -567,6 +567,81 @@ typedef union SysIB {
>   } SysIB;
>   QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
>   
> +/*
> + * CPU Topology List provided by STSI with fc=15 provides a list
> + * of two different Topology List Entries (TLE) types to specify
> + * the topology hierarchy.
> + *
> + * - Container Topology List Entry
> + *   Defines a container to contain other Topology List Entries
> + *   of any type, nested containers or CPU.
> + * - CPU Topology List Entry
> + *   Specifies the CPUs position, type, entitlement and polarization
> + *   of the CPUs contained in the last Container TLE.
> + *
> + * There can be theoretically up to five levels of containers, QEMU
> + * uses only one level, the socket level.
> + *
> + * A container of with a nesting level (NL) greater than 1 can only
> + * contain another container of nesting level NL-1.
> + *
> + * A container of nesting level 1 (socket), contains as many CPU TLE
> + * as needed to describe the position and qualities of all CPUs inside
> + * the container.
> + * The qualities of a CPU are polarization, entitlement and type.
> + *
> + * The CPU TLE defines the position of the CPUs of identical qualities
> + * using a 64bits mask which first bit has its offset defined by
> + * the CPU address orgin field of the CPU TLE like in:
> + * CPU address = origin * 64 + bit position within the mask
> + *
> + */
> +/* Container type Topology List Entry */
> +typedef struct SysIBTl_container {
> +        uint8_t nl;
> +        uint8_t reserved[6];
> +        uint8_t id;
> +} QEMU_PACKED QEMU_ALIGNED(8) SysIBTl_container;
> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_container) != 8);
> +
> +/* CPU type Topology List Entry */
> +typedef struct SysIBTl_cpu {
> +        uint8_t nl;
> +        uint8_t reserved0[3];
> +        uint8_t reserved1:5;
> +        uint8_t dedicated:1;
> +        uint8_t polarity:2;
> +        uint8_t type;
> +        uint16_t origin;
> +        uint64_t mask;
> +} QEMU_PACKED QEMU_ALIGNED(8) SysIBTl_cpu;
> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16);
> +
> +#define S390_TOPOLOGY_MAG  6
> +#define S390_TOPOLOGY_MAG6 0
> +#define S390_TOPOLOGY_MAG5 1
> +#define S390_TOPOLOGY_MAG4 2
> +#define S390_TOPOLOGY_MAG3 3
> +#define S390_TOPOLOGY_MAG2 4
> +#define S390_TOPOLOGY_MAG1 5
> +/* Configuration topology */
> +typedef struct SysIB_151x {
> +    uint8_t  reserved0[2];
> +    uint16_t length;
> +    uint8_t  mag[S390_TOPOLOGY_MAG];
> +    uint8_t  reserved1;
> +    uint8_t  mnest;
> +    uint32_t reserved2;
> +    char tle[0];
> +} QEMU_PACKED QEMU_ALIGNED(8) SysIB_151x;
> +QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
> +
> +/* Max size of a SYSIB structure is when all CPU are alone in a container */
> +#define S390_TOPOLOGY_SYSIB_SIZE (sizeof(SysIB_151x) +                         \
> +                                  S390_MAX_CPUS * (sizeof(SysIBTl_container) + \
> +                                                   sizeof(SysIBTl_cpu)))
> +
> +
>   /* MMU defines */
>   #define ASCE_ORIGIN           (~0xfffULL) /* segment table origin             */
>   #define ASCE_SUBSPACE         0x200       /* subspace group control           */
> @@ -845,4 +920,6 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
>   
>   #include "exec/cpu-all.h"
>   
> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar);
> +
>   #endif
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> index 6af41d3d7b..9fa8ca1261 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -44,7 +44,6 @@ void s390_topology_new_cpu(S390CPU *cpu)
>       int socket_id;
>   
>       socket_id = core_id / topo->nr_cpus;
> -
>       /*
>        * At the core level, each CPU is represented by a bit in a 64bit
>        * uint64_t which represent the presence of a CPU.
> diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
> new file mode 100644
> index 0000000000..a1179d8e95
> --- /dev/null
> +++ b/target/s390x/cpu_topology.c
> @@ -0,0 +1,100 @@
> +/*
> + * QEMU S390x CPU Topology
> + *
> + * Copyright IBM Corp. 2022
> + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#include "hw/s390x/pv.h"
> +#include "hw/sysbus.h"
> +#include "hw/s390x/cpu-topology.h"
> +#include "hw/s390x/sclp.h"
> +
> +static char *fill_container(char *p, int level, int id)
> +{
> +    SysIBTl_container *tle = (SysIBTl_container *)p;
> +
> +    tle->nl = level;
> +    tle->id = id;
> +    return p + sizeof(*tle);
> +}
> +
> +static char *fill_tle_cpu(char *p, uint64_t mask, int origin)
> +{
> +    SysIBTl_cpu *tle = (SysIBTl_cpu *)p;
> +
> +    tle->nl = 0;
> +    tle->dedicated = 1;
> +    tle->polarity = S390_TOPOLOGY_POLARITY_HORIZONTAL;
> +    tle->type = S390_TOPOLOGY_CPU_IFL;
> +    tle->origin = cpu_to_be64(origin * 64);
> +    tle->mask = cpu_to_be64(mask);
> +    return p + sizeof(*tle);
> +}
> +
> +static char *s390_top_set_level2(S390Topology *topo, char *p)
> +{
> +    int i, origin;
> +
> +    for (i = 0; i < topo->nr_sockets; i++) {
> +        if (!topo->socket[i].active_count) {
> +            continue;
> +        }
> +        p = fill_container(p, 1, i);
> +        for (origin = 0; origin < S390_TOPOLOGY_MAX_ORIGIN; origin++) {
> +            uint64_t mask = 0L;
> +
> +            mask = topo->socket[i].mask[origin];
> +            if (mask) {
> +                p = fill_tle_cpu(p, mask, origin);
> +            }
> +        }
> +    }
> +    return p;
> +}

Why is it not possible to compute this topo information at "runtime",
when stsi is called, without maintaining state in an extra S390Topology
object ? Couldn't we loop on the CPU list to gather the topology bits
for the same result ?

It would greatly simplify the feature.

C.

> +static int setup_stsi(S390CPU *cpu, SysIB_151x *sysib, int level)
> +{
> +    S390Topology *topo = (S390Topology *)cpu->topology;
> +    char *p = sysib->tle;
> +
> +    sysib->mnest = level;
> +    switch (level) {
> +    case 2:
> +        sysib->mag[S390_TOPOLOGY_MAG2] = topo->nr_sockets;
> +        sysib->mag[S390_TOPOLOGY_MAG1] = topo->nr_cpus;
> +        p = s390_top_set_level2(topo, p);
> +        break;
> +    }
> +
> +    return p - (char *)sysib;
> +}
> +
> +#define S390_TOPOLOGY_MAX_MNEST 2
> +
> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
> +{
> +    union {
> +        char place_holder[S390_TOPOLOGY_SYSIB_SIZE];
> +        SysIB_151x sysib;
> +    } buffer QEMU_ALIGNED(8);
> +    int len;
> +
> +    if (s390_is_pv() || !s390_has_topology() ||
> +        sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
> +        setcc(cpu, 3);
> +        return;
> +    }
> +
> +    len = setup_stsi(cpu, &buffer.sysib, sel2);
> +
> +    buffer.sysib.length = cpu_to_be16(len);
> +    s390_cpu_virt_mem_write(cpu, addr, ar, &buffer.sysib, len);
> +    setcc(cpu, 0);
> +}
> +
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 3ac7ec9acf..7dc96f3663 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -51,6 +51,7 @@
>   #include "hw/s390x/s390-virtio-ccw.h"
>   #include "hw/s390x/s390-virtio-hcall.h"
>   #include "hw/s390x/pv.h"
> +#include "hw/s390x/cpu-topology.h"
>   
>   #ifndef DEBUG_KVM
>   #define DEBUG_KVM  0
> @@ -1919,9 +1920,12 @@ static int handle_stsi(S390CPU *cpu)
>           if (run->s390_stsi.sel1 != 2 || run->s390_stsi.sel2 != 2) {
>               return 0;
>           }
> -        /* Only sysib 3.2.2 needs post-handling for now. */
>           insert_stsi_3_2_2(cpu, run->s390_stsi.addr, run->s390_stsi.ar);
>           return 0;
> +    case 15:
> +        insert_stsi_15_1_x(cpu, run->s390_stsi.sel2, run->s390_stsi.addr,
> +                           run->s390_stsi.ar);
> +        return 0;
>       default:
>           return 0;
>       }
> diff --git a/target/s390x/meson.build b/target/s390x/meson.build
> index 84c1402a6a..890ccfa789 100644
> --- a/target/s390x/meson.build
> +++ b/target/s390x/meson.build
> @@ -29,6 +29,7 @@ s390x_softmmu_ss.add(files(
>     'sigp.c',
>     'cpu-sysemu.c',
>     'cpu_models_sysemu.c',
> +  'cpu_topology.c',
>   ))
>   
>   s390x_user_ss = ss.source_set()


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 04/11] s390x/cpu topology: reporting the CPU topology to the guest
  2022-11-17  8:40   ` Cédric Le Goater
@ 2022-11-17  9:32     ` Pierre Morel
  2022-11-21 14:13       ` Cédric Le Goater
  0 siblings, 1 reply; 36+ messages in thread
From: Pierre Morel @ 2022-11-17  9:32 UTC (permalink / raw)
  To: Cédric Le Goater, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange



On 11/17/22 09:40, Cédric Le Goater wrote:
> On 11/3/22 18:01, Pierre Morel wrote:
>> The guest can use the STSI instruction to get a buffer filled
>> with the CPU topology description.
>>
>> Let us implement the STSI instruction for the basis CPU topology
>> level, level 2.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   include/hw/s390x/cpu-topology.h |   6 ++
>>   target/s390x/cpu.h              |  77 ++++++++++++++++++++++++
>>   hw/s390x/cpu-topology.c         |   1 -
>>   target/s390x/cpu_topology.c     | 100 ++++++++++++++++++++++++++++++++
>>   target/s390x/kvm/kvm.c          |   6 +-
>>   target/s390x/meson.build        |   1 +
>>   6 files changed, 189 insertions(+), 2 deletions(-)
>>   create mode 100644 target/s390x/cpu_topology.c
>>
>> diff --git a/include/hw/s390x/cpu-topology.h 
>> b/include/hw/s390x/cpu-topology.h
>> index 4e16a2153d..6fec10e032 100644
>> --- a/include/hw/s390x/cpu-topology.h
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -16,6 +16,11 @@
>>   #define S390_TOPOLOGY_CPU_IFL 0x03
>>   #define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
>> +#define S390_TOPOLOGY_POLARITY_HORIZONTAL      0x00
>> +#define S390_TOPOLOGY_POLARITY_VERTICAL_LOW    0x01
>> +#define S390_TOPOLOGY_POLARITY_VERTICAL_MEDIUM 0x02
>> +#define S390_TOPOLOGY_POLARITY_VERTICAL_HIGH   0x03
>> +
>>   typedef struct S390TopoSocket {
>>       int active_count;
>>       uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
>> @@ -26,6 +31,7 @@ struct S390Topology {
>>       uint32_t nr_cpus;
>>       uint32_t nr_sockets;
>>       S390TopoSocket *socket;
>> +    bool topology_needed;
>>   };
>>   #define TYPE_S390_CPU_TOPOLOGY "s390-topology"
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index c9066b2496..69a7523146 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -567,6 +567,81 @@ typedef union SysIB {
>>   } SysIB;
>>   QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
>> +/*
>> + * CPU Topology List provided by STSI with fc=15 provides a list
>> + * of two different Topology List Entries (TLE) types to specify
>> + * the topology hierarchy.
>> + *
>> + * - Container Topology List Entry
>> + *   Defines a container to contain other Topology List Entries
>> + *   of any type, nested containers or CPU.
>> + * - CPU Topology List Entry
>> + *   Specifies the CPUs position, type, entitlement and polarization
>> + *   of the CPUs contained in the last Container TLE.
>> + *
>> + * There can be theoretically up to five levels of containers, QEMU
>> + * uses only one level, the socket level.
>> + *
>> + * A container of with a nesting level (NL) greater than 1 can only
>> + * contain another container of nesting level NL-1.
>> + *
>> + * A container of nesting level 1 (socket), contains as many CPU TLE
>> + * as needed to describe the position and qualities of all CPUs inside
>> + * the container.
>> + * The qualities of a CPU are polarization, entitlement and type.
>> + *
>> + * The CPU TLE defines the position of the CPUs of identical qualities
>> + * using a 64bits mask which first bit has its offset defined by
>> + * the CPU address orgin field of the CPU TLE like in:
>> + * CPU address = origin * 64 + bit position within the mask
>> + *
>> + */
>> +/* Container type Topology List Entry */
>> +typedef struct SysIBTl_container {
>> +        uint8_t nl;
>> +        uint8_t reserved[6];
>> +        uint8_t id;
>> +} QEMU_PACKED QEMU_ALIGNED(8) SysIBTl_container;
>> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_container) != 8);
>> +
>> +/* CPU type Topology List Entry */
>> +typedef struct SysIBTl_cpu {
>> +        uint8_t nl;
>> +        uint8_t reserved0[3];
>> +        uint8_t reserved1:5;
>> +        uint8_t dedicated:1;
>> +        uint8_t polarity:2;
>> +        uint8_t type;
>> +        uint16_t origin;
>> +        uint64_t mask;
>> +} QEMU_PACKED QEMU_ALIGNED(8) SysIBTl_cpu;
>> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16);
>> +
>> +#define S390_TOPOLOGY_MAG  6
>> +#define S390_TOPOLOGY_MAG6 0
>> +#define S390_TOPOLOGY_MAG5 1
>> +#define S390_TOPOLOGY_MAG4 2
>> +#define S390_TOPOLOGY_MAG3 3
>> +#define S390_TOPOLOGY_MAG2 4
>> +#define S390_TOPOLOGY_MAG1 5
>> +/* Configuration topology */
>> +typedef struct SysIB_151x {
>> +    uint8_t  reserved0[2];
>> +    uint16_t length;
>> +    uint8_t  mag[S390_TOPOLOGY_MAG];
>> +    uint8_t  reserved1;
>> +    uint8_t  mnest;
>> +    uint32_t reserved2;
>> +    char tle[0];
>> +} QEMU_PACKED QEMU_ALIGNED(8) SysIB_151x;
>> +QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
>> +
>> +/* Max size of a SYSIB structure is when all CPU are alone in a 
>> container */
>> +#define S390_TOPOLOGY_SYSIB_SIZE (sizeof(SysIB_151x) 
>> +                         \
>> +                                  S390_MAX_CPUS * 
>> (sizeof(SysIBTl_container) + \
>> +                                                   sizeof(SysIBTl_cpu)))
>> +
>> +
>>   /* MMU defines */
>>   #define ASCE_ORIGIN           (~0xfffULL) /* segment table 
>> origin             */
>>   #define ASCE_SUBSPACE         0x200       /* subspace group 
>> control           */
>> @@ -845,4 +920,6 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
>>   #include "exec/cpu-all.h"
>> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar);
>> +
>>   #endif
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> index 6af41d3d7b..9fa8ca1261 100644
>> --- a/hw/s390x/cpu-topology.c
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -44,7 +44,6 @@ void s390_topology_new_cpu(S390CPU *cpu)
>>       int socket_id;
>>       socket_id = core_id / topo->nr_cpus;
>> -
>>       /*
>>        * At the core level, each CPU is represented by a bit in a 64bit
>>        * uint64_t which represent the presence of a CPU.
>> diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
>> new file mode 100644
>> index 0000000000..a1179d8e95
>> --- /dev/null
>> +++ b/target/s390x/cpu_topology.c
>> @@ -0,0 +1,100 @@
>> +/*
>> + * QEMU S390x CPU Topology
>> + *
>> + * Copyright IBM Corp. 2022
>> + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
>> (at
>> + * your option) any later version. See the COPYING file in the top-level
>> + * directory.
>> + */
>> +#include "qemu/osdep.h"
>> +#include "cpu.h"
>> +#include "hw/s390x/pv.h"
>> +#include "hw/sysbus.h"
>> +#include "hw/s390x/cpu-topology.h"
>> +#include "hw/s390x/sclp.h"
>> +
>> +static char *fill_container(char *p, int level, int id)
>> +{
>> +    SysIBTl_container *tle = (SysIBTl_container *)p;
>> +
>> +    tle->nl = level;
>> +    tle->id = id;
>> +    return p + sizeof(*tle);
>> +}
>> +
>> +static char *fill_tle_cpu(char *p, uint64_t mask, int origin)
>> +{
>> +    SysIBTl_cpu *tle = (SysIBTl_cpu *)p;
>> +
>> +    tle->nl = 0;
>> +    tle->dedicated = 1;
>> +    tle->polarity = S390_TOPOLOGY_POLARITY_HORIZONTAL;
>> +    tle->type = S390_TOPOLOGY_CPU_IFL;
>> +    tle->origin = cpu_to_be64(origin * 64);
>> +    tle->mask = cpu_to_be64(mask);
>> +    return p + sizeof(*tle);
>> +}
>> +
>> +static char *s390_top_set_level2(S390Topology *topo, char *p)
>> +{
>> +    int i, origin;
>> +
>> +    for (i = 0; i < topo->nr_sockets; i++) {
>> +        if (!topo->socket[i].active_count) {
>> +            continue;
>> +        }
>> +        p = fill_container(p, 1, i);
>> +        for (origin = 0; origin < S390_TOPOLOGY_MAX_ORIGIN; origin++) {
>> +            uint64_t mask = 0L;
>> +
>> +            mask = topo->socket[i].mask[origin];
>> +            if (mask) {
>> +                p = fill_tle_cpu(p, mask, origin);
>> +            }
>> +        }
>> +    }
>> +    return p;
>> +}
> 
> Why is it not possible to compute this topo information at "runtime",
> when stsi is called, without maintaining state in an extra S390Topology
> object ? Couldn't we loop on the CPU list to gather the topology bits
> for the same result ?
> 
> It would greatly simplify the feature.
> 
> C.
> 

The vCPU are not stored in order of creation in the CPU list and not in 
a topology order.
To be able to build the SYSIB we need an intermediate structure to 
reorder the CPUs per container.

We can do this re-ordering during the STSI interception but the idea was 
to keep this instruction as fast as possible.

The second reason is to have a structure ready for the QEMU migration 
when we introduce vCPU migration from a socket to another socket, having 
then a different internal representation of the topology.


However, if as discussed yesterday we use a new cpu flag we would not 
need any special migration structure in the current series.

So it only stays the first reason to do the re-ordering preparation 
during the plugging of a vCPU, to optimize the STSI instruction.

If we think the optimization is not worth it or do not bring enough to 
be consider, we can do everything during the STSI interception.

Regards,
Pierre

>> +static int setup_stsi(S390CPU *cpu, SysIB_151x *sysib, int level)
>> +{
>> +    S390Topology *topo = (S390Topology *)cpu->topology;
>> +    char *p = sysib->tle;
>> +
>> +    sysib->mnest = level;
>> +    switch (level) {
>> +    case 2:
>> +        sysib->mag[S390_TOPOLOGY_MAG2] = topo->nr_sockets;
>> +        sysib->mag[S390_TOPOLOGY_MAG1] = topo->nr_cpus;
>> +        p = s390_top_set_level2(topo, p);
>> +        break;
>> +    }
>> +
>> +    return p - (char *)sysib;
>> +}
>> +
>> +#define S390_TOPOLOGY_MAX_MNEST 2
>> +
>> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
>> +{
>> +    union {
>> +        char place_holder[S390_TOPOLOGY_SYSIB_SIZE];
>> +        SysIB_151x sysib;
>> +    } buffer QEMU_ALIGNED(8);
>> +    int len;
>> +
>> +    if (s390_is_pv() || !s390_has_topology() ||
>> +        sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
>> +        setcc(cpu, 3);
>> +        return;
>> +    }
>> +
>> +    len = setup_stsi(cpu, &buffer.sysib, sel2);
>> +
>> +    buffer.sysib.length = cpu_to_be16(len);
>> +    s390_cpu_virt_mem_write(cpu, addr, ar, &buffer.sysib, len);
>> +    setcc(cpu, 0);
>> +}
>> +
>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>> index 3ac7ec9acf..7dc96f3663 100644
>> --- a/target/s390x/kvm/kvm.c
>> +++ b/target/s390x/kvm/kvm.c
>> @@ -51,6 +51,7 @@
>>   #include "hw/s390x/s390-virtio-ccw.h"
>>   #include "hw/s390x/s390-virtio-hcall.h"
>>   #include "hw/s390x/pv.h"
>> +#include "hw/s390x/cpu-topology.h"
>>   #ifndef DEBUG_KVM
>>   #define DEBUG_KVM  0
>> @@ -1919,9 +1920,12 @@ static int handle_stsi(S390CPU *cpu)
>>           if (run->s390_stsi.sel1 != 2 || run->s390_stsi.sel2 != 2) {
>>               return 0;
>>           }
>> -        /* Only sysib 3.2.2 needs post-handling for now. */
>>           insert_stsi_3_2_2(cpu, run->s390_stsi.addr, run->s390_stsi.ar);
>>           return 0;
>> +    case 15:
>> +        insert_stsi_15_1_x(cpu, run->s390_stsi.sel2, 
>> run->s390_stsi.addr,
>> +                           run->s390_stsi.ar);
>> +        return 0;
>>       default:
>>           return 0;
>>       }
>> diff --git a/target/s390x/meson.build b/target/s390x/meson.build
>> index 84c1402a6a..890ccfa789 100644
>> --- a/target/s390x/meson.build
>> +++ b/target/s390x/meson.build
>> @@ -29,6 +29,7 @@ s390x_softmmu_ss.add(files(
>>     'sigp.c',
>>     'cpu-sysemu.c',
>>     'cpu_models_sysemu.c',
>> +  'cpu_topology.c',
>>   ))
>>   s390x_user_ss = ss.source_set()
> 

-- 
Pierre Morel
IBM Lab Boeblingen

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 04/11] s390x/cpu topology: reporting the CPU topology to the guest
  2022-11-17  9:32     ` Pierre Morel
@ 2022-11-21 14:13       ` Cédric Le Goater
  2022-11-22  9:05         ` Pierre Morel
  0 siblings, 1 reply; 36+ messages in thread
From: Cédric Le Goater @ 2022-11-21 14:13 UTC (permalink / raw)
  To: Pierre Morel, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange

>>> +static char *s390_top_set_level2(S390Topology *topo, char *p)
>>> +{
>>> +    int i, origin;
>>> +
>>> +    for (i = 0; i < topo->nr_sockets; i++) {
>>> +        if (!topo->socket[i].active_count) {
>>> +            continue;
>>> +        }
>>> +        p = fill_container(p, 1, i);
>>> +        for (origin = 0; origin < S390_TOPOLOGY_MAX_ORIGIN; origin++) {
>>> +            uint64_t mask = 0L;
>>> +
>>> +            mask = topo->socket[i].mask[origin];
>>> +            if (mask) {
>>> +                p = fill_tle_cpu(p, mask, origin);
>>> +            }
>>> +        }
>>> +    }
>>> +    return p;
>>> +}
>>
>> Why is it not possible to compute this topo information at "runtime",
>> when stsi is called, without maintaining state in an extra S390Topology
>> object ? Couldn't we loop on the CPU list to gather the topology bits
>> for the same result ?
>>
>> It would greatly simplify the feature.
>>
>> C.
>>
> 
> The vCPU are not stored in order of creation in the CPU list and not in a topology order.
> To be able to build the SYSIB we need an intermediate structure to reorder the CPUs per container.
> 
> We can do this re-ordering during the STSI interception but the idea was to keep this instruction as fast as possible.> 
> The second reason is to have a structure ready for the QEMU migration when we introduce vCPU migration from a socket to another socket, having then a different internal representation of the topology.
> 
> 
> However, if as discussed yesterday we use a new cpu flag we would not need any special migration structure in the current series.
> 
> So it only stays the first reason to do the re-ordering preparation during the plugging of a vCPU, to optimize the STSI instruction.
> 
> If we think the optimization is not worth it or do not bring enough to be consider, we can do everything during the STSI interception.

Is it called on a hot code path ? AFAICT, it is only called once
per cpu when started. insert_stsi_3_2_2 is also a guest exit andit queries the machine definition in a very similar way.

Thanks,

C.


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 04/11] s390x/cpu topology: reporting the CPU topology to the guest
  2022-11-21 14:13       ` Cédric Le Goater
@ 2022-11-22  9:05         ` Pierre Morel
  2022-11-27 10:46           ` Pierre Morel
  0 siblings, 1 reply; 36+ messages in thread
From: Pierre Morel @ 2022-11-22  9:05 UTC (permalink / raw)
  To: Cédric Le Goater, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange



On 11/21/22 15:13, Cédric Le Goater wrote:
>>>> +static char *s390_top_set_level2(S390Topology *topo, char *p)
>>>> +{
>>>> +    int i, origin;
>>>> +
>>>> +    for (i = 0; i < topo->nr_sockets; i++) {
>>>> +        if (!topo->socket[i].active_count) {
>>>> +            continue;
>>>> +        }
>>>> +        p = fill_container(p, 1, i);
>>>> +        for (origin = 0; origin < S390_TOPOLOGY_MAX_ORIGIN; 
>>>> origin++) {
>>>> +            uint64_t mask = 0L;
>>>> +
>>>> +            mask = topo->socket[i].mask[origin];
>>>> +            if (mask) {
>>>> +                p = fill_tle_cpu(p, mask, origin);
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +    return p;
>>>> +}
>>>
>>> Why is it not possible to compute this topo information at "runtime",
>>> when stsi is called, without maintaining state in an extra S390Topology
>>> object ? Couldn't we loop on the CPU list to gather the topology bits
>>> for the same result ?
>>>
>>> It would greatly simplify the feature.
>>>
>>> C.
>>>
>>
>> The vCPU are not stored in order of creation in the CPU list and not 
>> in a topology order.
>> To be able to build the SYSIB we need an intermediate structure to 
>> reorder the CPUs per container.
>>
>> We can do this re-ordering during the STSI interception but the idea 
>> was to keep this instruction as fast as possible.> The second reason 
>> is to have a structure ready for the QEMU migration when we introduce 
>> vCPU migration from a socket to another socket, having then a 
>> different internal representation of the topology.
>>
>>
>> However, if as discussed yesterday we use a new cpu flag we would not 
>> need any special migration structure in the current series.
>>
>> So it only stays the first reason to do the re-ordering preparation 
>> during the plugging of a vCPU, to optimize the STSI instruction.
>>
>> If we think the optimization is not worth it or do not bring enough to 
>> be consider, we can do everything during the STSI interception.
> 
> Is it called on a hot code path ? AFAICT, it is only called once
> per cpu when started. insert_stsi_3_2_2 is also a guest exit andit 
> queries the machine definition in a very similar way.


It is not fully exact, stsi(15) is called at several moments, not only 
on CPU creation, but each time the core calls rebuild_sched_domains() 
that is for s390 on:
- change in the host topology
- changes in CPUSET: for allowed CPU or load balancing

Regards,
Pierre

> 
> Thanks,
> 
> C.
> 

-- 
Pierre Morel
IBM Lab Boeblingen

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v11 04/11] s390x/cpu topology: reporting the CPU topology to the guest
  2022-11-22  9:05         ` Pierre Morel
@ 2022-11-27 10:46           ` Pierre Morel
  0 siblings, 0 replies; 36+ messages in thread
From: Pierre Morel @ 2022-11-27 10:46 UTC (permalink / raw)
  To: Cédric Le Goater, qemu-s390x
  Cc: qemu-devel, borntraeger, pasic, richard.henderson, david, thuth,
	cohuck, mst, pbonzini, kvm, ehabkost, marcel.apfelbaum, eblake,
	armbru, seiden, nrb, scgl, frankja, berrange



On 11/22/22 10:05, Pierre Morel wrote:
> 
> 
> On 11/21/22 15:13, Cédric Le Goater wrote:
>>>>> +static char *s390_top_set_level2(S390Topology *topo, char *p)
>>>>> +{
>>>>> +    int i, origin;
>>>>> +
>>>>> +    for (i = 0; i < topo->nr_sockets; i++) {
>>>>> +        if (!topo->socket[i].active_count) {
>>>>> +            continue;
>>>>> +        }
>>>>> +        p = fill_container(p, 1, i);
>>>>> +        for (origin = 0; origin < S390_TOPOLOGY_MAX_ORIGIN; 
>>>>> origin++) {
>>>>> +            uint64_t mask = 0L;
>>>>> +
>>>>> +            mask = topo->socket[i].mask[origin];
>>>>> +            if (mask) {
>>>>> +                p = fill_tle_cpu(p, mask, origin);
>>>>> +            }
>>>>> +        }
>>>>> +    }
>>>>> +    return p;
>>>>> +}
>>>>
>>>> Why is it not possible to compute this topo information at "runtime",
>>>> when stsi is called, without maintaining state in an extra S390Topology
>>>> object ? Couldn't we loop on the CPU list to gather the topology bits
>>>> for the same result ?
>>>>
>>>> It would greatly simplify the feature.
>>>>
>>>> C.
>>>>
>>>
>>> The vCPU are not stored in order of creation in the CPU list and not 
>>> in a topology order.
>>> To be able to build the SYSIB we need an intermediate structure to 
>>> reorder the CPUs per container.
>>>
>>> We can do this re-ordering during the STSI interception but the idea 
>>> was to keep this instruction as fast as possible.> The second reason 
>>> is to have a structure ready for the QEMU migration when we introduce 
>>> vCPU migration from a socket to another socket, having then a 
>>> different internal representation of the topology.
>>>
>>>
>>> However, if as discussed yesterday we use a new cpu flag we would not 
>>> need any special migration structure in the current series.
>>>
>>> So it only stays the first reason to do the re-ordering preparation 
>>> during the plugging of a vCPU, to optimize the STSI instruction.
>>>
>>> If we think the optimization is not worth it or do not bring enough 
>>> to be consider, we can do everything during the STSI interception.
>>
>> Is it called on a hot code path ? AFAICT, it is only called once
>> per cpu when started. insert_stsi_3_2_2 is also a guest exit andit 
>> queries the machine definition in a very similar way.
> 
> 
> It is not fully exact, stsi(15) is called at several moments, not only 
> on CPU creation, but each time the core calls rebuild_sched_domains() 
> that is for s390 on:
> - change in the host topology
> - changes in CPUSET: for allowed CPU or load balancing
> 
> Regards,
> Pierre

These are no good reasons to not make as you propose.
This allows to use the s390_has_feature() and use the cpu feature as 
proposed Christian.
What I can not do with the early topology initialization.

Regards,
Pierre

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

-- 
Pierre Morel
IBM Lab Boeblingen

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2022-11-27 10:47 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-03 17:01 [PATCH v11 00/11] s390x: CPU Topology Pierre Morel
2022-11-03 17:01 ` [PATCH v11 01/11] s390x: Register TYPE_S390_CCW_MACHINE properties as class properties Pierre Morel
2022-11-04  6:32   ` Thomas Huth
2022-11-04 10:16     ` Pierre Morel
2022-11-04 10:53       ` Cédric Le Goater
2022-11-04 13:58         ` Pierre Morel
2022-11-04 14:29         ` Thomas Huth
2022-11-04 14:57           ` Pierre Morel
2022-11-06 11:37             ` Thomas Huth
2022-11-07  9:52               ` Pierre Morel
2022-11-03 17:01 ` [PATCH v11 02/11] s390x/cpu topology: add max_threads machine class attribute Pierre Morel
2022-11-03 17:01 ` [PATCH v11 03/11] s390x/cpu topology: core_id sets s390x CPU topology Pierre Morel
2022-11-15 11:15   ` Cédric Le Goater
2022-11-16 10:17     ` Pierre Morel
2022-11-03 17:01 ` [PATCH v11 04/11] s390x/cpu topology: reporting the CPU topology to the guest Pierre Morel
2022-11-15 11:21   ` Cédric Le Goater
2022-11-16 10:27     ` Pierre Morel
2022-11-17  8:40   ` Cédric Le Goater
2022-11-17  9:32     ` Pierre Morel
2022-11-21 14:13       ` Cédric Le Goater
2022-11-22  9:05         ` Pierre Morel
2022-11-27 10:46           ` Pierre Morel
2022-11-03 17:01 ` [PATCH v11 05/11] s390x/cpu_topology: resetting the Topology-Change-Report Pierre Morel
2022-11-03 17:01 ` [PATCH v11 06/11] s390x/cpu_topology: CPU topology migration Pierre Morel
2022-11-03 17:01 ` [PATCH v11 07/11] target/s390x: interception of PTF instruction Pierre Morel
2022-11-03 17:01 ` [PATCH v11 08/11] s390x/cpu topology: add topology_capable QEMU capability Pierre Morel
2022-11-15 13:27   ` Cédric Le Goater
2022-11-16 11:23     ` Pierre Morel
2022-11-03 17:01 ` [PATCH v11 09/11] s390x/cpu topology: add topology machine property Pierre Morel
2022-11-03 17:20   ` Cornelia Huck
2022-11-04 10:09     ` Pierre Morel
2022-11-15 13:48   ` Cédric Le Goater
2022-11-16 12:39     ` Pierre Morel
     [not found]       ` <PH0PR22MB3210864C22AD57E5B32F626991079@PH0PR22MB3210.namprd22.prod.outlook.com>
2022-11-16 13:17         ` Thank you! s390x/cpu topology Jadon
2022-11-03 17:01 ` [PATCH v11 10/11] s390x/cpu_topology: activating CPU topology Pierre Morel
2022-11-03 17:01 ` [PATCH v11 11/11] docs/s390x: document s390x cpu topology Pierre Morel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.