Re: [PATCH v21 08/20] qapi/s390x/cpu topology: set-cpu-topology qmp command

[Pierre Morel <pmorel@xxxxxxxxxxxxx>]

On 7/4/23 14:53, Thomas Huth wrote:
> On 30/06/2023 11.17, 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@xxxxxxxxxxxxx>
> > ---
> ...
> > diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> > index 1af70a5049..dfc4cb42a4 100644
> > --- a/hw/s390x/cpu-topology.c
> > +++ b/hw/s390x/cpu-topology.c
> > @@ -23,6 +23,7 @@
> >   #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"
> >     /*
> >    * s390_topology is used to keep the topology information.
> > @@ -260,6 +261,29 @@ static bool s390_topology_check(uint16_t 
> > socket_id, uint16_t book_id,
> >       return true;
> >   }
> >   +/**
> > + * s390_topology_need_report
> > + * @cpu: Current cpu
> > + * @drawer_id: future drawer ID
> > + * @book_id: future book ID
> > + * @socket_id: future socket ID
> > + * @entitlement: future entitlement
> > + * @dedicated: future dedicated
> > + *
> > + * A modified topology change report is needed if the topology
> > + * tree or the topology attributes change.
> > + */
> > +static bool 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
> > @@ -329,3 +353,113 @@ void s390_topology_setup_cpu(MachineState *ms, 
> > S390CPU *cpu, Error **errp)
> >       /* topology tree is reflected in props */
> >       s390_update_cpu_props(ms, cpu);
> >   }
> > +
> > +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,
> > +                                 CpuS390Entitlement entitlement,
> > +                                 bool has_dedicated, bool dedicated,
> > +                                 Error **errp)
> > +{
> > +    MachineState *ms = current_machine;
> > +    int old_socket_entry;
> > +    int new_socket_entry;
> > +    bool report_needed;
> > +    S390CPU *cpu;
> > +    ERRP_GUARD();
> > +
> > +    cpu = s390_cpu_addr2state(core_id);
> > +    if (!cpu) {
> > +        error_setg(errp, "Core-id %d does not exist!", core_id);
> > +        return;
> > +    }
> > +
> > +    /* Get attributes not provided from cpu and verify the new 
> > topology */
> > +    if (!has_socket_id) {
> > +        socket_id = cpu->env.socket_id;
> > +    }
> > +    if (!has_book_id) {
> > +        book_id = cpu->env.book_id;
> > +    }
> > +    if (!has_drawer_id) {
> > +        drawer_id = cpu->env.drawer_id;
> > +    }
> > +    if (!has_dedicated) {
> > +        dedicated = cpu->env.dedicated;
> > +    }
> > +
> > +    /*
> > +     * When the user specifies the entitlement as 'auto' on the 
> > command line,
> > +     * qemu will set the entitlement as:
> > +     * Medium when the CPU is not dedicated.
> > +     * High when dedicated is true.
> > +     */
> > +    if (!has_entitlement || (entitlement == 
> > S390_CPU_ENTITLEMENT_AUTO)) {
> > +        if (dedicated) {
> > +            entitlement = S390_CPU_ENTITLEMENT_HIGH;
> > +        } else {
> > +            entitlement = S390_CPU_ENTITLEMENT_MEDIUM;
> > +        }
> > +    }
> > +
> > +    if (!s390_topology_check(socket_id, book_id, drawer_id,
> > +                             entitlement, dedicated, errp))
> > +        return;
>
> Missing curly braces here.
> Not sure why this isn't caught by checkpatch.pl properly :-(
>
>  Thomas
Yes indeed.

I add the curly braces.

Thanks

Pierre