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

[Cornelia Huck]

On Fri, 15 May 2015 09:13:46 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

> 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)

Would the subsection approach be more acceptable if I default needed to
false and have only virtio-ccw override it in a callback?

> > 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)

I'm still not sure mucking with individual values is a good idea.

> > c: ?
> 
> c. Set old machine as non-migrateable.

Overkill?

> 
> I prefer b or c.

I still prefer a :) - mainly because I think introducing subsections
makes it more obvious what is happening. It's a pity I can't introduce
a subsection in ccw alone, but have to go to the core for that.