| Message ID | 20230222142105.84700-9-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: CPU Topology | expand |
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
On 24/02/2023 18.15, Nina Schoetterl-Glausch wrote: > 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? I'm not sure whether double inclusion works with the QAPI parser (since this might code to be generated twice) ... have you tried? Thomas
/o\ Sorry, I changed the HMP arguments in the C code from core.. to core-id.. and forgot to change them inside the hmp-command.hx file. Also I need to extend the info hotpluggable-cpus. I will provide alternate patches with these two corrections. Regards, Pierre 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', > + '*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 */ > + TOPOLOGY_SET(entitlement); > + TOPOLOGY_SET(dedicated); > + TOPOLOGY_SET(socket_id); > + TOPOLOGY_SET(book_id); > + TOPOLOGY_SET(drawer_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; > + > + 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); > + 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?", > + .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
On Mon, 2023-02-27 at 08:59 +0100, Thomas Huth wrote: > On 24/02/2023 18.15, Nina Schoetterl-Glausch wrote: > > 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? > > I'm not sure whether double inclusion works with the QAPI parser (since this > might code to be generated twice) ... have you tried? I haven't, the documentation says: > Include directives > ------------------ > > Syntax:: > > INCLUDE = { 'include': STRING } > > The QAPI schema definitions can be modularized using the 'include' directive:: > > { 'include': 'path/to/file.json' } > > The directive is evaluated recursively, and include paths are relative > to the file using the directive. Multiple includes of the same file > are idempotent. Which is why I thought it should work, but I guess this is a statement about including the same file twice in another file and not about including the same file from two files. But then, as far as I can tell, the build system only builds qapi-schema.json, which includes all other files, so it could apply. > > > Thomas >
On 2/24/23 18:15, Nina Schoetterl-Glausch wrote: > 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? Seems Thomas has questions, I wait until every body agree or not agree. Anyway this does not change the interface or the functionality. > >> + '*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... Thanks, I will change this. > >> + 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; Looks better > >> + >> + 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. IMHO it does not, the polarization is set by the guest through the PTF instruction, but entitlement is set by the host. > >> + >> + 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. right, I should have kept the original names or change both. > >> + 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. Yes, or we can let fall the hmp interface for this series, making it simpler, and add the hmp interface later. I am more in favor of letting it fall for now. Regards, Pierre > >> + .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
On 27/02/2023 11.57, Pierre Morel wrote: > > On 2/24/23 18:15, Nina Schoetterl-Glausch wrote: >> 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? > > Seems Thomas has questions, I wait until every body agree or not agree. I'd be fine with such a change if it works ... I just got no clue whether it works or not, so you've got to try it, I guess. But I think I'd rather avoid naming the file "machine-common.json" ... "machine.json" is already supposed to be the common code that can be shared between all targets, so having a "machine-common.json" file would be super confusing, I think. OTOH, what's the reason again for having CpuS390Entitlement in machine.json? Couldn't it be moved to machine-target.json instead? Thomas
On Mon, 2023-02-27 at 11:57 +0100, Pierre Morel wrote: > On 2/24/23 18:15, Nina Schoetterl-Glausch wrote: > > 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/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 @@ > > > [...] > > > + > > > +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. > > > IMHO it does not, the polarization is set by the guest through the PTF > instruction, but entitlement is set by the host. Yes, so when the guests requests vertical polarization, all cpus have a (vertical) entitlement assigned and will show up as vertical in STSI. But now, by using the qmp command, the polarization can be reset to horizontal, even though the guest didn't ask for 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. > > > right, I should have kept the original names or change both. > > > > > > > + 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. > > > Yes, or we can let fall the hmp interface for this series, making it > simpler, and add the hmp interface later. > > I am more in favor of letting it fall for now. Fine by me. > > > Regards, > > Pierre > > > > > > > + .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
Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: > On Mon, 2023-02-27 at 08:59 +0100, Thomas Huth wrote: [...] >> I'm not sure whether double inclusion works with the QAPI parser (since this >> might code to be generated twice) ... have you tried? > > I haven't, the documentation says: > >> Include directives >> ------------------ >> >> Syntax:: >> >> INCLUDE = { 'include': STRING } >> >> The QAPI schema definitions can be modularized using the 'include' directive:: >> >> { 'include': 'path/to/file.json' } >> >> The directive is evaluated recursively, and include paths are relative >> to the file using the directive. Multiple includes of the same file >> are idempotent. > > Which is why I thought it should work, but I guess this is a statement about > including the same file twice in another file and not about including the same > file from two files. No, this is intended to say multiple inclusion is fine, regardless where the include directives are. An include directive has two effects: 1. If the included file has not been included already, pull in its contents. 2. Insert #include in generated C. Example: qdev.json includes qom.json. The generated qapi-*-qdev.h include qapi-types-qom.h. Including any required modules, as recommended by qapi-code-gen.rst, results in properly self-contained generated headers. > But then, as far as I can tell, the build system only builds qapi-schema.json, > which includes all other files, so it could apply. Yes, qapi-schema.json is the main module, which includes all the others. In fact, it includes all the others *directly*. Why? We generate documentation in source order. Included material gets inserted right at the first inclusion; subsequent inclusions have no effect. If we put all first inclusions right into qapi-schema.json, the order of things in documentation is visible right there, and won't change just because we change inclusions deeper down. Questions?
On Mon, 2023-02-27 at 13:25 +0100, Markus Armbruster wrote: > Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: > > > On Mon, 2023-02-27 at 08:59 +0100, Thomas Huth wrote: > > [...] > > > > I'm not sure whether double inclusion works with the QAPI parser (since this > > > might code to be generated twice) ... have you tried? > > > > I haven't, the documentation says: > > > > > Include directives > > > ------------------ > > > > > > Syntax:: > > > > > > INCLUDE = { 'include': STRING } > > > > > > The QAPI schema definitions can be modularized using the 'include' directive:: > > > > > > { 'include': 'path/to/file.json' } > > > > > > The directive is evaluated recursively, and include paths are relative > > > to the file using the directive. Multiple includes of the same file > > > are idempotent. > > > > Which is why I thought it should work, but I guess this is a statement about > > including the same file twice in another file and not about including the same > > file from two files. > > No, this is intended to say multiple inclusion is fine, regardless where > the include directives are. > > An include directive has two effects: > > 1. If the included file has not been included already, pull in its > contents. > > 2. Insert #include in generated C. Example: qdev.json includes > qom.json. The generated qapi-*-qdev.h include qapi-types-qom.h. > > Including any required modules, as recommended by qapi-code-gen.rst, > results in properly self-contained generated headers. Ok, thanks. Not sure if another phrasing would be better given the intended meaning is the way I read it initially. > > > But then, as far as I can tell, the build system only builds qapi-schema.json, > > which includes all other files, so it could apply. > > Yes, qapi-schema.json is the main module, which includes all the others. > > In fact, it includes all the others *directly*. Why? > > We generate documentation in source order. Included material gets > inserted right at the first inclusion; subsequent inclusions have no > effect. > > If we put all first inclusions right into qapi-schema.json, the order of > things in documentation is visible right there, and won't change just > because we change inclusions deeper down. > > Questions? CpuS390Entitlement would be useful in both machine.json and machine-target.json because query-cpu-fast is defined in machine.json and set-cpu-topology is defined in machine-target.json. So then the question is where best to define CpuS390Entitlement. In machine.json and include machine.json in machine-target.json? Or define it in another file and include it from both? Thanks
On 2/27/23 13:15, Nina Schoetterl-Glausch wrote: > On Mon, 2023-02-27 at 11:57 +0100, Pierre Morel wrote: >> On 2/24/23 18:15, Nina Schoetterl-Glausch wrote: >>> 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/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 @@ >>>> > [...] >>>> + >>>> +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. >> >> IMHO it does not, the polarization is set by the guest through the PTF >> instruction, but entitlement is set by the host. > Yes, so when the guests requests vertical polarization, all cpus have a > (vertical) entitlement assigned and will show up as vertical in STSI. > But now, by using the qmp command, the polarization can be reset to horizontal, > even though the guest didn't ask for it. Right, I will change this Regards, Pierre
Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: > CpuS390Entitlement would be useful in both machine.json and machine-target.json > because query-cpu-fast is defined in machine.json and set-cpu-topology is defined > in machine-target.json. > So then the question is where best to define CpuS390Entitlement. > In machine.json and include machine.json in machine-target.json? > Or define it in another file and include it from both? Either should work. If you include machine.json into machine-target.json, the qapi-FOO-machine-target.h will include the qapi-FOO-machine.h. No big deal, I think: affects just eight .c.
On 2/24/23 18:15, Nina Schoetterl-Glausch wrote: > On Wed, 2023-02-22 at 15:21 +0100, Pierre Morel wrote: [...] >> 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. I think it is better if let fall the hmp interface for the moment and will send it later in another series. Regards, Pierre
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', + '*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 */ + TOPOLOGY_SET(entitlement); + TOPOLOGY_SET(dedicated); + TOPOLOGY_SET(socket_id); + TOPOLOGY_SET(book_id); + TOPOLOGY_SET(drawer_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; + + 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); + 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?", + .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
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(+)