Re: [Qemu-devel] [PATCH RFC 1/1] virtio: migrate config_vector

[Michael S. Tsirkin]

On Fri, May 15, 2015 at 09:08:07AM +0200, Christian Borntraeger wrote:
> Am 14.05.2015 um 19:00 schrieb Dr. David Alan Gilbert:
> > * Christian Borntraeger (address@hidden) wrote:
> >> Am 14.05.2015 um 11:36 schrieb Michael S. Tsirkin:
> >>> On Thu, May 14, 2015 at 11:22:13AM +0200, Christian Borntraeger wrote:
> >>>> Am 13.05.2015 um 23:47 schrieb Michael S. Tsirkin:
> >>>>> On Wed, May 13, 2015 at 08:57:00PM +0200, Christian Borntraeger wrote:
> >>>>>> Am 13.05.2015 um 18:14 schrieb Michael S. Tsirkin:
> >>>>>>>> - AFAICS, there's no easy way to add transport-specific subsections -
> >>>>>>>>   and simply adding config_vector in ccw would break compatibility
> >>>>>>>
> >>>>>>> subsections break compatibility too.  The only way around that is to 
> >>>>>>> set
> >>>>>>> a flag to skip migrating config_vector for old machine types.
> >>>>>>
> >>>>>> My main concern is about undetected compatibility issues. A subsection 
> >>>>>> will 
> >>>>>> tell the user that something went wrong. What happens if we just add a 
> >>>>>> new
> >>>>>> qemu_put_byte in the stream. Will the savevm core always detect that 
> >>>>>> we have
> >>>>>> too many or not enough bytes? If yes, adding new stuff in the stream 
> >>>>>> will
> >>>>>> always be detected in some way as error we can go with just adding
> >>>>>> qemu_put_be16/qemu_get_be16 in 
> >>>>>> virtio_ccw_save_config/virtio_ccw_load_config.
> >>>>>> Old/new QEMUs will then not be compatible - but thats probably ok as 
> >>>>>> long as it
> >>>>>> errors out.
> >>>>>>
> >>>>>> My understanding was that we do not have a guarentee that this will be
> >>>>>> detected all the time and having random junk in some variables is a 
> >>>>>> debugging
> >>>>>> nightmare. Is that correct?
> >>>>>>
> >>>>>>
> >>>>>> Christian
> >>>>>
> >>>>> It's not too bad - normally there's a bunch of strings that
> >>>>> helps you find out what's going on.
> >>>>> But if you really care about debuggability of migration streams, help 
> >>>>> move
> >>>>> forward dgilbert's RFC that switched to a self-delimiting format.
> >>>>> Just piling up random hacks in virtio seems like a wrong approach.
> >>>>>
> >>>>
> >>>> Thats not my question. PLEASE try to understand my question.
> >>>> I want a hard stop if migration changes in incompatible ways.
> >>>> If adding a qemu_put_byte in virtio_ccw gets detected we can just fix
> >>>> virtio_ccw AS YOU SUGGESTED. I just want to know if I can rely on that 
> >>>> or not.
> >>>>
> >>>> Christian 
> >>>
> >>> I answered exactly this question but let me try to spell the answer
> >>> out a bit more.
> >>>
> >>> There are three answers:
> >>> 1.  Yes, it's sure to get detected because everything gets shifted
> >>>     and then you get an unexpected string instead of next device name.
> >>
> >> Ok got it. So simply adding a qemu_put/get_byte will always fail the 
> >> migration and we
> >> can just fixup virtio-ccw.c at the cost of being not migrateable between 
> >> versions before/after that change. 
> > 
> > Gahhh!  No!   Adding an extra byte into the stream causes random horrible 
> > failures
> > that get very very confusing.  Yes, it will probably fail, but how it will
> > fail and what error you get is just guess work.
> > (And note, it's strictly a 'probably fail' - if you happen to land with
> > a zero byte where you expect the start of the next section the migration
> > code will think it's the end of the migration stream and blissfully start
> > the CPUs).
> 
> As Conny is away today, I will drive the dicussion a bit further :-)
> So we really want a feature that detects this change and prevents migration.
> I think its totally  fine to not be able to migrate between todays QEMUs and
> a fixed version for  s390 as there no supported environment today I am aware
> of. What would be the preferred way to go?
> a: Connies approach of a subsection that is only migrated if necessary 
> (config vector != 0xffff)
> b: change virtio-ccw.c with put/get_be16 and make a new version of the
> s390-ccw machine? The old version will set a property to not migrate the 
> config
> vector. (like Michaels 2nd suggestion)
> c: ?

c. Set old machine as non-migrateable.

> Christian


I prefer b or c.