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

[Greg Kurz]

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 ?

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