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

[Thomas Huth <thuth@redhat.com>]

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