| Message ID | 1373661422-23606-4-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, Jul 12, 2013 at 09:36:57PM +0100, Peter Maydell wrote: > The MMIO virtio transport spec allows the guest to tell the host how > large the queue size is. Add virtio_queue_set_num() function which > implements this in the QEMU common virtio support code. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Probably needs to go back to default value on reset? Need to migrate? Is the default value a max legal value? If yes probably a good idea to enforce this. > --- > hw/virtio/virtio.c | 8 ++++++++ > include/hw/virtio/virtio.h | 1 + > 2 files changed, 9 insertions(+) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 8176c14..01b05f3 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -667,6 +667,14 @@ hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n) > return vdev->vq[n].pa; > } > > +void virtio_queue_set_num(VirtIODevice *vdev, int n, int num) > +{ > + if (num <= VIRTQUEUE_MAX_SIZE) { > + vdev->vq[n].vring.num = num; > + virtqueue_init(&vdev->vq[n]); > + } > +} > + > int virtio_queue_get_num(VirtIODevice *vdev, int n) > { > return vdev->vq[n].vring.num; > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index a6c5c53..95c4772 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -198,6 +198,7 @@ void virtio_config_writew(VirtIODevice *vdev, uint32_t addr, uint32_t data); > void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data); > void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr); > hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n); > +void virtio_queue_set_num(VirtIODevice *vdev, int n, int num); > int virtio_queue_get_num(VirtIODevice *vdev, int n); > void virtio_queue_notify(VirtIODevice *vdev, int n); > uint16_t virtio_queue_vector(VirtIODevice *vdev, int n); > -- > 1.7.9.5 >
On 25 July 2013 06:38, Michael S. Tsirkin <mst@redhat.com> wrote: > On Fri, Jul 12, 2013 at 09:36:57PM +0100, Peter Maydell wrote: >> The MMIO virtio transport spec allows the guest to tell the host how >> large the queue size is. Add virtio_queue_set_num() function which >> implements this in the QEMU common virtio support code. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > Probably needs to go back to default value on reset? Tricky, since the default value is "whatever was passed to virtio_add_queue()" and we don't save that anywhere. For virtio-mmio it is a guest bug to fail to write to the QueueNum register, so the current behaviour is not out of specification (and not harmful either AFAICT). I guess we could add a vring.defaultnum, which would be set by virtio_add_queue/virtio_del_queue, and have reset copy defaultnum into num. No migration needed for defaultnum because it's always the same for a particular qemu config. > Need to migrate? It's already migrated (though I'm not entirely sure why). > Is the default value a max legal value? If yes probably a good > idea to enforce this. virtio_add_queue() already enforces this -- it will abort() if you try to set up a queue with a default size greater than VIRTQUEUE_MAX_SIZE. thanks -- PMM
On Thu, Jul 25, 2013 at 09:50:21AM +0100, Peter Maydell wrote: > On 25 July 2013 06:38, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Fri, Jul 12, 2013 at 09:36:57PM +0100, Peter Maydell wrote: > >> The MMIO virtio transport spec allows the guest to tell the host how > >> large the queue size is. Add virtio_queue_set_num() function which > >> implements this in the QEMU common virtio support code. > >> > >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > > > Probably needs to go back to default value on reset? > > Tricky, since the default value is "whatever was passed to > virtio_add_queue()" and we don't save that anywhere. > > For virtio-mmio it is a guest bug to fail to write to the > QueueNum register, so the current behaviour is not out of > specification (and not harmful either AFAICT). Best not to leak info across reboots. Also if guest sets num = 0 it will cause all kind of harm, no? > I guess we could add a vring.defaultnum, which would be > set by virtio_add_queue/virtio_del_queue, and have reset > copy defaultnum into num. No migration needed for defaultnum > because it's always the same for a particular qemu config. Sounds good. > > Need to migrate? > > It's already migrated (though I'm not entirely sure why). > > > Is the default value a max legal value? If yes probably a good > > idea to enforce this. > > virtio_add_queue() already enforces this -- it will abort() > if you try to set up a queue with a default size greater > than VIRTQUEUE_MAX_SIZE. No I mean does default value have some meaning? Does host supply a hint of a max value besides VIRTQUEUE_MAX_SIZE? > > thanks > -- PMM
On 25 July 2013 10:03, Michael S. Tsirkin <mst@redhat.com> wrote: > On Thu, Jul 25, 2013 at 09:50:21AM +0100, Peter Maydell wrote: >> On 25 July 2013 06:38, Michael S. Tsirkin <mst@redhat.com> wrote: >> > On Fri, Jul 12, 2013 at 09:36:57PM +0100, Peter Maydell wrote: >> >> The MMIO virtio transport spec allows the guest to tell the host how >> >> large the queue size is. Add virtio_queue_set_num() function which >> >> implements this in the QEMU common virtio support code. >> >> >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> > >> > Probably needs to go back to default value on reset? >> >> Tricky, since the default value is "whatever was passed to >> virtio_add_queue()" and we don't save that anywhere. >> >> For virtio-mmio it is a guest bug to fail to write to the >> QueueNum register, so the current behaviour is not out of >> specification (and not harmful either AFAICT). > > Best not to leak info across reboots. > Also if guest sets num = 0 it will cause all kind of > harm, no? Yes, I had the thought on the way into work that we shouldn't allow the guest to switch between 0 and non-zero. >> I guess we could add a vring.defaultnum, which would be >> set by virtio_add_queue/virtio_del_queue, and have reset >> copy defaultnum into num. No migration needed for defaultnum >> because it's always the same for a particular qemu config. > > Sounds good. > >> > Need to migrate? >> >> It's already migrated (though I'm not entirely sure why). >> >> > Is the default value a max legal value? If yes probably a good >> > idea to enforce this. >> >> virtio_add_queue() already enforces this -- it will abort() >> if you try to set up a queue with a default size greater >> than VIRTQUEUE_MAX_SIZE. > > No I mean does default value have some meaning? It's not clear to me -- virtio_add_queue() doesn't document the semantics of its queue_size parameter. > Does host supply a hint of a max value > besides VIRTQUEUE_MAX_SIZE? The spec says there's a QueueNumMax register which returns the maximum for the selected queue. The current implementation of virtio-mmio always returns VIRTQUEUE_MAX_SIZE. We could return the default size as the QueueNumMax instead but that would seem to be limiting the guest unnecessarily. -- PMM
On 25 July 2013 10:03, Michael S. Tsirkin <mst@redhat.com> wrote: > On Thu, Jul 25, 2013 at 09:50:21AM +0100, Peter Maydell wrote: >> On 25 July 2013 06:38, Michael S. Tsirkin <mst@redhat.com> wrote: >> > Probably needs to go back to default value on reset? >> >> Tricky, since the default value is "whatever was passed to >> virtio_add_queue()" and we don't save that anywhere. >> >> For virtio-mmio it is a guest bug to fail to write to the >> QueueNum register, so the current behaviour is not out of >> specification (and not harmful either AFAICT). > > Best not to leak info across reboots. > Also if guest sets num = 0 it will cause all kind of > harm, no? > >> I guess we could add a vring.defaultnum, which would be >> set by virtio_add_queue/virtio_del_queue, and have reset >> copy defaultnum into num. No migration needed for defaultnum >> because it's always the same for a particular qemu config. So I had a look at implementing this, and I noticed that we already have some odd behaviour on reset. Specifically, virtio backends like net can create virtio queues based on guest behaviour (ie setting feature bits). But on reset, these queues aren't deleted, so a post-reset QEMU will look different from a from-scratch restarted QEMU... This in turn makes 'save defaultnum and have reset copy it into num' awkward, because defaultnum now needs to be migrated (otherwise it would do the wrong thing on a reset after a VM migration). -- PMM
On Thu, Jul 25, 2013 at 01:30:15PM +0100, Peter Maydell wrote: > On 25 July 2013 10:03, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Jul 25, 2013 at 09:50:21AM +0100, Peter Maydell wrote: > >> On 25 July 2013 06:38, Michael S. Tsirkin <mst@redhat.com> wrote: > >> > Probably needs to go back to default value on reset? > >> > >> Tricky, since the default value is "whatever was passed to > >> virtio_add_queue()" and we don't save that anywhere. > >> > >> For virtio-mmio it is a guest bug to fail to write to the > >> QueueNum register, so the current behaviour is not out of > >> specification (and not harmful either AFAICT). > > > > Best not to leak info across reboots. > > Also if guest sets num = 0 it will cause all kind of > > harm, no? > > > >> I guess we could add a vring.defaultnum, which would be > >> set by virtio_add_queue/virtio_del_queue, and have reset > >> copy defaultnum into num. No migration needed for defaultnum > >> because it's always the same for a particular qemu config. > > So I had a look at implementing this, and I noticed that > we already have some odd behaviour on reset. Specifically, > virtio backends like net can create virtio queues based on > guest behaviour (ie setting feature bits). But on reset, > these queues aren't deleted, so a post-reset QEMU will look > different from a from-scratch restarted QEMU... That's a bug. Thanks for the report. :) > This in turn makes 'save defaultnum and have reset copy it > into num' awkward, because defaultnum now needs to be > migrated (otherwise it would do the wrong thing on a reset > after a VM migration). > > -- PMM Looks like we'll have to fix the bug first :(
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 8176c14..01b05f3 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -667,6 +667,14 @@ hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n) return vdev->vq[n].pa; } +void virtio_queue_set_num(VirtIODevice *vdev, int n, int num) +{ + if (num <= VIRTQUEUE_MAX_SIZE) { + vdev->vq[n].vring.num = num; + virtqueue_init(&vdev->vq[n]); + } +} + int virtio_queue_get_num(VirtIODevice *vdev, int n) { return vdev->vq[n].vring.num; diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index a6c5c53..95c4772 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -198,6 +198,7 @@ void virtio_config_writew(VirtIODevice *vdev, uint32_t addr, uint32_t data); void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data); void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr); hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n); +void virtio_queue_set_num(VirtIODevice *vdev, int n, int num); int virtio_queue_get_num(VirtIODevice *vdev, int n); void virtio_queue_notify(VirtIODevice *vdev, int n); uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
The MMIO virtio transport spec allows the guest to tell the host how large the queue size is. Add virtio_queue_set_num() function which implements this in the QEMU common virtio support code. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/virtio/virtio.c | 8 ++++++++ include/hw/virtio/virtio.h | 1 + 2 files changed, 9 insertions(+)