| 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: <20240617201937.GB8447@kernel.org>
Date: Mon, 17 Jun 2024 21:19:37 +0100
From: Simon Horman <horms@...nel.org>
To: Geetha sowjanya <gakula@...vell.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, kuba@...nel.org,
davem@...emloft.net, pabeni@...hat.com, edumazet@...gle.com,
sgoutham@...vell.com, sbhatta@...vell.com, hkelam@...vell.com
Subject: Re: [net-next PATCH v5 05/10] octeontx2-af: Add packet path between
representor and VF
On Tue, Jun 11, 2024 at 09:52:08PM +0530, Geetha sowjanya wrote:
> Current HW, do not support in-built switch which will forward pkts
> between representee and representor. When representor is put under
> a bridge and pkts needs to be sent to representee, then pkts from
> representor are sent on a HW internal loopback channel, which again
> will be punted to ingress pkt parser. Now the rules that this patch
> installs are the MCAM filters/rules which will match against these
> pkts and forward them to representee.
> The rules that this patch installs are for basic
> representor <=> representee path similar to Tun/TAP between VM and
> Host.
>
> Signed-off-by: Geetha sowjanya <gakula@...vell.com>
Hi Geetha,
Some minor feedback from my side.
...
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
> index cf13c5f0a3c5..e137bb9383a2 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
> @@ -13,6 +13,252 @@
> #include "rvu.h"
> #include "rvu_reg.h"
>
> +static int rvu_rep_get_vlan_id(struct rvu *rvu, u16 pcifunc)
> +{
> + int id;
> +
> + for (id = 0; id < rvu->rep_cnt; id++)
> + if (rvu->rep2pfvf_map[id] == pcifunc)
> + return id;
> + return -ENODEV;
> +}
rvu_rep_get_vlan_id() can return an error,
but it is not checked by callers. Should it be?
If not, perhaps rvu_rep_get_vlan_id can return a u16?
> +
> +static int rvu_rep_tx_vlan_cfg(struct rvu *rvu, u16 pcifunc,
> + u16 vlan_tci, int *vidx)
> +{
> + struct nix_vtag_config_rsp rsp = {};
> + struct nix_vtag_config req = {};
> + u64 etype = ETH_P_8021Q;
> + int err;
> +
> + /* Insert vlan tag */
> + req.hdr.pcifunc = pcifunc;
> + req.vtag_size = VTAGSIZE_T4;
> + req.cfg_type = 0; /* tx vlan cfg */
> + req.tx.cfg_vtag0 = true;
> + req.tx.vtag0 = etype << 48 | ntohs(vlan_tci);
This does not seem correct. vlan_tci is host byte-order,
but ntohs expects a big-endian value as it's argument.
Flagged by Sparse.
> +
> + err = rvu_mbox_handler_nix_vtag_cfg(rvu, &req, &rsp);
> + if (err) {
> + dev_err(rvu->dev, "Tx vlan config failed\n");
> + return err;
> + }
> + *vidx = rsp.vtag0_idx;
> + return 0;
> +}
...
> +static int rvu_rep_install_rx_rule(struct rvu *rvu, u16 pcifunc,
> + u16 entry, bool rte)
> +{
> + struct npc_install_flow_req req = {};
> + struct npc_install_flow_rsp rsp = {};
> + struct rvu_pfvf *pfvf;
> + u16 vlan_tci, rep_id;
> +
> + pfvf = rvu_get_pfvf(rvu, pcifunc);
> +
> + /* To stree the traffic from Representee to Representor */
nit: steer
> + rep_id = (u16)rvu_rep_get_vlan_id(rvu, pcifunc);
This cast seems unnecessary, or at least inconsistent
with the other call to rvu_rep_get_vlan_id.
> + if (rte) {
> + vlan_tci = rep_id | 0x1ull << 8;
ull seems a bit excessive as these are otherwise 16bit values.
And in any case, perhaps BIT(8) can be used here.
> + req.vf = rvu->rep_pcifunc;
> + req.op = NIX_RX_ACTIONOP_UCAST;
> + req.index = rep_id;
> + } else {
> + vlan_tci = rep_id;
> + req.vf = pcifunc;
> + req.op = NIX_RX_ACTION_DEFAULT;
> + }
> +
> + rvu_rep_rx_vlan_cfg(rvu, req.vf);
> + req.entry = entry;
> + req.hdr.pcifunc = 0; /* AF is requester */
> + req.features = BIT_ULL(NPC_OUTER_VID) | BIT_ULL(NPC_VLAN_ETYPE_CTAG);
> + req.vtag0_valid = true;
> + req.vtag0_type = NIX_AF_LFX_RX_VTAG_TYPE0;
> + req.packet.vlan_etype = (__be16)ETH_P_8021Q;
> + req.mask.vlan_etype = (__be16)ETH_P_8021Q;
> + req.packet.vlan_tci = (__be16)vlan_tci;
> + req.mask.vlan_tci = (__be16)0xffff;
0xffff is isomorphic, so this point isn't particularly relevant,
but the 3 casts above that don't look right: these are host-byte
order values, they shouldn't be cast as big-endian..
Also flagged by Sparse.
> +
> + req.channel = RVU_SWITCH_LBK_CHAN;
> + req.chan_mask = 0xffff;
> + req.intf = pfvf->nix_rx_intf;
> +
> + return rvu_mbox_handler_npc_install_flow(rvu, &req, &rsp);
> +}
> +
> +static int rvu_rep_install_tx_rule(struct rvu *rvu, u16 pcifunc, u16 entry,
> + bool rte)
> +{
> + struct npc_install_flow_req req = {};
> + struct npc_install_flow_rsp rsp = {};
> + struct rvu_pfvf *pfvf;
> + int vidx, err;
> + u16 vlan_tci;
> + u8 lbkid;
> +
> + pfvf = rvu_get_pfvf(rvu, pcifunc);
> + vlan_tci = rvu_rep_get_vlan_id(rvu, pcifunc);
> + if (rte)
> + vlan_tci |= 0x1ull << 8;
BIT(8) seems appropriate here too.
> +
> + err = rvu_rep_tx_vlan_cfg(rvu, pcifunc, vlan_tci, &vidx);
> + if (err)
> + return err;
> +
> + lbkid = pfvf->nix_blkaddr == BLKADDR_NIX0 ? 0 : 1;
> + req.hdr.pcifunc = 0; /* AF is requester */
> + if (rte) {
> + req.vf = pcifunc;
> + } else {
> + req.vf = rvu->rep_pcifunc;
> + req.packet.sq_id = vlan_tci;
> + req.mask.sq_id = 0xffff;
> + }
> +
> + req.entry = entry;
> + req.intf = pfvf->nix_tx_intf;
> + req.op = NIX_TX_ACTIONOP_UCAST_CHAN;
> + req.index = (lbkid << 8) | RVU_SWITCH_LBK_CHAN;
> + req.set_cntr = 1;
> + req.vtag0_def = vidx;
> + req.vtag0_op = 1;
> + return rvu_mbox_handler_npc_install_flow(rvu, &req, &rsp);
> +}
...
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> index 3cb8dc820fdd..e276a354d9e4 100644
...
> @@ -230,7 +248,7 @@ int rvu_rep_create(struct otx2_nic *priv, struct netlink_ext_ack *extack)
> return err;
> }
>
> -static int rvu_rep_rsrc_free(struct otx2_nic *priv)
> +static void rvu_rep_rsrc_free(struct otx2_nic *priv)
> {
> struct otx2_qset *qset = &priv->qset;
> int wrk;
> @@ -241,13 +259,12 @@ static int rvu_rep_rsrc_free(struct otx2_nic *priv)
>
> otx2_free_hw_resources(priv);
> otx2_free_queue_mem(qset);
> - return 0;
> }
>
> static int rvu_rep_rsrc_init(struct otx2_nic *priv)
> {
> struct otx2_qset *qset = &priv->qset;
> - int err = 0;
> + int err;
>
> err = otx2_alloc_queue_mem(priv);
> if (err)
The last two hunks don't seem strictly related to the rest of this patch.
Perhaps they belong squashed into earlier patches of this series?
Powered by blists - more mailing lists