Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Greg Kurz]

On Thu, 12 Jun 2014 13:59:59 +0300
"Michael S. Tsirkin" <address@hidden> wrote:

> On Thu, Jun 12, 2014 at 12:50:56PM +0200, Greg Kurz wrote:
> > On Thu, 12 Jun 2014 12:39:27 +0200
> > Alexander Graf <address@hidden> wrote:
> > 
> > > 
> > > 
> > > > Am 12.06.2014 um 12:14 schrieb Greg Kurz <address@hidden>:
> > > > 
> > > > On Thu, 12 Jun 2014 11:43:20 +0200
> > > > Alexander Graf <address@hidden> wrote:
> > > > 
> > > >> 
> > > >>> On 12.06.14 11:39, Paolo Bonzini wrote:
> > > >>> Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
> > > >>>> Maybe just drop unnecessary stuff for new machine types?
> > > >>>> Then we won't need hacks to migrate it.
> > > >>> 
> > > >>> For any machine type it's trivial to migrate it.  All machine types 
> > > >>> including old ones can disregard the migrated values.
> > > >> 
> > > >> How about a patch like this before the actual LE awareness ones? With 
> > > >> this we should make virtio-serial config space completely independent 
> > > >> of 
> > > >> live migration.
> > > >> 
> > > >> Also since QEMU versions that do read these swapped values during 
> > > >> migration are not bi-endian aware, we can never get into a case where 
> > > >> a 
> > > >> cross-endian save needs to be considered ;).
> > > >> 
> > > >> 
> > > >> Alex
> > > >> 
> > > >> 
> > > >> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> > > >> index 2b647b6..73cb9b7 100644
> > > >> --- a/hw/char/virtio-serial-bus.c
> > > >> +++ b/hw/char/virtio-serial-bus.c
> > > >> @@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void 
> > > >> *opaque, int version_id)
> > > >>      uint32_t max_nr_ports, nr_active_ports, ports_map;
> > > >>      unsigned int i;
> > > >>      int ret;
> > > >> +    uint32_t tmp;
> > > >> 
> > > >>      if (version_id > 3) {
> > > >>          return -EINVAL;
> > > >> @@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void 
> > > >> *opaque, int version_id)
> > > >>          return 0;
> > > >>      }
> > > >> 
> > > >> -    /* The config space */
> > > >> -    qemu_get_be16s(f, &s->config.cols);
> > > >> -    qemu_get_be16s(f, &s->config.rows);
> > > >> -
> > > >> -    qemu_get_be32s(f, &max_nr_ports);
> > > >> -    tswap32s(&max_nr_ports);
> > > >> -    if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> > > >> -        /* Source could have had more ports than us. Fail migration. 
> > > >> */
> > > >> -        return -EINVAL;
> > > >> -    }
> > > >> +    /* Unused */
> > > >> +    qemu_get_be16s(f, &tmp);
> > > >> +    qemu_get_be16s(f, &tmp);
> > > >> +    qemu_get_be32s(f, &tmp);
> > > >> 
> > > >> +    max_nr_ports = tswap32(s->config.max_nr_ports);
> > > >>      for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
> > > >>          qemu_get_be32s(f, &ports_map);
> > > > 
> > > > For the moment, we have 0 < max_nr_ports < 32 so the source
> > > > machine only stores a single 32 bit value... If this limit
> > > > gets raised, we can end up sending more than that... and
> > > > only the source machine max_nr_ports value can give the
> > > > information...
> > > 
> > > Why? This value only ever gets set in realize, so it will not change 
> > > during the lifetime of the device - which means we don't need to migrate 
> > > it.
> > > 
> > 
> > I agree with the fact that the value does not change and should not be 
> > migrated in the first place.
> > I am just worried about the size of the active ports bitmap that is 
> > streamed in the for loop... it
> > is only 32 bit as of today, because we are limited to 32 ports. How would 
> > this work if the limit is
> > raised ? How can the destination machine know how many bits have to be read 
> > ?
> 
> When the destination machine is started with -M 2.1, it
> knows that it has to read 32 bit.
> If started with -M 3.0 it reads in 42 bits :)
> 

Okay ! I was completely missing this point... now things make sense at last ! :)
About Alex's patch, will you or Amit or Anthony apply it directly or should
I send it along with my patches for a full review ?

Thanks for your time. 

-- 
Gregory Kurz                                     address@hidden
                                                 address@hidden
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.