Re: [PATCH v14 08/11] qapi/s390/cpu topology: change-topology monitor command

[Thomas Huth]

On 16/01/2023 22.09, Nina Schoetterl-Glausch wrote:
> On Thu, 2023-01-05 at 15:53 +0100, Pierre Morel wrote:
> > The modification of the CPU attributes are done through a monitor
> > commands.
> >
> > It allows to move the core inside the topology tree to optimise
> > the cache usage in the case the host's hypervizor previously
> > moved the CPU.
> >
> > The same command allows to modifiy 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.
> >
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
...
> > diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> > index b69955a1cd..0faffe657e 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.
> > @@ -203,6 +207,21 @@ static void s390_topology_set_entry(S390TopologyEntry 
> > *entry,
> >       s390_topology.sockets[s390_socket_nb(id)]++;
> >   }
> >   
> > +/**
> > + * s390_topology_clear_entry:
> > + * @entry: Topology entry to setup
> > + * @id: topology id to use for the setup
> > + *
> > + * Clear the core bit inside the topology mask and
> > + * decrements the number of cores for the socket.
> > + */
> > +static void s390_topology_clear_entry(S390TopologyEntry *entry,
> > +                                      s390_topology_id id)
> > +{
> > +    clear_bit(63 - id.core, &entry->mask);
>
> This doesn't take the origin into account.
>
> > +    s390_topology.sockets[s390_socket_nb(id)]--;
>
> I suppose this function cannot run concurrently, so the same CPU doesn't get 
> removed twice.

QEMU has the so-called BQL - the Big Qemu Lock. Instructions handlers are 
normally called with the lock taken, see qemu_mutex_lock_iothread() in 
target/s390x/kvm/kvm.c.

(if you feel unsure, you can add a "assert(qemu_mutex_iothread_locked())" 
statement, but I think it is not necessary here)

 Thomas