Patchwork [v2,3/8] virtio: Add support for guest setting of queue size

login
register
mail settings
Submitter Peter Maydell
Date July 12, 2013, 8:36 p.m.
Message ID <1373661422-23606-4-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/258805/
State New
Headers show

Comments

Peter Maydell - July 12, 2013, 8:36 p.m.
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(+)
Michael S. Tsirkin - July 25, 2013, 5:38 a.m.
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
>
Peter Maydell - July 25, 2013, 8:50 a.m.
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
Michael S. Tsirkin - July 25, 2013, 9:03 a.m.
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
Peter Maydell - July 25, 2013, 9:34 a.m.
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
Peter Maydell - July 25, 2013, 12:30 p.m.
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
Michael S. Tsirkin - July 25, 2013, 12:38 p.m.
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 :(

Patch

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);