Re: [PATCH v3 net-next 04/13] octeontx2-af: Add mbox messages to install and delete MCAM rules

Hi Alexander,

Thanks for the review.

-----Original Message-----
From: Alexander Duyck <redacted>
Sent: Friday, November 13, 2020 3:17 AM
To: Naveen Mamindlapalli <redacted>
Cc: Netdev <redacted>; LKML <redacted>;
Jakub Kicinski [off-list ref]; David Miller [off-list ref];
saeed@kernel.org; Sunil Kovvuri Goutham [off-list ref]; Linu
Cherian [off-list ref]; Geethasowjanya Akula
[off-list ref]; Jerin Jacob Kollanukkaran [off-list ref];
Subbaraya Sundeep Bhatta [off-list ref]; Hariprasad Kelam
[off-list ref]
Subject: Re: [PATCH v3 net-next 04/13] octeontx2-af: Add mbox messages
to install and delete MCAM rules

On Tue, Nov 10, 2020 at 11:22 PM Naveen Mamindlapalli
[off-list ref] wrote:

quoted

From: Subbaraya Sundeep <sbhatta@marvell.com>

Added new mailbox messages to install and delete MCAM rules.
These mailbox messages will be used for adding/deleting ethtool
n-tuple filters by NIX PF. The installed MCAM rules are stored in a
list that will be traversed later to delete the MCAM entries when the
interface is brought down or when PCIe FLR is received.
The delete mailbox supports deleting a single MCAM entry or range of
entries or all the MCAM entries owned by the pcifunc. Each MCAM entry
can be associated with a HW match stat entry if the mailbox requester
wants to check the hit count for debugging.

Modified adding default unicast DMAC match rule using install flow
API. The default unicast DMAC match entry installed by Administrative
Function is saved and can be changed later by the mailbox user to fit
additional fields, or the default MCAM entry rule action can be used
for other flow rules installed later.

Modified rvu_mbox_handler_nix_lf_free mailbox to add a flag to disable
or delete the MCAM entries. The MCAM entries are disabled when the
interface is brought down and deleted in FLR handler.
The disabled MCAM entries will be re-enabled when the interface is
brought up again.

Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com>
Signed-off-by: Sunil Goutham <sgoutham@marvell.com>
Signed-off-by: Naveen Mamindlapalli <redacted>

A couple minor issues to address, called out in comments below.

quoted

---
 drivers/net/ethernet/marvell/octeontx2/af/common.h |   2 +
 drivers/net/ethernet/marvell/octeontx2/af/mbox.h   |  76 ++-
 drivers/net/ethernet/marvell/octeontx2/af/npc.h    |  57 +-
 drivers/net/ethernet/marvell/octeontx2/af/rvu.h    |  13 +
 .../net/ethernet/marvell/octeontx2/af/rvu_nix.c    |  19 +-
 .../net/ethernet/marvell/octeontx2/af/rvu_npc.c    | 217 ++++++-
 .../net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c | 721

+++++++++++++++++++++

quoted

 .../net/ethernet/marvell/octeontx2/nic/otx2_pf.c   |  12 +-
 8 files changed, 1065 insertions(+), 52 deletions(-)

<snip>

quoted

diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c

b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
index eb4eaa7ece3a..a7759ecfa586 100644

--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c

@@ -219,7 +219,7 @@ static int npc_get_nixlf_mcam_index(struct

npc_mcam *mcam,

quoted

        return npc_get_ucast_mcam_index(mcam, pcifunc, nixlf);  }

-static int npc_get_bank(struct npc_mcam *mcam, int index)
+int npc_get_bank(struct npc_mcam *mcam, int index)
 {
        int bank = index / mcam->banksize;

@@ -241,8 +241,8 @@ static bool is_mcam_entry_enabled(struct rvu *rvu,

struct npc_mcam *mcam,

quoted

        return (cfg & 1);
 }

-static void npc_enable_mcam_entry(struct rvu *rvu, struct npc_mcam

*mcam,

quoted

-                                 int blkaddr, int index, bool enable)
+void npc_enable_mcam_entry(struct rvu *rvu, struct npc_mcam *mcam,
+                          int blkaddr, int index, bool enable)
 {
        int bank = npc_get_bank(mcam, index);
        int actbank = bank;

@@ -359,6 +359,41 @@ static void npc_get_keyword(struct mcam_entry

*entry, int idx,

quoted

        *cam0 = ~*cam1 & kw_mask;
 }

+static void npc_get_default_entry_action(struct rvu *rvu, struct npc_mcam

*mcam,

quoted

+                                        int blkaddr, int index,
+                                        struct mcam_entry *entry) {
+       u16 owner, target_func;
+       struct rvu_pfvf *pfvf;
+       int bank, nixlf;
+       u64 rx_action;
+
+       owner = mcam->entry2pfvf_map[index];
+       target_func = (entry->action >> 4) & 0xffff;
+       /* return incase target is PF or LBK or rule owner is not PF */
+       if (is_afvf(target_func) || (owner & RVU_PFVF_FUNC_MASK) ||
+           !(target_func & RVU_PFVF_FUNC_MASK))
+               return;
+
+       pfvf = rvu_get_pfvf(rvu, target_func);
+       mcam->entry2target_pffunc[index] = target_func;
+       /* return if nixlf is not attached or initialized */
+       if (!is_nixlf_attached(rvu, target_func) || !pfvf->def_ucast_rule)
+               return;
+
+       /* get VF ucast entry rule */
+       nix_get_nixlf(rvu, target_func, &nixlf, NULL);
+       index = npc_get_nixlf_mcam_index(mcam, target_func,
+                                        nixlf, NIXLF_UCAST_ENTRY);
+       bank = npc_get_bank(mcam, index);
+       index &= (mcam->banksize - 1);
+
+       rx_action = rvu_read64(rvu, blkaddr,
+                              NPC_AF_MCAMEX_BANKX_ACTION(index, bank));
+       if (rx_action)
+               entry->action = rx_action; }
+
 static void npc_config_mcam_entry(struct rvu *rvu, struct npc_mcam *mcam,
                                  int blkaddr, int index, u8 intf,
                                  struct mcam_entry *entry, bool
enable) @@ -406,6 +441,11 @@ static void npc_config_mcam_entry(struct

rvu *rvu, struct npc_mcam *mcam,

quoted

                            NPC_AF_MCAMEX_BANKX_CAMX_W1(index, bank, 0), cam0);
        }

+       /* copy VF default entry action to the VF mcam entry */
+       if (intf == NIX_INTF_RX && actindex < mcam->bmap_entries)
+               npc_get_default_entry_action(rvu, mcam, blkaddr, actindex,
+                                            entry);
+
        /* Set 'action' */
        rvu_write64(rvu, blkaddr,
                    NPC_AF_MCAMEX_BANKX_ACTION(index, actbank),
entry->action); @@ -473,11 +513,12 @@ void

rvu_npc_install_ucast_entry(struct rvu *rvu, u16 pcifunc,

quoted

                                 int nixlf, u64 chan, u8 *mac_addr)  {
        struct rvu_pfvf *pfvf = rvu_get_pfvf(rvu, pcifunc);
+       u8 mac_mask[] = { [0 ... ETH_ALEN] = 0xFF };

Is this supposed to be a 7 byte long array? I assume that is what is
meant by the 0 ... ETH_ALEN which would imply it covers entries 0 - 6.
This might be better as:
        u8 mac_mask[ETH_ALEN] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF };

I will use eth_broadcast_addr() to avoid this variable.

quoted

+       struct npc_install_flow_req req = { 0 };
+       struct npc_install_flow_rsp rsp = { 0 };
        struct npc_mcam *mcam = &rvu->hw->mcam;

Since the mcam line is longer normally it should be before the 2 new
lines in order to maintain the reverse xmas tree format.

This looks like my outlook email client issue. I don't see any problem with vim.

quoted

-       struct mcam_entry entry = { {0} };
        struct nix_rx_action action;
-       int blkaddr, index, kwi;
-       u64 mac = 0;
+       int blkaddr, index;

        /* AF's VFs work in promiscuous mode */
        if (is_afvf(pcifunc))

@@ -487,20 +528,9 @@ void rvu_npc_install_ucast_entry(struct rvu *rvu, u16

pcifunc,

quoted

        if (blkaddr < 0)
                return;

-       for (index = ETH_ALEN - 1; index >= 0; index--)
-               mac |= ((u64)*mac_addr++) << (8 * index);
-
        index = npc_get_nixlf_mcam_index(mcam, pcifunc,
                                         nixlf, NIXLF_UCAST_ENTRY);

-       /* Match ingress channel and DMAC */
-       entry.kw[0] = chan;
-       entry.kw_mask[0] = 0xFFFULL;
-
-       kwi = NPC_PARSE_RESULT_DMAC_OFFSET / sizeof(u64);
-       entry.kw[kwi] = mac;
-       entry.kw_mask[kwi] = BIT_ULL(48) - 1;
-
        /* Don't change the action if entry is already enabled
         * Otherwise RSS action may get overwritten.
         */

@@ -513,20 +543,20 @@ void rvu_npc_install_ucast_entry(struct rvu *rvu,

u16 pcifunc,

quoted

                action.pf_func = pcifunc;
        }

-       entry.action = *(u64 *)&action;
-       npc_config_mcam_entry(rvu, mcam, blkaddr, index,
-                             pfvf->nix_rx_intf, &entry, true);
-
-       /* add VLAN matching, setup action and save entry back for later */
-       entry.kw[0] |= (NPC_LT_LB_STAG_QINQ | NPC_LT_LB_CTAG) << 20;
-       entry.kw_mask[0] |= (NPC_LT_LB_STAG_QINQ & NPC_LT_LB_CTAG) <<

20;

quoted

+       req.default_rule = 1;
+       ether_addr_copy(req.packet.dmac, mac_addr);
+       ether_addr_copy(req.mask.dmac, mac_mask);

If this is all you were using the mac_mask for you could probably just
use a memset here to achieve the same thing and save yourself the
trouble of allocating the mac_mask. See eth_broadcast_addr().

I will update the code to use eth_broadcast_addr() and send in v4.

quoted

+       req.features = BIT_ULL(NPC_DMAC);
+       req.channel = chan;
+       req.intf = pfvf->nix_rx_intf;
+       req.op = action.op;
+       req.hdr.pcifunc = 0; /* AF is requester */
+       req.vf = action.pf_func;
+       req.index = action.index;
+       req.match_id = action.match_id;
+       req.flow_key_alg = action.flow_key_alg;

-       entry.vtag_action = VTAG0_VALID_BIT |
-                           FIELD_PREP(VTAG0_TYPE_MASK, 0) |
-                           FIELD_PREP(VTAG0_LID_MASK, NPC_LID_LA) |
-                           FIELD_PREP(VTAG0_RELPTR_MASK, 12);
-
-       memcpy(&pfvf->entry, &entry, sizeof(entry));
+       rvu_mbox_handler_npc_install_flow(rvu, &req, &rsp);
 }

<snip>

quoted

+static void npc_update_flow(struct rvu *rvu, struct mcam_entry *entry,
+                           u64 features, struct flow_msg *pkt,
+                           struct flow_msg *mask,
+                           struct rvu_npc_mcam_rule *output, u8 intf)
+{
+       u64 dmac_mask = ether_addr_to_u64(mask->dmac);
+       u64 smac_mask = ether_addr_to_u64(mask->smac);
+       u64 dmac_val = ether_addr_to_u64(pkt->dmac);
+       u64 smac_val = ether_addr_to_u64(pkt->smac);
+       struct flow_msg *opkt = &output->packet;
+       struct flow_msg *omask = &output->mask;
+
+       if (!features)
+               return;
+
+#define NPC_WRITE_FLOW(field, member, val_lo, val_hi, mask_lo, mask_hi)

\

quoted

+do {                                                                         \
+       if (features & BIT_ULL((field))) {                                    \
+               npc_update_entry(rvu, (field), entry, (val_lo), (val_hi),     \
+                                (mask_lo), (mask_hi), intf);                 \
+               memcpy(&opkt->member, &pkt->member, sizeof(pkt->member));     \
+               memcpy(&omask->member, &mask->member, sizeof(mask-
member));  \
+       }                                                                     \
+} while (0)
+

The placement of this macro seems wierd to me. Either it needs to be
boe moved before the function declaration, or it should be defined
where it is used instead of this block above it.

I will move the macro to a block below where it is being used.

quoted

+       /* For tcp/udp/sctp LTYPE should be present in entry */
+       if (features & (BIT_ULL(NPC_SPORT_TCP) | BIT_ULL(NPC_DPORT_TCP)))
+               npc_update_entry(rvu, NPC_LD, entry, NPC_LT_LD_TCP,
+                                0, ~0ULL, 0, intf);
+       if (features & (BIT_ULL(NPC_SPORT_UDP) | BIT_ULL(NPC_DPORT_UDP)))
+               npc_update_entry(rvu, NPC_LD, entry, NPC_LT_LD_UDP,
+                                0, ~0ULL, 0, intf);
+       if (features & (BIT_ULL(NPC_SPORT_SCTP) |

BIT_ULL(NPC_DPORT_SCTP)))

quoted

+               npc_update_entry(rvu, NPC_LD, entry, NPC_LT_LD_SCTP,
+                                0, ~0ULL, 0, intf);
+
+       if (features & BIT_ULL(NPC_OUTER_VID))
+               npc_update_entry(rvu, NPC_LB, entry,
+                                NPC_LT_LB_STAG_QINQ | NPC_LT_LB_CTAG, 0,
+                                NPC_LT_LB_STAG_QINQ & NPC_LT_LB_CTAG, 0, intf);
+
+       NPC_WRITE_FLOW(NPC_DMAC, dmac, dmac_val, 0, dmac_mask, 0);
+       NPC_WRITE_FLOW(NPC_SMAC, smac, smac_val, 0, smac_mask, 0);
+       NPC_WRITE_FLOW(NPC_ETYPE, etype, ntohs(pkt->etype), 0,
+                      ntohs(mask->etype), 0);
+       NPC_WRITE_FLOW(NPC_SIP_IPV4, ip4src, ntohl(pkt->ip4src), 0,
+                      ntohl(mask->ip4src), 0);
+       NPC_WRITE_FLOW(NPC_DIP_IPV4, ip4dst, ntohl(pkt->ip4dst), 0,
+                      ntohl(mask->ip4dst), 0);
+       NPC_WRITE_FLOW(NPC_SPORT_TCP, sport, ntohs(pkt->sport), 0,
+                      ntohs(mask->sport), 0);
+       NPC_WRITE_FLOW(NPC_SPORT_UDP, sport, ntohs(pkt->sport), 0,
+                      ntohs(mask->sport), 0);
+       NPC_WRITE_FLOW(NPC_DPORT_TCP, dport, ntohs(pkt->dport), 0,
+                      ntohs(mask->dport), 0);
+       NPC_WRITE_FLOW(NPC_DPORT_UDP, dport, ntohs(pkt->dport), 0,
+                      ntohs(mask->dport), 0);
+       NPC_WRITE_FLOW(NPC_SPORT_SCTP, sport, ntohs(pkt->sport), 0,
+                      ntohs(mask->sport), 0);
+       NPC_WRITE_FLOW(NPC_DPORT_SCTP, dport, ntohs(pkt->dport), 0,
+                      ntohs(mask->dport), 0);
+
+       NPC_WRITE_FLOW(NPC_OUTER_VID, vlan_tci, ntohs(pkt->vlan_tci), 0,
+                      ntohs(mask->vlan_tci), 0);
+
+       npc_update_ipv6_flow(rvu, entry, features, pkt, mask, output, intf);
+}
+