| Message ID | 1499942429-55449-4-git-send-email-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 13 Jul 2017 12:40:29 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > From: Yi Min Zhao <zyimin@linux.vnet.ibm.com> > > During migration we should transfer ais states to the target guest. > This patch introduces a subsection to kvm_s390_flic_vmstate and new > vmsd for qemu_flic. The ais states need to be migrated only when > ais is supported. > > Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > hw/intc/s390_flic.c | 20 ++++++++++++ > hw/intc/s390_flic_kvm.c | 75 ++++++++++++++++++++++++++++++++++++++++++++ > include/hw/s390x/s390_flic.h | 1 + > 3 files changed, 96 insertions(+) > > diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c > index d93503f..4cf73ee 100644 > --- a/hw/intc/s390_flic_kvm.c > +++ b/hw/intc/s390_flic_kvm.c > @@ -413,7 +413,78 @@ out: > return r; > } > > +typedef struct KVMS390FLICStateMigTmp { > + KVMS390FLICState *parent; > + uint8_t simm; > + uint8_t nimm; > +} KVMS390FLICStateMigTmp; > + > +static void kvm_flic_ais_pre_save(void *opaque) > +{ > + KVMS390FLICStateMigTmp *tmp = opaque; > + KVMS390FLICState *flic = tmp->parent; > + struct kvm_s390_ais_all ais; > + struct kvm_device_attr attr = { > + .group = KVM_DEV_FLIC_AISM_ALL, > + .addr = (uint64_t)&ais, > + .attr = sizeof(ais), > + }; > + > + if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) { > + error_report("Failed to retrieve kvm flic ais states"); There's not much else we can do in that case, is there? > + return; > + } > + > + tmp->simm = ais.simm; > + tmp->nimm = ais.nimm; > +} > + > +static int kvm_flic_ais_post_load(void *opaque, int version_id) > +{ > + KVMS390FLICStateMigTmp *tmp = opaque; > + KVMS390FLICState *flic = tmp->parent; > + struct kvm_s390_ais_all ais = { > + .simm = tmp->simm, > + .nimm = tmp->nimm, > + }; > + struct kvm_device_attr attr = { > + .group = KVM_DEV_FLIC_AISM_ALL, > + .addr = (uint64_t)&ais, > + }; > + > + if (!ais_needed(flic)) { > + return -ENOSYS; > + } I do not understand this... does that mean that - we should never get here or - we should not try to change the fields or - I need coffee? > + > + return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0; > +} > + > +static const VMStateDescription kvm_s390_flic_ais_tmp = { > + .name = "s390-flic-ais-tmp", > + .pre_save = kvm_flic_ais_pre_save, > + .post_load = kvm_flic_ais_post_load, > + .fields = (VMStateField[]) { > + VMSTATE_UINT8(simm, KVMS390FLICStateMigTmp), > + VMSTATE_UINT8(nimm, KVMS390FLICStateMigTmp), > + VMSTATE_END_OF_LIST() > + } > +}; > + > +static const VMStateDescription kvm_s390_flic_vmstate_ais = { > + .name = "s390-flic/ais", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = ais_needed, > + .fields = (VMStateField[]) { > + VMSTATE_WITH_TMP(KVMS390FLICState, KVMS390FLICStateMigTmp, > + kvm_s390_flic_ais_tmp), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static const VMStateDescription kvm_s390_flic_vmstate = { > + /* should have been like kvm-s390-flic, > + * can't change without breaking compat */ :/ > .name = "s390-flic", > .version_id = FLIC_SAVEVM_VERSION, > .minimum_version_id = FLIC_SAVEVM_VERSION,
On 07/13/2017 02:27 PM, Cornelia Huck wrote: > On Thu, 13 Jul 2017 12:40:29 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com> >> >> During migration we should transfer ais states to the target guest. >> This patch introduces a subsection to kvm_s390_flic_vmstate and new >> vmsd for qemu_flic. The ais states need to be migrated only when >> ais is supported. >> >> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> hw/intc/s390_flic.c | 20 ++++++++++++ >> hw/intc/s390_flic_kvm.c | 75 ++++++++++++++++++++++++++++++++++++++++++++ >> include/hw/s390x/s390_flic.h | 1 + >> 3 files changed, 96 insertions(+) >> > >> diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c >> index d93503f..4cf73ee 100644 >> --- a/hw/intc/s390_flic_kvm.c >> +++ b/hw/intc/s390_flic_kvm.c >> @@ -413,7 +413,78 @@ out: >> return r; >> } >> >> +typedef struct KVMS390FLICStateMigTmp { >> + KVMS390FLICState *parent; >> + uint8_t simm; >> + uint8_t nimm; >> +} KVMS390FLICStateMigTmp; >> + >> +static void kvm_flic_ais_pre_save(void *opaque) >> +{ >> + KVMS390FLICStateMigTmp *tmp = opaque; >> + KVMS390FLICState *flic = tmp->parent; >> + struct kvm_s390_ais_all ais; >> + struct kvm_device_attr attr = { >> + .group = KVM_DEV_FLIC_AISM_ALL, >> + .addr = (uint64_t)&ais, >> + .attr = sizeof(ais), >> + }; >> + >> + if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) { >> + error_report("Failed to retrieve kvm flic ais states"); > > There's not much else we can do in that case, is there? > >> + return; >> + } >> + >> + tmp->simm = ais.simm; >> + tmp->nimm = ais.nimm; >> +} >> + >> +static int kvm_flic_ais_post_load(void *opaque, int version_id) >> +{ >> + KVMS390FLICStateMigTmp *tmp = opaque; >> + KVMS390FLICState *flic = tmp->parent; >> + struct kvm_s390_ais_all ais = { >> + .simm = tmp->simm, >> + .nimm = tmp->nimm, >> + }; >> + struct kvm_device_attr attr = { >> + .group = KVM_DEV_FLIC_AISM_ALL, >> + .addr = (uint64_t)&ais, >> + }; >> + >> + if (!ais_needed(flic)) { >> + return -ENOSYS; >> + } > > I do not understand this... does that mean that > - we should never get here or > - we should not try to change the fields or > - I need coffee? My understanding is that this should not happen with a normal setup, but it can happen if the user does something "wrong". For example use qemu without libvirt and with -cpu host (which is not migration safe) and do a migration from a host that has AIS to a host that has no AIS. In that case the target system will reject the migration (late, but hopefully not too late).
On Thu, 13 Jul 2017 15:02:08 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 07/13/2017 02:27 PM, Cornelia Huck wrote: > > On Thu, 13 Jul 2017 12:40:29 +0200 > > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > >> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com> > >> > >> During migration we should transfer ais states to the target guest. > >> This patch introduces a subsection to kvm_s390_flic_vmstate and new > >> vmsd for qemu_flic. The ais states need to be migrated only when > >> ais is supported. > >> > >> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com> > >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > >> --- > >> hw/intc/s390_flic.c | 20 ++++++++++++ > >> hw/intc/s390_flic_kvm.c | 75 ++++++++++++++++++++++++++++++++++++++++++++ > >> include/hw/s390x/s390_flic.h | 1 + > >> 3 files changed, 96 insertions(+) > >> +static int kvm_flic_ais_post_load(void *opaque, int version_id) > >> +{ > >> + KVMS390FLICStateMigTmp *tmp = opaque; > >> + KVMS390FLICState *flic = tmp->parent; > >> + struct kvm_s390_ais_all ais = { > >> + .simm = tmp->simm, > >> + .nimm = tmp->nimm, > >> + }; > >> + struct kvm_device_attr attr = { > >> + .group = KVM_DEV_FLIC_AISM_ALL, > >> + .addr = (uint64_t)&ais, > >> + }; > >> + > >> + if (!ais_needed(flic)) { > >> + return -ENOSYS; > >> + } > > > > I do not understand this... does that mean that > > - we should never get here or > > - we should not try to change the fields or > > - I need coffee? > > My understanding is that this should not happen with a normal setup, > but it can happen if the user does something "wrong". For example > use qemu without libvirt and with -cpu host (which is not migration safe) > and do a migration from a host that has AIS to a host that has no AIS. > In that case the target system will reject the migration (late, but hopefully > not too late). A comment would be helpful here.
On 07/13/2017 02:27 PM, Cornelia Huck wrote: >> +static void kvm_flic_ais_pre_save(void *opaque) >> +{ >> + KVMS390FLICStateMigTmp *tmp = opaque; >> + KVMS390FLICState *flic = tmp->parent; >> + struct kvm_s390_ais_all ais; >> + struct kvm_device_attr attr = { >> + .group = KVM_DEV_FLIC_AISM_ALL, >> + .addr = (uint64_t)&ais, >> + .attr = sizeof(ais), >> + }; >> + >> + if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) { >> + error_report("Failed to retrieve kvm flic ais states"); > There's not much else we can do in that case, is there? > I think this is a very good question! The ioctl should not fail under any circumstances, but if it does we have a problem. Carrying on happily (what we do now) means effectively discarding ais state. In general just discarding state ain't a good idea. In particular it might be OK, but the patch should explain that! Regarding what could/should we do in such a case (instead of discarding state and carrying on happily) I don't know, so I tend to agree with you regarding 'not much else we can do'. Adding Dave and Juan. Maybe they can tell. Regards, Halil >> + return; >> + } >> + >> + tmp->simm = ais.simm; >> + tmp->nimm = ais.nimm; >> +} >> +
On 07/13/2017 03:10 PM, Cornelia Huck wrote: > On Thu, 13 Jul 2017 15:02:08 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> On 07/13/2017 02:27 PM, Cornelia Huck wrote: >>> On Thu, 13 Jul 2017 12:40:29 +0200 >>> Christian Borntraeger <borntraeger@de.ibm.com> wrote: >>> >>>> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com> >>>> >>>> During migration we should transfer ais states to the target guest. >>>> This patch introduces a subsection to kvm_s390_flic_vmstate and new >>>> vmsd for qemu_flic. The ais states need to be migrated only when >>>> ais is supported. >>>> >>>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com> >>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>> --- >>>> hw/intc/s390_flic.c | 20 ++++++++++++ >>>> hw/intc/s390_flic_kvm.c | 75 ++++++++++++++++++++++++++++++++++++++++++++ >>>> include/hw/s390x/s390_flic.h | 1 + >>>> 3 files changed, 96 insertions(+) > >>>> +static int kvm_flic_ais_post_load(void *opaque, int version_id) >>>> +{ >>>> + KVMS390FLICStateMigTmp *tmp = opaque; >>>> + KVMS390FLICState *flic = tmp->parent; >>>> + struct kvm_s390_ais_all ais = { >>>> + .simm = tmp->simm, >>>> + .nimm = tmp->nimm, >>>> + }; >>>> + struct kvm_device_attr attr = { >>>> + .group = KVM_DEV_FLIC_AISM_ALL, >>>> + .addr = (uint64_t)&ais, >>>> + }; >>>> + >>>> + if (!ais_needed(flic)) { >>>> + return -ENOSYS; >>>> + } >>> >>> I do not understand this... does that mean that >>> - we should never get here or >>> - we should not try to change the fields or >>> - I need coffee? >> >> My understanding is that this should not happen with a normal setup, >> but it can happen if the user does something "wrong". For example >> use qemu without libvirt and with -cpu host (which is not migration safe) >> and do a migration from a host that has AIS to a host that has no AIS. >> In that case the target system will reject the migration (late, but hopefully >> not too late). > > A comment would be helpful here. > I was actually pushing for explaining the design decisions regarding these corner cases (there is the inverse problem too) in the commit message (during the internal review). I'm not sure about this "wrong" though. Can one of you provide a reference why is the scenario described "wrong". This question decomposes into three: 1) How do I, as a qemu developer, know what is the management software supposed to do? (E.g. has to use a cpu model other than host if it wants to migrate (probably).) 2) What can I assume about the stuff not properly covered in the user manual when I reason "right" usage and "wrong" usage. 3) Is being 'fool-proof' a design goal, or are we satisfied with providing mechanisms for using something safely and (like with undefined behavior in C) don't really care about possible damage if the user does not stick to these safe mechanisms (which are documented as such)? I've looked up the user manual regarding migration. It was not very helpful regarding the matter of differentiating between right and wrong usage. Regards, Halil
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 07/13/2017 02:27 PM, Cornelia Huck wrote: > >> +static void kvm_flic_ais_pre_save(void *opaque) > >> +{ > >> + KVMS390FLICStateMigTmp *tmp = opaque; > >> + KVMS390FLICState *flic = tmp->parent; > >> + struct kvm_s390_ais_all ais; > >> + struct kvm_device_attr attr = { > >> + .group = KVM_DEV_FLIC_AISM_ALL, > >> + .addr = (uint64_t)&ais, > >> + .attr = sizeof(ais), > >> + }; > >> + > >> + if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) { > >> + error_report("Failed to retrieve kvm flic ais states"); > > There's not much else we can do in that case, is there? > > > > I think this is a very good question! The ioctl should not fail > under any circumstances, but if it does we have a problem. > > Carrying on happily (what we do now) means effectively discarding > ais state. In general just discarding state ain't a good idea. > > In particular it might be OK, but the patch should explain that! > > Regarding what could/should we do in such a case (instead > of discarding state and carrying on happily) I don't know, so > I tend to agree with you regarding 'not much else we can do'. > > Adding Dave and Juan. Maybe they can tell. I keep meaning to make the pre_save give a return value for failure, but it hasn't currently got one. You could try something like: qemu_file_set_error(migrate_get_current()->to_dst_file, -EINVAL); I *think* the migration code should spot that before it finishes but it might carry on for a little while before it does. Dave > Regards, > Halil > > >> + return; > >> + } > >> + > >> + tmp->simm = ais.simm; > >> + tmp->nimm = ais.nimm; > >> +} > >> + > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >> >> >> On 07/13/2017 02:27 PM, Cornelia Huck wrote: >>>> +static void kvm_flic_ais_pre_save(void *opaque) >>>> +{ >>>> + KVMS390FLICStateMigTmp *tmp = opaque; >>>> + KVMS390FLICState *flic = tmp->parent; >>>> + struct kvm_s390_ais_all ais; >>>> + struct kvm_device_attr attr = { >>>> + .group = KVM_DEV_FLIC_AISM_ALL, >>>> + .addr = (uint64_t)&ais, >>>> + .attr = sizeof(ais), >>>> + }; >>>> + >>>> + if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) { >>>> + error_report("Failed to retrieve kvm flic ais states"); >>> There's not much else we can do in that case, is there? >>> >> >> I think this is a very good question! The ioctl should not fail >> under any circumstances, but if it does we have a problem. >> >> Carrying on happily (what we do now) means effectively discarding >> ais state. In general just discarding state ain't a good idea. >> >> In particular it might be OK, but the patch should explain that! >> >> Regarding what could/should we do in such a case (instead >> of discarding state and carrying on happily) I don't know, so >> I tend to agree with you regarding 'not much else we can do'. >> >> Adding Dave and Juan. Maybe they can tell. > > I keep meaning to make the pre_save give a return value for failure, > but it hasn't currently got one. Would you accept patches for that? > > You could try something like: > > qemu_file_set_error(migrate_get_current()->to_dst_file, -EINVAL); > > I *think* the migration code should spot that before it finishes > but it might carry on for a little while before it does. I will keep this patch as is, since this is one of the "should not happen" cases.
* Christian Borntraeger (borntraeger@de.ibm.com) wrote: > On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote: > > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >> > >> > >> On 07/13/2017 02:27 PM, Cornelia Huck wrote: > >>>> +static void kvm_flic_ais_pre_save(void *opaque) > >>>> +{ > >>>> + KVMS390FLICStateMigTmp *tmp = opaque; > >>>> + KVMS390FLICState *flic = tmp->parent; > >>>> + struct kvm_s390_ais_all ais; > >>>> + struct kvm_device_attr attr = { > >>>> + .group = KVM_DEV_FLIC_AISM_ALL, > >>>> + .addr = (uint64_t)&ais, > >>>> + .attr = sizeof(ais), > >>>> + }; > >>>> + > >>>> + if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) { > >>>> + error_report("Failed to retrieve kvm flic ais states"); > >>> There's not much else we can do in that case, is there? > >>> > >> > >> I think this is a very good question! The ioctl should not fail > >> under any circumstances, but if it does we have a problem. > >> > >> Carrying on happily (what we do now) means effectively discarding > >> ais state. In general just discarding state ain't a good idea. > >> > >> In particular it might be OK, but the patch should explain that! > >> > >> Regarding what could/should we do in such a case (instead > >> of discarding state and carrying on happily) I don't know, so > >> I tend to agree with you regarding 'not much else we can do'. > >> > >> Adding Dave and Juan. Maybe they can tell. > > > > I keep meaning to make the pre_save give a return value for failure, > > but it hasn't currently got one. > > Would you accept patches for that? Sure. > > > > You could try something like: > > > > qemu_file_set_error(migrate_get_current()->to_dst_file, -EINVAL); > > > > I *think* the migration code should spot that before it finishes > > but it might carry on for a little while before it does. > > I will keep this patch as is, since this is one of the "should not happen" > cases. And you've got a log message, so when the impossible happens at least we should be able to debug it. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, 13 Jul 2017 17:05:45 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote: > > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >> > >> > >> On 07/13/2017 02:27 PM, Cornelia Huck wrote: > >>>> +static void kvm_flic_ais_pre_save(void *opaque) > >>>> +{ > >>>> + KVMS390FLICStateMigTmp *tmp = opaque; > >>>> + KVMS390FLICState *flic = tmp->parent; > >>>> + struct kvm_s390_ais_all ais; > >>>> + struct kvm_device_attr attr = { > >>>> + .group = KVM_DEV_FLIC_AISM_ALL, > >>>> + .addr = (uint64_t)&ais, > >>>> + .attr = sizeof(ais), > >>>> + }; > >>>> + > >>>> + if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) { > >>>> + error_report("Failed to retrieve kvm flic ais states"); > >>> There's not much else we can do in that case, is there? > >>> > >> > >> I think this is a very good question! The ioctl should not fail > >> under any circumstances, but if it does we have a problem. > >> > >> Carrying on happily (what we do now) means effectively discarding > >> ais state. In general just discarding state ain't a good idea. > >> > >> In particular it might be OK, but the patch should explain that! > >> > >> Regarding what could/should we do in such a case (instead > >> of discarding state and carrying on happily) I don't know, so > >> I tend to agree with you regarding 'not much else we can do'. > >> > >> Adding Dave and Juan. Maybe they can tell. > > > > I keep meaning to make the pre_save give a return value for failure, > > but it hasn't currently got one. > > Would you accept patches for that? > > > > > You could try something like: > > > > qemu_file_set_error(migrate_get_current()->to_dst_file, -EINVAL); > > > > I *think* the migration code should spot that before it finishes > > but it might carry on for a little while before it does. > > I will keep this patch as is, since this is one of the "should not happen" > cases. > Fine with me.
On 07/13/2017 05:11 PM, Dr. David Alan Gilbert wrote: > * Christian Borntraeger (borntraeger@de.ibm.com) wrote: >> On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote: >>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >>>> >>>> >>>> On 07/13/2017 02:27 PM, Cornelia Huck wrote: >>>>>> +static void kvm_flic_ais_pre_save(void *opaque) >>>>>> +{ >>>>>> + KVMS390FLICStateMigTmp *tmp = opaque; >>>>>> + KVMS390FLICState *flic = tmp->parent; >>>>>> + struct kvm_s390_ais_all ais; >>>>>> + struct kvm_device_attr attr = { >>>>>> + .group = KVM_DEV_FLIC_AISM_ALL, >>>>>> + .addr = (uint64_t)&ais, >>>>>> + .attr = sizeof(ais), >>>>>> + }; >>>>>> + >>>>>> + if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) { >>>>>> + error_report("Failed to retrieve kvm flic ais states"); >>>>> There's not much else we can do in that case, is there? >>>>> >>>> >>>> I think this is a very good question! The ioctl should not fail >>>> under any circumstances, but if it does we have a problem. >>>> >>>> Carrying on happily (what we do now) means effectively discarding >>>> ais state. In general just discarding state ain't a good idea. >>>> >>>> In particular it might be OK, but the patch should explain that! >>>> >>>> Regarding what could/should we do in such a case (instead >>>> of discarding state and carrying on happily) I don't know, so >>>> I tend to agree with you regarding 'not much else we can do'. >>>> >>>> Adding Dave and Juan. Maybe they can tell. >>> >>> I keep meaning to make the pre_save give a return value for failure, >>> but it hasn't currently got one. >> >> Would you accept patches for that? > > Sure. > >>> >>> You could try something like: >>> >>> qemu_file_set_error(migrate_get_current()->to_dst_file, -EINVAL); @Dave: Thanks Dave! I was not aware of that! Had a quick look at the code, I think qemu_file_set_error would indeed do the right thing. I would prefer error handling being part of the pre_save interface, because that would be easier to understand, and would provoke thinking about these problems. @Christian: Would you like to implement 'return value for pre_save' yourself? I mean, I the meanwhile I'm familiar with the code in question and I enjoy working with Dave and Juan, so if you aren't interested in doing it yourself but think it's important enough to get it done, I could take it too? @Dave: There are a couple of questions I'm gonna have to ask/investigate should it be me doing the 'return value for pre_save' (also notes to myself): Would you see this error handling via pre_save as a parallel infrastructure (keep the current qemu_file_set_error mechanism) or would you prefer things converted? IMHO having a single method would be cleaner, but I have not looked into this in great detail. Also the question what is the semantic of qemu_file_set_error arises. It ain't documented and I would intuitively suspect that it's rather about the 'file' (that is transport) than the whole migration. >>> >>> I *think* the migration code should spot that before it finishes >>> but it might carry on for a little while before it does. >> >> I will keep this patch as is, since this is one of the "should not happen" >> cases. @Christian I'm OK with it, because knowing the kernel code behind the ioctl this is really unlikely and even if it should happen the risks involved are rather limited. But I would be much happier if all such cases would result in refusing migration. Regards, Halil
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 07/13/2017 05:11 PM, Dr. David Alan Gilbert wrote: > > * Christian Borntraeger (borntraeger@de.ibm.com) wrote: > >> On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote: > >>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >>>> > >>>> > >>>> On 07/13/2017 02:27 PM, Cornelia Huck wrote: > >>>>>> +static void kvm_flic_ais_pre_save(void *opaque) > >>>>>> +{ > >>>>>> + KVMS390FLICStateMigTmp *tmp = opaque; > >>>>>> + KVMS390FLICState *flic = tmp->parent; > >>>>>> + struct kvm_s390_ais_all ais; > >>>>>> + struct kvm_device_attr attr = { > >>>>>> + .group = KVM_DEV_FLIC_AISM_ALL, > >>>>>> + .addr = (uint64_t)&ais, > >>>>>> + .attr = sizeof(ais), > >>>>>> + }; > >>>>>> + > >>>>>> + if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) { > >>>>>> + error_report("Failed to retrieve kvm flic ais states"); > >>>>> There's not much else we can do in that case, is there? > >>>>> > >>>> > >>>> I think this is a very good question! The ioctl should not fail > >>>> under any circumstances, but if it does we have a problem. > >>>> > >>>> Carrying on happily (what we do now) means effectively discarding > >>>> ais state. In general just discarding state ain't a good idea. > >>>> > >>>> In particular it might be OK, but the patch should explain that! > >>>> > >>>> Regarding what could/should we do in such a case (instead > >>>> of discarding state and carrying on happily) I don't know, so > >>>> I tend to agree with you regarding 'not much else we can do'. > >>>> > >>>> Adding Dave and Juan. Maybe they can tell. > >>> > >>> I keep meaning to make the pre_save give a return value for failure, > >>> but it hasn't currently got one. > >> > >> Would you accept patches for that? > > > > Sure. > > > >>> > >>> You could try something like: > >>> > >>> qemu_file_set_error(migrate_get_current()->to_dst_file, -EINVAL); > > @Dave: > Thanks Dave! I was not aware of that! Had a quick look at the > code, I think qemu_file_set_error would indeed do the right thing. > > I would prefer error handling being part of the pre_save interface, > because that would be easier to understand, and would provoke thinking > about these problems. > > @Christian: > Would you like to implement 'return value for pre_save' > yourself? I mean, I the meanwhile I'm familiar with the code in question > and I enjoy working with Dave and Juan, so if you aren't interested in > doing it yourself but think it's important enough to get it done, I could > take it too? > > @Dave: > There are a couple of questions I'm gonna have to ask/investigate should > it be me doing the 'return value for pre_save' (also notes to myself): > > Would you see this error handling via pre_save as a parallel infrastructure > (keep the current qemu_file_set_error mechanism) or would you prefer > things converted? IMHO having a single method would be cleaner, but I > have not looked into this in great detail. The only thing I'd like to change is make pre_save be: int (*pre_save)(void *opaque) rather than void. If there are any current pre_save's that call set_error or assert or anything like that then they could be converted, but I don't think there are many. > Also the question what is the semantic of qemu_file_set_error arises. > It ain't documented and I would intuitively suspect that it's rather > about the 'file' (that is transport) than the whole migration. It's really an internal interface in migration (where it's not very nice either); but really it shouldn't be used for anything else. Dave > > > >>> > >>> I *think* the migration code should spot that before it finishes > >>> but it might carry on for a little while before it does. > >> > >> I will keep this patch as is, since this is one of the "should not happen" > >> cases. > > @Christian > I'm OK with it, because knowing the kernel code behind the ioctl > this is really unlikely and even if it should happen the risks involved > are rather limited. But I would be much happier if all such > cases would result in refusing migration. > > Regards, > Halil > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 07/13/2017 05:11 PM, Dr. David Alan Gilbert wrote: >> * Christian Borntraeger (borntraeger@de.ibm.com) wrote: >>> On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote: >>>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > @Dave: > There are a couple of questions I'm gonna have to ask/investigate should > it be me doing the 'return value for pre_save' (also notes to myself): As Dave said, just changing the prototype and fix callers will do a lot of good. > (keep the current qemu_file_set_error mechanism) or would you prefer > things converted? IMHO having a single method would be cleaner, but I > have not looked into this in great detail. Error handling is a mess in qemu (in general), and worse in migration (in particular). History lesson (that is even older than my involvement with qemu) It used to be that evrything was done with callbacks in the main loop. So there were not an easy way to pass error messages. Everything was done through qemu_file_set_error() (well, actually it was done by hand). Then the migration thread came, and we can do things asyncronously, so we don't need to store the state in callbacks. I did some changes long, long ago about returning error messages instead of: if (error) { qemu_file_set_error(f, -error); return; } to if (error) { return -error; } /* fix things to make errors negative, etc, etc, you get the idea */ But we need to fix some things to make this work. Reception side is done through a coroutine, and then some of such things still stand. Moving to a thread would be a good idea at some point O:-) > Also the question what is the semantic of qemu_file_set_error arises. > It ain't documented and I would intuitively suspect that it's rather > about the 'file' (that is transport) than the whole migration. We test in the "interesting" parts if there is an error on the file, from vmstate_load_state() } else { ret = field->info->get(f, curr_elem, size, field); } if (ret >= 0) { ret = qemu_file_get_error(f); } if (ret < 0) { qemu_file_set_error(f, ret); error_report("Failed to load %s:%s", vmsd->name, field->name); trace_vmstate_load_field_error(field->name, ret); return ret; } You can see how many convolutions we do to maintain the error returned and the one in QEMUFile in sync. Removing the one in QEMUFile would be a good idea, but requires lots of changes. For starters, see that all qemu_get_* return void. To add insult to injury: int qemu_peek_byte(QEMUFile *f, int offset) { int index = f->buf_index + offset; assert(!qemu_file_is_writable(f)); assert(offset < IO_BUF_SIZE); if (index >= f->buf_size) { qemu_fill_buffer(f); index = f->buf_index + offset; if (index >= f->buf_size) { return 0; } } return f->buf[index]; } I.e. it never returns errors, if there is nothing to read, it just return 0. /me stops > > > >>>> >>>> I *think* the migration code should spot that before it finishes >>>> but it might carry on for a little while before it does. >>> >>> I will keep this patch as is, since this is one of the "should not happen" >>> cases. Later, Juan.
* Juan Quintela (quintela@redhat.com) wrote: > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > > On 07/13/2017 05:11 PM, Dr. David Alan Gilbert wrote: > >> * Christian Borntraeger (borntraeger@de.ibm.com) wrote: > >>> On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote: > >>>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > @Dave: > > There are a couple of questions I'm gonna have to ask/investigate should > > it be me doing the 'return value for pre_save' (also notes to myself): > > As Dave said, just changing the prototype and fix callers will do a lot > of good. > > > (keep the current qemu_file_set_error mechanism) or would you prefer > > things converted? IMHO having a single method would be cleaner, but I > > have not looked into this in great detail. > > Error handling is a mess in qemu (in general), and worse in migration > (in particular). > > History lesson (that is even older than my involvement with qemu) > > It used to be that evrything was done with callbacks in the main loop. > So there were not an easy way to pass error messages. Everything was > done through qemu_file_set_error() (well, actually it was done by hand). > > Then the migration thread came, and we can do things asyncronously, so > we don't need to store the state in callbacks. I did some changes long, > long ago about returning error messages instead of: > > if (error) { > qemu_file_set_error(f, -error); > return; > } > > to > > if (error) { > return -error; > } > > /* fix things to make errors negative, etc, etc, you get the idea */ > > But we need to fix some things to make this work. > > Reception side is done through a coroutine, and then some of such things > still stand. Moving to a thread would be a good idea at some point O:-) > > > Also the question what is the semantic of qemu_file_set_error arises. > > It ain't documented and I would intuitively suspect that it's rather > > about the 'file' (that is transport) than the whole migration. > > We test in the "interesting" parts if there is an error on the file, > > from vmstate_load_state() > > } else { > ret = field->info->get(f, curr_elem, size, field); > } > if (ret >= 0) { > ret = qemu_file_get_error(f); > } > if (ret < 0) { > qemu_file_set_error(f, ret); > error_report("Failed to load %s:%s", vmsd->name, > field->name); > trace_vmstate_load_field_error(field->name, ret); > return ret; > } > > > You can see how many convolutions we do to maintain the error returned > and the one in QEMUFile in sync. Removing the one in QEMUFile would be > a good idea, but requires lots of changes. Yes, I'd like to get to having the ones in QEMUFile just representing actual file IO errors rather than other types of problem. Dave > For starters, see that all qemu_get_* return void. > > To add insult to injury: > > int qemu_peek_byte(QEMUFile *f, int offset) > { > int index = f->buf_index + offset; > > assert(!qemu_file_is_writable(f)); > assert(offset < IO_BUF_SIZE); > > if (index >= f->buf_size) { > qemu_fill_buffer(f); > index = f->buf_index + offset; > if (index >= f->buf_size) { > return 0; > } > } > return f->buf[index]; > } > > I.e. it never returns errors, if there is nothing to read, it just > return 0. > > /me stops > > > > > > > > >>>> > >>>> I *think* the migration code should spot that before it finishes > >>>> but it might carry on for a little while before it does. > >>> > >>> I will keep this patch as is, since this is one of the "should not happen" > >>> cases. > > Later, Juan. -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 07/18/2017 12:31 PM, Juan Quintela wrote: > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: >> On 07/13/2017 05:11 PM, Dr. David Alan Gilbert wrote: >>> * Christian Borntraeger (borntraeger@de.ibm.com) wrote: >>>> On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote: >>>>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >> @Dave: >> There are a couple of questions I'm gonna have to ask/investigate should >> it be me doing the 'return value for pre_save' (also notes to myself): > > As Dave said, just changing the prototype and fix callers will do a lot > of good. > >> (keep the current qemu_file_set_error mechanism) or would you prefer >> things converted? IMHO having a single method would be cleaner, but I >> have not looked into this in great detail. > > Error handling is a mess in qemu (in general), and worse in migration > (in particular). > > History lesson (that is even older than my involvement with qemu) > > It used to be that evrything was done with callbacks in the main loop. > So there were not an easy way to pass error messages. Everything was > done through qemu_file_set_error() (well, actually it was done by hand). > > Then the migration thread came, and we can do things asyncronously, so > we don't need to store the state in callbacks. I did some changes long, > long ago about returning error messages instead of: > > if (error) { > qemu_file_set_error(f, -error); > return; > } > > to > > if (error) { > return -error; > } > > /* fix things to make errors negative, etc, etc, you get the idea */ > > But we need to fix some things to make this work. > > Reception side is done through a coroutine, and then some of such things > still stand. Moving to a thread would be a good idea at some point O:-) > >> Also the question what is the semantic of qemu_file_set_error arises. >> It ain't documented and I would intuitively suspect that it's rather >> about the 'file' (that is transport) than the whole migration. > > We test in the "interesting" parts if there is an error on the file, > > from vmstate_load_state() > > } else { > ret = field->info->get(f, curr_elem, size, field); > } > if (ret >= 0) { > ret = qemu_file_get_error(f); > } > if (ret < 0) { > qemu_file_set_error(f, ret); > error_report("Failed to load %s:%s", vmsd->name, > field->name); > trace_vmstate_load_field_error(field->name, ret); > return ret; > } > > > You can see how many convolutions we do to maintain the error returned > and the one in QEMUFile in sync. Removing the one in QEMUFile would be > a good idea, but requires lots of changes. > > For starters, see that all qemu_get_* return void. > > To add insult to injury: > > int qemu_peek_byte(QEMUFile *f, int offset) > { > int index = f->buf_index + offset; > > assert(!qemu_file_is_writable(f)); > assert(offset < IO_BUF_SIZE); > > if (index >= f->buf_size) { > qemu_fill_buffer(f); > index = f->buf_index + offset; > if (index >= f->buf_size) { > return 0; > } > } > return f->buf[index]; > } > > I.e. it never returns errors, if there is nothing to read, it just > return 0. > > /me stops > Hi Juan, Many thanks for providing me with your perspective and a little history lesson too. Knowing the history things do make much more sense. For me this is quite often the case (history helps understanding) but at the same time extracting the historical perspective form git is in my experience quite time consuming in average case, so I don't try too often (without having a very good reason). I will keep the things you explained in mind and try to push the migration subsystem into the right direction as far my time allows (or at least avoid pushing in the wrong direction). Regards, Halil >> >> >> >>>>> >>>>> I *think* the migration code should spot that before it finishes >>>>> but it might carry on for a little while before it does. >>>> >>>> I will keep this patch as is, since this is one of the "should not happen" >>>> cases. > > Later, Juan. >
diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c index 6e7c610..6eaf178 100644 --- a/hw/intc/s390_flic.c +++ b/hw/intc/s390_flic.c @@ -134,12 +134,32 @@ static void qemu_s390_flic_reset(DeviceState *dev) flic->nimm = 0; } +bool ais_needed(void *opaque) +{ + S390FLICState *s = opaque; + + return s->ais_supported; +} + +static const VMStateDescription qemu_s390_flic_vmstate = { + .name = "qemu-s390-flic", + .version_id = 1, + .minimum_version_id = 1, + .needed = ais_needed, + .fields = (VMStateField[]) { + VMSTATE_UINT8(simm, QEMUS390FLICState), + VMSTATE_UINT8(nimm, QEMUS390FLICState), + VMSTATE_END_OF_LIST() + } +}; + static void qemu_s390_flic_class_init(ObjectClass *oc, void *data) { DeviceClass *dc = DEVICE_CLASS(oc); S390FLICStateClass *fsc = S390_FLIC_COMMON_CLASS(oc); dc->reset = qemu_s390_flic_reset; + dc->vmsd = &qemu_s390_flic_vmstate; fsc->register_io_adapter = qemu_s390_register_io_adapter; fsc->io_adapter_map = qemu_s390_io_adapter_map; fsc->add_adapter_routes = qemu_s390_add_adapter_routes; diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c index d93503f..4cf73ee 100644 --- a/hw/intc/s390_flic_kvm.c +++ b/hw/intc/s390_flic_kvm.c @@ -413,7 +413,78 @@ out: return r; } +typedef struct KVMS390FLICStateMigTmp { + KVMS390FLICState *parent; + uint8_t simm; + uint8_t nimm; +} KVMS390FLICStateMigTmp; + +static void kvm_flic_ais_pre_save(void *opaque) +{ + KVMS390FLICStateMigTmp *tmp = opaque; + KVMS390FLICState *flic = tmp->parent; + struct kvm_s390_ais_all ais; + struct kvm_device_attr attr = { + .group = KVM_DEV_FLIC_AISM_ALL, + .addr = (uint64_t)&ais, + .attr = sizeof(ais), + }; + + if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) { + error_report("Failed to retrieve kvm flic ais states"); + return; + } + + tmp->simm = ais.simm; + tmp->nimm = ais.nimm; +} + +static int kvm_flic_ais_post_load(void *opaque, int version_id) +{ + KVMS390FLICStateMigTmp *tmp = opaque; + KVMS390FLICState *flic = tmp->parent; + struct kvm_s390_ais_all ais = { + .simm = tmp->simm, + .nimm = tmp->nimm, + }; + struct kvm_device_attr attr = { + .group = KVM_DEV_FLIC_AISM_ALL, + .addr = (uint64_t)&ais, + }; + + if (!ais_needed(flic)) { + return -ENOSYS; + } + + return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0; +} + +static const VMStateDescription kvm_s390_flic_ais_tmp = { + .name = "s390-flic-ais-tmp", + .pre_save = kvm_flic_ais_pre_save, + .post_load = kvm_flic_ais_post_load, + .fields = (VMStateField[]) { + VMSTATE_UINT8(simm, KVMS390FLICStateMigTmp), + VMSTATE_UINT8(nimm, KVMS390FLICStateMigTmp), + VMSTATE_END_OF_LIST() + } +}; + +static const VMStateDescription kvm_s390_flic_vmstate_ais = { + .name = "s390-flic/ais", + .version_id = 1, + .minimum_version_id = 1, + .needed = ais_needed, + .fields = (VMStateField[]) { + VMSTATE_WITH_TMP(KVMS390FLICState, KVMS390FLICStateMigTmp, + kvm_s390_flic_ais_tmp), + VMSTATE_END_OF_LIST() + } +}; + static const VMStateDescription kvm_s390_flic_vmstate = { + /* should have been like kvm-s390-flic, + * can't change without breaking compat */ .name = "s390-flic", .version_id = FLIC_SAVEVM_VERSION, .minimum_version_id = FLIC_SAVEVM_VERSION, @@ -428,6 +499,10 @@ static const VMStateDescription kvm_s390_flic_vmstate = { .flags = VMS_SINGLE, }, VMSTATE_END_OF_LIST() + }, + .subsections = (const VMStateDescription * []) { + &kvm_s390_flic_vmstate_ais, + NULL } }; diff --git a/include/hw/s390x/s390_flic.h b/include/hw/s390x/s390_flic.h index 2f173d9..7aab6ef 100644 --- a/include/hw/s390x/s390_flic.h +++ b/include/hw/s390x/s390_flic.h @@ -89,6 +89,7 @@ typedef struct QEMUS390FLICState { void s390_flic_init(void); S390FLICState *s390_get_flic(void); +bool ais_needed(void *opaque); #ifdef CONFIG_KVM DeviceState *s390_flic_kvm_create(void);