Re: [dpdk-dev] [PATCH v2] ixgbe: Drop flow control frames from VFs

["Lu, Wenzhuo" <wenzhuo.lu@intel.com>]

Hi Helin,

> -----Original Message-----
> From: Zhang, Helin
> Sent: Friday, October 23, 2015 10:49 AM
> To: Lu, Wenzhuo; dev@dpdk.org
> Subject: RE: [PATCH v2] ixgbe: Drop flow control frames from VFs
> 
> 
> 
> > -----Original Message-----
> > From: Lu, Wenzhuo
> > Sent: Thursday, October 22, 2015 3:34 PM
> > To: dev@dpdk.org
> > Cc: Zhang, Helin; Lu, Wenzhuo
> > Subject: [PATCH v2] ixgbe: Drop flow control frames from VFs
> >
> > This patch will drop flow control frames from being transmitted from VSIs.
> > With this patch in place a malicious VF cannot send flow control or
> > PFC packets out on the wire.
> >
> > V2:
> > Reword the comments.
> >
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > ---
> >  drivers/net/ixgbe/ixgbe_pf.c | 43
> > +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_pf.c
> > b/drivers/net/ixgbe/ixgbe_pf.c index
> > fd1c4ca..b33f4e9 100644
> > --- a/drivers/net/ixgbe/ixgbe_pf.c
> > +++ b/drivers/net/ixgbe/ixgbe_pf.c
> > @@ -55,6 +55,7 @@
> >  #define IXGBE_MAX_VFTA     (128)
> >  #define IXGBE_VF_MSG_SIZE_DEFAULT 1
> >  #define IXGBE_VF_GET_QUEUE_MSG_SIZE 5
> > +#define IXGBE_ETHERTYPE_FLOW_CTRL 0x8808
> >
> >  static inline uint16_t
> >  dev_num_vf(struct rte_eth_dev *eth_dev) @@ -166,6 +167,46 @@ void
> > ixgbe_pf_host_uninit(struct rte_eth_dev *eth_dev)
> >  	*vfinfo = NULL;
> >  }
> >
> > +static void
> > +ixgbe_add_tx_flow_control_drop_filter(struct rte_eth_dev *eth_dev) {
> > +	struct ixgbe_hw *hw =
> > +		IXGBE_DEV_PRIVATE_TO_HW(eth_dev->data->dev_private);
> > +	struct ixgbe_filter_info *filter_info =
> > +		IXGBE_DEV_PRIVATE_TO_FILTER_INFO(eth_dev->data-
> >dev_private);
> > +	uint16_t vf_num;
> > +	int i;
> > +
> > +	/* occupy an entity of ether type filter */
> > +	for (i = 0; i < IXGBE_MAX_ETQF_FILTERS; i++) {
> > +		if (!(filter_info->ethertype_mask & (1 << i))) {
> > +			filter_info->ethertype_mask |= 1 << i;
> > +			filter_info->ethertype_filters[i] =
> > +				IXGBE_ETHERTYPE_FLOW_CTRL;
> > +			break;
> > +		}
> > +	}
> > +	if (i == IXGBE_MAX_ETQF_FILTERS) {
> > +		RTE_LOG(ERR, PMD, "Cannot find an unused ether type
> filter"
> > +				" entity for flow control.\n");
> > +		return;
> > +	}
> > +
> > +	if (hw->mac.ops.set_ethertype_anti_spoofing) {
> > +		IXGBE_WRITE_REG(hw, IXGBE_ETQF(i),
> > +				(IXGBE_ETQF_FILTER_EN |
> > +				IXGBE_ETQF_TX_ANTISPOOF |
> > +				IXGBE_ETHERTYPE_FLOW_CTRL));
> > +
> > +		vf_num = dev_num_vf(eth_dev);
> > +		for (i = 0; i < vf_num; i++) {
> > +			hw->mac.ops.set_ethertype_anti_spoofing(hw, true,
> i);
> > +		}
> > +	}
> ixgbe_set_ethertype_anti_spoofing() is exposed by ixgbe_api.h, and can be
> used directly.
> I think we need a return value for above function, and then the caller can
> check it.
> If it fails, does it need to return out, or just skip the failure?
For it's an additional check, I don't want to let it break the normal process.
If there's a failure (suppose not, because it's executed during init, there should
be enough ether type entities.), only output error log.

> In addition, is this operation only for x550, right? If yes, it may need a check
> above.
It's depends on if this NIC supports this function " hw->mac.ops.set_ethertype_anti_spoofing",
If some new ixgbe NICs can support it in future, we need not change the code.
But seems I should check it first to avoid occupy a ethertype_filter entity without using it. I'll send a V2.

> 
> Regards,
> Helin
> 
> > +
> > +	return;
> > +}
> > +
> >  int ixgbe_pf_host_configure(struct rte_eth_dev *eth_dev)  {
> >  	uint32_t vtctl, fcrth;
> > @@ -262,6 +303,8 @@ int ixgbe_pf_host_configure(struct rte_eth_dev
> > *eth_dev)
> >  		IXGBE_WRITE_REG(hw, IXGBE_FCRTH_82599(i), fcrth);
> >  	}
> >
> > +	ixgbe_add_tx_flow_control_drop_filter(eth_dev);
> > +
> >  	return 0;
> >  }
> >
> > --
> > 1.9.3