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

[Juan Quintela]

"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);
>> >> +        }
>> >>      }
>> >>  }
>> >> 
>> >
>> > 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)$

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

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

One is bad and the other is worse :(

Will take a look at creating that get_nvectors() helper.

Later, Juan.