[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V2 RFT] vhost_net: don't set backend for the uni
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH V2 RFT] vhost_net: don't set backend for the uninitialized virtqueue |
Date: |
Tue, 26 Mar 2019 08:49:06 -0400 |
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 <address@hidden>
> > > ---
> > > 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