RE: [PATCH] net/ixgbevf: fix promiscuous and allmulti

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

> -----Original Message-----
> From: Wu, Wenjun1 <wenjun1.wu@intel.com>
> Sent: Friday, October 14, 2022 9:14 AM
> To: Matz, Olivier <olivier.matz@6wind.com>
> Cc: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Zhao1, Wei
> <wei.zhao1@intel.com>
> Subject: RE: [PATCH] net/ixgbevf: fix promiscuous and allmulti
> 
> 
> 
> > -----Original Message-----
> > From: Olivier Matz <olivier.matz@6wind.com>
> > Sent: Thursday, October 13, 2022 10:46 PM
> > To: Wu, Wenjun1 <wenjun1.wu@intel.com>
> > Cc: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Zhao1, Wei
> > <wei.zhao1@intel.com>
> > Subject: Re: [PATCH] net/ixgbevf: fix promiscuous and allmulti
> >
> > Hi Wenjun,
> >
> > On Mon, Oct 10, 2022 at 01:30:54AM +0000, Wu, Wenjun1 wrote:
> > > Hi Olivier,
> > >
> > > > -----Original Message-----
> > > > From: Olivier Matz <olivier.matz@6wind.com>
> > > > Sent: Thursday, September 29, 2022 8:22 PM
> > > > To: dev@dpdk.org
> > > > Cc: Yang, Qiming <qiming.yang@intel.com>; Wu, Wenjun1
> > > > <wenjun1.wu@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>
> > > > Subject: [PATCH] net/ixgbevf: fix promiscuous and allmulti
> > > >
> > > > The configuration of allmulti and promiscuous modes conflicts
> > > > together. For instance, if we enable promiscuous mode, then enable
> > > > and disable allmulti, then the promiscuous mode is wrongly disabled.
> > > >
> > > > Fix this behavior by:
> > > > - doing nothing when we set/unset allmulti if promiscuous mode is
> > > > on
> > > > - restorting the proper mode (none or allmulti) when we disable
> > > >   promiscuous mode
> > > >
> > > > Fixes: 1f4564ed7696 ("net/ixgbevf: enable promiscuous mode")
> > > >
> > > > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > > > ---
> > > >
> > > > Hi,
> > > >
> > > > For reference, this was tested with this plan:
> > > >
> > > > echo 8 > "/sys/bus/pci/devices/0000:01:00.1/sriov_numvfs"
> > > > ip link set dev eno2 up
> > > > ip link set dev eno2 promisc on
> > > > bridge link set dev eno2 hwmode veb ip link set dev eno2 mtu 9000
> > > >
> > > > ip link set dev eno2 vf 0 mac ac:1f:6b:fe:ba:b0 ip link set dev
> > > > eno2 vf 0 spoofchk off ip link set dev eno2 vf 0 trust on
> > > >
> > > > ip link set dev eno2 vf 1 mac ac:1f:6b:fe:ba:b1 ip link set dev
> > > > eno2 vf 1 spoofchk off ip link set dev eno2 vf 1 trust on
> > > >
> > > > python3 usertools/dpdk-devbind.py -s
> > > > python3 usertools/dpdk-devbind.py -b vfio-pci 0000:01:10.1   # vf 0
> > > > python3 usertools/dpdk-devbind.py -b ixgbevf 0000:01:10.3    # vf 1
> > > >
> > > >
> > > > # in another terminal
> > > > scapy
> > > > while True:
> > > >   sendp(Ether(dst='ac:1f:6b:00:00:00'), iface='eno2v1')  # wrong mac
> > > >   sendp(Ether(dst='ac:1f:6b:fe:ba:b0'), iface='eno2v1')  # correct mac
> > > >   time.sleep(1)
> > > >
> > > >
> > > > ./build/app/dpdk-testpmd -l 1,2 -a 0000:01:10.1 -- -i --total-num-
> > > > mbufs=32768 show port info all set fwd rxonly set verbose 1 set
> > > > promisc all off set allmulti all off start
> > > >
> > > > # ok, only packets to dst='ac:1f:6b:fe:ba:b0' are received
> > > >
> > > >
> > > > # ok, both packets are received
> > > > set promisc all on
> > > >
> > > >
> > > > # nok, only packets to dst='ac:1f:6b:fe:ba:b0' are received set
> > > > allmulti all on set allmulti all off
> > > >
> > > >
> > > >  drivers/net/ixgbe/ixgbe_ethdev.c | 12 +++++++++++-
> > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > index 8cec951d94..cc8383c5a9 100644
> > > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > @@ -7785,9 +7785,13 @@ static int
> > > >  ixgbevf_dev_promiscuous_disable(struct rte_eth_dev *dev)  {
> > > >  	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> > > > >dev_private);
> > > > +	int mode = IXGBEVF_XCAST_MODE_NONE;
> > > >  	int ret;
> > > >
> > > > -	switch (hw->mac.ops.update_xcast_mode(hw,
> > > > IXGBEVF_XCAST_MODE_NONE)) {
> > > > +	if (dev->data->all_multicast)
> > > > +		mode = IXGBEVF_XCAST_MODE_ALLMULTI;
> > > > +
> > > > +	switch (hw->mac.ops.update_xcast_mode(hw, mode)) {
> > > >  	case IXGBE_SUCCESS:
> > > >  		ret = 0;
> > > >  		break;
> > > > @@ -7809,6 +7813,9 @@ ixgbevf_dev_allmulticast_enable(struct
> > > > rte_eth_dev *dev)
> > > >  	int ret;
> > > >  	int mode = IXGBEVF_XCAST_MODE_ALLMULTI;
> > > >
> > > > +	if (dev->data->promiscuous)
> > > > +		return 0;
> > > > +
> > > >  	switch (hw->mac.ops.update_xcast_mode(hw, mode)) {
> > > >  	case IXGBE_SUCCESS:
> > > >  		ret = 0;
> > > > @@ -7830,6 +7837,9 @@ ixgbevf_dev_allmulticast_disable(struct
> > > > rte_eth_dev *dev)
> > > >  	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> > > > >dev_private);
> > > >  	int ret;
> > > >
> > > > +	if (dev->data->promiscuous)
> > > > +		return 0;
> > > > +
> > >
> > > It seems that we cannot actually turn off allmulticast mode when
> > > promiscuous mode is enabled, so can we return error and add a log
> > > message here as a reminder?
> >
> > I think we should not return an error here: when we disable all_multi,
> > there are
> > 2 cases:
> >
> > 1/ promiscuous is off: no issue here, we do as before, we ask the PF
> > to disable
> >    the all_multi mode
> >
> > 2/ promiscuous is on: we have nothing to ask to the PF, because we still
> want
> >    to stay in promisc mode. We just need to remember that all_multi is
> > disabled,
> >    and this is done already done by the ethdev layer.
> >
> > On the PF dpdk driver, the behavior is the same: we can enable or
> > disable promisc and all_multi independently, and changing the
> > all_multi value when promisc is enabled is allowed and does not impact the
> rx traffic.
> >
> >
> > Thanks,
> > Olivier
> >
> 
> No more concern if the behavior is consistent.
> 
> > > Thanks,
> > > Wenjun
> > >
> > > >  	switch (hw->mac.ops.update_xcast_mode(hw,
> > > > IXGBEVF_XCAST_MODE_MULTI)) {
> > > >  	case IXGBE_SUCCESS:
> > > >  		ret = 0;
> > > > --
> > > > 2.30.2
> > >
> 
> Acked-by: Wenjun Wu <wenjun1.wu@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi
> 
> Thanks,
> Wenjun