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.
> + s390_topology.sockets[s390_socket_nb(id)]--;
I suppose this function cannot run concurrently, so the same CPU doesn't get removed twice.
> +}
> +
> /**
> * 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.
> + return entry;
> + }
> + }
> + return NULL;
This should not return NULL unless the core id is invalid.
Might be better to validate that somewhere else.
> +}
> +
> +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.
> +
> + /* 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?
> + s390_change_topology(&local_err, core, socket, book, drawer,
> + polarity, dedicated);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + }
> +}
> +
> +void hmp_change_topology(Monitor *mon, const QDict *qdict)
> +{
> + const int64_t core = qdict_get_int(qdict, "core");
> + const int64_t socket = qdict_get_int(qdict, "socket");
> + const int64_t book = qdict_get_int(qdict, "book");
> + const int64_t drawer = qdict_get_int(qdict, "drawer");
> + bool has_polarity = qdict_haskey(qdict, "polarity");
> + const int64_t polarity = qdict_get_try_int(qdict, "polarity", 0);
> + bool has_dedicated = qdict_haskey(qdict, "dedicated");
> + const bool dedicated = qdict_get_try_bool(qdict, "dedicated", false);
> + Error *local_err = NULL;
> +
> + qmp_change_topology(core, socket, book, drawer,
> + has_polarity, polarity,
> + has_dedicated, dedicated,
> + &local_err);
> + if (hmp_handle_error(mon, local_err)) {
> + return;
> + }
> +}
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 673e39a697..a617cfed0d 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1815,3 +1815,19 @@ SRST
> Dump the FDT in dtb format to *filename*.
> ERST
> #endif
> +
> +#if defined(TARGET_S390X) && defined(CONFIG_KVM)
> + {
> + .name = "change-topology",
> + .args_type = "core:l,socket:l,book:l,drawer:l,polarity:l?,dedicated:b?",
> + .params = "core socket book drawer [polarity] [dedicated]",
> + .help = "Move CPU 'core' to 'socket/book/drawer' "
> + "optionaly modifies polarity and dedication",
> + .cmd = hmp_change_topology,
> + },
> +
> +SRST
> +``change-topology`` *core* *socket* *book* *drawer* *polarity* *dedicated*
> + Moves the CPU *core* to *socket* *book* *drawer* with *polarity* *dedicated*.
> +ERST
> +#endif
