[PATCH net-next v2] eth: fbnic: Add support to write TCE TCAM entries

[PATCH net-next v2] eth: fbnic: Add support to write TCE TCAM entries

[Mohsin Bashir]

Add support for writing to the tce tcam to enable host to bmc traffic.
Currently, we lack metadata to track where addresses have been written
in the tcam, except for the last entry written. To address this issue,
we start at the opposite end of the table in each pass, so that adding
or deleting entries does not affect the availability of all entries,
assuming there is no significant reordering of entries.

Signed-off-by: Mohsin Bashir <mohsin.bashr@gmail.com>
---
V2:
- Fixed spelling error from 'entrie' to 'entries' in the summary.

V1: https://lore.kernel.org/netdev/20241021185544.713305-1-mohsin.bashr@gmail.com
---
 drivers/net/ethernet/meta/fbnic/fbnic.h       |   1 +
 drivers/net/ethernet/meta/fbnic/fbnic_csr.h   |  20 ++++
 .../net/ethernet/meta/fbnic/fbnic_netdev.c    |   1 +
 drivers/net/ethernet/meta/fbnic/fbnic_rpc.c   | 110 ++++++++++++++++++
 drivers/net/ethernet/meta/fbnic/fbnic_rpc.h   |   4 +
 5 files changed, 136 insertions(+)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic.h b/drivers/net/ethernet/meta/fbnic/fbnic.h
index fec567c8fe4a..9f9cb9b3e74e 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic.h
@@ -48,6 +48,7 @@ struct fbnic_dev {
 	struct fbnic_act_tcam act_tcam[FBNIC_RPC_TCAM_ACT_NUM_ENTRIES];
 	struct fbnic_mac_addr mac_addr[FBNIC_RPC_TCAM_MACDA_NUM_ENTRIES];
 	u8 mac_addr_boundary;
+	u8 tce_tcam_last;
 
 	/* Number of TCQs/RCQs available on hardware */
 	u16 max_num_queues;
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
index 79cdd231d327..dd407089ca47 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
@@ -397,6 +397,14 @@ enum {
 #define FBNIC_TCE_DROP_CTRL_TTI_FRM_DROP_EN	CSR_BIT(1)
 #define FBNIC_TCE_DROP_CTRL_TTI_TBI_DROP_EN	CSR_BIT(2)
 
+#define FBNIC_TCE_TCAM_IDX2DEST_MAP	0x0404A		/* 0x10128 */
+#define FBNIC_TCE_TCAM_IDX2DEST_MAP_DEST_ID_0	CSR_GENMASK(3, 0)
+enum {
+	FBNIC_TCE_TCAM_DEST_MAC		= 1,
+	FBNIC_TCE_TCAM_DEST_BMC		= 2,
+	FBNIC_TCE_TCAM_DEST_FW		= 4,
+};
+
 #define FBNIC_TCE_TXB_TX_BMC_Q_CTRL	0x0404B		/* 0x1012c */
 #define FBNIC_TCE_TXB_BMC_DWRR_CTRL	0x0404C		/* 0x10130 */
 #define FBNIC_TCE_TXB_BMC_DWRR_CTRL_QUANTUM0	CSR_GENMASK(7, 0)
@@ -407,6 +415,18 @@ enum {
 #define FBNIC_TCE_TXB_BMC_DWRR_CTRL_EXT	0x0404F		/* 0x1013c */
 #define FBNIC_CSR_END_TCE		0x04050	/* CSR section delimiter */
 
+/* TCE RAM registers */
+#define FBNIC_CSR_START_TCE_RAM		0x04200	/* CSR section delimiter */
+#define FBNIC_TCE_RAM_TCAM(m, n) \
+	(0x04200 + 0x8 * (n) + (m))		/* 0x10800 + 32*n + 4*m */
+#define FBNIC_TCE_RAM_TCAM_MASK			CSR_GENMASK(15, 0)
+#define FBNIC_TCE_RAM_TCAM_VALUE		CSR_GENMASK(31, 16)
+#define FBNIC_TCE_RAM_TCAM3(n)		(0x04218 + (n))	/* 0x010860 + 4*n */
+#define FBNIC_TCE_RAM_TCAM3_DEST_MASK		CSR_GENMASK(5, 3)
+#define FBNIC_TCE_RAM_TCAM3_MCQ_MASK		CSR_BIT(7)
+#define FBNIC_TCE_RAM_TCAM3_VALIDATE		CSR_BIT(31)
+#define FBNIC_CSR_END_TCE_RAM		0x0421F	/* CSR section delimiter */
+
 /* TMI registers */
 #define FBNIC_CSR_START_TMI		0x04400	/* CSR section delimiter */
 #define FBNIC_TMI_SOP_PROT_CTRL		0x04400		/* 0x11000 */
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
index c08798fad203..fc7d80db5fa6 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
@@ -273,6 +273,7 @@ void __fbnic_set_rx_mode(struct net_device *netdev)
 	/* Write updates to hardware */
 	fbnic_write_rules(fbd);
 	fbnic_write_macda(fbd);
+	fbnic_write_tce_tcam(fbd);
 }
 
 static void fbnic_set_rx_mode(struct net_device *netdev)
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
index 337b8b3aef2f..908c098cd59e 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
@@ -587,6 +587,116 @@ static void fbnic_clear_act_tcam(struct fbnic_dev *fbd, unsigned int idx)
 		wr32(fbd, FBNIC_RPC_TCAM_ACT(idx, i), 0);
 }
 
+static void fbnic_clear_tce_tcam_entry(struct fbnic_dev *fbd, unsigned int idx)
+{
+	int i;
+
+	/* Invalidate entry and clear addr state info */
+	for (i = 0; i <= FBNIC_TCE_TCAM_WORD_LEN; i++)
+		wr32(fbd, FBNIC_TCE_RAM_TCAM(idx, i), 0);
+}
+
+static void fbnic_write_tce_tcam_dest(struct fbnic_dev *fbd, unsigned int idx,
+				      struct fbnic_mac_addr *mac_addr)
+{
+	u32 dest = FBNIC_TCE_TCAM_DEST_BMC;
+	u32 idx2dest_map;
+
+	if (is_multicast_ether_addr(mac_addr->value.addr8))
+		dest |= FBNIC_TCE_TCAM_DEST_MAC;
+
+	idx2dest_map = rd32(fbd, FBNIC_TCE_TCAM_IDX2DEST_MAP);
+	idx2dest_map &= ~(FBNIC_TCE_TCAM_IDX2DEST_MAP_DEST_ID_0 << (4 * idx));
+	idx2dest_map |= dest << (4 * idx);
+
+	wr32(fbd, FBNIC_TCE_TCAM_IDX2DEST_MAP, idx2dest_map);
+}
+
+static void fbnic_write_tce_tcam_entry(struct fbnic_dev *fbd, unsigned int idx,
+				       struct fbnic_mac_addr *mac_addr)
+{
+	__be16 *mask, *value;
+	int i;
+
+	mask = &mac_addr->mask.addr16[FBNIC_TCE_TCAM_WORD_LEN - 1];
+	value = &mac_addr->value.addr16[FBNIC_TCE_TCAM_WORD_LEN - 1];
+
+	for (i = 0; i < FBNIC_TCE_TCAM_WORD_LEN; i++)
+		wr32(fbd, FBNIC_TCE_RAM_TCAM(idx, i),
+		     FIELD_PREP(FBNIC_TCE_RAM_TCAM_MASK, ntohs(*mask--)) |
+		     FIELD_PREP(FBNIC_TCE_RAM_TCAM_VALUE, ntohs(*value--)));
+
+	wrfl(fbd);
+
+	wr32(fbd, FBNIC_TCE_RAM_TCAM3(idx), FBNIC_TCE_RAM_TCAM3_MCQ_MASK |
+				       FBNIC_TCE_RAM_TCAM3_DEST_MASK |
+				       FBNIC_TCE_RAM_TCAM3_VALIDATE);
+}
+
+static void __fbnic_write_tce_tcam_rev(struct fbnic_dev *fbd)
+{
+	int tcam_idx = FBNIC_TCE_TCAM_NUM_ENTRIES;
+	int mac_idx;
+
+	for (mac_idx = ARRAY_SIZE(fbd->mac_addr); mac_idx--;) {
+		struct fbnic_mac_addr *mac_addr = &fbd->mac_addr[mac_idx];
+
+		/* Verify BMC bit is set */
+		if (!test_bit(FBNIC_MAC_ADDR_T_BMC, mac_addr->act_tcam))
+			continue;
+
+		if (!tcam_idx) {
+			dev_err(fbd->dev, "TCE TCAM overflow\n");
+			return;
+		}
+
+		tcam_idx--;
+		fbnic_write_tce_tcam_dest(fbd, tcam_idx, mac_addr);
+		fbnic_write_tce_tcam_entry(fbd, tcam_idx, mac_addr);
+	}
+
+	while (tcam_idx)
+		fbnic_clear_tce_tcam_entry(fbd, --tcam_idx);
+
+	fbd->tce_tcam_last = tcam_idx;
+}
+
+static void __fbnic_write_tce_tcam(struct fbnic_dev *fbd)
+{
+	int tcam_idx = 0;
+	int mac_idx;
+
+	for (mac_idx = 0; mac_idx < ARRAY_SIZE(fbd->mac_addr); mac_idx++) {
+		struct fbnic_mac_addr *mac_addr = &fbd->mac_addr[mac_idx];
+
+		/* Verify BMC bit is set */
+		if (!test_bit(FBNIC_MAC_ADDR_T_BMC, mac_addr->act_tcam))
+			continue;
+
+		if (tcam_idx == FBNIC_TCE_TCAM_NUM_ENTRIES) {
+			dev_err(fbd->dev, "TCE TCAM overflow\n");
+			return;
+		}
+
+		fbnic_write_tce_tcam_dest(fbd, tcam_idx, mac_addr);
+		fbnic_write_tce_tcam_entry(fbd, tcam_idx, mac_addr);
+		tcam_idx++;
+	}
+
+	while (tcam_idx < FBNIC_TCE_TCAM_NUM_ENTRIES)
+		fbnic_clear_tce_tcam_entry(fbd, tcam_idx++);
+
+	fbd->tce_tcam_last = tcam_idx;
+}
+
+void fbnic_write_tce_tcam(struct fbnic_dev *fbd)
+{
+	if (fbd->tce_tcam_last)
+		__fbnic_write_tce_tcam_rev(fbd);
+	else
+		__fbnic_write_tce_tcam(fbd);
+}
+
 void fbnic_clear_rules(struct fbnic_dev *fbd)
 {
 	u32 dest = FIELD_PREP(FBNIC_RPC_ACT_TBL0_DEST_MASK,
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.h b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.h
index d62935f722a2..0d8285fa5b45 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.h
@@ -35,6 +35,9 @@ enum {
 #define FBNIC_RPC_TCAM_ACT_WORD_LEN		11
 #define FBNIC_RPC_TCAM_ACT_NUM_ENTRIES		64
 
+#define FBNIC_TCE_TCAM_WORD_LEN			3
+#define FBNIC_TCE_TCAM_NUM_ENTRIES		8
+
 struct fbnic_mac_addr {
 	union {
 		unsigned char addr8[ETH_ALEN];
@@ -186,4 +189,5 @@ static inline int __fbnic_mc_unsync(struct fbnic_mac_addr *mac_addr)
 
 void fbnic_clear_rules(struct fbnic_dev *fbd);
 void fbnic_write_rules(struct fbnic_dev *fbd);
+void fbnic_write_tce_tcam(struct fbnic_dev *fbd);
 #endif /* _FBNIC_RPC_H_ */
-- 
2.43.5

Re: [PATCH net-next v2] eth: fbnic: Add support to write TCE TCAM entries

[Simon Horman]

On Thu, Oct 24, 2024 at 03:31:35PM -0700, Mohsin Bashir wrote:
> Add support for writing to the tce tcam to enable host to bmc traffic.
> Currently, we lack metadata to track where addresses have been written
> in the tcam, except for the last entry written. To address this issue,
> we start at the opposite end of the table in each pass, so that adding
> or deleting entries does not affect the availability of all entries,
> assuming there is no significant reordering of entries.
> 
> Signed-off-by: Mohsin Bashir <mohsin.bashr@gmail.com>
> ---
> V2:
> - Fixed spelling error from 'entrie' to 'entries' in the summary.
> 
> V1: https://lore.kernel.org/netdev/20241021185544.713305-1-mohsin.bashr@gmail.com

Thanks for the update.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v2] eth: fbnic: Add support to write TCE TCAM entries

[Alexander Lobakin]

From: Mohsin Bashir <mohsin.bashr@gmail.com>
Date: Thu, 24 Oct 2024 15:31:35 -0700

> Add support for writing to the tce tcam to enable host to bmc traffic.
> Currently, we lack metadata to track where addresses have been written
> in the tcam, except for the last entry written. To address this issue,
> we start at the opposite end of the table in each pass, so that adding
> or deleting entries does not affect the availability of all entries,
> assuming there is no significant reordering of entries.
> 
> Signed-off-by: Mohsin Bashir <mohsin.bashr@gmail.com>

[...]

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> index 337b8b3aef2f..908c098cd59e 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> @@ -587,6 +587,116 @@ static void fbnic_clear_act_tcam(struct fbnic_dev *fbd, unsigned int idx)
>  		wr32(fbd, FBNIC_RPC_TCAM_ACT(idx, i), 0);
>  }
>  
> +static void fbnic_clear_tce_tcam_entry(struct fbnic_dev *fbd, unsigned int idx)
> +{
> +	int i;
> +
> +	/* Invalidate entry and clear addr state info */
> +	for (i = 0; i <= FBNIC_TCE_TCAM_WORD_LEN; i++)

Please declare loop iterators right in loop declarations, we're GNU11
for a couple years already.

	for (u32 i = 0; ...

(+ don't use signed when it can't be < 0)

(this all applies to all your code)

> +		wr32(fbd, FBNIC_TCE_RAM_TCAM(idx, i), 0);
> +}

Thanks,
Olek

Re: [PATCH net-next v2] eth: fbnic: Add support to write TCE TCAM entries

[Jakub Kicinski]

On Fri, 25 Oct 2024 17:19:03 +0200 Alexander Lobakin wrote:
> > +static void fbnic_clear_tce_tcam_entry(struct fbnic_dev *fbd, unsigned int idx)
> > +{
> > +	int i;
> > +
> > +	/* Invalidate entry and clear addr state info */
> > +	for (i = 0; i <= FBNIC_TCE_TCAM_WORD_LEN; i++)  
> 
> Please declare loop iterators right in loop declarations, we're GNU11
> for a couple years already.
> 
> 	for (u32 i = 0; ...

Why?

Please avoid giving people subjective stylistic feedback, especially
when none of the maintainers have given such feedback in the past.

> (+ don't use signed when it can't be < 0)

Again, why. int is the most basic type in C, why is using a fixed side
kernel type necessary here?

Re: [PATCH net-next v2] eth: fbnic: Add support to write TCE TCAM entries

[Alexander Lobakin]

From: Jakub Kicinski <kuba@kernel.org>
Date: Mon, 28 Oct 2024 16:35:54 -0700

> On Fri, 25 Oct 2024 17:19:03 +0200 Alexander Lobakin wrote:
>>> +static void fbnic_clear_tce_tcam_entry(struct fbnic_dev *fbd, unsigned int idx)
>>> +{
>>> +	int i;
>>> +
>>> +	/* Invalidate entry and clear addr state info */
>>> +	for (i = 0; i <= FBNIC_TCE_TCAM_WORD_LEN; i++)  
>>
>> Please declare loop iterators right in loop declarations, we're GNU11
>> for a couple years already.
>>
>> 	for (u32 i = 0; ...
> 
> Why?

Because we usually declare variables only inside the scopes within which
they're used, IOW

	for (...) {
		void *data;

		data = ...
	}

is preferred over

	void *data;

	for (...) {
		data = ...
	}

Here it's the same. `for (int` reduces the scope of the iterator.
The iter is not used outside the loop.

>
> Please avoid giving people subjective stylistic feedback, especially

I didn't say "You must do X" anywhere, only proposed some stuff, which
from my PoV would improve the code.
And make the style more consistent. "Avoiding giving people subjective
stylistic feedback" led to that it's not really consistent beyond the
level of checkpatch's complaints.

> when none of the maintainers have given such feedback in the past.

I don't think my mission as a reviewer is to be a parrot?

> 
>> (+ don't use signed when it can't be < 0)
> 
> Again, why. int is the most basic type in C, why is using a fixed side
> kernel type necessary here?

Because the negative part is not used at all here. Why not __u128 or
double then if it doesn't matter?

Thanks,
Olek

Re: [PATCH net-next v2] eth: fbnic: Add support to write TCE TCAM entries

[Jakub Kicinski]

On Tue, 29 Oct 2024 16:30:54 +0100 Alexander Lobakin wrote:
> >> Please declare loop iterators right in loop declarations, we're GNU11
> >> for a couple years already.
> >>
> >> 	for (u32 i = 0; ...  
> > 
> > Why?  
> 
> Because we usually declare variables only inside the scopes within which
> they're used, IOW
> 
> 	for (...) {
> 		void *data;
> 
> 		data = ...
> 	}
> 
> is preferred over
> 
> 	void *data;
> 
> 	for (...) {
> 		data = ...
> 	}

Are you actually actively pointing that out in review?
If it was an important rule why is there no automation
to catch cases where variable is only used in a single
basic block but is declared at function scope.

> Here it's the same. `for (int` reduces the scope of the iterator.
> The iter is not used outside the loop.
> 
> > Please avoid giving people subjective stylistic feedback, especially  
> 
> I didn't say "You must do X" anywhere, only proposed some stuff, which
> from my PoV would improve the code.

You said "please do XYZ" which in English is pretty strong.

> And make the style more consistent. "Avoiding giving people subjective
> stylistic feedback" led to that it's not really consistent beyond the
> level of checkpatch's complaints.

checkpatch is obviously bad at its job but I don't think random people
giving subjective stylistic feedback will improve the situation.
We have a handful of reviewers who review maybe 1 in 10 patches.
The reviews are very much appreciated but since those reviewers are not
covering substantial portion of the code merged they should not come up
with guidelines of their own making.

I see plenty of cases where one patch gets nit picked to death on small
stylistic issues and another gets merged even tho its far below average.
Doesn't feel very fair.

> > when none of the maintainers have given such feedback in the past.  
> 
> I don't think my mission as a reviewer is to be a parrot?

Not what I'm saying. Please focus on functional review of the code,
and process / stylistic review only to the extent to which such
rules are widely applied. We even documented this in the netdev "FAQ".

> >> (+ don't use signed when it can't be < 0)  
> > 
> > Again, why. int is the most basic type in C, why is using a fixed side
> > kernel type necessary here?  
> 
> Because the negative part is not used at all here. Why not __u128 or
> double then if it doesn't matter?

We have plenty of bugs because someone decided to use an unsigned type 
and then decided to decrement as long as its >= 0..

Re: [PATCH net-next v2] eth: fbnic: Add support to write TCE TCAM entries

[Andrew Lunn]

On Thu, Oct 24, 2024 at 03:31:35PM -0700, Mohsin Bashir wrote:
> Add support for writing to the tce tcam to enable host to bmc traffic.

The commit message is not really helping me understand what is going
on here. What entries are you adding to the TCAM? Normally a TCAM is
used on ingress, but you are talking about host to BMC, which is
egress?

	Andrew

Re: [PATCH net-next v2] eth: fbnic: Add support to write TCE TCAM entries

[Mohsin Bashir]

Hi Andrew,

Basically, in addition to the RX TCAM (RPC) that you mentioned, we also 
have a TCAM on the TX path that enables traffic redirection for BMC. 
Unlike other NICs where BMC diversion is typically handled by firmware, 
FBNIC firmware does not touch anything host-related. In this patch, we 
are writing MACDA entries from the RPC (Rx Parser and Classifier) to the 
TX TCAM, allowing us to reroute any host traffic destined for BMC.

I will ensure that the commit message for the revised version clearly 
explains this.

On 10/30/24 11:34 AM, Andrew Lunn wrote:
> On Thu, Oct 24, 2024 at 03:31:35PM -0700, Mohsin Bashir wrote:
>> Add support for writing to the tce tcam to enable host to bmc traffic.
> 
> The commit message is not really helping me understand what is going
> on here. What entries are you adding to the TCAM? Normally a TCAM is
> used on ingress, but you are talking about host to BMC, which is
> egress?
> 
> 	Andrew

Re: [PATCH net-next v2] eth: fbnic: Add support to write TCE TCAM entries

[Andrew Lunn]

On Wed, Oct 30, 2024 at 05:51:53PM -0700, Mohsin Bashir wrote:
> Hi Andrew,
> 

> Basically, in addition to the RX TCAM (RPC) that you mentioned, we
> also have a TCAM on the TX path that enables traffic redirection for
> BMC. Unlike other NICs where BMC diversion is typically handled by
> firmware, FBNIC firmware does not touch anything host-related. In
> this patch, we are writing MACDA entries from the RPC (Rx Parser and
> Classifier) to the TX TCAM, allowing us to reroute any host traffic
> destined for BMC.

Two TCAMs, that makes a bit more sense.

But why is this hooked into set_rx_mode? It is nothing to do with RX.
I assume you have some mechanism to get the MAC address of the BMC. I
would of thought you need to write one entry into the TCAM during
probe, and you are done?

	Andrew

Re: [PATCH net-next v2] eth: fbnic: Add support to write TCE TCAM entries

[Mohsin Bashir]

On 10/31/24 5:43 AM, Andrew Lunn wrote:
> On Wed, Oct 30, 2024 at 05:51:53PM -0700, Mohsin Bashir wrote:
>> Hi Andrew,
>>
> 
>> Basically, in addition to the RX TCAM (RPC) that you mentioned, we
>> also have a TCAM on the TX path that enables traffic redirection for
>> BMC. Unlike other NICs where BMC diversion is typically handled by
>> firmware, FBNIC firmware does not touch anything host-related. In
>> this patch, we are writing MACDA entries from the RPC (Rx Parser and
>> Classifier) to the TX TCAM, allowing us to reroute any host traffic
>> destined for BMC.
> 
> Two TCAMs, that makes a bit more sense.
> 
> But why is this hooked into set_rx_mode? It is nothing to do with RX.

We are trying to maintain a single central function to handle MAC updates.

> I assume you have some mechanism to get the MAC address of the BMC. I
> would of thought you need to write one entry into the TCAM during
> probe, and you are done?
> 
> 	Andrew

Actually, we may need to write entries in other cases as well. The fact 
that the BMC can come and go independently of the host would result in 
firmware notifying the host of the resulting change. Consequently, the 
host would need to make some changes that will be added in the following 
patch(es).

Re: [PATCH net-next v2] eth: fbnic: Add support to write TCE TCAM entries

[Andrew Lunn]

On Thu, Oct 31, 2024 at 03:13:40PM -0700, Mohsin Bashir wrote:
> On 10/31/24 5:43 AM, Andrew Lunn wrote:
> > On Wed, Oct 30, 2024 at 05:51:53PM -0700, Mohsin Bashir wrote:
> > > Hi Andrew,
> > > 
> > 
> > > Basically, in addition to the RX TCAM (RPC) that you mentioned, we
> > > also have a TCAM on the TX path that enables traffic redirection for
> > > BMC. Unlike other NICs where BMC diversion is typically handled by
> > > firmware, FBNIC firmware does not touch anything host-related. In
> > > this patch, we are writing MACDA entries from the RPC (Rx Parser and
> > > Classifier) to the TX TCAM, allowing us to reroute any host traffic
> > > destined for BMC.
> > 
> > Two TCAMs, that makes a bit more sense.
> > 
> > But why is this hooked into set_rx_mode? It is nothing to do with RX.
> 
> We are trying to maintain a single central function to handle MAC updates.

So you plan to call set_rx_mode() for any change to the TCAM, for any
reason? When IGMP snooping asks you to add an multicast entry due to
snooping?  ethtool --config-ntuple? Some TC actions? i assume you are
going to call it yourself, not rely on the stack call set_rx_mode()
for you?

> 
> > I assume you have some mechanism to get the MAC address of the BMC. I
> > would of thought you need to write one entry into the TCAM during
> > probe, and you are done?
> > 
> > 	Andrew
> 
> Actually, we may need to write entries in other cases as well. The fact that
> the BMC can come and go independently of the host would result in firmware
> notifying the host of the resulting change. Consequently, the host would
> need to make some changes that will be added in the following patch(es).

And the MAC address changes when the firmware goes away and comes back
again? It is not burned into an OTP/EEPROM? What triggers
set_rx_mode() being called in this condition?

	Andrew

Re: [PATCH net-next v2] eth: fbnic: Add support to write TCE TCAM entries

[Alexander H Duyck]

On Fri, 2024-11-01 at 13:33 +0100, Andrew Lunn wrote:
> On Thu, Oct 31, 2024 at 03:13:40PM -0700, Mohsin Bashir wrote:
> > On 10/31/24 5:43 AM, Andrew Lunn wrote:
> > > On Wed, Oct 30, 2024 at 05:51:53PM -0700, Mohsin Bashir wrote:
> > > > Hi Andrew,
> > > > 
> > > 
> > > > Basically, in addition to the RX TCAM (RPC) that you mentioned, we
> > > > also have a TCAM on the TX path that enables traffic redirection for
> > > > BMC. Unlike other NICs where BMC diversion is typically handled by
> > > > firmware, FBNIC firmware does not touch anything host-related. In
> > > > this patch, we are writing MACDA entries from the RPC (Rx Parser and
> > > > Classifier) to the TX TCAM, allowing us to reroute any host traffic
> > > > destined for BMC.
> > > 
> > > Two TCAMs, that makes a bit more sense.
> > > 
> > > But why is this hooked into set_rx_mode? It is nothing to do with RX.
> > 
> > We are trying to maintain a single central function to handle MAC updates.
> 
> So you plan to call set_rx_mode() for any change to the TCAM, for any
> reason? When IGMP snooping asks you to add an multicast entry due to
> snooping?  ethtool --config-ntuple? Some TC actions? i assume you are
> going to call it yourself, not rely on the stack call set_rx_mode()
> for you?
> 

What the set_rx_mode function is doing is taking in all the MAC
addresses from all the various lists, unicast, multicast, BMC, and then
updating the TCAM as needed. We are keeping a copy of the TCAM in
software so if we call set_rx_mode with no change there is no change to
the hardware setup.

I think the TCE TCAM may be the only exception to that as it might
shift up or down as we are just parsing the list for BMC addresses and
writing them top down, or bottom up in order to preserve the entries in
the case of one being added or removed. So the table may shift if there
is no change, but the contents should stay the same.

Basically the BMC makes it so that we have to modify the ACT_TCAM rules
if we add/remove the BMC addresses, or if we toggle the ALL_MULTI flag
since the BMC can force us to have to place that rule at either entry
24 in the MACDA TCAM, or if it isn't present we place it at entry 31.

For the --config-ntuple we don't call it as the assumption is that we
aren't going to be modifying the multicast or unicast promiscuous flags
due to the rule addition. Instead that does a subset where it will use
fbnic_write_macda to update the MACDA TCAM if there is a new MAC
destination address added.

> > > I assume you have some mechanism to get the MAC address of the BMC. I
> > > would of thought you need to write one entry into the TCAM during
> > > probe, and you are done?
> > > 
> > > 	Andrew
> > 
> > Actually, we may need to write entries in other cases as well. The fact that
> > the BMC can come and go independently of the host would result in firmware
> > notifying the host of the resulting change. Consequently, the host would
> > need to make some changes that will be added in the following patch(es).
> 
> And the MAC address changes when the firmware goes away and comes back
> again? It is not burned into an OTP/EEPROM? What triggers
> set_rx_mode() being called in this condition?
> 
> 	Andrew

So there are BMC addresses that are passed in via the Firmware
Capabilities Response message. Unfortunately we have to deal with BMCs
that will bail on us and force us to remove the entries for it as soon
as they see the link go down and come back as soon as they see the link
come up. In addition the NCSI spec basically makes it so that we have
to support the BMC deciding to change the config or enable/disable the
channel on a whim.

In such a case we have to go through and take care of several items
including pointing our TCE TCAM at the BMC addresses, and then sending
a message to the firmware with the list of our unicast MAC addresses so
that it can send the BMC packets back to us if it is sending to our
unicast addresses. Unfortunately the BMC also tends to enjoy running in
multicast promiscuous so we don't have any way of knowing what
mutlicast entries it has so we normally will just have it send us all
it's multicast traffic and likewise all our multicast traffic will be
routed to it. It simplifies the handling a bit, but also is kind of
sloppy as far as I am concerned.

- Alex