Re: [PATCH v2 net] octeontx2-af: Block VFs from clobbering special CGX PKIND state

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
octeontx2-af: Block VFs from clobbering special CGX PKIND state

This patch prevents VF NIX LF allocation from resetting the CGX RX PKIND
and default TX parse config when the shared LMAC has HiGig2 or EDSA
parsing enabled. It adds cgx_get_pkind() and a new
rvu_cgx_is_pkind_config_permitted() helper, then gates the pkind and TX
parse writes in nix_interface_init() and rvu_mbox_handler_nix_lf_alloc().

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c
> index 4ff3935ed3fe..2be1da3476ac 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c
> @@ -1355,3 +1355,35 @@ void rvu_mac_reset(struct rvu *rvu, u16 pcifunc)
>  	if (mac_ops->mac_reset(cgxd, lmac, !is_vf(pcifunc)))
>  		dev_err(rvu->dev, "Failed to reset MAC\n");
>  }
> +
> +/* Do not allow CGX-mapped VFs to overwrite PKIND when special parse kinds
> + * (HiGig, EDSA, etc.) are in use on the shared LMAC.
> + */
> +bool rvu_cgx_is_pkind_config_permitted(struct rvu *rvu, u16 pcifunc)
> +{
> +	int pf, err, rxpkind;
> +	u8 cgx_id, lmac_id;
> +	void *cgxd;
> +
> +	pf = rvu_get_pf(rvu->pdev, pcifunc);
> +
> +	if (!(pcifunc & RVU_PFVF_FUNC_MASK))
> +		return true;
> +
> +	if (!is_pf_cgxmapped(rvu, pf))
> +		return true;
> +
> +	rvu_get_cgx_lmac_id(rvu->pf2cgxlmac_map[pf], &cgx_id, &lmac_id);
> +	cgxd = rvu_cgx_pdata(cgx_id, rvu);
> +	err = cgx_get_pkind(cgxd, lmac_id, &rxpkind);
> +	if (err)
> +		return false;
> +
> +	switch (rxpkind) {
> +	case NPC_RX_HIGIG_PKIND:
> +	case NPC_RX_EDSA_PKIND:
> +		return false;
> +	default:
> +		return true;
> +	}
> +}

[High]
Is the exclusion actually enforced against the other writers of these
registers?

rvu_npc_set_parse_mode() also calls cgx_set_pkind() and writes
NIX_AF_LFX_TX_PARSE_CFG(nixlf) directly, and it is reachable from the
rvu_mbox_handler_npc_set_pkind mbox handler as well as from
rvu_nix_lf_teardown(). None of those paths take cgxd->lock.

Consider this interleaving on a shared LMAC:

    CPU A (PF)                       CPU B (VF)
    rvu_mbox_handler_npc_set_pkind
      rvu_npc_set_parse_mode
        (no cgxd->lock held)
                                     mutex_lock(&cgxd->lock)
                                     cgx_get_pkind() -> default
                                     rvu_cgx_is_pkind_config_permitted()
                                       returns true
                                     cgx_set_pkind(default)
                                     rvu_npc_set_pkind(default)
                                     mutex_unlock(&cgxd->lock)
        cgx_set_pkind(HIGIG)
        write NIX_AF_LFX_TX_PARSE_CFG

Or the reverse order, where CPU B's write clobbers CPU A's HiGig
programming after CPU A has already installed it. Since
rvu_npc_set_parse_mode() does not acquire cgxd->lock, does the check in
rvu_cgx_is_pkind_config_permitted() effectively close the race the
commit message describes?

Additionally, the comment on struct cgx's lock in lmac_common.h reads:

    /* Lock to serialize read/write of global csrs like
     * RPMX_MTI_STAT_DATA_HI_CDC etc
     */
    struct mutex		lock;

Is cgxd->lock the intended lock to serialize per-NIX-LF register writes
like NIX_AF_LFX_TX_PARSE_CFG, or should a different mechanism be used?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> index 0297c7ab0614..4e72d6e072d5 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c

[ ... ]

> @@ -1509,11 +1517,14 @@ int rvu_mbox_handler_nix_lf_alloc(struct rvu *rvu,
>  	u16 bcast, mcast, promisc, ucast;
>  	struct rvu_hwinfo *hw = rvu->hw;
>  	u16 pcifunc = req->hdr.pcifunc;
> +	u8 cgx_id = 0, lmac_id = 0;
>  	bool rules_created = false;
>  	struct rvu_block *block;
>  	struct rvu_pfvf *pfvf;
>  	u64 cfg, ctx_cfg;
> +	struct cgx *cgxd;
>  	int blkaddr;
> +	int pf;
>  
>  	if (!req->rq_cnt || !req->sq_cnt || !req->cq_cnt)
>  		return NIX_AF_ERR_PARAM;
> @@ -1685,8 +1696,18 @@ int rvu_mbox_handler_nix_lf_alloc(struct rvu *rvu,
>  	rvu_write64(rvu, blkaddr, NIX_AF_LFX_RX_CFG(nixlf), req->rx_cfg);
>  
>  	/* Configure pkind for TX parse config */
> -	cfg = NPC_TX_DEF_PKIND;
> -	rvu_write64(rvu, blkaddr, NIX_AF_LFX_TX_PARSE_CFG(nixlf), cfg);
> +	if (is_pf_cgxmapped(rvu, rvu_get_pf(rvu->pdev, pcifunc))) {
> +		pf = rvu_get_pf(rvu->pdev, pcifunc);
> +		rvu_get_cgx_lmac_id(rvu->pf2cgxlmac_map[pf], &cgx_id, &lmac_id);
> +		cgxd = rvu_cgx_pdata(cgx_id, rvu);
> +
> +		mutex_lock(&cgxd->lock);
> +		if (rvu_cgx_is_pkind_config_permitted(rvu, pcifunc)) {
> +			cfg = NPC_TX_DEF_PKIND;
> +			rvu_write64(rvu, blkaddr, NIX_AF_LFX_TX_PARSE_CFG(nixlf), cfg);
> +		}
> +		mutex_unlock(&cgxd->lock);
> +	}

[High]
Does this outer is_pf_cgxmapped() guard change behavior for LBK, SDP and
representor NIX LFs?

Prior to this patch the write of NPC_TX_DEF_PKIND to
NIX_AF_LFX_TX_PARSE_CFG(nixlf) was unconditional for every NIX LF that
reached this point. is_pf_cgxmapped() returns false when
pf < PF_CGXMAP_BASE (e.g. AF PF0 that owns LBK VFs) and for SDP PFs (via
the is_sdp_pf() exclusion):

    static inline bool is_pf_cgxmapped(struct rvu *rvu, u8 pf)
    {
        return (pf >= PF_CGXMAP_BASE && pf <= rvu->cgx_mapped_pfs) &&
            !is_sdp_pf(rvu, rvu_make_pcifunc(rvu->pdev, pf, 0));
    }

For those non-CGX cases the register is now left at whatever
rvu_lf_reset() leaves behind rather than NPC_TX_DEF_PKIND.

The commit message describes the intent narrowly:

    a VF NIX LF alloc must not reset the LMAC RX PKIND or default TX
    parse config over the PF setup.

rvu_cgx_is_pkind_config_permitted() already returns true for non-VF and
for non-CGX-mapped pcifuncs. Would gating solely on
rvu_cgx_is_pkind_config_permitted() (without the outer
is_pf_cgxmapped()) match the stated intent and preserve prior behavior
for LBK, SDP and representor NIX LFs?