| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZykNzvV3d5SXe7Yn@LQ3V64L9R2>
Date: Mon, 4 Nov 2024 10:09:18 -0800
From: Joe Damato <jdamato@...tly.com>
To: Mohsin Bashir <mohsin.bashr@...il.com>
Cc: netdev@...r.kernel.org, alexanderduyck@...com, kuba@...nel.org,
andrew@...n.ch, andrew+netdev@...n.ch, davem@...emloft.net,
edumazet@...gle.com, pabeni@...hat.com, kernel-team@...a.com,
sanmanpradhan@...a.com, sdf@...ichev.me, vadim.fedorenko@...ux.dev,
horms@...nel.org
Subject: Re: [PATCH net-next v5] eth: fbnic: Add support to write TCE TCAM
entries
Feel free to take my review with a large grain of salt as I am not
as experienced as others on this with giving good reviews, but I
left a few comments you may want to consider below:
On Sun, Nov 03, 2024 at 07:13:00PM -0800, Mohsin Bashir wrote:
> Add support to redirect host-to-BMC traffic by writing MACDA entries
> from the RPC (RX Parser and Classifier) to TCE-TCAM. The TCE TCAM is a
> small L2 destination TCAM which is placed at the end of the TX path (TCE).
>
> Unlike other NICs, where BMC diversion is typically handled by firmware,
> for fbnic, firmware does not touch anything related to the host; hence,
> the host uses TCE TCAM to divert 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@...il.com>
> ---
> V5: Add sign off at the right place
> V4: https://lore.kernel.org/netdev/20241101204116.1368328-1-mohsin.bashr@gmail.com
> V3: https://lore.kernel.org/netdev/20241025225910.30187-1-mohsin.bashr@gmail.com
> V2: https://lore.kernel.org/netdev/20241024223135.310733-1-mohsin.bashr@gmail.com
> 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 */
Is the 0x04200 here FBNIC_CSR_START_TCE_RAM ? If so, maybe it can be
replaced with define?
Does the macro need to be on its own line? Maybe it does due to line
length, not sure.
[...]
> 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--;) {
This is probably not a kernel / networking cosmetic thing and
probably just me, but I personally find this style of for loop quite
odd where one field is elided.
Maybe a while loop could be used instead and it'd be easier to read?
int mac_idex = ARRAY_SIZE(fbd->mac_addr);
while (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");
In the error case does fbd->tce_tcam_last need to be set ?
> + 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;
Wouldn't this end up setting tce_tcam_last to zero every time or am
I missing something?
> +}
> +
> +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");
As above, in the error case does fbd->tce_tcam_last need to be set ?
> + 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;
As above, wouldn't this always set tce_tcam_last to
FBNIC_TCE_TCAM_NUM_ENTRIES every time?
Sorry if I'm missing something here.
> +}
Powered by blists - more mailing lists