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

[Michael S. Tsirkin]

On Wed, May 13, 2015 at 05:03:35PM +0200, Cornelia Huck wrote:
> On Wed, 13 May 2015 16:58:58 +0200
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Wed, May 13, 2015 at 04:42:02PM +0200, Cornelia Huck wrote:
> > > Currently, config_vector is managed in VirtIODevice, but migration is
> > > handled by the transport; this led to one transport using config_vector
> > > migrating correctly (pci), while the other forgot it (ccw).
> > > 
> > > Let's have the core handle migration of config_vector in a vmstate
> > > subsection instead, so that we can keep it backwards compatible and
> > > no additional code is needed if config_vector is not used at all. Also
> > > provide a callback for virtio-pci so they can avoid introducing a
> > > subsection that is not needed and still be compatible in both directions.
> > > 
> > > Reported-by: "Jason J. Herne" <address@hidden>
> > > Signed-off-by: Cornelia Huck <address@hidden>
> > 
> > I'm inclined to say just fix ccw, don't touch core.
> > queue vectors are still saved by transports, why special-case
> > config_vector?
> 
> - config_vector is in the common VirtIODevice structure, not in
>   transport-specific code

So is the queue vector.

> - new transports may make the same mistake

Well, we can't just pile up new interfaces that all do the same
thing slightly differently.
If you really want to migrate vectors in core, you can do

    if (k->should_save_vectors &&
        k->should_save_vectors(qbus->parent)) {
        qemu_put_be16(f, virtio_queue_vector(vdev, n));
    }

and similarly for queue vectors.

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


> > 
> > > ---
> > >  hw/virtio/virtio-pci.c         | 14 ++++++++++++++
> > >  hw/virtio/virtio.c             | 25 +++++++++++++++++++++++++
> > >  include/hw/virtio/virtio-bus.h |  5 +++++
> > >  3 files changed, 44 insertions(+)
> > > 
> > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > index 867c9d1..4959b7d 100644
> > > --- a/hw/virtio/virtio-pci.c
> > > +++ b/hw/virtio/virtio-pci.c
> > > @@ -971,6 +971,19 @@ static void virtio_pci_device_unplugged(DeviceState 
> > > *d)
> > >      virtio_pci_stop_ioeventfd(proxy);
> > >  }
> > >  
> > > +static bool virtio_pci_needs_confvec(DeviceState *d)
> > > +{
> > > +    VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
> > > +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> > > +
> > > +    /*
> > > +     * We don't want the core to create an unneeded vmstate subsection
> > > +     * when we already migrate the config vector ourselves.
> > > +     */
> > > +    return (vdev->config_vector != VIRTIO_NO_VECTOR) &&
> > > +        !msix_present(&proxy->pci_dev);
> > 
> > As config_vector is ignored without msix,
> > why is it a good idea to break migration if it's wrong?
> 
> This should be covered by the first check, no?

Why should it?

> > 
> > > +}
> > > +
> > >  static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
> > >  {
> > >      VirtIOPCIProxy *dev = VIRTIO_PCI(pci_dev);
> > > @@ -1505,6 +1518,7 @@ static void virtio_pci_bus_class_init(ObjectClass 
> > > *klass, void *data)
> > >      k->device_plugged = virtio_pci_device_plugged;
> > >      k->device_unplugged = virtio_pci_device_unplugged;
> > >      k->query_nvectors = virtio_pci_query_nvectors;
> > > +    k->needs_confvec = virtio_pci_needs_confvec;
> > >  }
> > >  
> > >  static const TypeInfo virtio_pci_bus_info = {
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index 6985e76..3af530e 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -903,6 +903,27 @@ static const VMStateDescription 
> > > vmstate_virtio_device_endian = {
> > >      }
> > >  };
> > >  
> > > +static bool virtio_device_confvec_needed(void *opaque)
> > > +{
> > > +    VirtIODevice *vdev = opaque;
> > > +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
> > > +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> > > +
> > > +    return k->needs_confvec ?
> > > +        k->needs_confvec(qbus->parent) :
> > > +        (vdev->config_vector != VIRTIO_NO_VECTOR);
> > > +}
> > > +
> > > +static const VMStateDescription vmstate_virtio_device_confvec = {
> > > +    .name = "virtio/config_vector",
> > > +    .version_id = 1,
> > > +    .minimum_version_id = 1,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_UINT16(config_vector, VirtIODevice),
> > > +        VMSTATE_END_OF_LIST()
> > > +    }
> > > +};
> > > +
> > >  static const VMStateDescription vmstate_virtio = {
> > >      .name = "virtio",
> > >      .version_id = 1,
> > > @@ -916,6 +937,10 @@ static const VMStateDescription vmstate_virtio = {
> > >              .vmsd = &vmstate_virtio_device_endian,
> > >              .needed = &virtio_device_endian_needed
> > >          },
> > > +        {
> > > +            .vmsd = &vmstate_virtio_device_confvec,
> > > +            .needed = &virtio_device_confvec_needed,
> > > +        },
> > >          { 0 }
> > >      }
> > >  };
> > > diff --git a/include/hw/virtio/virtio-bus.h 
> > > b/include/hw/virtio/virtio-bus.h
> > > index a4588ca..79e6e8b 100644
> > > --- a/include/hw/virtio/virtio-bus.h
> > > +++ b/include/hw/virtio/virtio-bus.h
> > > @@ -69,6 +69,11 @@ typedef struct VirtioBusClass {
> > >       * Note that changing this will break migration for this transport.
> > >       */
> > >      bool has_variable_vring_alignment;
> > > +    /*
> > > +     * (optional) Does the bus want the core to handle config_vector
> > > +     * migration? This is for backwards compatibility only.
> > > +     */
> > > +    bool (*needs_confvec)(DeviceState *d);
> > >  } VirtioBusClass;
> > >  
> > >  struct VirtioBusState {
> > > -- 
> > > 2.1.4
> >