On 1/16/23 22:09, Nina Schoetterl-Glausch wrote:
On Thu, 2023-01-05 at 15:53 +0100, Pierre Morel wrote:
The modification of the CPU attributes are done through a monitor
commands.
It allows to move the core inside the topology tree to optimise
the cache usage in the case the host's hypervizor previously
moved the CPU.
The same command allows to modifiy the CPU attributes modifiers
like polarization entitlement and the dedicated attribute to notify
the guest if the host admin modified scheduling or dedication of a vCPU.
With this knowledge the guest has the possibility to optimize the
usage of the vCPUs.
Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
---
qapi/machine-target.json | 29 ++++++++
include/monitor/hmp.h | 1 +
hw/s390x/cpu-topology.c | 141 +++++++++++++++++++++++++++++++++++++++
hmp-commands.hx | 16 +++++
4 files changed, 187 insertions(+)
diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index 2e267fa458..75b0aa254d 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -342,3 +342,32 @@
'TARGET_S390X',
'TARGET_MIPS',
'TARGET_LOONGARCH64' ] } }
+
+##
+# @change-topology:
+#
+# @core: the vCPU ID to be moved
+# @socket: the destination socket where to move the vCPU
+# @book: the destination book where to move the vCPU
+# @drawer: the destination drawer where to move the vCPU
+# @polarity: optional polarity, default is last polarity set by the guest
+# @dedicated: optional, if the vCPU is dedicated to a real CPU
+#
+# Modifies the topology by moving the CPU inside the topology
+# tree or by changing a modifier attribute of a CPU.
+#
+# Returns: Nothing on success, the reason on failure.
+#
+# Since: <next qemu stable release, eg. 1.0>
+##
+{ 'command': 'change-topology',
+ 'data': {
+ 'core': 'int',
+ 'socket': 'int',
+ 'book': 'int',
+ 'drawer': 'int',
+ '*polarity': 'int',
+ '*dedicated': 'bool'
+ },
+ 'if': { 'all': [ 'TARGET_S390X', 'CONFIG_KVM' ] }
+}
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 27f86399f7..15c36bf549 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -144,5 +144,6 @@ void hmp_human_readable_text_helper(Monitor *mon,
HumanReadableText *(*qmp_handler)(Error **));
void hmp_info_stats(Monitor *mon, const QDict *qdict);
void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict);
+void hmp_change_topology(Monitor *mon, const QDict *qdict);
#endif
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index b69955a1cd..0faffe657e 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -18,6 +18,10 @@
#include "target/s390x/cpu.h"
#include "hw/s390x/s390-virtio-ccw.h"
#include "hw/s390x/cpu-topology.h"
+#include "qapi/qapi-commands-machine-target.h"
+#include "qapi/qmp/qdict.h"
+#include "monitor/hmp.h"
+#include "monitor/monitor.h"
/*
* s390_topology is used to keep the topology information.
@@ -203,6 +207,21 @@ static void s390_topology_set_entry(S390TopologyEntry *entry,
s390_topology.sockets[s390_socket_nb(id)]++;
}
+/**
+ * s390_topology_clear_entry:
+ * @entry: Topology entry to setup
+ * @id: topology id to use for the setup
+ *
+ * Clear the core bit inside the topology mask and
+ * decrements the number of cores for the socket.
+ */
+static void s390_topology_clear_entry(S390TopologyEntry *entry,
+ s390_topology_id id)
+{
+ clear_bit(63 - id.core, &entry->mask);
This doesn't take the origin into account.
Yes, this error has been done everywhere, I modify this.
+ s390_topology.sockets[s390_socket_nb(id)]--;
I suppose this function cannot run concurrently, so the same CPU doesn't get removed twice.
This is only called on a change of topology and the qapi commands are
serialized by the BQL.
+}
+
/**
* s390_topology_new_entry:
* @id: s390_topology_id to add
@@ -383,3 +402,125 @@ void s390_topology_set_cpu(MachineState *ms, S390CPU *cpu, Error **errp)
s390_topology_insert(id);
}
+
+/*
+ * qmp and hmp implementations
+ */
+
+static S390TopologyEntry *s390_topology_core_to_entry(int core)
+{
+ S390TopologyEntry *entry;
+
+ QTAILQ_FOREACH(entry, &s390_topology.list, next) {
+ if (entry->mask & (1UL << (63 - core))) {
origin here also.
yes
+ return entry;
+ }
+ }
+ return NULL;
This should not return NULL unless the core id is invalid.
yes
Might be better to validate that somewhere else.
How?
I can use CPU_FOREACH() but it would be quite the same and I will need
to rerun a loop for setting the topology anyway.
+}
+
+static void s390_change_topology(Error **errp, int64_t core, int64_t socket,
+ int64_t book, int64_t drawer,
+ int64_t polarity, bool dedicated)
+{
+ S390TopologyEntry *entry;
+ s390_topology_id new_id;
+ s390_topology_id old_id;
+ Error *local_error = NULL;
I think you could use ERRP_GUARD here also.
You are right, I could.
Should I ?
I do not need it and find it obfuscating but if you insist I can do it.
+
+ /* Get the old entry */
+ entry = s390_topology_core_to_entry(core);
+ if (!entry) {
+ error_setg(errp, "No core %ld", core);
+ return;
+ }
+
+ /* Compute old topology id */
+ old_id = entry->id;
+ old_id.core = core;
+
+ /* Compute new topology id */
+ new_id = entry->id;
+ new_id.core = core;
+ new_id.socket = socket;
+ new_id.book = book;
+ new_id.drawer = drawer;
+ new_id.p = polarity;
+ new_id.d = dedicated;
+ new_id.type = S390_TOPOLOGY_CPU_IFL;
+
+ /* Same topology entry, nothing to do */
+ if (entry->id.id == new_id.id) {
+ return;
+ }
+
+ /* Check for space on the socket if ids are different */
+ if ((s390_socket_nb(old_id) != s390_socket_nb(new_id)) &&
+ (s390_topology.sockets[s390_socket_nb(new_id)] >=
+ s390_topology.smp->sockets)) {
+ error_setg(errp, "No more space on this socket");
+ return;
+ }
+
+ /* Verify the new topology */
+ s390_topology_check(&local_error, new_id);
+ if (local_error) {
+ error_propagate(errp, local_error);
+ return;
+ }
+
+ /* Clear the old topology */
+ s390_topology_clear_entry(entry, old_id);
+
+ /* Insert the new topology */
+ s390_topology_insert(new_id);
+
+ /* Remove unused entry */
+ if (!entry->mask) {
+ QTAILQ_REMOVE(&s390_topology.list, entry, next);
+ g_free(entry);
+ }
+
+ /* Advertise the topology change */
+ s390_cpu_topology_set();
+}
+
+void qmp_change_topology(int64_t core, int64_t socket,
+ int64_t book, int64_t drawer,
+ bool has_polarity, int64_t polarity,
+ bool has_dedicated, bool dedicated,
+ Error **errp)
+{
+ Error *local_err = NULL;
+
+ if (!s390_has_topology()) {
+ error_setg(&local_err, "This machine doesn't support topology");
+ return;
+ }
Do you really want to ignore has_polarity and has_dedicated here?
What happens in this case?
no, seems I forgot to check it.
Thanks,
Regards,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
