| Message ID | 20200309112206.20261-4-frankja@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: Protected Virtualization support | expand |
On 09.03.20 12:21, Janosch Frank wrote: > Migration is not yet supported. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index f718cfc591..4bb38704ff 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) > { > @@ -331,16 +334,33 @@ static void s390_machine_unprotect(S390CcwMachineState *ms) > } > ms->pv = false; > } > + migrate_del_blocker(pv_mig_blocker); > + error_free(pv_mig_blocker); > + pv_mig_blocker = NULL; Maybe use error_free_or_abort(&pv_mig_blocker); > } > > static int s390_machine_protect(S390CcwMachineState *ms) > { > + static Error *local_err; > CPUState *t; > int rc; > > + error_setg(&pv_mig_blocker, > + "protected VMs are currently not migrateable."); > + rc = migrate_add_blocker(pv_mig_blocker, &local_err); > + if (local_err) { > + error_report_err(local_err); > + error_free(pv_mig_blocker); > + pv_mig_blocker = NULL; Dito. > + return rc; > + } > + > /* Create SE VM */ > rc = s390_pv_vm_enable(); > if (rc) { > + error_report_err(local_err); > + error_free(pv_mig_blocker); > + pv_mig_blocker = NULL; Dito. > return rc; > } > > @@ -470,11 +490,13 @@ 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: > + /* Continue after the diag308 so the guest knows something went wrong. */ > + s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); > break; Why does this change *not* belong into path #2? I think I asked this already.
On 3/9/20 2:41 PM, David Hildenbrand wrote: > On 09.03.20 12:21, Janosch Frank wrote: >> Migration is not yet supported. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> hw/s390x/s390-virtio-ccw.c | 26 ++++++++++++++++++++++++-- >> 1 file changed, 24 insertions(+), 2 deletions(-) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index f718cfc591..4bb38704ff 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) >> { >> @@ -331,16 +334,33 @@ static void s390_machine_unprotect(S390CcwMachineState *ms) >> } >> ms->pv = false; >> } >> + migrate_del_blocker(pv_mig_blocker); >> + error_free(pv_mig_blocker); >> + pv_mig_blocker = NULL; > > Maybe use error_free_or_abort(&pv_mig_blocker); I dislike hiding setting the ptr to NULL with that function. The assert for NULL in error_setg has caused me a lot of headache initially. But if that's how it's done I'll fix it. > >> } >> >> static int s390_machine_protect(S390CcwMachineState *ms) >> { >> + static Error *local_err; >> CPUState *t; >> int rc; >> >> + error_setg(&pv_mig_blocker, >> + "protected VMs are currently not migrateable."); >> + rc = migrate_add_blocker(pv_mig_blocker, &local_err); >> + if (local_err) { >> + error_report_err(local_err); >> + error_free(pv_mig_blocker); >> + pv_mig_blocker = NULL; > > Dito. > >> + return rc; >> + } >> + >> /* Create SE VM */ >> rc = s390_pv_vm_enable(); >> if (rc) { >> + error_report_err(local_err); >> + error_free(pv_mig_blocker); >> + pv_mig_blocker = NULL; > > Dito. > >> return rc; >> } >> >> @@ -470,11 +490,13 @@ 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: >> + /* Continue after the diag308 so the guest knows something went wrong. */ >> + s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); >> break; > > Why does this change *not* belong into path #2? I think I asked this > already. Yes, I'll move it
On 09.03.20 14:51, Janosch Frank wrote: > On 3/9/20 2:41 PM, David Hildenbrand wrote: >> On 09.03.20 12:21, Janosch Frank wrote: >>> Migration is not yet supported. >>> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>> --- >>> hw/s390x/s390-virtio-ccw.c | 26 ++++++++++++++++++++++++-- >>> 1 file changed, 24 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >>> index f718cfc591..4bb38704ff 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) >>> { >>> @@ -331,16 +334,33 @@ static void s390_machine_unprotect(S390CcwMachineState *ms) >>> } >>> ms->pv = false; >>> } >>> + migrate_del_blocker(pv_mig_blocker); >>> + error_free(pv_mig_blocker); >>> + pv_mig_blocker = NULL; >> >> Maybe use error_free_or_abort(&pv_mig_blocker); > > I dislike hiding setting the ptr to NULL with that function. > The assert for NULL in error_setg has caused me a lot of headache initially. > > But if that's how it's done I'll fix it. No strong feelings on my side. Seemed like a nice function to me :)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index f718cfc591..4bb38704ff 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) { @@ -331,16 +334,33 @@ static void s390_machine_unprotect(S390CcwMachineState *ms) } ms->pv = false; } + migrate_del_blocker(pv_mig_blocker); + error_free(pv_mig_blocker); + pv_mig_blocker = NULL; } static int s390_machine_protect(S390CcwMachineState *ms) { + static Error *local_err; CPUState *t; int rc; + error_setg(&pv_mig_blocker, + "protected VMs are currently not migrateable."); + rc = migrate_add_blocker(pv_mig_blocker, &local_err); + if (local_err) { + error_report_err(local_err); + error_free(pv_mig_blocker); + pv_mig_blocker = NULL; + return rc; + } + /* Create SE VM */ rc = s390_pv_vm_enable(); if (rc) { + error_report_err(local_err); + error_free(pv_mig_blocker); + pv_mig_blocker = NULL; return rc; } @@ -470,11 +490,13 @@ 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: + /* Continue after the diag308 so the guest knows something went wrong. */ + 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 | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-)