| Message ID | 20190325035613.18192-1-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V2,RFT] vhost_net: don't set backend for the uninitialized virtqueue | expand |
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 >
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 >>
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 > >>
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
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
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
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
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.
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
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); }
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(+)