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

[Pierre Morel]

On 2/8/23 19:40, Nina Schoetterl-Glausch wrote:
> On Wed, 2023-02-01 at 14:20 +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 optimise
> > 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 is made experimental for the moment.
> >
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >   qapi/machine-target.json | 29 +++++++++++++
> >   include/monitor/hmp.h    |  1 +
> >   hw/s390x/cpu-topology.c  | 88 ++++++++++++++++++++++++++++++++++++++++
> >   hmp-commands.hx          | 16 ++++++++
> >   4 files changed, 134 insertions(+)
> >
> > diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> > index 2e267fa458..58df0f5061 100644
> > --- a/qapi/machine-target.json
> > +++ b/qapi/machine-target.json
> > @@ -342,3 +342,32 @@
> >                      'TARGET_S390X',
> >                      'TARGET_MIPS',
> >                      'TARGET_LOONGARCH64' ] } }
> > +
> > +##
> > +# @x-set-cpu-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
>
> I wonder if it wouldn't be more convenient for the caller if everything is 
> optional.

Yes, it is a good point.

> > +# @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': 'x-set-cpu-topology',
> > +  'data': {
> > +      'core': 'int',
> > +      'socket': 'int',
> > +      'book': 'int',
> > +      'drawer': 'int',
>
> Did you consider naming those core-id, etc.? It would be consistent with
> query-cpus-fast/CpuInstanceProperties. Also all your variables end with _id.
> I don't care really just wanted to point it out.

OK, core-id etc. looks better

> > +      '*polarity': 'int',
> > +      '*dedicated': 'bool'
> > +  },
> > +  'if': { 'all': [ 'TARGET_S390X', 'CONFIG_KVM' ] }
> > +}
>
> So apparently this is the old way of doing an experimental api.
>
> > Names beginning with ``x-`` used to signify "experimental".  This
> > convention has been replaced by special feature "unstable".
>
> > Feature "unstable" marks a command, event, enum value, or struct
> > member as unstable.  It is not supported elsewhere so far.  Interfaces
> > so marked may be withdrawn or changed incompatibly in future releases.

OK

> > diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> > index 1b3bdcb446..12827479cf 100644
> > --- a/include/monitor/hmp.h
> > +++ b/include/monitor/hmp.h
> > @@ -151,5 +151,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_x_set_cpu_topology(Monitor *mon, const QDict *qdict);
> >   
> >   #endif
> > diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> > index c33378577b..6c50050991 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.
> > @@ -379,3 +383,87 @@ void s390_topology_set_cpu(MachineState *ms, S390CPU *cpu, 
> > Error **errp)
> >       /* topology tree is reflected in props */
> >       s390_update_cpu_props(ms, cpu);
> >   }
> > +
> > +/*
> > + * qmp and hmp implementations
> > + */
> > +
> > +static void s390_change_topology(int64_t core_id, int64_t socket_id,
> > +                                 int64_t book_id, int64_t drawer_id,
> > +                                 int64_t polarity, bool dedicated,
> > +                                 Error **errp)
> > +{
> > +    MachineState *ms = current_machine;
> > +    S390CPU *cpu;
> > +    ERRP_GUARD();
> > +
> > +    cpu = (S390CPU *)ms->possible_cpus->cpus[core_id].cpu;
> > +    if (!cpu) {
> > +        error_setg(errp, "Core-id %ld does not exist!", core_id);
> > +        return;
> > +    }
> > +
> > +    /* Verify the new topology */
> > +    s390_topology_check(cpu, errp);
> > +    if (*errp) {
> > +        return;
> > +    }
> > +
> > +    /* Move the CPU into its new socket */
> > +    s390_set_core_in_socket(cpu, drawer_id, book_id, socket_id, true, errp);
>
> The cpu isn't being created, so that should be false instead of true, right?

Yes right, should be "false"

> > +
> > +    /* All checks done, report topology in environment */
> > +    cpu->env.drawer_id = drawer_id;
> > +    cpu->env.book_id = book_id;
> > +    cpu->env.socket_id = socket_id;
> > +    cpu->env.dedicated = dedicated;
> > +    cpu->env.entitlement = polarity;
> > +
> > +    /* topology tree is reflected in props */
> > +    s390_update_cpu_props(ms, cpu);
> > +
> > +    /* Advertise the topology change */
> > +    s390_cpu_topology_set_modified();
> > +}
> > +
> > +void qmp_x_set_cpu_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)
> > +{
> > +    ERRP_GUARD();
> > +
> > +    if (!s390_has_topology()) {
> > +        error_setg(errp, "This machine doesn't support topology");
> > +        return;
> > +    }
> > +    if (!has_polarity) {
> > +        polarity = POLARITY_VERTICAL_MEDIUM;
> > +    }
> > +    if (!has_dedicated) {
> > +        dedicated = false;
> > +    }
> > +    s390_change_topology(core, socket, book, drawer, polarity, dedicated, 
> > errp);
> > +}
> > +
> > +void hmp_x_set_cpu_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_x_set_cpu_topology(core, socket, book, drawer,
> > +                           has_polarity, polarity,
> > +                           has_dedicated, dedicated,
> > +                           &local_err);
> > +    if (hmp_handle_error(mon, local_err)) {
> > +        return;
> > +    }
>
> What is the if for? The function ends anyway.

Right, I take it away.
Thanks.

Regards,
Pierre

--
Pierre Morel
IBM Lab Boeblingen