[dpdk-dev,01/12] lib/librte_vhost: enable VIRTIO_NET_F_CTRL_RX

Message ID 1422599787-12009-2-git-send-email-huawei.xie@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Huawei Xie Jan. 30, 2015, 6:36 a.m. UTC
VIRTIO_NET_F_CTRL_RX is dependant on VIRTIO_NET_F_CTRL_VQ.

Observed that virtio-net driver in guest would crash with only CTRL_RX enabled.

In virtnet_send_command:

	/* Caller should know better */
	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ||
		(out + in > VIRTNET_SEND_COMMAND_SG_MAX));

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_vhost/virtio-net.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
  

Comments

Tetsuya Mukawa Jan. 30, 2015, 10:03 a.m. UTC | #1
On 2015/01/30 15:36, Huawei Xie wrote:
> VIRTIO_NET_F_CTRL_RX is dependant on VIRTIO_NET_F_CTRL_VQ.
>
> Observed that virtio-net driver in guest would crash with only CTRL_RX enabled.
>
> In virtnet_send_command:
>
> 	/* Caller should know better */
> 	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ||
> 		(out + in > VIRTNET_SEND_COMMAND_SG_MAX));
>
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> ---
>  lib/librte_vhost/virtio-net.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> index b041849..52b4957 100644
> --- a/lib/librte_vhost/virtio-net.c
> +++ b/lib/librte_vhost/virtio-net.c
> @@ -73,7 +73,8 @@ static struct virtio_net_config_ll *ll_root;
>  
>  /* Features supported by this lib. */
>  #define VHOST_SUPPORTED_FEATURES ((1ULL << VIRTIO_NET_F_MRG_RXBUF) | \
> -				  (1ULL << VIRTIO_NET_F_CTRL_RX))
> +				(1ULL << VIRTIO_NET_F_CTRL_VQ) | \
> +				(1ULL << VIRTIO_NET_F_CTRL_RX))
>  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
>  
>  /* Line size for reading maps file. */

Hi Xie,

Could you please check below code?

---------------------
examples/vhost/main.c
---------------------
                case 'P':
                        promiscuous = 1;
                        vmdq_conf_default.rx_adv_conf.vmdq_rx_conf.rx_mode =
                                ETH_VMDQ_ACCEPT_BROADCAST |
                                ETH_VMDQ_ACCEPT_MULTICAST;
                        rte_vhost_feature_enable(1ULL <<
VIRTIO_NET_F_CTRL_RX);


VIRTIO_NET_F_CTRL_RX is always enabled by this patch.
So if 'P' isn't specified in vhost example, does it need to be disabled?

Thanks,
Tetsuya
  
Huawei Xie Jan. 31, 2015, 3:13 p.m. UTC | #2
> @@ -73,7 +73,8 @@ static struct virtio_net_config_ll *ll_root;

> >

> >  /* Features supported by this lib. */

> >  #define VHOST_SUPPORTED_FEATURES ((1ULL <<

> VIRTIO_NET_F_MRG_RXBUF) | \

> > -				  (1ULL << VIRTIO_NET_F_CTRL_RX))

> > +				(1ULL << VIRTIO_NET_F_CTRL_VQ) | \

> > +				(1ULL << VIRTIO_NET_F_CTRL_RX))

> >  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;

> >

> >  /* Line size for reading maps file. */

> 

> Hi Xie,

> 

> Could you please check below code?

> 

> ---------------------

> examples/vhost/main.c

> ---------------------

>                 case 'P':

>                         promiscuous = 1;

>                         vmdq_conf_default.rx_adv_conf.vmdq_rx_conf.rx_mode =

>                                 ETH_VMDQ_ACCEPT_BROADCAST |

>                                 ETH_VMDQ_ACCEPT_MULTICAST;

>                         rte_vhost_feature_enable(1ULL <<

> VIRTIO_NET_F_CTRL_RX);

> 

> 

> VIRTIO_NET_F_CTRL_RX is always enabled by this patch.

> So if 'P' isn't specified in vhost example, does it need to be disabled?


Sounds reasonable.
I find that the subject shoud be "enable CTRL_VQ".
So issue with CTRL_RX should be fixed in other patch rather than in  this patchset.

Besides, even CTRL_VQ is a little weird, as we don't do anything with 
multiple enabling in vhost library.

> 

> Thanks,

> Tetsuya
  

Patch

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index b041849..52b4957 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -73,7 +73,8 @@  static struct virtio_net_config_ll *ll_root;
 
 /* Features supported by this lib. */
 #define VHOST_SUPPORTED_FEATURES ((1ULL << VIRTIO_NET_F_MRG_RXBUF) | \
-				  (1ULL << VIRTIO_NET_F_CTRL_RX))
+				(1ULL << VIRTIO_NET_F_CTRL_VQ) | \
+				(1ULL << VIRTIO_NET_F_CTRL_RX))
 static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
 
 /* Line size for reading maps file. */