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

[Michael S. Tsirkin]

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


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