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

[Janis Schoetterl-Glausch]

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.
> 
> 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?

> +        if (topo->topology_needed) {
> +            error_report("Topology facility is needed in destination");
> +        } else {
> +            error_report("Topology facility can not be used in destination");
> +        }
> +        return -EINVAL;
> +    }
> +
> +    /* We do not support CPU Topology, all is good */
> +    if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
> +        return 0;
> +    }
> +
> +    /* We support CPU Topology, set the MTCR */
> +    ret = s390_cpu_topology_mtcr_set();
> +    if (ret) {
> +        error_report("Failed to set MTCR: %s", strerror(-ret));
> +    }
> +    return ret;
> +}
> +
> +/**
> + * cpu_topology_presave:
> + * @opaque: The pointer to the S390Topology
> + *
> + * Save the usage of the CPU Topology in the VM State.
> + */
> +static int cpu_topology_presave(void *opaque)
> +{
> +    S390Topology *topo = opaque;
> +
> +    topo->topology_needed = s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY);
> +    return 0;
> +}
> +
> +/**
> + * cpu_topology_needed:
> + * @opaque: The pointer to the S390Topology
> + *
> + * If we use the CPU Topology on the source it will be needed on the 
> destination.
> + */
> +static bool cpu_topology_needed(void *opaque)
> +{
> +    return s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY);
> +}
> +
> +
> +const VMStateDescription vmstate_cpu_topology = {
> +    .name = "cpu_topology",
> +    .version_id = 1,
> +    .post_load = cpu_topology_postload,
> +    .pre_save = cpu_topology_presave,
> +    .minimum_version_id = 1,
> +    .needed = cpu_topology_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BOOL(topology_needed, S390Topology),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
[...]