Re: [PATCH v12 1/2] i40e: support floating VEB config

[Zhe Tao <zhe.tao@intel.com>]

On Fri, Jun 24, 2016 at 12:14:14PM +0100, Ferruh Yigit wrote:
> Hi Zhe,
> 
> On 6/24/2016 9:29 AM, Zhe Tao wrote:
> > Add the new floating VEB related arguments option in the devarg.
> > Using this parameter, all the applications can decide whether to use legacy
> > VEB/VEPA or floating VEB.
> > To enable this feature, the user should pass a devargs parameter to the
> > EAL like "-w 84:00.0,enable_floating_veb=1", and the application will
> > tell PMD whether to use the floating VEB feature or not.
> Is providing the enable_floating_veb=1 devarg enough, or should
> application call an driver API to enable this feature, if so documenting
> what application needs to do helps app developers.
yes only provide the devarg is enough, the APP don't need to modify.
> 
> > Once the floating VEB feature is enabled, all the VFs created by
> > this PF device are connected to the floating VEB.
> Technically there can be multiple floating VEBs, right? And with this
> param only one floating VEB created and all VFs connected to it. Is
> there any use case to create multiple VEBs with selective VFs connected
> to them?
discuss this feature internally with Walter and FAE team, have an agreement
for this release we only implement one floating VEB.
> 
> > 
> > Also user can specify which VF need to connect to this floating veb using
> > "floating_veb_list".
> > Like "-w 84:00.0,enable_floating_veb=1,floating_veb_list=1/3-4", means VF1, VF3,
> > VF4 connect to the floating VEB, other VFs connect to the legacy VEB.The "/"
> > is used for delimiter of the floating VEB list.
> Is there a use case to change VF VEB connection on runtime?
maybe there is a case, but from the customer requirement, we only support the static
configuration for floating VEB.

> 
> > 
> > All the VEB/VEPA concepts are not specific for FVL, they are defined in
> > the 802.1Qbg spec.
> > 
> > But for floating VEB, it has two major difference.
> > 1. doesn't has a up link connection which means
> > the traffic cannot go to outside world.
> > 2. doesn't need to connect to the physical port which means
> > when the physical link is down the floating VEB can still works
> > fine.
> > 
> > Signed-off-by: Zhe Tao <zhe.tao@intel.com>
> > ---
> 
> ...
> 
> >  
> > +static int vf_floating_veb_handler(__rte_unused const char *key,
> What about floating_veb_list_handler, to be consistent with argument name?
> 
> > +				   const char *floating_veb_value,
> > +				   void *opaque)
> > +{
> > +	int idx = 0;
> > +	bool floating_veb; /* The flag to use the floating VEB */
> > +	/* The floating enable flag for the specific VF */
> > +	bool vf_floating_veb[I40E_MAX_VF];
> what about floating_veb_list, to be consistent with argument name?
> 
> >  };
> >  
> >  enum pending_msg {
> > 
All the comments in the code is great, I changed the code according to your
comments,
thanks again
Zhe