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

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

On Tue, 2023-01-17 at 08:30 +0100, Thomas Huth wrote:
> 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.

That is good to know, but is that the relevant lock here?
We don't want to concurrent qmp commands. I looked at the code and it's pretty complicated.
> 
> (if you feel unsure, you can add a "assert(qemu_mutex_iothread_locked())" 
> statement, but I think it is not necessary here)
> 
>   Thomas
>