On Wed, 2023-02-22 at 15:21 +0100, Pierre Morel wrote:
> The modification of the CPU attributes are done through a monitor
> command.
>
> It allows to move the core inside the topology tree to optimize
> the cache usage in the case the host's hypervisor previously
> moved the CPU.
>
> The same command allows to modify 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.
>
> The command has a feature unstable for the moment.
>
> Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
> ---
> qapi/machine-target.json | 35 +++++++++
> include/monitor/hmp.h | 1 +
> hw/s390x/cpu-topology.c | 154 +++++++++++++++++++++++++++++++++++++++
> hmp-commands.hx | 17 +++++
> 4 files changed, 207 insertions(+)
>
> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> index a52cc32f09..baa9d273cf 100644
> --- a/qapi/machine-target.json
> +++ b/qapi/machine-target.json
> @@ -354,3 +354,38 @@
> { 'enum': 'CpuS390Polarization',
> 'prefix': 'S390_CPU_POLARIZATION',
> 'data': [ 'horizontal', 'vertical' ] }
> +
> +##
> +# @set-cpu-topology:
> +#
> +# @core-id: the vCPU ID to be moved
> +# @socket-id: optional destination socket where to move the vCPU
> +# @book-id: optional destination book where to move the vCPU
> +# @drawer-id: optional destination drawer where to move the vCPU
> +# @entitlement: optional entitlement
> +# @dedicated: optional, if the vCPU is dedicated to a real CPU
> +#
> +# Features:
> +# @unstable: This command may still be modified.
> +#
> +# Modifies the topology by moving the CPU inside the topology
> +# tree or by changing a modifier attribute of a CPU.
> +# Default value for optional parameter is the current value
> +# used by the CPU.
> +#
> +# Returns: Nothing on success, the reason on failure.
> +#
> +# Since: 8.0
> +##
> +{ 'command': 'set-cpu-topology',
> + 'data': {
> + 'core-id': 'uint16',
> + '*socket-id': 'uint16',
> + '*book-id': 'uint16',
> + '*drawer-id': 'uint16',
> + '*entitlement': 'str',
How about you add a machine-common.json and define CpuS390Entitlement there,
and then include it from both machine.json and machine-target.json?
Then you can declare it as CpuS390Entitlement and don't need to do the parsing
manually.
You could also put it in common.json, but that seems a bit too generic.
Anyone have objections?
> + '*dedicated': 'bool'
> + },
> + 'features': [ 'unstable' ],
> + 'if': { 'all': [ 'TARGET_S390X' ] }
> +}
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index 2220f14fc9..4e65e6d08e 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -178,5 +178,6 @@ void hmp_ioport_read(Monitor *mon, const QDict *qdict);
> void hmp_ioport_write(Monitor *mon, const QDict *qdict);
> void hmp_boot_set(Monitor *mon, const QDict *qdict);
> void hmp_info_mtree(Monitor *mon, const QDict *qdict);
> +void hmp_set_cpu_topology(Monitor *mon, const QDict *qdict);
>
> #endif
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> index ed5fc75381..3a7eb441a3 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -19,6 +19,12 @@
> #include "hw/s390x/s390-virtio-ccw.h"
> #include "hw/s390x/cpu-topology.h"
> #include "qapi/qapi-types-machine-target.h"
> +#include "qapi/qapi-types-machine.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.
> * .cores_per_socket: tracks information on the count of cores
> @@ -310,6 +316,26 @@ static void s390_topology_add_core_to_socket(S390CPU *cpu, int drawer_id,
> }
> }
>
> +/**
> + * s390_topology_need_report
> + * @cpu: Current cpu
> + * @drawer_id: future drawer ID
> + * @book_id: future book ID
> + * @socket_id: future socket ID
> + *
> + * A modified topology change report is needed if the
> + */
> +static int s390_topology_need_report(S390CPU *cpu, int drawer_id,
> + int book_id, int socket_id,
> + uint16_t entitlement, bool dedicated)
> +{
> + return cpu->env.drawer_id != drawer_id ||
> + cpu->env.book_id != book_id ||
> + cpu->env.socket_id != socket_id ||
> + cpu->env.entitlement != entitlement ||
> + cpu->env.dedicated != dedicated;
> +}
> +
> /**
> * s390_update_cpu_props:
> * @ms: the machine state
> @@ -376,3 +402,131 @@ void s390_topology_setup_cpu(MachineState *ms, S390CPU *cpu, Error **errp)
> /* topology tree is reflected in props */
> s390_update_cpu_props(ms, cpu);
> }
> +
> +/*
> + * qmp and hmp implementations
> + */
> +
> +#define TOPOLOGY_SET(n) do { \
> + if (has_ ## n) { \
> + calc_ ## n = n; \
> + } else { \
> + calc_ ## n = cpu->env.n; \
> + } \
> + } while (0)
> +
> +static void s390_change_topology(uint16_t core_id,
> + bool has_socket_id, uint16_t socket_id,
> + bool has_book_id, uint16_t book_id,
> + bool has_drawer_id, uint16_t drawer_id,
> + bool has_entitlement, uint16_t entitlement,
> + bool has_dedicated, bool dedicated,
> + Error **errp)
> +{
> + MachineState *ms = current_machine;
> + uint16_t calc_dedicated, calc_entitlement;
> + uint16_t calc_socket_id, calc_book_id, calc_drawer_id;
> + S390CPU *cpu;
> + int report_needed;
> + ERRP_GUARD();
> +
> + if (core_id >= ms->smp.max_cpus) {
> + error_setg(errp, "Core-id %d out of range!", core_id);
> + return;
> + }
> +
> + cpu = (S390CPU *)ms->possible_cpus->cpus[core_id].cpu;
> + if (!cpu) {
> + error_setg(errp, "Core-id %d does not exist!", core_id);
> + return;
> + }
> +
> + /* Get unprovided attributes from cpu and verify the new topology */
Get attributes not provided...
> + TOPOLOGY_SET(entitlement);
> + TOPOLOGY_SET(dedicated);
> + TOPOLOGY_SET(socket_id);
> + TOPOLOGY_SET(book_id);
> + TOPOLOGY_SET(drawer_id);
You could also just assign to the arguments, i.e.
if (!has_socket_id)
socket_id = cpu->env.socket_id;
> +
> + s390_topology_check(calc_socket_id, calc_book_id, calc_drawer_id,
> + calc_entitlement, calc_dedicated, errp);
> + if (*errp) {
> + return;
> + }
> +
> + /* Move the CPU into its new socket */
> + s390_topology_add_core_to_socket(cpu, calc_drawer_id, calc_book_id,
> + calc_socket_id, false, errp);
> + if (*errp) {
> + return;
> + }
> +
> + /* Check if we need to report the modified topology */
> + report_needed = s390_topology_need_report(cpu, calc_drawer_id, calc_book_id,
> + calc_socket_id, calc_entitlement,
> + calc_dedicated);
> +
> + /* All checks done, report new topology into the vCPU */
> + cpu->env.drawer_id = calc_drawer_id;
> + cpu->env.book_id = calc_book_id;
> + cpu->env.socket_id = calc_socket_id;
> + cpu->env.dedicated = calc_dedicated;
> + cpu->env.entitlement = calc_entitlement;
> +
> + /* topology tree is reflected in props */
> + s390_update_cpu_props(ms, cpu);
> +
> + /* Advertise the topology change */
> + if (report_needed) {
> + s390_cpu_topology_set_changed(true);
> + }
> +}
> +
> +void qmp_set_cpu_topology(uint16_t core,
> + bool has_socket, uint16_t socket,
> + bool has_book, uint16_t book,
> + bool has_drawer, uint16_t drawer,
> + const char *entitlement_str,
> + bool has_dedicated, bool dedicated,
> + Error **errp)
> +{
> + bool has_entitlement = false;
> + int entitlement;
> + ERRP_GUARD();
> +
> + if (!s390_has_topology()) {
> + error_setg(errp, "This machine doesn't support topology");
> + return;
> + }
> +
> + entitlement = qapi_enum_parse(&CpuS390Entitlement_lookup, entitlement_str,
> + -1, errp);
> + if (*errp) {
> + return;
> + }
> + has_entitlement = entitlement >= 0;
Doesn't this allow setting horizontal entitlement? Which shouldn't be possible,
only the guest can do it.
> +
> + s390_change_topology(core, has_socket, socket, has_book, book,
> + has_drawer, drawer, has_entitlement, entitlement,
> + has_dedicated, dedicated, errp);
> +}
> +
> +void hmp_set_cpu_topology(Monitor *mon, const QDict *qdict)
> +{
> + const uint16_t core = qdict_get_int(qdict, "core-id");
> + bool has_socket = qdict_haskey(qdict, "socket-id");
> + const uint16_t socket = qdict_get_try_int(qdict, "socket-id", 0);
> + bool has_book = qdict_haskey(qdict, "book-id");
> + const uint16_t book = qdict_get_try_int(qdict, "book-id", 0);
> + bool has_drawer = qdict_haskey(qdict, "drawer-id");
> + const uint16_t drawer = qdict_get_try_int(qdict, "drawer-id", 0);
The names here don't match the definition below, leading to a crash,
because core-id is a mandatory argument.
> + const char *entitlement = qdict_get_try_str(qdict, "entitlement");
> + bool has_dedicated = qdict_haskey(qdict, "dedicated");
> + const bool dedicated = qdict_get_try_bool(qdict, "dedicated", false);
> + Error *local_err = NULL;
> +
> + qmp_set_cpu_topology(core, has_socket, socket, has_book, book,
> + has_drawer, drawer, entitlement,
> + has_dedicated, dedicated, &local_err);
> + hmp_handle_error(mon, local_err);
> +}
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index fbb5daf09b..d8c37808c7 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1815,3 +1815,20 @@ SRST
> Dump the FDT in dtb format to *filename*.
> ERST
> #endif
> +
> +#if defined(TARGET_S390X)
> + {
> + .name = "set-cpu-topology",
> + .args_type = "core:l,socket:l?,book:l?,drawer:l?,entitlement:s?,dedicated:b?",
Can you use ":O" for the ids? It would allow for some more flexibility.
> + .params = "core [socket] [book] [drawer] [entitlement] [dedicated]",
> + .help = "Move CPU 'core' to 'socket/book/drawer' "
> + "optionally modifies entitlement and dedication",
> + .cmd = hmp_set_cpu_topology,
> + },
> +
> +SRST
> +``set-cpu-topology`` *core* *socket* *book* *drawer* *entitlement* *dedicated*
> + Modify CPU topology for the CPU *core* to move on *socket* *book* *drawer*
> + with topology attributes *entitlement* *dedicated*.
> +ERST
> +#endif
