| Message ID | 20230201132051.126868-9-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: CPU Topology | expand |
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? > 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? > +# @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! > +{ '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. > + 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? > + /* 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 > + .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
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
On 2/1/23 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 > 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 > +# @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', > + '*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; > + if (!cpu) { > + error_setg(errp, "Core-id %ld does not exist!", core_id); > + return; > + } > + > + /* Verify the new topology */ !! Should verify the new topology but ... > + s390_topology_check(cpu, errp); ... verifies the old one => I will have to change this. Sorry, this will impact the patch 2 because I think I should modify the s390_topology_check() function to take as arguments the individual attributes. Like: static void s390_topology_check( uint16_t core_id, uint16_t socket_id, uint16_t book_id, uint16_t drawer_id, uint16_t entitlement, bool dedicated, Error **errp) Regards, Pierre
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. > +# @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. > + '*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. > 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? > + > + /* 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. > +} > 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", > + .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
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
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 +# @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', + '*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; + 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; + + /* 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", + .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
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(+)