| Message ID | 20220902075531.188916-8-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: CPU Topology | expand |
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() > + } > +}; > + [...]
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
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
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)) { + 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() + } +}; + /** * topology_class_init: * @oc: Object class @@ -146,6 +224,7 @@ static void topology_class_init(ObjectClass *oc, void *data) dc->realize = s390_topology_realize; set_bit(DEVICE_CATEGORY_MISC, dc->categories); dc->reset = s390_topology_reset; + dc->vmsd = &vmstate_cpu_topology; } static const TypeInfo cpu_topology_info = { diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h index 4f8ac39ca0..a15567baca 100644 --- a/include/hw/s390x/cpu-topology.h +++ b/include/hw/s390x/cpu-topology.h @@ -29,6 +29,7 @@ typedef struct S390TopoTLE { struct S390Topology { SysBusDevice parent_obj; + bool topology_needed; int total_books; int total_sockets; int drawers; diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c index 707c0b658c..78cb11c0f8 100644 --- a/target/s390x/cpu-sysemu.c +++ b/target/s390x/cpu-sysemu.c @@ -313,3 +313,11 @@ void s390_cpu_topology_reset(void) kvm_s390_topology_set_mtcr(0); } } + +int s390_cpu_topology_mtcr_set(void) +{ + if (kvm_enabled()) { + return kvm_s390_topology_set_mtcr(1); + } + return -ENOENT; +} diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index cff5cc0415..0eb2d219d3 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -827,6 +827,7 @@ void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg); int s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch_id, int vq, bool assign); void s390_cpu_topology_reset(void); +int s390_cpu_topology_mtcr_set(void); #ifndef CONFIG_USER_ONLY unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu); #else
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. 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(+)