Re: [PATCH v9 07/10] s390x/cpu_topology: CPU topology migration

[Pierre Morel <pmorel@linux.ibm.com>]

On 9/28/22 10:34, Pierre Morel wrote:
> 
> 
> On 9/8/22 20:04, Janis Schoetterl-Glausch wrote:
>> On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
>>> The migration can only take place if both source and destination
>>> of the migration both use or both do not use the CPU topology
>>> facility.
>>>
>>> We indicate a change in topology during migration postload for the
>>> case the topology changed between source and destination.
>>
>> You always set the report bit after migration, right?
>> In the last series you actually migrated the bit.
>> Why the change? With the code you have actually migrating the bit isn't
>> hard.
> 
> As for the moment the vCPU do not migrate from real CPU I thought based 
> on the remark of Nico that there is no need to set the bit after a 
> migration.
> 
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   hw/s390x/cpu-topology.c         | 79 +++++++++++++++++++++++++++++++++
>>>   include/hw/s390x/cpu-topology.h |  1 +
>>>   target/s390x/cpu-sysemu.c       |  8 ++++
>>>   target/s390x/cpu.h              |  1 +
>>>   4 files changed, 89 insertions(+)
>>>
>>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>>> index 6098d6ea1f..b6bf839e40 100644
>>> --- a/hw/s390x/cpu-topology.c
>>> +++ b/hw/s390x/cpu-topology.c
>>> @@ -19,6 +19,7 @@
>>>   #include "target/s390x/cpu.h"
>>>   #include "hw/s390x/s390-virtio-ccw.h"
>>>   #include "hw/s390x/cpu-topology.h"
>>> +#include "migration/vmstate.h"
>>>   S390Topology *s390_get_topology(void)
>>>   {
>>> @@ -132,6 +133,83 @@ static void s390_topology_reset(DeviceState *dev)
>>>       s390_cpu_topology_reset();
>>>   }
>>> +/**
>>> + * cpu_topology_postload
>>> + * @opaque: a pointer to the S390Topology
>>> + * @version_id: version identifier
>>> + *
>>> + * We check that the topology is used or is not used
>>> + * on both side identically.
>>> + *
>>> + * If the topology is in use we set the Modified Topology Change Report
>>> + * on the destination host.
>>> + */
>>> +static int cpu_topology_postload(void *opaque, int version_id)
>>> +{
>>> +    S390Topology *topo = opaque;
>>> +    int ret;
>>> +
>>> +    if (topo->topology_needed != 
>>> s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
>>
>> Does this function even run if topology_needed is false?
>> In that case there is no data saved, so no reason to load it either.
>> If so you can only check that both the source and the destination have
>> the feature enabled. You would need to always send the topology VMSD in
>> order to check that the feature is disabled.
>>
>> Does qemu allow you to attempt to migrate to a host with another cpu
>> model?
>> If it disallowes that you wouldn't need to do any checks, right?
> 
> hum yes I must rework this
> 
> Thanks,
> Pierre
> 
> 

I think my answer was wrong but it helped to correct a bug:
- Only machines > 7.x will be able to use topology and it is correct 
that migration to another machine is not possible.

- However we keep the possibility to not use the topology in 7.x and we 
are not able to migrate if one end use the topology and the other do not.

So I think cpu_topology_needed() must return true (no test needed) but 
the implementation of presave and postload are OK.

Regards,
Pierre



-- 
Pierre Morel
IBM Lab Boeblingen