Re: [dpdk-dev] [PATCH] net/virtio-user: fix feature setting with vhost-net backend

[Tiwei Bie <tiwei.bie@intel.com>]

On Tue, May 08, 2018 at 03:44:22PM +0800, Jiayu Hu wrote:
> Hi Tiwei,
> 
> On Tue, May 08, 2018 at 10:49:50AM +0800, Tiwei Bie wrote:
> > On Mon, May 07, 2018 at 02:50:06PM +0800, Jiayu Hu wrote:
> > > Currently, when the backend is vhost-net, which implies
> > > virtio-user works in client mode, virtio-user is assigned
> > > to the default feature VIRTIO_USER_SUPPORTED_FEATURES.
> > > However, virtio-user should request features from the
> > > backend in this case.
> > > 
> > > Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")
> > > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > > ---
> > >  drivers/net/virtio/virtio_user/virtio_user_dev.c | 15 +++++++--------
> > >  1 file changed, 7 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > > index 38b8bc9..f745163 100644
> > > --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > > +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > > @@ -353,20 +353,22 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
> > >  		return -1;
> > >  	}
> > >  
> > > -	if (dev->vhostfd >= 0) {
> > > +	if (!dev->is_server) {
> > 
> > Server mode also needs to run below code when the
> > connection to the backend is established.
> 
> For server mode, the below code is called by virtio_user_start_device(),
> instead of virtio_user_dev_init(). Only client mode virtio-user
> needs to call it when init.

Okay. So in server mode, virtio-user never get a chance
to do GET_FEATURES.

> 
> > 
> > >  		if (dev->ops->send_request(dev, VHOST_USER_SET_OWNER,
> > > -					   NULL) < 0) {
> > > +					NULL) < 0) {
> > 
> > There is no need to change the indent.
> 
> Thanks, change in the next patch.
> 
> > 
> > >  			PMD_INIT_LOG(ERR, "set_owner fails: %s",
> > > -				     strerror(errno));
> > > +					strerror(errno));
> > 
> > There is no need to change the indent.
> 
> ditto.
> 
> > 
> > >  			return -1;
> > >  		}
> > >  
> > >  		if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
> > > -					   &dev->device_features) < 0) {
> > > +					&dev->device_features) < 0) {
> > 
> > There is no need to change the indent.
> 
> ditto.
> 
> > 
> > >  			PMD_INIT_LOG(ERR, "get_features failed: %s",
> > > -				     strerror(errno));
> > > +					strerror(errno));
> > 
> > There is no need to change the indent.
> 
> ditto.
> 
> > 
> > >  			return -1;
> > >  		}
> > > +		if (dev->mac_specified)
> > > +			dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
> > 
> > Why move above code?
> 
> The server mode virtio-user is assigned the default features, which include
> VIRTIO_NET_F_MAC. So checking if support VIRTIO_NET_F_MAC is only necessary
> for client mode.

I think we should set this bit only when mac is
specified. And in server mode, we need to clear
this bit when mac isn't specified. So instead
of moving it, it should be changed to something
like this:

	if (dev->mac_specified)
		dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
	else
		dev->device_features &= ~(1ull << VIRTIO_NET_F_MAC);

Thanks

> 
> Thanks,
> Jiayu
> 
> > 
> > >  	} else {
> > >  		/* We just pretend vhost-user can support all these features.
> > >  		 * Note that this could be problematic that if some feature is
> > > @@ -376,9 +378,6 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
> > >  		dev->device_features = VIRTIO_USER_SUPPORTED_FEATURES;
> > >  	}
> > >  
> > > -	if (dev->mac_specified)
> > > -		dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
> > > -
> > >  	if (cq) {
> > >  		/* device does not really need to know anything about CQ,
> > >  		 * so if necessary, we just claim to support CQ
> > > -- 
> > > 2.7.4
> > >