[Intel-wired-lan] [PATCH net v2] ice: add profile conflict check for AVF FDIR

[Guo, Junfeng]

> -----Original Message-----
> From: Brandeburg, Jesse <jesse.brandeburg at intel.com>
> Sent: Monday, March 13, 2023 23:51
> To: Guo, Junfeng <junfeng.guo at intel.com>; intel-wired-
> lan at lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH net v2] ice: add profile conflict
> check for AVF FDIR
> 
> On 3/12/2023 7:17 PM, Junfeng Guo wrote:
> > Add profile conflict check while adding some FDIR rules to aviod
> > unexpected flow behavior, rules may have conflict including:
> >         IPv4 <---> {IPv4_UDP, IPv4_TCP, IPv4_SCTP}
> >         IPv6 <---> {IPv6_UDP, IPv6_TCP, IPv6_SCTP}
> >
> > For example, when we create an FDIR rule for IPv4, this rule will work
> > on packets including IPv4, IPv4_UDP, IPv4_TCP and IPv4_SCTP. But if we
> > then create an FDIR rule for IPv4_UDP and then destroy it, the first
> > FDIR rule for IPv4 cannot work on pkt IPv4_UDP then.
> >
> > To prevent this unexpected behavior, we add restriction in software
> > when creating FDIR rules by adding necessary profile conflict check.
> >
> > Fixes: 1f7ea1cd6a37 ("ice: Enable FDIR Configure for AVF")
> > Signed-off-by: Junfeng Guo <junfeng.guo at intel.com>
> > ---
> >  .../ethernet/intel/ice/ice_virtchnl_fdir.c    | 73 +++++++++++++++++++
> >  1 file changed, 73 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > index e6ef6b303222..06296ffc3608 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > @@ -541,6 +541,73 @@ static void ice_vc_fdir_rem_prof_all(struct
> ice_vf *vf)
> >  	}
> >  }
> >
> > +/**
> > + * ice_vc_fdir_has_prof_conflict
> > + * @vf: pointer to the VF structure
> > + * @conf: FDIR configuration for each filter
> > + *
> > + * Check if @conf has conflicting profile with existing profiles
> > + *
> > + * Return: true on success, and false on error.
> > + */
> > +static bool
> > +ice_vc_fdir_has_prof_conflict(struct ice_vf *vf,
> > +			      struct virtchnl_fdir_fltr_conf *conf)
> > +{
> > +	struct ice_fdir_fltr *desc;
> > +
> > +	list_for_each_entry(desc, &vf->fdir.fdir_rule_list, fltr_node) {
> > +		struct virtchnl_fdir_fltr_conf *existing_conf =
> > +
> 	to_fltr_conf_from_desc(desc);
> > +		enum ice_fltr_ptype flow_type_a, flow_type_b;
> > +		struct ice_fdir_fltr *a, *b;
> > +
> > +		a = &existing_conf->input;
> > +		b = &conf->input;
> > +		flow_type_a = a->flow_type;
> > +		flow_type_b = b->flow_type;
> > +
> > +		/**
> > +		 * No need to compare two rules with different tunnel
> types or
> 
> "/**" is special magic for declaring documentation in kernel and
> doxygen. Just use:
> 
> /* multi-line
>  * comment
>  */
> 
> which is netdev style.

Sure, will follow this style. Thanks for the reminding!

> 
> > +		 * with the same protocol type
> > +		 */
> > +		if (existing_conf->ttype != conf->ttype ||
> > +		    flow_type_a == flow_type_b)
> > +			continue;
> > +
> > +		switch (flow_type_a) {
> > +		case ICE_FLTR_PTYPE_NONF_IPV4_UDP:
> > +		case ICE_FLTR_PTYPE_NONF_IPV4_TCP:
> > +		case ICE_FLTR_PTYPE_NONF_IPV4_SCTP:
> > +			if (flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV4_OTHER)
> > +				return true;
> > +			break;
> > +		case ICE_FLTR_PTYPE_NONF_IPV4_OTHER:
> > +			if (flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV4_UDP ||
> > +			    flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV4_TCP ||
> > +			    flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV4_SCTP)
> > +				return true;
> > +			break;
> > +		case ICE_FLTR_PTYPE_NONF_IPV6_UDP:
> > +		case ICE_FLTR_PTYPE_NONF_IPV6_TCP:
> > +		case ICE_FLTR_PTYPE_NONF_IPV6_SCTP:
> > +			if (flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV6_OTHER)
> > +				return true;
> > +			break;
> > +		case ICE_FLTR_PTYPE_NONF_IPV6_OTHER:
> > +			if (flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV6_UDP ||
> > +			    flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV6_TCP ||
> > +			    flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV6_SCTP)
> > +				return true;
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  /**
> >   * ice_vc_fdir_write_flow_prof
> >   * @vf: pointer to the VF structure
> > @@ -677,6 +744,12 @@ ice_vc_fdir_config_input_set(struct ice_vf *vf,
> struct virtchnl_fdir_add *fltr,
> >  	enum ice_fltr_ptype flow;
> >  	int ret;
> >
> > +	ret = ice_vc_fdir_has_prof_conflict(vf, conf);
> > +	if (ret) {
> > +		dev_dbg(dev, "Found flow prof conflict for VF %d\n", vf-
> >vf_id);
> 
> This is just a nit, but generally messages to the log should always
> contain full words and not abbreviations.
> 
> "Found flow profile conflict for VF %d\n"

Will update this, thanks!

> 
> 
> > +		return ret;
> > +	}
> > +
> >  	flow = input->flow_type;
> >  	ret = ice_vc_fdir_alloc_prof(vf, flow);
> >  	if (ret) {