Re: [PATCH v3 net-next 02/13] octeontx2-af: Verify MCAM entry channel and PF_FUNC

[Naveen Mamindlapalli]

Hi Alexander,

> -----Original Message-----
> From: Alexander Duyck <alexander.duyck@xxxxxxxxx>
> Sent: Friday, November 13, 2020 1:33 AM
> To: Naveen Mamindlapalli <naveenm@xxxxxxxxxxx>
> Cc: Netdev <netdev@xxxxxxxxxxxxxxx>; LKML <linux-kernel@xxxxxxxxxxxxxxx>;
> Jakub Kicinski <kuba@xxxxxxxxxx>; David Miller <davem@xxxxxxxxxxxxx>;
> saeed@xxxxxxxxxx; Sunil Kovvuri Goutham <sgoutham@xxxxxxxxxxx>; Linu
> Cherian <lcherian@xxxxxxxxxxx>; Geethasowjanya Akula
> <gakula@xxxxxxxxxxx>; Jerin Jacob Kollanukkaran <jerinj@xxxxxxxxxxx>;
> Subbaraya Sundeep Bhatta <sbhatta@xxxxxxxxxxx>; Hariprasad Kelam
> <hkelam@xxxxxxxxxxx>
> Subject: Re: [PATCH v3 net-next 02/13] octeontx2-af: Verify MCAM entry
> channel and PF_FUNC
> 
> On Tue, Nov 10, 2020 at 11:18 PM Naveen Mamindlapalli
> <naveenm@xxxxxxxxxxx> wrote:
> >
> > From: Subbaraya Sundeep <sbhatta@xxxxxxxxxxx>
> >
> > This patch adds support to verify the channel number sent by mailbox
> > requester before writing MCAM entry for Ingress packets.
> > Similarly for Egress packets, verifying the PF_FUNC sent by the
> > mailbox user.
> >
> > Signed-off-by: Subbaraya Sundeep <sbhatta@xxxxxxxxxxx>
> > Signed-off-by: Kiran Kumar K <kirankumark@xxxxxxxxxxx>
> > Signed-off-by: Sunil Goutham <sgoutham@xxxxxxxxxxx>
> > Signed-off-by: Naveen Mamindlapalli <naveenm@xxxxxxxxxxx>
> 
> One minor nit below. Otherwise looks good to me.
> 
> Reviewed-by: Alexander Duyck <alexanderduyck@xxxxxx>
> 
> > ---
> >  drivers/net/ethernet/marvell/octeontx2/af/rvu.c    |  4 +-
> >  drivers/net/ethernet/marvell/octeontx2/af/rvu.h    |  2 +
> >  .../net/ethernet/marvell/octeontx2/af/rvu_npc.c    | 78
> ++++++++++++++++++++++
> >  3 files changed, 82 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> > b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> > index a28a518c0eae..e8b5aaf73201 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> > @@ -2642,7 +2642,7 @@ static void rvu_enable_afvf_intr(struct rvu
> > *rvu)
> >
> >  #define PCI_DEVID_OCTEONTX2_LBK 0xA061
> >
> > -static int lbk_get_num_chans(void)
> > +int rvu_get_num_lbk_chans(void)
> >  {
> >         struct pci_dev *pdev;
> >         void __iomem *base;
> > @@ -2677,7 +2677,7 @@ static int rvu_enable_sriov(struct rvu *rvu)
> >                 return 0;
> >         }
> >
> > -       chans = lbk_get_num_chans();
> > +       chans = rvu_get_num_lbk_chans();
> >         if (chans < 0)
> >                 return chans;
> >
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> > b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> > index 5ac9bb12415f..1724dbd18847 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> > @@ -445,6 +445,7 @@ int rvu_get_lf(struct rvu *rvu, struct rvu_block
> > *block, u16 pcifunc, u16 slot);  int rvu_lf_reset(struct rvu *rvu,
> > struct rvu_block *block, int lf);  int rvu_get_blkaddr(struct rvu
> > *rvu, int blktype, u16 pcifunc);  int rvu_poll_reg(struct rvu *rvu,
> > u64 block, u64 offset, u64 mask, bool zero);
> > +int rvu_get_num_lbk_chans(void);
> >
> >  /* RVU HW reg validation */
> >  enum regmap_block {
> > @@ -535,6 +536,7 @@ bool is_npc_intf_tx(u8 intf);  bool
> > is_npc_intf_rx(u8 intf);  bool is_npc_interface_valid(struct rvu *rvu,
> > u8 intf);  int rvu_npc_get_tx_nibble_cfg(struct rvu *rvu, u64
> > nibble_ena);
> > +int npc_mcam_verify_channel(struct rvu *rvu, u16 pcifunc, u8 intf,
> > +u16 channel);
> >
> >  #ifdef CONFIG_DEBUG_FS
> >  void rvu_dbg_init(struct rvu *rvu);
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> > b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> > index 989533a3d2ce..3666159bb6b6 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> > @@ -28,6 +28,8 @@
> >
> >  #define NPC_PARSE_RESULT_DMAC_OFFSET   8
> >  #define NPC_HW_TSTAMP_OFFSET           8
> > +#define NPC_KEX_CHAN_MASK              0xFFFULL
> > +#define NPC_KEX_PF_FUNC_MASK           0xFFFFULL
> >
> >  static const char def_pfl_name[] = "default";
> >
> > @@ -63,6 +65,54 @@ int rvu_npc_get_tx_nibble_cfg(struct rvu *rvu, u64
> nibble_ena)
> >         return 0;
> >  }
> >
> > +static int npc_mcam_verify_pf_func(struct rvu *rvu,
> > +                                  struct mcam_entry *entry_data, u8 intf,
> > +                                  u16 pcifunc) {
> > +       u16 pf_func, pf_func_mask;
> > +
> > +       if (is_npc_intf_rx(intf))
> > +               return 0;
> > +
> > +       pf_func_mask = (entry_data->kw_mask[0] >> 32) &
> > +               NPC_KEX_PF_FUNC_MASK;
> > +       pf_func = (entry_data->kw[0] >> 32) & NPC_KEX_PF_FUNC_MASK;
> > +
> > +       pf_func = be16_to_cpu((__force __be16)pf_func);
> > +       if (pf_func_mask != NPC_KEX_PF_FUNC_MASK ||
> > +           ((pf_func & ~RVU_PFVF_FUNC_MASK) !=
> > +            (pcifunc & ~RVU_PFVF_FUNC_MASK)))
> > +               return -EINVAL;
> > +
> > +       return 0;
> > +}
> > +
> > +int npc_mcam_verify_channel(struct rvu *rvu, u16 pcifunc, u8 intf,
> > +u16 channel) {
> > +       int pf = rvu_get_pf(pcifunc);
> > +       u8 cgx_id, lmac_id;
> > +       int base = 0, end;
> > +
> > +       if (is_npc_intf_tx(intf))
> > +               return 0;
> > +
> > +       if (is_afvf(pcifunc)) {
> > +               end = rvu_get_num_lbk_chans();
> > +               if (end < 0)
> > +                       return -EINVAL;
> > +       } else {
> > +               rvu_get_cgx_lmac_id(rvu->pf2cgxlmac_map[pf], &cgx_id,
> &lmac_id);
> > +               base = NIX_CHAN_CGX_LMAC_CHX(cgx_id, lmac_id, 0x0);
> > +               /* CGX mapped functions has maximum of 16 channels */
> > +               end = NIX_CHAN_CGX_LMAC_CHX(cgx_id, lmac_id, 0xF);
> > +       }
> > +
> > +       if (channel < base || channel > end)
> > +               return -EINVAL;
> > +
> > +       return 0;
> > +}
> > +
> >  void rvu_npc_set_pkind(struct rvu *rvu, int pkind, struct rvu_pfvf
> > *pfvf)  {
> >         int blkaddr;
> > @@ -1935,6 +1985,7 @@ int
> rvu_mbox_handler_npc_mcam_write_entry(struct rvu *rvu,
> >         struct rvu_pfvf *pfvf = rvu_get_pfvf(rvu, req->hdr.pcifunc);
> >         struct npc_mcam *mcam = &rvu->hw->mcam;
> >         u16 pcifunc = req->hdr.pcifunc;
> > +       u16 channel, chan_mask;
> >         int blkaddr, rc;
> >         u8 nix_intf;
> >
> > @@ -1942,6 +1993,10 @@ int
> rvu_mbox_handler_npc_mcam_write_entry(struct rvu *rvu,
> >         if (blkaddr < 0)
> >                 return NPC_MCAM_INVALID_REQ;
> >
> > +       chan_mask = req->entry_data.kw_mask[0] & NPC_KEX_CHAN_MASK;
> > +       channel = req->entry_data.kw[0] & NPC_KEX_CHAN_MASK;
> > +       channel &= chan_mask;
> > +
> >         mutex_lock(&mcam->lock);
> >         rc = npc_mcam_verify_entry(mcam, pcifunc, req->entry);
> >         if (rc)
> > @@ -1963,6 +2018,17 @@ int
> rvu_mbox_handler_npc_mcam_write_entry(struct rvu *rvu,
> >         else
> >                 nix_intf = pfvf->nix_rx_intf;
> >
> > +       if (npc_mcam_verify_channel(rvu, pcifunc, req->intf, channel)) {
> > +               rc = NPC_MCAM_INVALID_REQ;
> > +               goto exit;
> > +       }
> > +
> > +       if (npc_mcam_verify_pf_func(rvu, &req->entry_data, req->intf,
> > +                                   pcifunc)) {
> > +               rc = NPC_MCAM_INVALID_REQ;
> > +               goto exit;
> > +       }
> > +
> >         npc_config_mcam_entry(rvu, mcam, blkaddr, req->entry, nix_intf,
> >                               &req->entry_data, req->enable_entry);
> >
> > @@ -2299,6 +2365,7 @@ int
> rvu_mbox_handler_npc_mcam_alloc_and_write_entry(struct rvu *rvu,
> >         struct npc_mcam *mcam = &rvu->hw->mcam;
> >         u16 entry = NPC_MCAM_ENTRY_INVALID;
> >         u16 cntr = NPC_MCAM_ENTRY_INVALID;
> > +       u16 channel, chan_mask;
> >         int blkaddr, rc;
> >         u8 nix_intf;
> >
> > @@ -2309,6 +2376,17 @@ int
> rvu_mbox_handler_npc_mcam_alloc_and_write_entry(struct rvu *rvu,
> >         if (!is_npc_interface_valid(rvu, req->intf))
> >                 return NPC_MCAM_INVALID_REQ;
> >
> > +       chan_mask = req->entry_data.kw_mask[0] & NPC_KEX_CHAN_MASK;
> > +       channel = req->entry_data.kw[0] & NPC_KEX_CHAN_MASK;
> > +       channel &= chan_mask;
> > +
> > +       if (npc_mcam_verify_channel(rvu, req->hdr.pcifunc, req->intf, channel))
> > +               return NPC_MCAM_INVALID_REQ;
> > +
> 
> Why not just move the code for pulling the channel into the
> npc_mcam_verify_channel function like you did with the pf_func? Then you can
> avoid declaring variables here that won't be used anywhere else in the function.

The npc_mcam_verify_channel is called by other functions that can't pass the raw MCAM entry data. See patch 04.

> 
> 
> > +       if (npc_mcam_verify_pf_func(rvu, &req->entry_data, req->intf,
> > +                                   req->hdr.pcifunc))
> > +               return NPC_MCAM_INVALID_REQ;
> > +
> >         /* Try to allocate a MCAM entry */
> >         entry_req.hdr.pcifunc = req->hdr.pcifunc;
> >         entry_req.contig = true;
> > --
> > 2.16.5
> >