On 04.03.20 12:42, Janosch Frank wrote: > Migration is not yet supported. > > Signed-off-by: Janosch Frank <fran...@linux.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 33 ++++++++++++++++++++++++--------- > 1 file changed, 24 insertions(+), 9 deletions(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index dd39890f89..272531a9ee 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -43,6 +43,9 @@ > #include "sysemu/sysemu.h" > #include "hw/s390x/pv.h" > #include <linux/kvm.h> > +#include "migration/blocker.h" > + > +static Error *pv_mig_blocker; > > S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) > { > @@ -324,19 +327,30 @@ static void s390_machine_unprotect(S390CcwMachineState > *ms) > { > CPUState *t; > > - if (!ms->pv) > - return; > - s390_pv_vm_disable(); > - CPU_FOREACH(t) { > - S390_CPU(t)->env.pv = false; > + if (ms->pv) { > + s390_pv_vm_disable(); > + CPU_FOREACH(t) { > + S390_CPU(t)->env.pv = false; > + } > + ms->pv = false; > } > - ms->pv = false; > + migrate_del_blocker(pv_mig_blocker);
Is it just me or is this unnecessary code churn that wants to challenge my reviewing capabilities? ;) Please perform that rework in the previous patch already. Also, I do wonder why the blocker is always removed (IOW, how we could end up unprotecting, although there is nothing to unprotect). This needs a comment in the patch description. > } > > static int s390_machine_protect(S390CcwMachineState *ms) > { > + static Error *local_err; > CPUState *t; > - int rc; > + int rc = -1; > + > + if (!pv_mig_blocker) { I think we should instead error_free() when unprotecting ... > + error_setg(&pv_mig_blocker, > + "protected VMs are currently not migrateable."); > + } > + migrate_add_blocker(pv_mig_blocker, &local_err); > + if (local_err) { > + goto out_err; ... and on the error path. > + } > > /* Create SE VM */ > rc = s390_pv_vm_enable(); > @@ -440,11 +454,12 @@ static void s390_machine_reset(MachineState *machine) > > if (s390_machine_protect(ms)) { > s390_machine_inject_pv_error(cs); ew, injecting an exception during a reset. (I know this belongs into the previous patch) > - s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); > - return; > + goto pv_err; I have no idea why this hunk is in this patch. What am I missing? > } > > run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL); > +pv_err: > + s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); > break; > default: > g_assert_not_reached(); > -- Thanks, David / dhildenb