Hi Alexander,
Thanks for the review.
> -----Original Message-----
> From: Alexander Duyck <alexander.du...@gmail.com>
> Sent: Friday, November 13, 2020 1:32 AM
> To: Naveen Mamindlapalli <nave...@marvell.com>
> Cc: Netdev <netdev@vger.kernel.org>; LKML <linux-ker...@vger.kernel.org>;
> Jakub Kicinski <k...@kernel.org>; David Miller <da...@davemloft.net>;
> sa...@kernel.org; Sunil Kovvuri Goutham <sgout...@marvell.com>; Linu
> Cherian <lcher...@marvell.com>; Geethasowjanya Akula
> <gak...@marvell.com>; Jerin Jacob Kollanukkaran <jer...@marvell.com>;
> Subbaraya Sundeep Bhatta <sbha...@marvell.com>; Hariprasad Kelam
> <hke...@marvell.com>
> Subject: Re: [PATCH v3 net-next 01/13] octeontx2-af: Modify default KEX
> profile to extract TX packet fields
>
> On Tue, Nov 10, 2020 at 11:22 PM Naveen Mamindlapalli
> <nave...@marvell.com> wrote:
> >
> > From: Stanislaw Kardach <skard...@marvell.com>
> >
> > The current default Key Extraction(KEX) profile can only use RX packet
> > fields while generating the MCAM search key. The profile can't be used
> > for matching TX packet fields. This patch modifies the default KEX
> > profile to add support for extracting TX packet fields into MCAM
> > search key. Enabled Tx KPU packet parsing by configuring TX PKIND in
> > tx_parse_cfg.
> >
> > Also modified the default KEX profile to extract VLAN TCI from the
> > LB_PTR and exact byte offset of VLAN header. The NPC KPU parser was
> > modified to point LB_PTR to the starting byte offset of VLAN header
> > which points to the tpid field.
> >
> > Signed-off-by: Stanislaw Kardach <skard...@marvell.com>
> > Signed-off-by: Sunil Goutham <sgout...@marvell.com>
> > Signed-off-by: Naveen Mamindlapalli <nave...@marvell.com>
>
> A bit more documentation would be useful. However other than that the code
> itself appears to make sense.
>
> Reviewed-by: Alexander Duyck <alexanderdu...@fb.com>
>
> > ---
> > .../ethernet/marvell/octeontx2/af/npc_profile.h | 71
> ++++++++++++++++++++--
> > .../net/ethernet/marvell/octeontx2/af/rvu_nix.c | 6 ++
> > 2 files changed, 72 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/npc_profile.h
> > b/drivers/net/ethernet/marvell/octeontx2/af/npc_profile.h
> > index 199448610e3e..c5b13385c81d 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/npc_profile.h
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/npc_profile.h
> > @@ -13386,8 +13386,8 @@ static struct npc_mcam_kex npc_mkex_default =
> {
> > .kpu_version = NPC_KPU_PROFILE_VER,
> > .keyx_cfg = {
> > /* nibble: LA..LE (ltype only) + Channel */
> > - [NIX_INTF_RX] = ((u64)NPC_MCAM_KEY_X2 << 32) | 0x49247,
> > - [NIX_INTF_TX] = ((u64)NPC_MCAM_KEY_X2 << 32) | ((1ULL <<
> > 19) -
> 1),
> > + [NIX_INTF_RX] = ((u64)NPC_MCAM_KEY_X2 << 32) | 0x249207,
> > + [NIX_INTF_TX] = ((u64)NPC_MCAM_KEY_X2 << 32) |
> > + 0x249200,
> > },
> > .intf_lid_lt_ld = {
> > /* Default RX MCAM KEX profile */
> //
> Any sort of explanation for what some of these magic numbers means might be
> useful. I'm left wondering if the lower 32b is a bitfield or a fixed value. I
> am
> guessing bit field based on the fact that it was originally being set using
> ((1ULL
> << X) - 1) however if there were macros defined for each bit explaining what
> each bit was that would be useful.
I will add the macros for each bit in v4.
>
> > @@ -13405,12 +13405,14 @@ static struct npc_mcam_kex npc_mkex_default
> = {
> > /* Layer B: Single VLAN (CTAG) */
> > /* CTAG VLAN[2..3] + Ethertype, 4 bytes, KW0[63:32]
> > */
> > [NPC_LT_LB_CTAG] = {
> > - KEX_LD_CFG(0x03, 0x0, 0x1, 0x0, 0x4),
> > + KEX_LD_CFG(0x03, 0x2, 0x1, 0x0, 0x4),
> > },
>
> Similarly here some explanation for KEX_LD_CFG would be useful. From what I
> can tell it seems like this may be some sort of fix as you are adjusting the
> "hdr_ofs" field from 0 to 2.
The fix description is added in the commit msg. I will try to clarify a bit
more in v4 commit description about the fix.
>
> > /* Layer B: Stacked VLAN (STAG|QinQ) */
> > [NPC_LT_LB_STAG_QINQ] = {
> > - /* CTAG VLAN[2..3] + Ethertype, 4 bytes,
> > KW0[63:32] */
> > - KEX_LD_CFG(0x03, 0x4, 0x1, 0x0, 0x4),
> > + /* Outer VLAN: 2 bytes, KW0[63:48] */
> > + KEX_LD_CFG(0x01, 0x2, 0x1, 0x0, 0x6),
> > + /* Ethertype: 2 bytes, KW0[47:32] */
> > + KEX_LD_CFG(0x01, 0x8, 0x1, 0x0, 0x4),
>
> Just to confirm, are you matching up the outer VLAN with the inner Ethertype
> here? It seems like an odd combination. I assume you need the inner ethertype
> in order to identify the L3 traffic?
In case of Q-in-Q, we are going to extract outer VLAN TCI 2 bytes and Ethertype
(Ex: 0x0800 in case of IPv4) after CTAG Header. We don't support inner VLAN TCI
match, so we don't extract.
>
> > },
> > [NPC_LT_LB_FDSA] = {
> > /* SWITCH PORT: 1 byte, KW0[63:48] */
> > @@ -13450,6 +13452,65 @@ static struct npc_mcam_kex npc_mkex_default
> = {
> > },
> > },
> > },
> > +
> > + /* Default TX MCAM KEX profile */
> > + [NIX_INTF_TX] = {
> > + [NPC_LID_LA] = {
> > + /* Layer A: Ethernet: */
> > + [NPC_LT_LA_IH_NIX_ETHER] = {
> > + /* PF_FUNC: 2B , KW0 [47:32] */
> > + KEX_LD_CFG(0x01, 0x0, 0x1, 0x0, 0x4),
>
> I'm assuming you have an 8B internal header that is being parsed? A comment
> explaining that this is parsing a preamble that is at the start of things
> might be
> useful.
Will add in v4.
>
> > + /* DMAC: 6 bytes, KW1[63:16] */
> > + KEX_LD_CFG(0x05, 0x8, 0x1, 0x0, 0xa),
> > + },
> > + },
> > + [NPC_LID_LB] = {
> > + /* Layer B: Single VLAN (CTAG) */
> > + [NPC_LT_LB_CTAG] = {
> > + /* CTAG VLAN[2..3] KW0[63:48] */
> > + KEX_LD_CFG(0x01, 0x2, 0x1, 0x0, 0x6),
> > + /* CTAG VLAN[2..3] KW1[15:0] */
> > + KEX_LD_CFG(0x01, 0x4, 0x1, 0x0, 0x8),
> > + },
> > + /* Layer B: Stacked VLAN (STAG|QinQ) */
> > + [NPC_LT_LB_STAG_QINQ] = {
> > + /* Outer VLAN: 2 bytes, KW0[63:48] */
> > + KEX_LD_CFG(0x01, 0x2, 0x1, 0x0, 0x6),
> > + /* Outer VLAN: 2 Bytes, KW1[15:0] */
> > + KEX_LD_CFG(0x01, 0x8, 0x1, 0x0, 0x8),
> > + },
> > + },
> > + [NPC_LID_LC] = {
> > + /* Layer C: IPv4 */
> > + [NPC_LT_LC_IP] = {
> > + /* SIP+DIP: 8 bytes, KW2[63:0] */
> > + KEX_LD_CFG(0x07, 0xc, 0x1, 0x0, 0x10),
> > + /* TOS: 1 byte, KW1[63:56] */
> > + KEX_LD_CFG(0x0, 0x1, 0x1, 0x0, 0xf),
> > + },
> > + /* Layer C: IPv6 */
> > + [NPC_LT_LC_IP6] = {
> > + /* Everything up to SADDR: 8 bytes,
> > KW2[63:0] */
> > + KEX_LD_CFG(0x07, 0x0, 0x1, 0x0, 0x10),
> > + },
> > + },
> > + [NPC_LID_LD] = {
> > + /* Layer D:UDP */
> > + [NPC_LT_LD_UDP] = {
> > + /* SPORT: 2 bytes, KW3[15:0] */
> > + KEX_LD_CFG(0x1, 0x0, 0x1, 0x0, 0x18),
> > + /* DPORT: 2 bytes, KW3[31:16] */
> > + KEX_LD_CFG(0x1, 0x2, 0x1, 0x0, 0x1a),
>
> I am curious why this is being done as two pieces instead of just one.
> From what I can tell you could just have a single rule that copies the
> 4 bytes for both ports in one shot and they would end up in the same place
> wouldn't they?
Yes correct, the will work. I will modify and push the changes in v4.
>
> > + },
> > + /* Layer D:TCP */
> > + [NPC_LT_LD_TCP] = {
> > + /* SPORT: 2 bytes, KW3[15:0] */
> > + KEX_LD_CFG(0x1, 0x0, 0x1, 0x0, 0x18),
> > + /* DPORT: 2 bytes, KW3[31:16] */
> > + KEX_LD_CFG(0x1, 0x2, 0x1, 0x0, 0x1a),
> > + },
> > + },
> > + },
> > },
> > };
> >
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> > b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> > index 8bac1dd3a1c2..8c11abdbd9d1 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> > @@ -57,6 +57,8 @@ enum nix_makr_fmt_indexes {
> > NIX_MARK_CFG_MAX,
> > };
> >
> > +#define NIX_TX_PKIND 63ULL
> > +
>
> A comment explaining the magic number would be useful. From what I can tell
> this looks like a "just turn everything on" sort of config where you are
> enabling
> bit flags 0 - 5.
Sure, will add in v4.
>
>
> > /* For now considering MC resources needed for broadcast
> > * pkt replication only. i.e 256 HWVFs + 12 PFs.
> > */
> > @@ -1182,6 +1184,10 @@ int rvu_mbox_handler_nix_lf_alloc(struct rvu *rvu,
> > /* Config Rx pkt length, csum checks and apad enable / disable */
> > rvu_write64(rvu, blkaddr, NIX_AF_LFX_RX_CFG(nixlf),
> > req->rx_cfg);
> >
> > + /* Configure pkind for TX parse config, 63 from npc_profile */
> > + cfg = NIX_TX_PKIND;
> > + rvu_write64(rvu, blkaddr, NIX_AF_LFX_TX_PARSE_CFG(nixlf),
> > + cfg);
> > +
> > intf = is_afvf(pcifunc) ? NIX_INTF_TYPE_LBK : NIX_INTF_TYPE_CGX;
> > err = nix_interface_init(rvu, pcifunc, intf, nixlf);
> > if (err)
> > --
> > 2.16.5
> >
