1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH V2 RFT] vhost_net: don't set backend for the uninitialized virtqueue

Jason Wang posted 1 patch 5 years, 1 month ago
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test asan passed
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190325035613.18192-1-jasowang@redhat.com
Maintainers: Jason Wang <jasowang@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>
There is a newer version of this series
hw/net/vhost_net.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
[Qemu-devel] [PATCH V2 RFT] vhost_net: don't set backend for the uninitialized virtqueue
Posted by Jason Wang 5 years, 1 month ago 
We used to set backend unconditionally, this won't work for some
guests (e.g windows driver) who may not initialize all virtqueues. For
kernel backend, this will fail since it may try to validate the rings
during setting backend.

Fixing this by simply skipping the backend set when we find desc is
not ready.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/vhost_net.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index be3cc88370..04fd924d15 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -221,6 +221,7 @@ static int vhost_net_start_one(struct vhost_net *net,
                                VirtIODevice *dev)
 {
     struct vhost_vring_file file = { };
+    hwaddr a;
     int r;
 
     net->dev.nvqs = 2;
@@ -244,6 +245,13 @@ static int vhost_net_start_one(struct vhost_net *net,
         qemu_set_fd_handler(net->backend, NULL, NULL, NULL);
         file.fd = net->backend;
         for (file.index = 0; file.index < net->dev.nvqs; ++file.index) {
+            a = virtio_queue_get_desc_addr(dev,
+                                           net->dev.vq_index +
+                                           file.index);
+            if (a == 0) {
+                /* Queue might not be ready for start */
+                continue;
+            }
             r = vhost_net_set_backend(&net->dev, &file);
             if (r < 0) {
                 r = -errno;
@@ -256,6 +264,13 @@ fail:
     file.fd = -1;
     if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
         while (file.index-- > 0) {
+            a = virtio_queue_get_desc_addr(dev,
+                                           net->dev.vq_index +
+                                           file.index);
+            if (a == 0) {
+                /* Queue might not be ready for start */
+                continue;
+            }
             int r = vhost_net_set_backend(&net->dev, &file);
             assert(r >= 0);
         }
-- 
2.19.1
Re: [Qemu-devel] [PATCH V2 RFT] vhost_net: don't set backend for the uninitialized virtqueue
Posted by Yuri Benditovich 5 years, 1 month ago 
Hi Jason,

This seems ok now (4 previous patches + this one)

On Mon, Mar 25, 2019 at 5:56 AM Jason Wang <jasowang@redhat.com> wrote:
>
> We used to set backend unconditionally, this won't work for some
> guests (e.g windows driver) who may not initialize all virtqueues. For
> kernel backend, this will fail since it may try to validate the rings
> during setting backend.
>
> Fixing this by simply skipping the backend set when we find desc is
> not ready.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/vhost_net.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index be3cc88370..04fd924d15 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -221,6 +221,7 @@ static int vhost_net_start_one(struct vhost_net *net,
>                                 VirtIODevice *dev)
>  {
>      struct vhost_vring_file file = { };
> +    hwaddr a;
>      int r;
>
>      net->dev.nvqs = 2;
> @@ -244,6 +245,13 @@ static int vhost_net_start_one(struct vhost_net *net,
>          qemu_set_fd_handler(net->backend, NULL, NULL, NULL);
>          file.fd = net->backend;
>          for (file.index = 0; file.index < net->dev.nvqs; ++file.index) {
> +            a = virtio_queue_get_desc_addr(dev,
> +                                           net->dev.vq_index +
> +                                           file.index);
> +            if (a == 0) {
> +                /* Queue might not be ready for start */
> +                continue;
> +            }
>              r = vhost_net_set_backend(&net->dev, &file);
>              if (r < 0) {
>                  r = -errno;
> @@ -256,6 +264,13 @@ fail:
>      file.fd = -1;
>      if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
>          while (file.index-- > 0) {
> +            a = virtio_queue_get_desc_addr(dev,
> +                                           net->dev.vq_index +
> +                                           file.index);
> +            if (a == 0) {
> +                /* Queue might not be ready for start */
> +                continue;
> +            }
>              int r = vhost_net_set_backend(&net->dev, &file);
>              assert(r >= 0);
>          }
> --
> 2.19.1
>
Re: [Qemu-devel] [PATCH V2 RFT] vhost_net: don't set backend for the uninitialized virtqueue
Posted by Jason Wang 5 years, 1 month ago 
On 2019/3/25 下午5:56, Yuri Benditovich wrote:
> Hi Jason,
>
> This seems ok now (4 previous patches + this one)


Thanks for the testing. Could you test with just this patch? Since even 
without 4 previous patch, we've already had the check through 
virtio_queue_get_desc_addr() in vhost_virtqueue_start().


>
> On Mon, Mar 25, 2019 at 5:56 AM Jason Wang <jasowang@redhat.com> wrote:
>> We used to set backend unconditionally, this won't work for some
>> guests (e.g windows driver) who may not initialize all virtqueues. For
>> kernel backend, this will fail since it may try to validate the rings
>> during setting backend.
>>
>> Fixing this by simply skipping the backend set when we find desc is
>> not ready.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   hw/net/vhost_net.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>> index be3cc88370..04fd924d15 100644
>> --- a/hw/net/vhost_net.c
>> +++ b/hw/net/vhost_net.c
>> @@ -221,6 +221,7 @@ static int vhost_net_start_one(struct vhost_net *net,
>>                                  VirtIODevice *dev)
>>   {
>>       struct vhost_vring_file file = { };
>> +    hwaddr a;
>>       int r;
>>
>>       net->dev.nvqs = 2;
>> @@ -244,6 +245,13 @@ static int vhost_net_start_one(struct vhost_net *net,
>>           qemu_set_fd_handler(net->backend, NULL, NULL, NULL);
>>           file.fd = net->backend;
>>           for (file.index = 0; file.index < net->dev.nvqs; ++file.index) {
>> +            a = virtio_queue_get_desc_addr(dev,
>> +                                           net->dev.vq_index +
>> +                                           file.index);
>> +            if (a == 0) {
>> +                /* Queue might not be ready for start */
>> +                continue;
>> +            }
>>               r = vhost_net_set_backend(&net->dev, &file);
>>               if (r < 0) {
>>                   r = -errno;
>> @@ -256,6 +264,13 @@ fail:
>>       file.fd = -1;
>>       if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
>>           while (file.index-- > 0) {
>> +            a = virtio_queue_get_desc_addr(dev,
>> +                                           net->dev.vq_index +
>> +                                           file.index);
>> +            if (a == 0) {
>> +                /* Queue might not be ready for start */
>> +                continue;
>> +            }
>>               int r = vhost_net_set_backend(&net->dev, &file);
>>               assert(r >= 0);
>>           }
>> --
>> 2.19.1
>>
Re: [Qemu-devel] [PATCH V2 RFT] vhost_net: don't set backend for the uninitialized virtqueue
Posted by Yuri Benditovich 5 years, 1 month ago 
You're right, with just one latest patch things work.

On Mon, Mar 25, 2019 at 11:59 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2019/3/25 下午5:56, Yuri Benditovich wrote:
> > Hi Jason,
> >
> > This seems ok now (4 previous patches + this one)
>
>
> Thanks for the testing. Could you test with just this patch? Since even
> without 4 previous patch, we've already had the check through
> virtio_queue_get_desc_addr() in vhost_virtqueue_start().
>
>
> >
> > On Mon, Mar 25, 2019 at 5:56 AM Jason Wang <jasowang@redhat.com> wrote:
> >> We used to set backend unconditionally, this won't work for some
> >> guests (e.g windows driver) who may not initialize all virtqueues. For
> >> kernel backend, this will fail since it may try to validate the rings
> >> during setting backend.
> >>
> >> Fixing this by simply skipping the backend set when we find desc is
> >> not ready.
> >>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> ---
> >>   hw/net/vhost_net.c | 15 +++++++++++++++
> >>   1 file changed, 15 insertions(+)
> >>
> >> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> >> index be3cc88370..04fd924d15 100644
> >> --- a/hw/net/vhost_net.c
> >> +++ b/hw/net/vhost_net.c
> >> @@ -221,6 +221,7 @@ static int vhost_net_start_one(struct vhost_net *net,
> >>                                  VirtIODevice *dev)
> >>   {
> >>       struct vhost_vring_file file = { };
> >> +    hwaddr a;
> >>       int r;
> >>
> >>       net->dev.nvqs = 2;
> >> @@ -244,6 +245,13 @@ static int vhost_net_start_one(struct vhost_net *net,
> >>           qemu_set_fd_handler(net->backend, NULL, NULL, NULL);
> >>           file.fd = net->backend;
> >>           for (file.index = 0; file.index < net->dev.nvqs; ++file.index) {
> >> +            a = virtio_queue_get_desc_addr(dev,
> >> +                                           net->dev.vq_index +
> >> +                                           file.index);
> >> +            if (a == 0) {
> >> +                /* Queue might not be ready for start */
> >> +                continue;
> >> +            }
> >>               r = vhost_net_set_backend(&net->dev, &file);
> >>               if (r < 0) {
> >>                   r = -errno;
> >> @@ -256,6 +264,13 @@ fail:
> >>       file.fd = -1;
> >>       if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
> >>           while (file.index-- > 0) {
> >> +            a = virtio_queue_get_desc_addr(dev,
> >> +                                           net->dev.vq_index +
> >> +                                           file.index);
> >> +            if (a == 0) {
> >> +                /* Queue might not be ready for start */
> >> +                continue;
> >> +            }
> >>               int r = vhost_net_set_backend(&net->dev, &file);
> >>               assert(r >= 0);
> >>           }
> >> --
> >> 2.19.1
> >>
Re: [Qemu-devel] [PATCH V2 RFT] vhost_net: don't set backend for the uninitialized virtqueue
Posted by Michael S. Tsirkin 5 years, 1 month ago 
On Mon, Mar 25, 2019 at 11:56:13AM +0800, Jason Wang wrote:
> We used to set backend unconditionally, this won't work for some
> guests (e.g windows driver) who may not initialize all virtqueues. For
> kernel backend, this will fail since it may try to validate the rings
> during setting backend.
> 
> Fixing this by simply skipping the backend set when we find desc is
> not ready.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/vhost_net.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index be3cc88370..04fd924d15 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -221,6 +221,7 @@ static int vhost_net_start_one(struct vhost_net *net,
>                                 VirtIODevice *dev)
>  {
>      struct vhost_vring_file file = { };
> +    hwaddr a;
>      int r;
>  
>      net->dev.nvqs = 2;
> @@ -244,6 +245,13 @@ static int vhost_net_start_one(struct vhost_net *net,
>          qemu_set_fd_handler(net->backend, NULL, NULL, NULL);
>          file.fd = net->backend;
>          for (file.index = 0; file.index < net->dev.nvqs; ++file.index) {
> +            a = virtio_queue_get_desc_addr(dev,
> +                                           net->dev.vq_index +
> +                                           file.index);
> +            if (a == 0) {
> +                /* Queue might not be ready for start */
> +                continue;
> +            }
>              r = vhost_net_set_backend(&net->dev, &file);
>              if (r < 0) {
>                  r = -errno;
> @@ -256,6 +264,13 @@ fail:
>      file.fd = -1;
>      if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
>          while (file.index-- > 0) {
> +            a = virtio_queue_get_desc_addr(dev,
> +                                           net->dev.vq_index +
> +                                           file.index);
> +            if (a == 0) {
> +                /* Queue might not be ready for start */
> +                continue;
> +            }
>              int r = vhost_net_set_backend(&net->dev, &file);
>              assert(r >= 0);
>          }


I think we want an API that explicitly says "queue is enabled".
For 0.X it will return !!addr. For 1.X it will return enabled.


At the moment enabled seems to be saved but ignored at least
in case of virtio-pci and I think that's the real bug.


> -- 
> 2.19.1
Re: [Qemu-devel] [PATCH V2 RFT] vhost_net: don't set backend for the uninitialized virtqueue
Posted by Jason Wang 5 years, 1 month ago 
On 2019/3/25 下午8:32, Michael S. Tsirkin wrote:
> On Mon, Mar 25, 2019 at 11:56:13AM +0800, Jason Wang wrote:
>> We used to set backend unconditionally, this won't work for some
>> guests (e.g windows driver) who may not initialize all virtqueues. For
>> kernel backend, this will fail since it may try to validate the rings
>> during setting backend.
>>
>> Fixing this by simply skipping the backend set when we find desc is
>> not ready.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   hw/net/vhost_net.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>> index be3cc88370..04fd924d15 100644
>> --- a/hw/net/vhost_net.c
>> +++ b/hw/net/vhost_net.c
>> @@ -221,6 +221,7 @@ static int vhost_net_start_one(struct vhost_net *net,
>>                                  VirtIODevice *dev)
>>   {
>>       struct vhost_vring_file file = { };
>> +    hwaddr a;
>>       int r;
>>   
>>       net->dev.nvqs = 2;
>> @@ -244,6 +245,13 @@ static int vhost_net_start_one(struct vhost_net *net,
>>           qemu_set_fd_handler(net->backend, NULL, NULL, NULL);
>>           file.fd = net->backend;
>>           for (file.index = 0; file.index < net->dev.nvqs; ++file.index) {
>> +            a = virtio_queue_get_desc_addr(dev,
>> +                                           net->dev.vq_index +
>> +                                           file.index);
>> +            if (a == 0) {
>> +                /* Queue might not be ready for start */
>> +                continue;
>> +            }
>>               r = vhost_net_set_backend(&net->dev, &file);
>>               if (r < 0) {
>>                   r = -errno;
>> @@ -256,6 +264,13 @@ fail:
>>       file.fd = -1;
>>       if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
>>           while (file.index-- > 0) {
>> +            a = virtio_queue_get_desc_addr(dev,
>> +                                           net->dev.vq_index +
>> +                                           file.index);
>> +            if (a == 0) {
>> +                /* Queue might not be ready for start */
>> +                continue;
>> +            }
>>               int r = vhost_net_set_backend(&net->dev, &file);
>>               assert(r >= 0);
>>           }
>
> I think we want an API that explicitly says "queue is enabled".
> For 0.X it will return !!addr. For 1.X it will return enabled.


For 1.x, desc.addr won't be set until queue_enabled is set through 
virtio_queue_set_vrings(). And it looks to me ccw did something similar 
of  CMD_SET_VQ.

So we're ok actually?

Thanks


>
>
> At the moment enabled seems to be saved but ignored at least
> in case of virtio-pci and I think that's the real bug.
>
>
>> -- 
>> 2.19.1
Re: [Qemu-devel] [PATCH V2 RFT] vhost_net: don't set backend for the uninitialized virtqueue
Posted by Michael S. Tsirkin 5 years, 1 month ago 
On Tue, Mar 26, 2019 at 02:59:19PM +0800, Jason Wang wrote:
> 
> On 2019/3/25 下午8:32, Michael S. Tsirkin wrote:
> > On Mon, Mar 25, 2019 at 11:56:13AM +0800, Jason Wang wrote:
> > > We used to set backend unconditionally, this won't work for some
> > > guests (e.g windows driver) who may not initialize all virtqueues. For
> > > kernel backend, this will fail since it may try to validate the rings
> > > during setting backend.
> > > 
> > > Fixing this by simply skipping the backend set when we find desc is
> > > not ready.
> > > 
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >   hw/net/vhost_net.c | 15 +++++++++++++++
> > >   1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > index be3cc88370..04fd924d15 100644
> > > --- a/hw/net/vhost_net.c
> > > +++ b/hw/net/vhost_net.c
> > > @@ -221,6 +221,7 @@ static int vhost_net_start_one(struct vhost_net *net,
> > >                                  VirtIODevice *dev)
> > >   {
> > >       struct vhost_vring_file file = { };
> > > +    hwaddr a;
> > >       int r;
> > >       net->dev.nvqs = 2;
> > > @@ -244,6 +245,13 @@ static int vhost_net_start_one(struct vhost_net *net,
> > >           qemu_set_fd_handler(net->backend, NULL, NULL, NULL);
> > >           file.fd = net->backend;
> > >           for (file.index = 0; file.index < net->dev.nvqs; ++file.index) {
> > > +            a = virtio_queue_get_desc_addr(dev,
> > > +                                           net->dev.vq_index +
> > > +                                           file.index);
> > > +            if (a == 0) {
> > > +                /* Queue might not be ready for start */
> > > +                continue;
> > > +            }
> > >               r = vhost_net_set_backend(&net->dev, &file);
> > >               if (r < 0) {
> > >                   r = -errno;
> > > @@ -256,6 +264,13 @@ fail:
> > >       file.fd = -1;
> > >       if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
> > >           while (file.index-- > 0) {
> > > +            a = virtio_queue_get_desc_addr(dev,
> > > +                                           net->dev.vq_index +
> > > +                                           file.index);
> > > +            if (a == 0) {
> > > +                /* Queue might not be ready for start */
> > > +                continue;
> > > +            }
> > >               int r = vhost_net_set_backend(&net->dev, &file);
> > >               assert(r >= 0);
> > >           }
> > 
> > I think we want an API that explicitly says "queue is enabled".
> > For 0.X it will return !!addr. For 1.X it will return enabled.
> 
> 
> For 1.x, desc.addr won't be set until queue_enabled is set through
> virtio_queue_set_vrings(). And it looks to me ccw did something similar of 
> CMD_SET_VQ.
> 
> So we're ok actually?
> 
> Thanks
> 

OK maybe but why can't we use an explicit API?
0.X can use addr != 0 trick since there's no queue_enabled.

> > 
> > 
> > At the moment enabled seems to be saved but ignored at least
> > in case of virtio-pci and I think that's the real bug.
> > 
> > 
> > > -- 
> > > 2.19.1
Re: [Qemu-devel] [PATCH V2 RFT] vhost_net: don't set backend for the uninitialized virtqueue
Posted by Jason Wang 5 years, 1 month ago 
On 2019/3/26 下午8:49, Michael S. Tsirkin wrote:
> On Tue, Mar 26, 2019 at 02:59:19PM +0800, Jason Wang wrote:
>> On 2019/3/25 下午8:32, Michael S. Tsirkin wrote:
>>> On Mon, Mar 25, 2019 at 11:56:13AM +0800, Jason Wang wrote:
>>>> We used to set backend unconditionally, this won't work for some
>>>> guests (e.g windows driver) who may not initialize all virtqueues. For
>>>> kernel backend, this will fail since it may try to validate the rings
>>>> during setting backend.
>>>>
>>>> Fixing this by simply skipping the backend set when we find desc is
>>>> not ready.
>>>>
>>>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>>>> ---
>>>>    hw/net/vhost_net.c | 15 +++++++++++++++
>>>>    1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>>>> index be3cc88370..04fd924d15 100644
>>>> --- a/hw/net/vhost_net.c
>>>> +++ b/hw/net/vhost_net.c
>>>> @@ -221,6 +221,7 @@ static int vhost_net_start_one(struct vhost_net *net,
>>>>                                   VirtIODevice *dev)
>>>>    {
>>>>        struct vhost_vring_file file = { };
>>>> +    hwaddr a;
>>>>        int r;
>>>>        net->dev.nvqs = 2;
>>>> @@ -244,6 +245,13 @@ static int vhost_net_start_one(struct vhost_net *net,
>>>>            qemu_set_fd_handler(net->backend, NULL, NULL, NULL);
>>>>            file.fd = net->backend;
>>>>            for (file.index = 0; file.index < net->dev.nvqs; ++file.index) {
>>>> +            a = virtio_queue_get_desc_addr(dev,
>>>> +                                           net->dev.vq_index +
>>>> +                                           file.index);
>>>> +            if (a == 0) {
>>>> +                /* Queue might not be ready for start */
>>>> +                continue;
>>>> +            }
>>>>                r = vhost_net_set_backend(&net->dev, &file);
>>>>                if (r < 0) {
>>>>                    r = -errno;
>>>> @@ -256,6 +264,13 @@ fail:
>>>>        file.fd = -1;
>>>>        if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
>>>>            while (file.index-- > 0) {
>>>> +            a = virtio_queue_get_desc_addr(dev,
>>>> +                                           net->dev.vq_index +
>>>> +                                           file.index);
>>>> +            if (a == 0) {
>>>> +                /* Queue might not be ready for start */
>>>> +                continue;
>>>> +            }
>>>>                int r = vhost_net_set_backend(&net->dev, &file);
>>>>                assert(r >= 0);
>>>>            }
>>> I think we want an API that explicitly says "queue is enabled".
>>> For 0.X it will return !!addr. For 1.X it will return enabled.
>> For 1.x, desc.addr won't be set until queue_enabled is set through
>> virtio_queue_set_vrings(). And it looks to me ccw did something similar of
>> CMD_SET_VQ.
>>
>> So we're ok actually?
>>
>> Thanks
>>
> OK maybe but why can't we use an explicit API?
> 0.X can use addr != 0 trick since there's no queue_enabled.
>

If I understand correctly, you want something similar to what V1 did? A 
bus specific queue_enable() method.

Thanks
Re: [Qemu-devel] [PATCH V2 RFT] vhost_net: don't set backend for the uninitialized virtqueue
Posted by Michael S. Tsirkin 5 years, 1 month ago 
On Wed, Mar 27, 2019 at 10:27:01AM +0800, Jason Wang wrote:
> 
> On 2019/3/26 下午8:49, Michael S. Tsirkin wrote:
> > On Tue, Mar 26, 2019 at 02:59:19PM +0800, Jason Wang wrote:
> > > On 2019/3/25 下午8:32, Michael S. Tsirkin wrote:
> > > > On Mon, Mar 25, 2019 at 11:56:13AM +0800, Jason Wang wrote:
> > > > > We used to set backend unconditionally, this won't work for some
> > > > > guests (e.g windows driver) who may not initialize all virtqueues. For
> > > > > kernel backend, this will fail since it may try to validate the rings
> > > > > during setting backend.
> > > > > 
> > > > > Fixing this by simply skipping the backend set when we find desc is
> > > > > not ready.
> > > > > 
> > > > > Signed-off-by: Jason Wang<jasowang@redhat.com>
> > > > > ---
> > > > >    hw/net/vhost_net.c | 15 +++++++++++++++
> > > > >    1 file changed, 15 insertions(+)
> > > > > 
> > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > > index be3cc88370..04fd924d15 100644
> > > > > --- a/hw/net/vhost_net.c
> > > > > +++ b/hw/net/vhost_net.c
> > > > > @@ -221,6 +221,7 @@ static int vhost_net_start_one(struct vhost_net *net,
> > > > >                                   VirtIODevice *dev)
> > > > >    {
> > > > >        struct vhost_vring_file file = { };
> > > > > +    hwaddr a;
> > > > >        int r;
> > > > >        net->dev.nvqs = 2;
> > > > > @@ -244,6 +245,13 @@ static int vhost_net_start_one(struct vhost_net *net,
> > > > >            qemu_set_fd_handler(net->backend, NULL, NULL, NULL);
> > > > >            file.fd = net->backend;
> > > > >            for (file.index = 0; file.index < net->dev.nvqs; ++file.index) {
> > > > > +            a = virtio_queue_get_desc_addr(dev,
> > > > > +                                           net->dev.vq_index +
> > > > > +                                           file.index);
> > > > > +            if (a == 0) {
> > > > > +                /* Queue might not be ready for start */
> > > > > +                continue;
> > > > > +            }
> > > > >                r = vhost_net_set_backend(&net->dev, &file);
> > > > >                if (r < 0) {
> > > > >                    r = -errno;
> > > > > @@ -256,6 +264,13 @@ fail:
> > > > >        file.fd = -1;
> > > > >        if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
> > > > >            while (file.index-- > 0) {
> > > > > +            a = virtio_queue_get_desc_addr(dev,
> > > > > +                                           net->dev.vq_index +
> > > > > +                                           file.index);
> > > > > +            if (a == 0) {
> > > > > +                /* Queue might not be ready for start */
> > > > > +                continue;
> > > > > +            }
> > > > >                int r = vhost_net_set_backend(&net->dev, &file);
> > > > >                assert(r >= 0);
> > > > >            }
> > > > I think we want an API that explicitly says "queue is enabled".
> > > > For 0.X it will return !!addr. For 1.X it will return enabled.
> > > For 1.x, desc.addr won't be set until queue_enabled is set through
> > > virtio_queue_set_vrings(). And it looks to me ccw did something similar of
> > > CMD_SET_VQ.
> > > 
> > > So we're ok actually?
> > > 
> > > Thanks
> > > 
> > OK maybe but why can't we use an explicit API?
> > 0.X can use addr != 0 trick since there's no queue_enabled.
> > 
> 
> If I understand correctly, you want something similar to what V1 did? A bus
> specific queue_enable() method.
> 
> Thanks

For now all I am asking for is simply:
	virtio_queue_is_enabled() {
		/* desc is only set when queue is enabled */
		return vdev->vq[n].vring.desc != 0;
}

and then we can look at moving the enabled flag into
struct VRing longer term.


-- 
MST
Re: [Qemu-devel] [PATCH V2 RFT] vhost_net: don't set backend for the uninitialized virtqueue
Posted by Jason Wang 5 years, 1 month ago 
On 2019/4/1 下午9:44, Michael S. Tsirkin wrote:
> On Wed, Mar 27, 2019 at 10:27:01AM +0800, Jason Wang wrote:
>> On 2019/3/26 下午8:49, Michael S. Tsirkin wrote:
>>> On Tue, Mar 26, 2019 at 02:59:19PM +0800, Jason Wang wrote:
>>>> On 2019/3/25 下午8:32, Michael S. Tsirkin wrote:
>>>>> On Mon, Mar 25, 2019 at 11:56:13AM +0800, Jason Wang wrote:
>>>>>> We used to set backend unconditionally, this won't work for some
>>>>>> guests (e.g windows driver) who may not initialize all virtqueues. For
>>>>>> kernel backend, this will fail since it may try to validate the rings
>>>>>> during setting backend.
>>>>>>
>>>>>> Fixing this by simply skipping the backend set when we find desc is
>>>>>> not ready.
>>>>>>
>>>>>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>>>>>> ---
>>>>>>     hw/net/vhost_net.c | 15 +++++++++++++++
>>>>>>     1 file changed, 15 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>>>>>> index be3cc88370..04fd924d15 100644
>>>>>> --- a/hw/net/vhost_net.c
>>>>>> +++ b/hw/net/vhost_net.c
>>>>>> @@ -221,6 +221,7 @@ static int vhost_net_start_one(struct vhost_net *net,
>>>>>>                                    VirtIODevice *dev)
>>>>>>     {
>>>>>>         struct vhost_vring_file file = { };
>>>>>> +    hwaddr a;
>>>>>>         int r;
>>>>>>         net->dev.nvqs = 2;
>>>>>> @@ -244,6 +245,13 @@ static int vhost_net_start_one(struct vhost_net *net,
>>>>>>             qemu_set_fd_handler(net->backend, NULL, NULL, NULL);
>>>>>>             file.fd = net->backend;
>>>>>>             for (file.index = 0; file.index < net->dev.nvqs; ++file.index) {
>>>>>> +            a = virtio_queue_get_desc_addr(dev,
>>>>>> +                                           net->dev.vq_index +
>>>>>> +                                           file.index);
>>>>>> +            if (a == 0) {
>>>>>> +                /* Queue might not be ready for start */
>>>>>> +                continue;
>>>>>> +            }
>>>>>>                 r = vhost_net_set_backend(&net->dev, &file);
>>>>>>                 if (r < 0) {
>>>>>>                     r = -errno;
>>>>>> @@ -256,6 +264,13 @@ fail:
>>>>>>         file.fd = -1;
>>>>>>         if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
>>>>>>             while (file.index-- > 0) {
>>>>>> +            a = virtio_queue_get_desc_addr(dev,
>>>>>> +                                           net->dev.vq_index +
>>>>>> +                                           file.index);
>>>>>> +            if (a == 0) {
>>>>>> +                /* Queue might not be ready for start */
>>>>>> +                continue;
>>>>>> +            }
>>>>>>                 int r = vhost_net_set_backend(&net->dev, &file);
>>>>>>                 assert(r >= 0);
>>>>>>             }
>>>>> I think we want an API that explicitly says "queue is enabled".
>>>>> For 0.X it will return !!addr. For 1.X it will return enabled.
>>>> For 1.x, desc.addr won't be set until queue_enabled is set through
>>>> virtio_queue_set_vrings(). And it looks to me ccw did something similar of
>>>> CMD_SET_VQ.
>>>>
>>>> So we're ok actually?
>>>>
>>>> Thanks
>>>>
>>> OK maybe but why can't we use an explicit API?
>>> 0.X can use addr != 0 trick since there's no queue_enabled.
>>>
>> If I understand correctly, you want something similar to what V1 did? A bus
>> specific queue_enable() method.
>>
>> Thanks
> For now all I am asking for is simply:
> 	virtio_queue_is_enabled() {
> 		/* desc is only set when queue is enabled */
> 		return vdev->vq[n].vring.desc != 0;
> }
>
> and then we can look at moving the enabled flag into
> struct VRing longer term.
>
>

Ok, let me post V3.

Thanks

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.