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

[Pierre Morel <pmorel@linux.ibm.com>]

On 2/6/23 13:21, Thomas Huth wrote:
> On 01/02/2023 14.20, 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
> 
> I'm not a native speaker, but "optimize" is more common, I think?

Yes. I will stick on it.

> 
>> 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
>> +# @polarity: optional polarity, default is last polarity set by the 
>> guest
> 
> Hmm, below you do something like that:
> 
>      if (!has_polarity) {
>          polarity = POLARITY_VERTICAL_MEDIUM;
>      } >
> ... so it rather seems like medium polarity is the default? ... that 
> looks weird. I think you should maybe rather pass the has_polarity and 
> has_dedication flags to the s390_change_topology() function and only 
> change the value if they have really been provided?

OK, I will change this incoherence.

> 
>> +# @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>
> 
> Please replace with "Since: 8.0" ... I hope we'll get it merged during 
> this cycle!

OK. :)

> 
>> +{ 'command': 'x-set-cpu-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 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;
> 
> I think you should add a sanity check for core_id being in a valid range 
> ... otherwise this will cause an access beyond the end of the array if 
> core_id is too big or negative.

Right, I will also change the int64_t for the declaration of the core, 
socket etc to uint16_t so avoiding a check for negative values.

> 
>> +    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);
>> +
>> +    /* 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;
> 
> As mentioned above, I think dedicated and polarity should only be 
> changed if they have really been provided?

right, I do this.

> 
>> +    /* 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;
>> +    }
>> +}
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 673e39a697..bb3c908356 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       = "x-set-cpu-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",
> 
> optionaly ==> optionally

OK

> 
>> +        .cmd        = hmp_x_set_cpu_topology,
>> +    },
>> +
>> +SRST
>> +``x-set-cpu-topology`` *core* *socket* *book* *drawer* *polarity* 
>> *dedicated*
>> +  Moves the CPU  *core* to *socket* *book* *drawer* with *polarity* 
>> *dedicated*.
>> +ERST
>> +#endif
> 
>   Thomas
> 

Thanks,

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen