Re: [PATCH 3/3] net/virtio_user: fix wrongly set features

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Fri, Dec 02, 2016 at 02:31:15PM +0000, Jianfeng Tan wrote:
> Before the commit 86d59b21468a ("net/virtio: support LRO"), features
> in virtio PMD, is decided and properly set at device initialization
> and will not be changed. But afterward, features could be changed in
> virtio_dev_configure(), and will be re-negotiated if it's changed.
> 
> In virtio_user, host features is obtained at device initialization
> only once, but we did not store it. So the added feature bits in
> re-negotiation will fail.

I think you misunderstood the features negotiation logic here, since
the beginning.

The feature negotiation is a work of 3 parts (the virtio device, the
virtio driver and the vhost). Each part has it's own feature bits
to claim what kind of features it supports.

On the init stage, the driver will have a sync with the device, and
that's what the "get_features" and "set_features" for. Once they have
come to an agreement on the negotiated features, it will have a
last sync with the vhost backend, through the GET_FEATURES and
SET_FEATURES request.

Last, we would have a features bits that would work for all of the
3 parts.

That said, you should not mix the driver's features bits with the
device one. It's Okay to introduce host_features, but it should not
be in the driver layer, it should be in the device layer. And naming
it to something like "device_features" is better, IMO.

And maybe you should re-visit the whole virtio_user features negotiation
logic, to see what can be improved.

Besides, I would put the bug fixing patch in the first of a series,
so that it has minimal chance to introduce conflics while backporting
it to a stable branch.

	--yliu

> 
> Fixes: e9efa4d93821 ("net/virtio-user: add new virtual PCI driver")
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
>  drivers/net/virtio/virtio_user/virtio_user_dev.c | 2 +-
>  drivers/net/virtio/virtio_user/virtio_user_dev.h | 1 +
>  drivers/net/virtio/virtio_user_ethdev.c          | 4 ++--
>  3 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index 3aef5f6..a9157a9 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -224,7 +224,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
>  	}
>  
>  	if (vhost_call(dev->vid, VHOST_USER_GET_FEATURES,
> -			    &dev->features) < 0) {
> +			    &dev->host_features) < 0) {
>  		PMD_INIT_LOG(ERR, "get_features failed: %s", strerror(errno));
>  		return -1;
>  	}
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> index 80efb6e..d219432 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> @@ -47,6 +47,7 @@ struct virtio_user_dev {
>  	uint32_t	queue_pairs;
>  	uint32_t	queue_size;
>  	uint64_t	features;
> +	uint64_t	host_features;
>  	uint8_t		status;
>  	uint8_t		mac_addr[ETHER_ADDR_LEN];
>  	char		path[PATH_MAX];
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
> index 406beea..cfe2bfc 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -117,7 +117,7 @@ virtio_user_get_features(struct virtio_hw *hw)
>  {
>  	struct virtio_user_dev *dev = virtio_user_get_dev(hw);
>  
> -	return dev->features;
> +	return dev->host_features;
>  }
>  
>  static void
> @@ -125,7 +125,7 @@ virtio_user_set_features(struct virtio_hw *hw, uint64_t features)
>  {
>  	struct virtio_user_dev *dev = virtio_user_get_dev(hw);
>  
> -	dev->features = features;
> +	dev->features = features & dev->host_features;
>  }
>  
>  static uint8_t
> -- 
> 2.7.4