[Qemu-devel] Re: [PATCH 15/41] virtio: remove save/load_queue for virtio

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 07:50:33PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <address@hidden> wrote:
> > On Wed, Dec 02, 2009 at 07:22:11PM +0100, Juan Quintela wrote:
> >> "Michael S. Tsirkin" <address@hidden> wrote:
> >> > On Wed, Dec 02, 2009 at 01:04:13PM +0100, Juan Quintela wrote:
> >> >> diff --git a/hw/virtio.c b/hw/virtio.c
> >> >> index c136005..b565bf9 100644
> >> >> --- a/hw/virtio.c
> >> >> +++ b/hw/virtio.c
> >> >> @@ -643,8 +643,10 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> >> >>          qemu_put_be32(f, vdev->vq[i].vring.num);
> >> >>          qemu_put_be64(f, vdev->vq[i].pa);
> >> >>          qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
> >> >> -        if (vdev->binding->save_queue)
> >> >> -            vdev->binding->save_queue(vdev->binding_opaque, i, f);
> >> >> +        if (vdev->type == VIRTIO_PCI &&
> >> >> +            virtio_pci_msix_present(vdev->binding_opaque)) {
> >> >> +            qemu_put_be16s(f, &vdev->vq[i].vector);
> >> >> +        }

Hmm, I just noticed that this also assumes
that vector # fits in 16 bit. correct for PCI, but
might be better not assume this in virtio.c

> >> >>      }
> >> >>  }
> >> >> 
> >> >
> >> > I think this will break build on systems without PCI
> >> > because virtio_pci.c is not linked in there.
> >> > Correct?
> >> 
> >> It compiles for syborg (it has pci).  There are no other users.
> >> 
> >> > Making generic virtio.c depend on virtio_pci.c looks
> >> > wrong in any case. We have bindings to
> >> > resolve exactly this dependency problem.
> >> 
> >> There is no way that you throw "this" blob to vmstate and it will know
> >> what to do there.  if it is needed, we can create an empty
> >> virtio_pci_msix_present() function for !CONFIG_PCI.
> >> 
> >> Later, Juan.
> >
> > That's not the idea. virtio knows nothing about msix etc.
> 
> this is patently false :)  I will agree if you would have done
> s/kwnows/shouldn't know/ :)
> 
>     int nvectors;
> 
> this is a field of VirtIODevice.
> and there is another one in virtio-pci.
> (master)$ grep -c nvectors hw/virtio.c
> 0
> (master)$
> 

vectors are the abstraction that we use.


> And you can see know what I mean by incestuous relation between virtio
> <-> virtio-pci.  To make things more interesting, it becomes a threesome
> with msix :(

These are just callbacks, no reason to call them names :)

> > This belongs
> > in the binding. If you want to know the number of vectors, please put
> > something like ->get_nvectors in the binding and call that to figure out
> > whether virtio has multivector.
> 
> We could do it whatever.  The big problem here is that virtio devices
> are (normally) virtio devices and pci devices.  There is no way that
> would fit it well with qemu.
> 
> There are two options:
> 
> - having virtio contain callbacks from virtio-pci.
> - having virtio-pci contain a virtio device.

Second one sounds sane. virtio provides
functions, virtio-pci calls them.

> One is bad and the other is worse :(
> 
> Will take a look at creating that get_nvectors() helper.
> 
> Later, Juan.