diff mbox

[dpdk-dev,v2,3/5] vhost: enable promisc mode and config VMDQ offload register for multicast feature

Message ID 1414381533-30370-4-git-send-email-changchun.ouyang@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Ouyang Changchun Oct. 27, 2014, 3:45 a.m. UTC 
  This patch is to let vhost receive and forward multicast and broadcast packets,
add promiscuous option into command line; and set VMDQ RX mode as:
ETH_VMDQ_ACCEPT_BROADCAST|ETH_VMDQ_ACCEPT_MULTICAST if promisc mode is on.

Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
---
 examples/vhost/main.c         | 25 ++++++++++++++++++++++---
 lib/librte_vhost/virtio-net.c |  4 +++-
 2 files changed, 25 insertions(+), 4 deletions(-)

Comments

Huawei Xie Oct. 29, 2014, 11:26 p.m. UTC | #1 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang Changchun
> Sent: Sunday, October 26, 2014 8:46 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v2 3/5] vhost: enable promisc mode and config
> VMDQ offload register for multicast feature
> 
> This patch is to let vhost receive and forward multicast and broadcast packets,
> add promiscuous option into command line; and set VMDQ RX mode as:
> ETH_VMDQ_ACCEPT_BROADCAST|ETH_VMDQ_ACCEPT_MULTICAST if promisc
> mode is on.
> 
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> ---
>  examples/vhost/main.c         | 25 ++++++++++++++++++++++---
>  lib/librte_vhost/virtio-net.c |  4 +++-
>  2 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index 291128e..c4947f7 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -161,6 +161,9 @@
>  /* mask of enabled ports */
>  static uint32_t enabled_port_mask = 0;
> 
> +/* Ports set in promiscuous mode off by default. */
comment is confusing
> +static uint32_t promiscuous_on;
> +
>  /*Number of switching cores enabled*/
>  static uint32_t num_switching_cores = 0;
Don't initialize static variables to zero/NULL
> 
> @@ -274,6 +277,7 @@ static struct rte_eth_conf vmdq_conf_default = {
>  			.enable_default_pool = 0,
>  			.default_pool = 0,
>  			.nb_pool_maps = 0,
> +			.rx_mode = 0,
>  			.pool_map = {{0, 0},},
>  		},
>  	},
Same as above, do we need to initialize static var?

> @@ -364,13 +368,15 @@ static inline int
>  get_eth_conf(struct rte_eth_conf *eth_conf, uint32_t num_devices)
>  {
>  	struct rte_eth_vmdq_rx_conf conf;
> +	struct rte_eth_vmdq_rx_conf *def_conf =
> +		&vmdq_conf_default.rx_adv_conf.vmdq_rx_conf;
>  	unsigned i;
> 
>  	memset(&conf, 0, sizeof(conf));
>  	conf.nb_queue_pools = (enum rte_eth_nb_pools)num_devices;
>  	conf.nb_pool_maps = num_devices;
> -	conf.enable_loop_back =
> -
> 	vmdq_conf_default.rx_adv_conf.vmdq_rx_conf.enable_loop_back;
> +	conf.enable_loop_back = def_conf->enable_loop_back;
> +	conf.rx_mode = def_conf->rx_mode;
> 
>  	for (i = 0; i < conf.nb_pool_maps; i++) {
>  		conf.pool_map[i].vlan_id = vlan_tags[ i ];
> @@ -468,6 +474,9 @@ port_init(uint8_t port)
>  		return retval;
>  	}
> 
> +	if (promiscuous_on)
> +		rte_eth_promiscuous_enable(port);
> +
>  	rte_eth_macaddr_get(port, &vmdq_ports_eth_addr[port]);
>  	RTE_LOG(INFO, VHOST_PORT, "Max virtio devices supported: %u\n",
> num_devices);
>  	RTE_LOG(INFO, VHOST_PORT, "Port %u
> MAC: %02"PRIx8" %02"PRIx8" %02"PRIx8
> @@ -598,7 +607,8 @@ us_vhost_parse_args(int argc, char **argv)
>  	};
> 
>  	/* Parse command line */
> -	while ((opt = getopt_long(argc, argv, "p:",long_option,
> &option_index)) != EOF) {
> +	while ((opt = getopt_long(argc, argv, "p:P",
> +			long_option, &option_index)) != EOF) {
>  		switch (opt) {
>  		/* Portmask */
>  		case 'p':
> @@ -610,6 +620,15 @@ us_vhost_parse_args(int argc, char **argv)
>  			}
>  			break;
> 
> +		case 'P':
> +			promiscuous_on = 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);

Alignment?
> +
> +			break;
> +
>  		case 0:
>  			/* Enable/disable vm2vm comms. */
>  			if (!strncmp(long_option[option_index].name, "vm2vm",
> diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> index 27ba175..744156c 100644
> --- a/lib/librte_vhost/virtio-net.c
> +++ b/lib/librte_vhost/virtio-net.c
> @@ -68,7 +68,9 @@ static struct virtio_net_device_ops const *notify_ops;
>  static struct virtio_net_config_ll	*ll_root;
> 
>  /* Features supported by this application. RX merge buffers are enabled by
> default. */
> -#define VHOST_SUPPORTED_FEATURES (1ULL << VIRTIO_NET_F_MRG_RXBUF)
> +#define VHOST_SUPPORTED_FEATURES ((1ULL << VIRTIO_NET_F_MRG_RXBUF)
> | \
> +	(1ULL << VIRTIO_NET_F_CTRL_RX))
> +
>  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
> 
>  /* Line size for reading maps file. */
> --
> 1.8.4.2
Ouyang Changchun Oct. 30, 2014, 12:49 a.m. UTC | #2 
Hi,

> -----Original Message-----
> From: Xie, Huawei
> Sent: Thursday, October 30, 2014 7:26 AM
> To: Ouyang, Changchun; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2 3/5] vhost: enable promisc mode and
> config VMDQ offload register for multicast feature
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang
> Changchun
> > Sent: Sunday, October 26, 2014 8:46 PM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH v2 3/5] vhost: enable promisc mode and
> > config VMDQ offload register for multicast feature
> >
> > This patch is to let vhost receive and forward multicast and broadcast
> > packets, add promiscuous option into command line; and set VMDQ RX
> mode as:
> > ETH_VMDQ_ACCEPT_BROADCAST|ETH_VMDQ_ACCEPT_MULTICAST if
> promisc mode is
> > on.
> >
> > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > ---
> >  examples/vhost/main.c         | 25 ++++++++++++++++++++++---
> >  lib/librte_vhost/virtio-net.c |  4 +++-
> >  2 files changed, 25 insertions(+), 4 deletions(-)
> >
> > diff --git a/examples/vhost/main.c b/examples/vhost/main.c index
> > 291128e..c4947f7 100644
> > --- a/examples/vhost/main.c
> > +++ b/examples/vhost/main.c
> > @@ -161,6 +161,9 @@
> >  /* mask of enabled ports */
> >  static uint32_t enabled_port_mask = 0;
> >
> > +/* Ports set in promiscuous mode off by default. */
> comment is confusing
Don't think it is confusing, 
But I can refine it a bit.
> > +static uint32_t promiscuous_on;
> > +
> >  /*Number of switching cores enabled*/  static uint32_t
> > num_switching_cores = 0;
> Don't initialize static variables to zero/NULL

I don't touch this line in my patch, and initialization to 0 is not an issue here. 

> >
> > @@ -274,6 +277,7 @@ static struct rte_eth_conf vmdq_conf_default = {
> >  			.enable_default_pool = 0,
> >  			.default_pool = 0,
> >  			.nb_pool_maps = 0,
> > +			.rx_mode = 0,
> >  			.pool_map = {{0, 0},},
> >  		},
> >  	},
> Same as above, do we need to initialize static var?
Response same as above,  no harm to initialize them to 0.
> 
> > @@ -364,13 +368,15 @@ static inline int  get_eth_conf(struct
> > rte_eth_conf *eth_conf, uint32_t num_devices)  {
> >  	struct rte_eth_vmdq_rx_conf conf;
> > +	struct rte_eth_vmdq_rx_conf *def_conf =
> > +		&vmdq_conf_default.rx_adv_conf.vmdq_rx_conf;
> >  	unsigned i;
> >
> >  	memset(&conf, 0, sizeof(conf));
> >  	conf.nb_queue_pools = (enum rte_eth_nb_pools)num_devices;
> >  	conf.nb_pool_maps = num_devices;
> > -	conf.enable_loop_back =
> > -
> > 	vmdq_conf_default.rx_adv_conf.vmdq_rx_conf.enable_loop_back;
> > +	conf.enable_loop_back = def_conf->enable_loop_back;
> > +	conf.rx_mode = def_conf->rx_mode;
> >
> >  	for (i = 0; i < conf.nb_pool_maps; i++) {
> >  		conf.pool_map[i].vlan_id = vlan_tags[ i ]; @@ -468,6 +474,9
> @@
> > port_init(uint8_t port)
> >  		return retval;
> >  	}
> >
> > +	if (promiscuous_on)
> > +		rte_eth_promiscuous_enable(port);
> > +
> >  	rte_eth_macaddr_get(port, &vmdq_ports_eth_addr[port]);
> >  	RTE_LOG(INFO, VHOST_PORT, "Max virtio devices supported: %u\n",
> > num_devices);
> >  	RTE_LOG(INFO, VHOST_PORT, "Port %u
> > MAC: %02"PRIx8" %02"PRIx8" %02"PRIx8
> > @@ -598,7 +607,8 @@ us_vhost_parse_args(int argc, char **argv)
> >  	};
> >
> >  	/* Parse command line */
> > -	while ((opt = getopt_long(argc, argv, "p:",long_option,
> > &option_index)) != EOF) {
> > +	while ((opt = getopt_long(argc, argv, "p:P",
> > +			long_option, &option_index)) != EOF) {
> >  		switch (opt) {
> >  		/* Portmask */
> >  		case 'p':
> > @@ -610,6 +620,15 @@ us_vhost_parse_args(int argc, char **argv)
> >  			}
> >  			break;
> >
> > +		case 'P':
> > +			promiscuous_on = 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);
> 
> Alignment?
Checked it again, already aligned, your mail display should has some issue. :-)
> > +
> > +			break;
> > +
> >  		case 0:
> >  			/* Enable/disable vm2vm comms. */
> >  			if (!strncmp(long_option[option_index].name,
> "vm2vm", diff --git
> > a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c index
> > 27ba175..744156c 100644
> > --- a/lib/librte_vhost/virtio-net.c
> > +++ b/lib/librte_vhost/virtio-net.c
> > @@ -68,7 +68,9 @@ static struct virtio_net_device_ops const *notify_ops;
> >  static struct virtio_net_config_ll	*ll_root;
> >
> >  /* Features supported by this application. RX merge buffers are
> > enabled by default. */ -#define VHOST_SUPPORTED_FEATURES (1ULL <<
> > VIRTIO_NET_F_MRG_RXBUF)
> > +#define VHOST_SUPPORTED_FEATURES ((1ULL <<
> VIRTIO_NET_F_MRG_RXBUF)
> > | \
> > +	(1ULL << VIRTIO_NET_F_CTRL_RX))
> > +
> >  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
> >
> >  /* Line size for reading maps file. */
> > --
> > 1.8.4.2

Thanks 
Changchun
Bruce Richardson Oct. 30, 2014, 10:10 a.m. UTC | #3 
On Thu, Oct 30, 2014 at 12:49:13AM +0000, Ouyang, Changchun wrote:
> Hi,
> 
> > -----Original Message-----
> > From: Xie, Huawei
> > Sent: Thursday, October 30, 2014 7:26 AM
> > To: Ouyang, Changchun; dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v2 3/5] vhost: enable promisc mode and
> > config VMDQ offload register for multicast feature
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang
> > Changchun
> > > Sent: Sunday, October 26, 2014 8:46 PM
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] [PATCH v2 3/5] vhost: enable promisc mode and
> > > config VMDQ offload register for multicast feature
> > >
> > > This patch is to let vhost receive and forward multicast and broadcast
> > > packets, add promiscuous option into command line; and set VMDQ RX
> > mode as:
> > > ETH_VMDQ_ACCEPT_BROADCAST|ETH_VMDQ_ACCEPT_MULTICAST if
> > promisc mode is
> > > on.
> > >
> > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > > ---
> > >  examples/vhost/main.c         | 25 ++++++++++++++++++++++---
> > >  lib/librte_vhost/virtio-net.c |  4 +++-
> > >  2 files changed, 25 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/examples/vhost/main.c b/examples/vhost/main.c index
> > > 291128e..c4947f7 100644
> > > --- a/examples/vhost/main.c
> > > +++ b/examples/vhost/main.c
> > > @@ -161,6 +161,9 @@
> > >  /* mask of enabled ports */
> > >  static uint32_t enabled_port_mask = 0;
> > >
> > > +/* Ports set in promiscuous mode off by default. */
> > comment is confusing
> Don't think it is confusing, 
> But I can refine it a bit.
> > > +static uint32_t promiscuous_on;
> > > +
> > >  /*Number of switching cores enabled*/  static uint32_t
> > > num_switching_cores = 0;
> > Don't initialize static variables to zero/NULL
> 
> I don't touch this line in my patch, and initialization to 0 is not an issue here. 
> 
> > >
> > > @@ -274,6 +277,7 @@ static struct rte_eth_conf vmdq_conf_default = {
> > >  			.enable_default_pool = 0,
> > >  			.default_pool = 0,
> > >  			.nb_pool_maps = 0,
> > > +			.rx_mode = 0,
> > >  			.pool_map = {{0, 0},},
> > >  		},
> > >  	},
> > Same as above, do we need to initialize static var?
> Response same as above,  no harm to initialize them to 0.

With this one, I would actually disagree. With something like this is it harder to read, since you have a whole list of values given here. The reader has to scan through the list of values to check in case any of them are non-zero. If there is no initializer, or just a single-value initializer, e.g. ... vmdq_conf_default = { .default_pool = 0 }, the user just has a single line to look at and can know that the rest of the values will be zero. Furthermore, having the initializers all spelled out like that uses up screen space, which, if the initializers weren't there would be filled instead by code which is meaningful. More meaningful code on screen again makes things easier for the reader, as they have to do less scrolling to find the context they need.

It's not a big deal either way, but that's my opinion on the matter. :-)

/Bruce
Ouyang Changchun Oct. 30, 2014, 3:18 p.m. UTC | #4 
Hi ,

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Thursday, October 30, 2014 6:10 PM
> To: Ouyang, Changchun
> Cc: Xie, Huawei; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 3/5] vhost: enable promisc mode and
> config VMDQ offload register for multicast feature
> 
> On Thu, Oct 30, 2014 at 12:49:13AM +0000, Ouyang, Changchun wrote:
> > Hi,
> >
> > > -----Original Message-----
> > > From: Xie, Huawei
> > > Sent: Thursday, October 30, 2014 7:26 AM
> > > To: Ouyang, Changchun; dev@dpdk.org
> > > Subject: RE: [dpdk-dev] [PATCH v2 3/5] vhost: enable promisc mode
> > > and config VMDQ offload register for multicast feature
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang
> > > Changchun
> > > > Sent: Sunday, October 26, 2014 8:46 PM
> > > > To: dev@dpdk.org
> > > > Subject: [dpdk-dev] [PATCH v2 3/5] vhost: enable promisc mode and
> > > > config VMDQ offload register for multicast feature
> > > >
> > > > This patch is to let vhost receive and forward multicast and
> > > > broadcast packets, add promiscuous option into command line; and
> > > > set VMDQ RX
> > > mode as:
> > > > ETH_VMDQ_ACCEPT_BROADCAST|ETH_VMDQ_ACCEPT_MULTICAST if
> > > promisc mode is
> > > > on.
> > > >
> > > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > > > ---
> > > >  examples/vhost/main.c         | 25 ++++++++++++++++++++++---
> > > >  lib/librte_vhost/virtio-net.c |  4 +++-
> > > >  2 files changed, 25 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/examples/vhost/main.c b/examples/vhost/main.c index
> > > > 291128e..c4947f7 100644
> > > > --- a/examples/vhost/main.c
> > > > +++ b/examples/vhost/main.c
> > > > @@ -161,6 +161,9 @@
> > > >  /* mask of enabled ports */
> > > >  static uint32_t enabled_port_mask = 0;
> > > >
> > > > +/* Ports set in promiscuous mode off by default. */
> > > comment is confusing
> > Don't think it is confusing,
> > But I can refine it a bit.
> > > > +static uint32_t promiscuous_on;
> > > > +
> > > >  /*Number of switching cores enabled*/  static uint32_t
> > > > num_switching_cores = 0;
> > > Don't initialize static variables to zero/NULL
> >
> > I don't touch this line in my patch, and initialization to 0 is not an issue here.
> >
> > > >
> > > > @@ -274,6 +277,7 @@ static struct rte_eth_conf vmdq_conf_default =
> {
> > > >  			.enable_default_pool = 0,
> > > >  			.default_pool = 0,
> > > >  			.nb_pool_maps = 0,
> > > > +			.rx_mode = 0,
> > > >  			.pool_map = {{0, 0},},
> > > >  		},
> > > >  	},
> > > Same as above, do we need to initialize static var?
> > Response same as above,  no harm to initialize them to 0.
> 
> With this one, I would actually disagree. With something like this is it harder
> to read, since you have a whole list of values given here. The reader has to
> scan through the list of values to check in case any of them are non-zero. If
> there is no initializer, or just a single-value initializer, e.g. ...
> vmdq_conf_default = { .default_pool = 0 }, the user just has a single line to
> look at and can know that the rest of the values will be zero. Furthermore,
> having the initializers all spelled out like that uses up screen space, which, if
> the initializers weren't there would be filled instead by code which is
> meaningful. More meaningful code on screen again makes things easier for
> the reader, as they have to do less scrolling to find the context they need.
> 
> It's not a big deal either way, but that's my opinion on the matter. :-)
> 
Seems at least you 2 guys has strong objection on the initializing 0 to a static var.
Ok for updating, and don't want to spend much time in arguing on the tiny stuff. :-)
Pls waiting for my next version update.

Changchun
Huawei Xie Jan. 8, 2015, 10:07 a.m. UTC | #5 
> diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> index 27ba175..744156c 100644
> --- a/lib/librte_vhost/virtio-net.c
> +++ b/lib/librte_vhost/virtio-net.c
> @@ -68,7 +68,9 @@ static struct virtio_net_device_ops const *notify_ops;
>  static struct virtio_net_config_ll	*ll_root;
> 
>  /* Features supported by this application. RX merge buffers are enabled by
> default. */
> -#define VHOST_SUPPORTED_FEATURES (1ULL << VIRTIO_NET_F_MRG_RXBUF)
> +#define VHOST_SUPPORTED_FEATURES ((1ULL << VIRTIO_NET_F_MRG_RXBUF)
> | \
> +	(1ULL << VIRTIO_NET_F_CTRL_RX))
> +
CTRL_RX is dependent on CTRL_VQ.
CTRL_VQ should be enabled if CTRL_RX is enabled.
Observed that virtio-net driver will crash if CTRL_VQ isn't enabled in vhost-user case.
	/* Caller should know better */
	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ||
		(out + in > VIRTNET_SEND_COMMAND_SG_MAX)); 
>  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
> 
>  /* Line size for reading maps file. */
> --
> 1.8.4.2
Ouyang Changchun Jan. 9, 2015, 5:21 a.m. UTC | #6 
Hi Huawei,

> -----Original Message-----
> From: Xie, Huawei
> Sent: Thursday, January 8, 2015 6:08 PM
> To: Ouyang, Changchun; dev@dpdk.org
> Cc: Tetsuya Mukawa
> Subject: RE: [dpdk-dev] [PATCH v2 3/5] vhost: enable promisc mode and
> config VMDQ offload register for multicast feature
> 
> > diff --git a/lib/librte_vhost/virtio-net.c
> > b/lib/librte_vhost/virtio-net.c index 27ba175..744156c 100644
> > --- a/lib/librte_vhost/virtio-net.c
> > +++ b/lib/librte_vhost/virtio-net.c
> > @@ -68,7 +68,9 @@ static struct virtio_net_device_ops const *notify_ops;
> >  static struct virtio_net_config_ll	*ll_root;
> >
> >  /* Features supported by this application. RX merge buffers are
> > enabled by default. */ -#define VHOST_SUPPORTED_FEATURES (1ULL <<
> > VIRTIO_NET_F_MRG_RXBUF)
> > +#define VHOST_SUPPORTED_FEATURES ((1ULL <<
> VIRTIO_NET_F_MRG_RXBUF)
> > | \
> > +	(1ULL << VIRTIO_NET_F_CTRL_RX))
> > +
> CTRL_RX is dependent on CTRL_VQ.
> CTRL_VQ should be enabled if CTRL_RX is enabled.
> Observed that virtio-net driver will crash if CTRL_VQ isn't enabled in vhost-
> user case.
> 	/* Caller should know better */
> 	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ||
> 		(out + in > VIRTNET_SEND_COMMAND_SG_MAX));

Thanks for identifying, after your patch sent out to fix it, I will act it. 

Changchun
Thomas Monjalon Jan. 14, 2015, 4:37 p.m. UTC | #7 
Hi Huawei,

2015-01-08 10:07, Xie, Huawei:
> CTRL_RX is dependent on CTRL_VQ.
> CTRL_VQ should be enabled if CTRL_RX is enabled.
> Observed that virtio-net driver will crash if CTRL_VQ isn't enabled in vhost-user case.
> 	/* Caller should know better */
> 	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ||
> 		(out + in > VIRTNET_SEND_COMMAND_SG_MAX)); 

Do you plan to send a patch?
Huawei Xie Jan. 23, 2015, 3:20 a.m. UTC | #8 
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, January 15, 2015 12:37 AM
> To: dev@dpdk.org
> Cc: Xie, Huawei; Ouyang, Changchun
> Subject: Re: [dpdk-dev] [PATCH v2 3/5] vhost: enable promisc mode and config
> VMDQ offload register for multicast feature
> 
> Hi Huawei,
> 
> 2015-01-08 10:07, Xie, Huawei:
> > CTRL_RX is dependent on CTRL_VQ.
> > CTRL_VQ should be enabled if CTRL_RX is enabled.
> > Observed that virtio-net driver will crash if CTRL_VQ isn't enabled in vhost-user
> case.
> > 	/* Caller should know better */
> > 	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ||
> > 		(out + in > VIRTNET_SEND_COMMAND_SG_MAX));
> 
> Do you plan to send a patch?
In the vhost-user patch, will temporarily turn this on.
However, we don't create any control queues in vhost.
Will investigate the root cause later.
> 
> --
> Thomas
diff mbox

Patch

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index 291128e..c4947f7 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -161,6 +161,9 @@ 
 /* mask of enabled ports */
 static uint32_t enabled_port_mask = 0;
 
+/* Ports set in promiscuous mode off by default. */
+static uint32_t promiscuous_on;
+
 /*Number of switching cores enabled*/
 static uint32_t num_switching_cores = 0;
 
@@ -274,6 +277,7 @@  static struct rte_eth_conf vmdq_conf_default = {
 			.enable_default_pool = 0,
 			.default_pool = 0,
 			.nb_pool_maps = 0,
+			.rx_mode = 0,
 			.pool_map = {{0, 0},},
 		},
 	},
@@ -364,13 +368,15 @@  static inline int
 get_eth_conf(struct rte_eth_conf *eth_conf, uint32_t num_devices)
 {
 	struct rte_eth_vmdq_rx_conf conf;
+	struct rte_eth_vmdq_rx_conf *def_conf =
+		&vmdq_conf_default.rx_adv_conf.vmdq_rx_conf;
 	unsigned i;
 
 	memset(&conf, 0, sizeof(conf));
 	conf.nb_queue_pools = (enum rte_eth_nb_pools)num_devices;
 	conf.nb_pool_maps = num_devices;
-	conf.enable_loop_back =
-		vmdq_conf_default.rx_adv_conf.vmdq_rx_conf.enable_loop_back;
+	conf.enable_loop_back = def_conf->enable_loop_back;
+	conf.rx_mode = def_conf->rx_mode;
 
 	for (i = 0; i < conf.nb_pool_maps; i++) {
 		conf.pool_map[i].vlan_id = vlan_tags[ i ];
@@ -468,6 +474,9 @@  port_init(uint8_t port)
 		return retval;
 	}
 
+	if (promiscuous_on)
+		rte_eth_promiscuous_enable(port);
+
 	rte_eth_macaddr_get(port, &vmdq_ports_eth_addr[port]);
 	RTE_LOG(INFO, VHOST_PORT, "Max virtio devices supported: %u\n", num_devices);
 	RTE_LOG(INFO, VHOST_PORT, "Port %u MAC: %02"PRIx8" %02"PRIx8" %02"PRIx8
@@ -598,7 +607,8 @@  us_vhost_parse_args(int argc, char **argv)
 	};
 
 	/* Parse command line */
-	while ((opt = getopt_long(argc, argv, "p:",long_option, &option_index)) != EOF) {
+	while ((opt = getopt_long(argc, argv, "p:P",
+			long_option, &option_index)) != EOF) {
 		switch (opt) {
 		/* Portmask */
 		case 'p':
@@ -610,6 +620,15 @@  us_vhost_parse_args(int argc, char **argv)
 			}
 			break;
 
+		case 'P':
+			promiscuous_on = 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);
+
+			break;
+
 		case 0:
 			/* Enable/disable vm2vm comms. */
 			if (!strncmp(long_option[option_index].name, "vm2vm",
diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index 27ba175..744156c 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -68,7 +68,9 @@  static struct virtio_net_device_ops const *notify_ops;
 static struct virtio_net_config_ll	*ll_root;
 
 /* Features supported by this application. RX merge buffers are enabled by default. */
-#define VHOST_SUPPORTED_FEATURES (1ULL << VIRTIO_NET_F_MRG_RXBUF)
+#define VHOST_SUPPORTED_FEATURES ((1ULL << VIRTIO_NET_F_MRG_RXBUF) | \
+	(1ULL << VIRTIO_NET_F_CTRL_RX))
+
 static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
 
 /* Line size for reading maps file. */