| Message ID | 20200214151636.8764-7-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: Protected Virtualization support | expand |
On Fri, 14 Feb 2020 10:16:25 -0500 Janosch Frank <frankja@linux.ibm.com> wrote: > Migration is not yet supported. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 5fa4372083..d64724af91 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -42,6 +42,9 @@ > #include "hw/s390x/tod.h" > #include "sysemu/sysemu.h" > #include "hw/s390x/pv.h" > +#include "migration/blocker.h" > + > +static Error *pv_mig_blocker; > > S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) > { > @@ -373,6 +376,7 @@ static void s390_machine_reset(MachineState *machine) > CPUState *cs, *t; > S390CPU *cpu; > S390CcwMachineState *ms = S390_CCW_MACHINE(machine); > + static Error *local_err; > > /* get the reset parameters, reset them once done */ > s390_ipl_get_reset_request(&cs, &reset_type); > @@ -422,6 +426,17 @@ static void s390_machine_reset(MachineState *machine) > } > run_on_cpu(cs, s390_do_cpu_reset, RUN_ON_CPU_NULL); > > + if (!pv_mig_blocker) { > + error_setg(&pv_mig_blocker, > + "protected VMs are currently not migrateable."); > + } > + migrate_add_blocker(pv_mig_blocker, &local_err); If I'm not lost in the context, that's during PV_RESET. I'm a bit confused why you'd add the blocker here? > + if (local_err) { > + error_report_err(local_err); > + error_free(pv_mig_blocker); > + exit(1); Why the exit()? Can't you fail the call? > + } > + > if (s390_machine_pv_secure(ms)) { > CPU_FOREACH(t) { > s390_pv_vcpu_destroy(t); > @@ -430,6 +445,7 @@ static void s390_machine_reset(MachineState *machine) > ms->pv = false; > > s390_machine_inject_pv_error(cs); > + migrate_del_blocker(pv_mig_blocker); > s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); > return; > }
On 2/20/20 11:48 AM, Cornelia Huck wrote: > On Fri, 14 Feb 2020 10:16:25 -0500 > Janosch Frank <frankja@linux.ibm.com> wrote: > >> Migration is not yet supported. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> hw/s390x/s390-virtio-ccw.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index 5fa4372083..d64724af91 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -42,6 +42,9 @@ >> #include "hw/s390x/tod.h" >> #include "sysemu/sysemu.h" >> #include "hw/s390x/pv.h" >> +#include "migration/blocker.h" >> + >> +static Error *pv_mig_blocker; >> >> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) >> { >> @@ -373,6 +376,7 @@ static void s390_machine_reset(MachineState *machine) >> CPUState *cs, *t; >> S390CPU *cpu; >> S390CcwMachineState *ms = S390_CCW_MACHINE(machine); >> + static Error *local_err; >> >> /* get the reset parameters, reset them once done */ >> s390_ipl_get_reset_request(&cs, &reset_type); >> @@ -422,6 +426,17 @@ static void s390_machine_reset(MachineState *machine) >> } >> run_on_cpu(cs, s390_do_cpu_reset, RUN_ON_CPU_NULL); >> >> + if (!pv_mig_blocker) { >> + error_setg(&pv_mig_blocker, >> + "protected VMs are currently not migrateable."); >> + } >> + migrate_add_blocker(pv_mig_blocker, &local_err); > > If I'm not lost in the context, that's during PV_RESET. I'm a bit > confused why you'd add the blocker here? Where would you want me to add it? It's here where we switch into secure mode and I need to block before switching and unblock if it fails. When having the blocker in diag.c, I'd have a hard time unblocking on a PV switch fail. > >> + if (local_err) { >> + error_report_err(local_err); >> + error_free(pv_mig_blocker); >> + exit(1); > > Why the exit()? Can't you fail the call? Well, if that fails and we go protected, I wouldn't be protected agains migrations, right? > >> + } >> + >> if (s390_machine_pv_secure(ms)) { >> CPU_FOREACH(t) { >> s390_pv_vcpu_destroy(t); >> @@ -430,6 +445,7 @@ static void s390_machine_reset(MachineState *machine) >> ms->pv = false; >> >> s390_machine_inject_pv_error(cs); >> + migrate_del_blocker(pv_mig_blocker); >> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); >> return; >> } > >
On Thu, 20 Feb 2020 12:24:23 +0100 Janosch Frank <frankja@linux.ibm.com> wrote: > On 2/20/20 11:48 AM, Cornelia Huck wrote: > > On Fri, 14 Feb 2020 10:16:25 -0500 > > Janosch Frank <frankja@linux.ibm.com> wrote: > > > >> Migration is not yet supported. > >> > >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > >> --- > >> hw/s390x/s390-virtio-ccw.c | 16 ++++++++++++++++ > >> 1 file changed, 16 insertions(+) > >> > >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > >> index 5fa4372083..d64724af91 100644 > >> --- a/hw/s390x/s390-virtio-ccw.c > >> +++ b/hw/s390x/s390-virtio-ccw.c > >> @@ -42,6 +42,9 @@ > >> #include "hw/s390x/tod.h" > >> #include "sysemu/sysemu.h" > >> #include "hw/s390x/pv.h" > >> +#include "migration/blocker.h" > >> + > >> +static Error *pv_mig_blocker; > >> > >> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) > >> { > >> @@ -373,6 +376,7 @@ static void s390_machine_reset(MachineState *machine) > >> CPUState *cs, *t; > >> S390CPU *cpu; > >> S390CcwMachineState *ms = S390_CCW_MACHINE(machine); > >> + static Error *local_err; > >> > >> /* get the reset parameters, reset them once done */ > >> s390_ipl_get_reset_request(&cs, &reset_type); > >> @@ -422,6 +426,17 @@ static void s390_machine_reset(MachineState *machine) > >> } > >> run_on_cpu(cs, s390_do_cpu_reset, RUN_ON_CPU_NULL); > >> > >> + if (!pv_mig_blocker) { > >> + error_setg(&pv_mig_blocker, > >> + "protected VMs are currently not migrateable."); > >> + } > >> + migrate_add_blocker(pv_mig_blocker, &local_err); > > > > If I'm not lost in the context, that's during PV_RESET. I'm a bit > > confused why you'd add the blocker here? > > Where would you want me to add it? > It's here where we switch into secure mode and I need to block before > switching and unblock if it fails. > > When having the blocker in diag.c, I'd have a hard time unblocking on a > PV switch fail. > > > > >> + if (local_err) { > >> + error_report_err(local_err); > >> + error_free(pv_mig_blocker); > >> + exit(1); > > > > Why the exit()? Can't you fail the call? > > Well, if that fails and we go protected, I wouldn't be protected agains > migrations, right? No, I meant not go protected, if that's possible. > > > > >> + } > >> + > >> if (s390_machine_pv_secure(ms)) { Ok, I think what confuses me is this call: it reads as if you actually tear down things if the machine is secure. Call it s390_machine_pv_make_secure() to make sure it is actively doing something and not checking a previously set value? > >> CPU_FOREACH(t) { > >> s390_pv_vcpu_destroy(t); > >> @@ -430,6 +445,7 @@ static void s390_machine_reset(MachineState *machine) > >> ms->pv = false; > >> > >> s390_machine_inject_pv_error(cs); > >> + migrate_del_blocker(pv_mig_blocker); > >> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); > >> return; > >> } > > > > > >
On 2/20/20 12:39 PM, Cornelia Huck wrote: > On Thu, 20 Feb 2020 12:24:23 +0100 > Janosch Frank <frankja@linux.ibm.com> wrote: > >> On 2/20/20 11:48 AM, Cornelia Huck wrote: >>> On Fri, 14 Feb 2020 10:16:25 -0500 >>> Janosch Frank <frankja@linux.ibm.com> wrote: >>> >>>> Migration is not yet supported. >>>> >>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>> --- >>>> hw/s390x/s390-virtio-ccw.c | 16 ++++++++++++++++ >>>> 1 file changed, 16 insertions(+) >>>> >>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >>>> index 5fa4372083..d64724af91 100644 >>>> --- a/hw/s390x/s390-virtio-ccw.c >>>> +++ b/hw/s390x/s390-virtio-ccw.c >>>> @@ -42,6 +42,9 @@ >>>> #include "hw/s390x/tod.h" >>>> #include "sysemu/sysemu.h" >>>> #include "hw/s390x/pv.h" >>>> +#include "migration/blocker.h" >>>> + >>>> +static Error *pv_mig_blocker; >>>> >>>> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) >>>> { >>>> @@ -373,6 +376,7 @@ static void s390_machine_reset(MachineState *machine) >>>> CPUState *cs, *t; >>>> S390CPU *cpu; >>>> S390CcwMachineState *ms = S390_CCW_MACHINE(machine); >>>> + static Error *local_err; >>>> >>>> /* get the reset parameters, reset them once done */ >>>> s390_ipl_get_reset_request(&cs, &reset_type); >>>> @@ -422,6 +426,17 @@ static void s390_machine_reset(MachineState *machine) >>>> } >>>> run_on_cpu(cs, s390_do_cpu_reset, RUN_ON_CPU_NULL); >>>> >>>> + if (!pv_mig_blocker) { >>>> + error_setg(&pv_mig_blocker, >>>> + "protected VMs are currently not migrateable."); >>>> + } >>>> + migrate_add_blocker(pv_mig_blocker, &local_err); >>> >>> If I'm not lost in the context, that's during PV_RESET. I'm a bit >>> confused why you'd add the blocker here? >> >> Where would you want me to add it? >> It's here where we switch into secure mode and I need to block before >> switching and unblock if it fails. >> >> When having the blocker in diag.c, I'd have a hard time unblocking on a >> PV switch fail. >> >>> >>>> + if (local_err) { >>>> + error_report_err(local_err); >>>> + error_free(pv_mig_blocker); >>>> + exit(1); >>> >>> Why the exit()? Can't you fail the call? >> >> Well, if that fails and we go protected, I wouldn't be protected agains >> migrations, right? > > No, I meant not go protected, if that's possible. That would be an option, now that we have a proper d308 rc for such a thing. Will add! > >> >>> >>>> + } >>>> + >>>> if (s390_machine_pv_secure(ms)) { > > Ok, I think what confuses me is this call: it reads as if you actually > tear down things if the machine is secure. Call it > s390_machine_pv_make_secure() to make sure it is actively doing > something and not checking a previously set value? Ok, will use something along these lines > >>>> CPU_FOREACH(t) { >>>> s390_pv_vcpu_destroy(t); >>>> @@ -430,6 +445,7 @@ static void s390_machine_reset(MachineState *machine) >>>> ms->pv = false; >>>> >>>> s390_machine_inject_pv_error(cs); >>>> + migrate_del_blocker(pv_mig_blocker); >>>> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); >>>> return; >>>> } >>> >>> >> >> >
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 5fa4372083..d64724af91 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -42,6 +42,9 @@ #include "hw/s390x/tod.h" #include "sysemu/sysemu.h" #include "hw/s390x/pv.h" +#include "migration/blocker.h" + +static Error *pv_mig_blocker; S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) { @@ -373,6 +376,7 @@ static void s390_machine_reset(MachineState *machine) CPUState *cs, *t; S390CPU *cpu; S390CcwMachineState *ms = S390_CCW_MACHINE(machine); + static Error *local_err; /* get the reset parameters, reset them once done */ s390_ipl_get_reset_request(&cs, &reset_type); @@ -422,6 +426,17 @@ static void s390_machine_reset(MachineState *machine) } run_on_cpu(cs, s390_do_cpu_reset, RUN_ON_CPU_NULL); + if (!pv_mig_blocker) { + error_setg(&pv_mig_blocker, + "protected VMs are currently not migrateable."); + } + migrate_add_blocker(pv_mig_blocker, &local_err); + if (local_err) { + error_report_err(local_err); + error_free(pv_mig_blocker); + exit(1); + } + if (s390_machine_pv_secure(ms)) { CPU_FOREACH(t) { s390_pv_vcpu_destroy(t); @@ -430,6 +445,7 @@ static void s390_machine_reset(MachineState *machine) ms->pv = false; s390_machine_inject_pv_error(cs); + migrate_del_blocker(pv_mig_blocker); s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); return; }
Migration is not yet supported. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- hw/s390x/s390-virtio-ccw.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)