| Message ID | 67f3d61df137cfd59fbf5da6b0b999cdf8d28758.1259754427.git.quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Dec 02, 2009 at 01:04:13PM +0100, Juan Quintela wrote: > It was used only for PCI virtio devices, state that explicitely > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > hw/virtio-pci.c | 24 ++++++------------------ > hw/virtio.c | 22 ++++++++++++++++------ > hw/virtio.h | 4 ++-- > 3 files changed, 24 insertions(+), 26 deletions(-) > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > index 45d0adc..12f3961 100644 > --- a/hw/virtio-pci.c > +++ b/hw/virtio-pci.c > @@ -151,28 +151,18 @@ const VMStateDescription vmstate_virtio_pci_config = { > } > }; > > -static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f) > +bool virtio_pci_msix_present(void *opaque) > { > VirtIOPCIProxy *proxy = opaque; > - if (msix_present(&proxy->pci_dev)) > - qemu_put_be16(f, virtio_queue_vector(proxy->vdev, n)); > -} > > + return msix_present(&proxy->pci_dev); > +} > > -static int virtio_pci_load_queue(void * opaque, int n, QEMUFile *f) > +int virtio_pci_msix_vector_use(void *opaque, unsigned vector) > { > VirtIOPCIProxy *proxy = opaque; > - uint16_t vector; > - if (msix_present(&proxy->pci_dev)) { > - qemu_get_be16s(f, &vector); > - } else { > - vector = VIRTIO_NO_VECTOR; > - } > - virtio_queue_set_vector(proxy->vdev, n, vector); > - if (vector != VIRTIO_NO_VECTOR) { > - return msix_vector_use(&proxy->pci_dev, vector); > - } > - return 0; > + > + return msix_vector_use(&proxy->pci_dev, vector); > } > > static void virtio_pci_reset(DeviceState *d) > @@ -402,8 +392,6 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > > static const VirtIOBindings virtio_pci_bindings = { > .notify = virtio_pci_notify, > - .save_queue = virtio_pci_save_queue, > - .load_queue = virtio_pci_load_queue, > }; > > static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev, > 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? Making generic virtio.c depend on virtio_pci.c looks wrong in any case. We have bindings to resolve exactly this dependency problem. > @@ -676,10 +678,18 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f) > if (vdev->vq[i].pa) { > virtqueue_init(&vdev->vq[i]); > } > - if (vdev->binding->load_queue) { > - ret = vdev->binding->load_queue(vdev->binding_opaque, i, f); > - if (ret) > - return ret; > + if (vdev->type == VIRTIO_PCI) { > + if (virtio_pci_msix_present(vdev->binding_opaque)) { > + qemu_get_be16s(f, &vdev->vq[i].vector); > + } else { > + vdev->vq[i].vector = VIRTIO_NO_VECTOR; > + } > + if (vdev->vq[i].vector != VIRTIO_NO_VECTOR) { > + ret = virtio_pci_msix_vector_use(vdev->binding_opaque, > + vdev->vq[i].vector); > + if (ret) > + return ret; > + } > } > } > > diff --git a/hw/virtio.h b/hw/virtio.h > index 9d2e2cc..91a6c10 100644 > --- a/hw/virtio.h > +++ b/hw/virtio.h > @@ -78,8 +78,6 @@ typedef struct VirtQueueElement > > typedef struct { > void (*notify)(void * opaque, uint16_t vector); > - void (*save_queue)(void * opaque, int n, QEMUFile *f); > - int (*load_queue)(void * opaque, int n, QEMUFile *f); > } VirtIOBindings; > > #define VIRTIO_PCI_QUEUE_MAX 16 > @@ -176,5 +174,7 @@ void virtio_net_exit(VirtIODevice *vdev); > > /* virtio-pci. */ > extern const VMStateDescription vmstate_virtio_pci_config; > +bool virtio_pci_msix_present(void *opaque); > +int virtio_pci_msix_vector_use(void *opaque, unsigned vector); > > #endif > -- > 1.6.5.2
"Michael S. Tsirkin" <mst@redhat.com> 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.
On Wed, Dec 02, 2009 at 07:22:11PM +0100, Juan Quintela wrote: > "Michael S. Tsirkin" <mst@redhat.com> 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 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.
"Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Dec 02, 2009 at 07:22:11PM +0100, Juan Quintela wrote: >> "Michael S. Tsirkin" <mst@redhat.com> 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.
On Wed, Dec 02, 2009 at 07:50:33PM +0100, Juan Quintela wrote: > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > On Wed, Dec 02, 2009 at 07:22:11PM +0100, Juan Quintela wrote: > >> "Michael S. Tsirkin" <mst@redhat.com> 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.
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index 45d0adc..12f3961 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -151,28 +151,18 @@ const VMStateDescription vmstate_virtio_pci_config = { } }; -static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f) +bool virtio_pci_msix_present(void *opaque) { VirtIOPCIProxy *proxy = opaque; - if (msix_present(&proxy->pci_dev)) - qemu_put_be16(f, virtio_queue_vector(proxy->vdev, n)); -} + return msix_present(&proxy->pci_dev); +} -static int virtio_pci_load_queue(void * opaque, int n, QEMUFile *f) +int virtio_pci_msix_vector_use(void *opaque, unsigned vector) { VirtIOPCIProxy *proxy = opaque; - uint16_t vector; - if (msix_present(&proxy->pci_dev)) { - qemu_get_be16s(f, &vector); - } else { - vector = VIRTIO_NO_VECTOR; - } - virtio_queue_set_vector(proxy->vdev, n, vector); - if (vector != VIRTIO_NO_VECTOR) { - return msix_vector_use(&proxy->pci_dev, vector); - } - return 0; + + return msix_vector_use(&proxy->pci_dev, vector); } static void virtio_pci_reset(DeviceState *d) @@ -402,8 +392,6 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, static const VirtIOBindings virtio_pci_bindings = { .notify = virtio_pci_notify, - .save_queue = virtio_pci_save_queue, - .load_queue = virtio_pci_load_queue, }; static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev, 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); + } } } @@ -676,10 +678,18 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f) if (vdev->vq[i].pa) { virtqueue_init(&vdev->vq[i]); } - if (vdev->binding->load_queue) { - ret = vdev->binding->load_queue(vdev->binding_opaque, i, f); - if (ret) - return ret; + if (vdev->type == VIRTIO_PCI) { + if (virtio_pci_msix_present(vdev->binding_opaque)) { + qemu_get_be16s(f, &vdev->vq[i].vector); + } else { + vdev->vq[i].vector = VIRTIO_NO_VECTOR; + } + if (vdev->vq[i].vector != VIRTIO_NO_VECTOR) { + ret = virtio_pci_msix_vector_use(vdev->binding_opaque, + vdev->vq[i].vector); + if (ret) + return ret; + } } } diff --git a/hw/virtio.h b/hw/virtio.h index 9d2e2cc..91a6c10 100644 --- a/hw/virtio.h +++ b/hw/virtio.h @@ -78,8 +78,6 @@ typedef struct VirtQueueElement typedef struct { void (*notify)(void * opaque, uint16_t vector); - void (*save_queue)(void * opaque, int n, QEMUFile *f); - int (*load_queue)(void * opaque, int n, QEMUFile *f); } VirtIOBindings; #define VIRTIO_PCI_QUEUE_MAX 16 @@ -176,5 +174,7 @@ void virtio_net_exit(VirtIODevice *vdev); /* virtio-pci. */ extern const VMStateDescription vmstate_virtio_pci_config; +bool virtio_pci_msix_present(void *opaque); +int virtio_pci_msix_vector_use(void *opaque, unsigned vector); #endif
It was used only for PCI virtio devices, state that explicitely Signed-off-by: Juan Quintela <quintela@redhat.com> --- hw/virtio-pci.c | 24 ++++++------------------ hw/virtio.c | 22 ++++++++++++++++------ hw/virtio.h | 4 ++-- 3 files changed, 24 insertions(+), 26 deletions(-)