diff mbox series

net/ixgbe: fix MAT enable for VF in multicast

Message ID 1546410760-24879-1-git-send-email-wei.zhao1@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers
Series net/ixgbe: fix MAT enable for VF in multicast |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Zhao1, Wei Jan. 2, 2019, 6:32 a.m. UTC 
  In ixgbe PMD code, all vf ars set with bit IXGBE_VMOLR_ROMPE,
which make vf accept packets that match the MTA table,
if some vf update IXGBE_MTA in function ixgbe_vf_set_multicast,
then all vf will receive packets from these address.
So thhere is need to set VMOLR register bit ROPE onlty after this
vf has been set multicast address. If this bit is when pf host doing
initialization, this vf will receive multicast packets with address
written in MTA table. Align to ixgbe pf kernel 5.3.7 code to fix this
bug.

Fixes: 00e30184daa0 ("ixgbe: add PF support")

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
---
 drivers/net/ixgbe/ixgbe_pf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Qi Zhang Jan. 3, 2019, 1:47 p.m. UTC | #1 
Hi Wei

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wei Zhao
> Sent: Wednesday, January 2, 2019 2:33 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Zhao1, Wei
> <wei.zhao1@intel.com>
> Subject: [dpdk-dev] [PATCH] net/ixgbe: fix MAT enable for VF in multicast

What is MAT means ? 
> 
> In ixgbe PMD code, all vf ars set with bit IXGBE_VMOLR_ROMPE, which make vf
> accept packets that match the MTA table, if some vf update IXGBE_MTA in
> function ixgbe_vf_set_multicast, then all vf will receive packets from these
> address.
> So thhere is need to set VMOLR register bit ROPE onlty after this vf has been
> set multicast address. If this bit is when pf host doing initialization, this vf will
> receive multicast packets with address written in MTA table. Align to ixgbe pf
> kernel 5.3.7 code to fix this bug.

There are some typo in you commit log.

> 
> Fixes: 00e30184daa0 ("ixgbe: add PF support")
> 
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_pf.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_pf.c b/drivers/net/ixgbe/ixgbe_pf.c index
> 4b833ff..0f4b96b 100644
> --- a/drivers/net/ixgbe/ixgbe_pf.c
> +++ b/drivers/net/ixgbe/ixgbe_pf.c
> @@ -351,7 +351,7 @@ ixgbe_vf_reset_event(struct rte_eth_dev *dev,
> uint16_t vf)
>  	int rar_entry = hw->mac.num_rar_entries - (vf + 1);
>  	uint32_t vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(vf));
> 
> -	vmolr |= (IXGBE_VMOLR_ROPE | IXGBE_VMOLR_ROMPE |
> +	vmolr |= (IXGBE_VMOLR_ROPE |
>  			IXGBE_VMOLR_BAM | IXGBE_VMOLR_AUPE);
>  	IXGBE_WRITE_REG(hw, IXGBE_VMOLR(vf), vmolr);
> 
> @@ -503,6 +503,7 @@ ixgbe_vf_set_multicast(struct rte_eth_dev *dev,
> uint32_t vf, uint32_t *msgbuf)
>  	const uint32_t IXGBE_MTA_BIT_MASK = (0x1 << IXGBE_MTA_BIT_SHIFT) -
> 1;
>  	uint32_t reg_val;
>  	int i;
> +	u32 vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(vf));
> 
>  	/* Disable multicast promiscuous first */
>  	ixgbe_disable_vf_mc_promisc(dev, vf);
> @@ -525,6 +526,9 @@ ixgbe_vf_set_multicast(struct rte_eth_dev *dev,
> uint32_t vf, uint32_t *msgbuf)
>  		IXGBE_WRITE_REG(hw, IXGBE_MTA(mta_idx), reg_val);
>  	}
> 
> +	vmolr |= IXGBE_VMOLR_ROMPE;
> +	IXGBE_WRITE_REG(hw, IXGBE_VMOLR(vf), vmolr);


Please correct me if I'm wrong

My understand is MTA table is shared by all VFs (I guess also pf), but what if two VFs both enable multi-cast but with different address filter?
Should we prevent the second one to enable multi-cast if any conflict be detected? Otherwise there still will be unexpected behavior.

> +
>  	return 0;
>  }
> 
> --
> 2.7.5
Zhao1, Wei Jan. 4, 2019, 8:34 a.m. UTC | #2 
> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Thursday, January 3, 2019 9:47 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Zhao1, Wei
> <wei.zhao1@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: fix MAT enable for VF in
> multicast
> 
> Hi Wei
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wei Zhao
> > Sent: Wednesday, January 2, 2019 2:33 PM
> > To: dev@dpdk.org
> > Cc: stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Zhao1, Wei
> > <wei.zhao1@intel.com>
> > Subject: [dpdk-dev] [PATCH] net/ixgbe: fix MAT enable for VF in
> > multicast
> 
> What is MAT means ?
> >
> > In ixgbe PMD code, all vf ars set with bit IXGBE_VMOLR_ROMPE, which
> > make vf accept packets that match the MTA table, if some vf update
> > IXGBE_MTA in function ixgbe_vf_set_multicast, then all vf will receive
> > packets from these address.
> > So thhere is need to set VMOLR register bit ROPE onlty after this vf
> > has been set multicast address. If this bit is when pf host doing
> > initialization, this vf will receive multicast packets with address
> > written in MTA table. Align to ixgbe pf kernel 5.3.7 code to fix this bug.
> 
> There are some typo in you commit log.

Sorry, v2 will commit.

> 
> >
> > Fixes: 00e30184daa0 ("ixgbe: add PF support")
> >
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > ---
> >  drivers/net/ixgbe/ixgbe_pf.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_pf.c
> > b/drivers/net/ixgbe/ixgbe_pf.c index 4b833ff..0f4b96b 100644
> > --- a/drivers/net/ixgbe/ixgbe_pf.c
> > +++ b/drivers/net/ixgbe/ixgbe_pf.c
> > @@ -351,7 +351,7 @@ ixgbe_vf_reset_event(struct rte_eth_dev *dev,
> > uint16_t vf)
> >  	int rar_entry = hw->mac.num_rar_entries - (vf + 1);
> >  	uint32_t vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(vf));
> >
> > -	vmolr |= (IXGBE_VMOLR_ROPE | IXGBE_VMOLR_ROMPE |
> > +	vmolr |= (IXGBE_VMOLR_ROPE |
> >  			IXGBE_VMOLR_BAM | IXGBE_VMOLR_AUPE);
> >  	IXGBE_WRITE_REG(hw, IXGBE_VMOLR(vf), vmolr);
> >
> > @@ -503,6 +503,7 @@ ixgbe_vf_set_multicast(struct rte_eth_dev *dev,
> > uint32_t vf, uint32_t *msgbuf)
> >  	const uint32_t IXGBE_MTA_BIT_MASK = (0x1 <<
> IXGBE_MTA_BIT_SHIFT) -
> > 1;
> >  	uint32_t reg_val;
> >  	int i;
> > +	u32 vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(vf));
> >
> >  	/* Disable multicast promiscuous first */
> >  	ixgbe_disable_vf_mc_promisc(dev, vf); @@ -525,6 +526,9 @@
> > ixgbe_vf_set_multicast(struct rte_eth_dev *dev, uint32_t vf, uint32_t
> > *msgbuf)
> >  		IXGBE_WRITE_REG(hw, IXGBE_MTA(mta_idx), reg_val);
> >  	}
> >
> > +	vmolr |= IXGBE_VMOLR_ROMPE;
> > +	IXGBE_WRITE_REG(hw, IXGBE_VMOLR(vf), vmolr);
> 
> 
> Please correct me if I'm wrong
> 
> My understand is MTA table is shared by all VFs (I guess also pf), but what if

Yes, vf share it but not pf, it is used in vf pool switch

> two VFs both enable multi-cast but with different address filter?
> Should we prevent the second one to enable multi-cast if any conflict be
> detected? Otherwise there still will be unexpected behavior.

Sorry, I do not known what is the confict.
Because IXGBE_VMOLR is vf specific, that is to say, each vf control itself for enable ROMPE.


> 
> > +
> >  	return 0;
> >  }
> >
> > --
> > 2.7.5
Qi Zhang Jan. 4, 2019, 12:13 p.m. UTC | #3 
> -----Original Message-----
> From: Zhao1, Wei
> Sent: Friday, January 4, 2019 4:35 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: fix MAT enable for VF in multicast
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Thursday, January 3, 2019 9:47 PM
> > To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Zhao1, Wei
> > <wei.zhao1@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: fix MAT enable for VF in
> > multicast
> >
> > Hi Wei
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wei Zhao
> > > Sent: Wednesday, January 2, 2019 2:33 PM
> > > To: dev@dpdk.org
> > > Cc: stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Zhao1,
> > > Wei <wei.zhao1@intel.com>
> > > Subject: [dpdk-dev] [PATCH] net/ixgbe: fix MAT enable for VF in
> > > multicast
> >
> > What is MAT means ?
> > >
> > > In ixgbe PMD code, all vf ars set with bit IXGBE_VMOLR_ROMPE, which
> > > make vf accept packets that match the MTA table, if some vf update
> > > IXGBE_MTA in function ixgbe_vf_set_multicast, then all vf will
> > > receive packets from these address.
> > > So thhere is need to set VMOLR register bit ROPE onlty after this vf
> > > has been set multicast address. If this bit is when pf host doing
> > > initialization, this vf will receive multicast packets with address
> > > written in MTA table. Align to ixgbe pf kernel 5.3.7 code to fix this bug.
> >
> > There are some typo in you commit log.
> 
> Sorry, v2 will commit.
> 
> >
> > >
> > > Fixes: 00e30184daa0 ("ixgbe: add PF support")
> > >
> > > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > > ---
> > >  drivers/net/ixgbe/ixgbe_pf.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/ixgbe/ixgbe_pf.c
> > > b/drivers/net/ixgbe/ixgbe_pf.c index 4b833ff..0f4b96b 100644
> > > --- a/drivers/net/ixgbe/ixgbe_pf.c
> > > +++ b/drivers/net/ixgbe/ixgbe_pf.c
> > > @@ -351,7 +351,7 @@ ixgbe_vf_reset_event(struct rte_eth_dev *dev,
> > > uint16_t vf)
> > >  	int rar_entry = hw->mac.num_rar_entries - (vf + 1);
> > >  	uint32_t vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(vf));
> > >
> > > -	vmolr |= (IXGBE_VMOLR_ROPE | IXGBE_VMOLR_ROMPE |
> > > +	vmolr |= (IXGBE_VMOLR_ROPE |
> > >  			IXGBE_VMOLR_BAM | IXGBE_VMOLR_AUPE);
> > >  	IXGBE_WRITE_REG(hw, IXGBE_VMOLR(vf), vmolr);
> > >
> > > @@ -503,6 +503,7 @@ ixgbe_vf_set_multicast(struct rte_eth_dev *dev,
> > > uint32_t vf, uint32_t *msgbuf)
> > >  	const uint32_t IXGBE_MTA_BIT_MASK = (0x1 <<
> > IXGBE_MTA_BIT_SHIFT) -
> > > 1;
> > >  	uint32_t reg_val;
> > >  	int i;
> > > +	u32 vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(vf));
> > >
> > >  	/* Disable multicast promiscuous first */
> > >  	ixgbe_disable_vf_mc_promisc(dev, vf); @@ -525,6 +526,9 @@
> > > ixgbe_vf_set_multicast(struct rte_eth_dev *dev, uint32_t vf,
> > > uint32_t
> > > *msgbuf)
> > >  		IXGBE_WRITE_REG(hw, IXGBE_MTA(mta_idx), reg_val);
> > >  	}
> > >
> > > +	vmolr |= IXGBE_VMOLR_ROMPE;
> > > +	IXGBE_WRITE_REG(hw, IXGBE_VMOLR(vf), vmolr);
> >
> >
> > Please correct me if I'm wrong
> >
> > My understand is MTA table is shared by all VFs (I guess also pf), but
> > what if
> 
> Yes, vf share it but not pf, it is used in vf pool switch
> 
> > two VFs both enable multi-cast but with different address filter?
> > Should we prevent the second one to enable multi-cast if any conflict
> > be detected? Otherwise there still will be unexpected behavior.
> 
> Sorry, I do not known what is the confict.
> Because IXGBE_VMOLR is vf specific, that is to say, each vf control itself for
> enable ROMPE.

The conflict what I mean is for example, vf1 set filter for address A while vf2 set filter for address B, then both VFs will receive A and B which is not expected.
But if the only issue for receive unexpected multi-cast packet is just a performance impact then I agree with most of patch, 
one more comment is we may need to reset IXGBE_VMOLR_ROMPE if nb_entries = 0, which is the case to clear all multi-cast filter.

> 
> 
> >
> > > +
> > >  	return 0;
> > >  }
> > >
> > > --
> > > 2.7.5
diff mbox series

Patch

diff --git a/drivers/net/ixgbe/ixgbe_pf.c b/drivers/net/ixgbe/ixgbe_pf.c
index 4b833ff..0f4b96b 100644
--- a/drivers/net/ixgbe/ixgbe_pf.c
+++ b/drivers/net/ixgbe/ixgbe_pf.c
@@ -351,7 +351,7 @@  ixgbe_vf_reset_event(struct rte_eth_dev *dev, uint16_t vf)
 	int rar_entry = hw->mac.num_rar_entries - (vf + 1);
 	uint32_t vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(vf));
 
-	vmolr |= (IXGBE_VMOLR_ROPE | IXGBE_VMOLR_ROMPE |
+	vmolr |= (IXGBE_VMOLR_ROPE |
 			IXGBE_VMOLR_BAM | IXGBE_VMOLR_AUPE);
 	IXGBE_WRITE_REG(hw, IXGBE_VMOLR(vf), vmolr);
 
@@ -503,6 +503,7 @@  ixgbe_vf_set_multicast(struct rte_eth_dev *dev, uint32_t vf, uint32_t *msgbuf)
 	const uint32_t IXGBE_MTA_BIT_MASK = (0x1 << IXGBE_MTA_BIT_SHIFT) - 1;
 	uint32_t reg_val;
 	int i;
+	u32 vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(vf));
 
 	/* Disable multicast promiscuous first */
 	ixgbe_disable_vf_mc_promisc(dev, vf);
@@ -525,6 +526,9 @@  ixgbe_vf_set_multicast(struct rte_eth_dev *dev, uint32_t vf, uint32_t *msgbuf)
 		IXGBE_WRITE_REG(hw, IXGBE_MTA(mta_idx), reg_val);
 	}
 
+	vmolr |= IXGBE_VMOLR_ROMPE;
+	IXGBE_WRITE_REG(hw, IXGBE_VMOLR(vf), vmolr);
+
 	return 0;
 }