Re: [Qemu-devel] [RFC 13/19] fuzz: add ctrl vq support to virtio-net in libqos

From: Stefan Hajnoczi <stefanha@gmail.com>
To: "Oleinik, Alexander" <alxndr@bu.edu>
Cc: "lvivier@redhat.com" <lvivier@redhat.com>,
	"thuth@redhat.com" <thuth@redhat.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"bsd@redhat.com" <bsd@redhat.com>,
	"superirishdonkey@gmail.com" <superirishdonkey@gmail.com>,
	"stefanha@redhat.com" <stefanha@redhat.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"jsnow@redhat.com" <jsnow@redhat.com>
Subject: Re: [Qemu-devel] [RFC 13/19] fuzz: add ctrl vq support to virtio-net in libqos
Date: Fri, 26 Jul 2019 14:09:07 +0100	[thread overview]
Message-ID: <20190726130907.GH25977@stefanha-x1.localdomain> (raw)
In-Reply-To: <b2503c69ad82bd13c1bab518154c6168e7eeb1cc.camel@bu.edu>

[-- Attachment #1: Type: text/plain, Size: 2097 bytes --]

On Thu, Jul 25, 2019 at 05:05:25PM +0000, Oleinik, Alexander wrote:
> On Thu, 2019-07-25 at 12:25 -0400, John Snow wrote:
> > 
> > On 7/24/19 11:23 PM, Oleinik, Alexander wrote:
> > > Signed-off-by: Alexander Oleinik <alxndr@bu.edu>
> > 
> > Is there some explanation for why the below patch does what the
> > subject
> > line claims for the uninitiated?
> When multiqueue mode (VIRTIO_NET_F_MQ) is disabled, virtio-net sets up
> three queues. 0:receiveq, 1:transmitq and 2:controlq. 
> > I don't know why increasing the number of queues from 2 to 3 here is
> > correct in the general case, OR why it would "add ctrl vq support".
> > (Or what it has to do with fuzzing, in general.)
> 
> Prior to the change, accessing the ctrl vq through QOS, would trigger a
> segfault, since only two queues were allocated to QVirtioDevice*
> interface->queues.
> 
> Also, when VIRTIO_NET_F_MQ is enabled, the number of queues is 2*N + 1,
> so I think in that case n->n_queues is also short by one in the code
> below.

I think the patch could be changed to:

> > [Only responding because this landed in tests/libqos, which I do try
> > to
> > keep an eye on, but this patch is opaque to me. --js]
> > 
> > > ---
> > >  tests/libqos/virtio-net.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/libqos/virtio-net.c b/tests/libqos/virtio-net.c
> > > index 66405b646e..247a0a17a8 100644
> > > --- a/tests/libqos/virtio-net.c
> > > +++ b/tests/libqos/virtio-net.c
> > > @@ -51,7 +51,7 @@ static void virtio_net_setup(QVirtioNet
> > > *interface)
> > >      if (features & (1u << VIRTIO_NET_F_MQ)) {
> > >          interface->n_queues = qvirtio_config_readw(vdev, 8) * 2;
> > >      } else {
> > > -        interface->n_queues = 2;
> > > +        interface->n_queues = 3;
> > >      }

interface->n_queues++; /* ctrl vq */

And a comment added to the QVirtQueue::n_queues field definition:

  /* total number of virtqueues (rx, tx, ctrl) */

This will prevent confusion about whether the ctrl queue is counted or
not.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]