Re: [PATCH v16 08/11] qapi/s390x/cpu topology: set-cpu-topology monitor command

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

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@linux.ibm.com>
> ---
>  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