| Message ID | 20200304114231.23493-5-frankja@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: Protected Virtualization support | expand |
On 04.03.20 12:42, Janosch Frank wrote: > Migration is not yet supported. > > Signed-off-by: Janosch Frank <frankja@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(); >
On 3/4/20 6:13 PM, David Hildenbrand wrote: > On 04.03.20 12:42, Janosch Frank wrote: >> Migration is not yet supported. >> >> Signed-off-by: Janosch Frank <frankja@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. It's rather more of personal style choice to do a "if return" to not encapsulate everything in ifs. > > 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. You mean the error check for migrate_add_blocker()? I'm still metabolizing my coffee... > > >> } >> >> 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 ... Ack > >> + 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. To also return the rc code, I changed it to: rc = migrate_add_blocker(pv_mig_blocker, &local_err); if (local_err) { error_free(pv_mig_blocker); return rc; } > >> + } >> >> /* 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) Yeah, not too proud about this one, but doing it at diag308 time is not possible, since we need to reset the devices before attempting to protect. > >> - 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? The error needs to be communicated to the guest, so we need to resume CPU operation instead of doing a load which we would do on success. > >> } >> >> 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(); >> > >
On 05.03.20 10:16, Janosch Frank wrote: > On 3/4/20 6:13 PM, David Hildenbrand wrote: >> On 04.03.20 12:42, Janosch Frank wrote: >>> Migration is not yet supported. >>> >>> Signed-off-by: Janosch Frank <frankja@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. > > It's rather more of personal style choice to do a "if return" to not > encapsulate everything in ifs. Yes, but please do that in the previous patch instead. This makes review of this patch unnecessary complicated. >> 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. > > You mean the error check for migrate_add_blocker()? > I'm still metabolizing my coffee... Why could we get a call to s390_machine_unprotect() and - have !ms->pv - still need to do a migrate_del_blocker() Not clear to me. >> >>> - 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? > > The error needs to be communicated to the guest, so we need to resume > CPU operation instead of doing a load which we would do on success. > The point I am making: This change should go into the previous patch unless I am missing something important.
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); } static int s390_machine_protect(S390CcwMachineState *ms) { + static Error *local_err; CPUState *t; - int rc; + int rc = -1; + + 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) { + goto out_err; + } /* 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); - s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); - return; + goto pv_err; } 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();
Migration is not yet supported. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- hw/s390x/s390-virtio-ccw.c | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-)