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

[Nina Schoetterl-Glausch]

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