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

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

[Michael S. Tsirkin]

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

[Jason Wang]

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