RE: [Intel-wired-lan] [PATCH v6 3/3] ixgbe, ixgbevf: Add new mbox API to enable MC promiscuous mode

2015-06-23 Thread Hiroshi Shimamoto
> Subject: Re: [Intel-wired-lan] [PATCH v6 3/3] ixgbe, ixgbevf: Add new mbox 
> API to enable MC promiscuous mode
> 
> On 06/17/2015 04:45 AM, Hiroshi Shimamoto wrote:
> > From: Hiroshi Shimamoto 
> >
> > The limitation of the number of multicast address for VF is not enough
> > for the large scale server with SR-IOV feature.
> > IPv6 requires the multicast MAC address for each IP address to handle
> > the Neighbor Solicitation message.
> > We couldn't assign over 30 IPv6 addresses to a single VF interface.
> >
> > The easy way to solve this is enabling multicast promiscuous mode.
> > It is good to have a functionality to enable multicast promiscuous mode
> > for each VF from VF driver.
> >
> > This patch introduces the new mbox API, IXGBE_VF_SET_MC_PROMISC, to
> > enable/disable multicast promiscuous mode in VF. If multicast
> > promiscuous mode is enabled the VF can receive all multicast packets.
> >
> > With this patch, the ixgbevf driver automatically enable multicast
> > promiscuous mode when the number of multicast addresses is over than 30
> > if possible.
> >
> > PF only allow to enbale VF multicast promiscuous mode if the VF is trusted.
> > If not trusted, PF returns an error to VF and VF will fallback the previous
> > behavior, that only 30 multicast addresses are registered to the filter.
> >
> > Signed-off-by: Hiroshi Shimamoto 
> > CC: Choi, Sy Jong 
> > ---
> >   drivers/net/ethernet/intel/ixgbe/ixgbe.h  |  1 +
> >   drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h  |  2 +
> >   drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c| 55 
> > +++
> >   drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |  3 ++
> >   drivers/net/ethernet/intel/ixgbevf/mbx.h  |  2 +
> >   drivers/net/ethernet/intel/ixgbevf/vf.c   | 49 
> > +++-
> >   drivers/net/ethernet/intel/ixgbevf/vf.h   |  1 +
> >   7 files changed, 112 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h 
> > b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> > index 7f76c12..054db64 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> > @@ -146,6 +146,7 @@ struct vf_data_storage {
> > u16 vlans_enabled;
> > bool clear_to_send;
> > bool pf_set_mac;
> > +   bool mc_promisc;
> > u16 pf_vlan; /* When set, guest VLAN config not allowed. */
> > u16 pf_qos;
> > u16 tx_rate;
> 
> Instead of casting this as a bool I think it might be better served as
> an enum.  You basically have 4 levels you could set:
> DISABLED  No traffic allowed, Rx disabled, PF only
> NONE  only L2 exact match addresses or Flow Director enabled
> MULTI BAM & ROMPE set
> ALLMULTI  BAM, ROMPE, & MPE set
> PROMISC   BAM, ROMPE, MPE, & UPE (available on x540)
> VLAN_PROMISC  BAM, ROMPE, MPE, UPE, & VPE (available on x540)
> 
> That just leaves AUPE and ROPE which are kind of special cases.  AUPE
> should be set if an port VLAN is not assigned by the PF, and as far as
> ROPE it could be thought of as a poor-mans promiscuous so it might be
> useful for 82599 to possibly try to put together some sort of
> promiscuous mode though I cannot say for certain.
> 
> The idea is to make use of the enum to enable higher or lower levels of
> escalation.  You could then limit a non-trusted VF to MULTI for any
> requests of ALLMULTI, PROMISC, or VLAN_PROMSIC and if the VF is trusted
> it would have access to ALLMULTI on 82599, and potentially PROMISC or
> VLAN_PROMISC on x540 and newer.
> 
> It hadn't occurred to me until just now that the NONE option might be
> desirable to some as well since it is possible that somebody would
> rather use flow director rules to send traffic to a VF rather than have
> it receive broadcast or multicast traffic.  By doing this we enable that
> as a possible use case.  It could all be controlled through the
> IFF_BROADCAST, IFF_MULTICAST, IFF_ALLMULTI, and IFF_PROMISC flags in
> set_rx_mode.
> 
> We did something like this for fm10k as it was a requirement of the
> Switch API.  You could probably do something similar for the
> ixgbe/ixgbevf mailbox interface as it seems like it might be a better
> fit than adding a new message to cover one specific case.

I'm considering and working about the above change.
I agree with having such mode change interface is better than adding a specific
feature message.

thanks,
Hiroshi

> 
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h

Re: [Intel-wired-lan] [PATCH v6 3/3] ixgbe, ixgbevf: Add new mbox API to enable MC promiscuous mode

2015-06-17 Thread Alexander Duyck

On 06/17/2015 04:45 AM, Hiroshi Shimamoto wrote:

From: Hiroshi Shimamoto 

The limitation of the number of multicast address for VF is not enough
for the large scale server with SR-IOV feature.
IPv6 requires the multicast MAC address for each IP address to handle
the Neighbor Solicitation message.
We couldn't assign over 30 IPv6 addresses to a single VF interface.

The easy way to solve this is enabling multicast promiscuous mode.
It is good to have a functionality to enable multicast promiscuous mode
for each VF from VF driver.

This patch introduces the new mbox API, IXGBE_VF_SET_MC_PROMISC, to
enable/disable multicast promiscuous mode in VF. If multicast
promiscuous mode is enabled the VF can receive all multicast packets.

With this patch, the ixgbevf driver automatically enable multicast
promiscuous mode when the number of multicast addresses is over than 30
if possible.

PF only allow to enbale VF multicast promiscuous mode if the VF is trusted.
If not trusted, PF returns an error to VF and VF will fallback the previous
behavior, that only 30 multicast addresses are registered to the filter.

Signed-off-by: Hiroshi Shimamoto 
CC: Choi, Sy Jong 
---
  drivers/net/ethernet/intel/ixgbe/ixgbe.h  |  1 +
  drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h  |  2 +
  drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c| 55 +++
  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |  3 ++
  drivers/net/ethernet/intel/ixgbevf/mbx.h  |  2 +
  drivers/net/ethernet/intel/ixgbevf/vf.c   | 49 +++-
  drivers/net/ethernet/intel/ixgbevf/vf.h   |  1 +
  7 files changed, 112 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h 
b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 7f76c12..054db64 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -146,6 +146,7 @@ struct vf_data_storage {
u16 vlans_enabled;
bool clear_to_send;
bool pf_set_mac;
+   bool mc_promisc;
u16 pf_vlan; /* When set, guest VLAN config not allowed. */
u16 pf_qos;
u16 tx_rate;


Instead of casting this as a bool I think it might be better served as 
an enum.  You basically have 4 levels you could set:

DISABLEDNo traffic allowed, Rx disabled, PF only
NONEonly L2 exact match addresses or Flow Director enabled
MULTI   BAM & ROMPE set
ALLMULTIBAM, ROMPE, & MPE set
PROMISC BAM, ROMPE, MPE, & UPE (available on x540)
VLAN_PROMISCBAM, ROMPE, MPE, UPE, & VPE (available on x540)

That just leaves AUPE and ROPE which are kind of special cases.  AUPE 
should be set if an port VLAN is not assigned by the PF, and as far as 
ROPE it could be thought of as a poor-mans promiscuous so it might be 
useful for 82599 to possibly try to put together some sort of 
promiscuous mode though I cannot say for certain.


The idea is to make use of the enum to enable higher or lower levels of 
escalation.  You could then limit a non-trusted VF to MULTI for any 
requests of ALLMULTI, PROMISC, or VLAN_PROMSIC and if the VF is trusted 
it would have access to ALLMULTI on 82599, and potentially PROMISC or 
VLAN_PROMISC on x540 and newer.


It hadn't occurred to me until just now that the NONE option might be 
desirable to some as well since it is possible that somebody would 
rather use flow director rules to send traffic to a VF rather than have 
it receive broadcast or multicast traffic.  By doing this we enable that 
as a possible use case.  It could all be controlled through the 
IFF_BROADCAST, IFF_MULTICAST, IFF_ALLMULTI, and IFF_PROMISC flags in 
set_rx_mode.


We did something like this for fm10k as it was a requirement of the 
Switch API.  You could probably do something similar for the 
ixgbe/ixgbevf mailbox interface as it seems like it might be a better 
fit than adding a new message to cover one specific case.



diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h 
b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
index b1e4703..703d40b 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
@@ -102,6 +102,8 @@ enum ixgbe_pfvf_api_rev {
  #define IXGBE_VF_GET_RETA 0x0a/* VF request for RETA */
  #define IXGBE_VF_GET_RSS_KEY  0x0b/* get RSS key */

+#define IXGBE_VF_SET_MC_PROMISC0x0c/* VF requests MC promiscuous */
+
  /* length of permanent address message returned from PF */
  #define IXGBE_VF_PERMADDR_MSG_LEN 4
  /* word in permanent address message with the current multicast type */


You might just want to refer to this as SET_XCAST_MODE since that is 
essentially what this command is doing.



diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c 
b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index 826f88e..925d9c6 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -119,6 +