Re: [dpdk-dev] [PATCH] net/bnxt: fix missing barriers in completion handling

[Ruifeng Wang <Ruifeng.Wang@arm.com>]

> -----Original Message-----
> From: Lance Richardson <lance.richardson@broadcom.com>
> Sent: Friday, July 9, 2021 3:15 AM
> To: Ajit Khaparde (ajit.khaparde@broadcom.com)
> <ajit.khaparde@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; Bruce Richardson
> <bruce.richardson@intel.com>; Konstantin Ananyev
> <konstantin.ananyev@intel.com>; jerinj@marvell.com; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Stephen Hurd <stephen.hurd@broadcom.com>;
> David Christensen <david.christensen@broadcom.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: [PATCH] net/bnxt: fix missing barriers in completion handling
> 
> Ensure that Rx/Tx/Async completion entry fields are accessed
> only after the completion's valid flag has been loaded and
> verified. This is needed for correct operation on systems that
> use relaxed memory consistency models.
> 
> Fixes: 2eb53b134aae ("net/bnxt: add initial Rx code")
> Fixes: 6eb3cc2294fd ("net/bnxt: add initial Tx code")
> Cc: stable@dpdk.org
> Signed-off-by: Lance Richardson <lance.richardson@broadcom.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
>  drivers/net/bnxt/bnxt_cpr.h           | 36 ++++++++++++++++++++++++---
>  drivers/net/bnxt/bnxt_ethdev.c        | 16 ++++++------
>  drivers/net/bnxt/bnxt_irq.c           |  7 +++---
>  drivers/net/bnxt/bnxt_rxr.c           |  9 ++++---
>  drivers/net/bnxt/bnxt_rxtx_vec_avx2.c |  2 +-
>  drivers/net/bnxt/bnxt_rxtx_vec_neon.c |  2 +-
>  drivers/net/bnxt/bnxt_rxtx_vec_sse.c  |  2 +-
>  drivers/net/bnxt/bnxt_txr.c           |  2 +-
>  8 files changed, 54 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/bnxt/bnxt_cpr.h b/drivers/net/bnxt/bnxt_cpr.h
> index 2a56ec52c..3ee6b74bc 100644
> --- a/drivers/net/bnxt/bnxt_cpr.h
> +++ b/drivers/net/bnxt/bnxt_cpr.h
> @@ -8,13 +8,10 @@
>  #include <stdbool.h>
> 
>  #include <rte_io.h>
> +#include "hsi_struct_def_dpdk.h"
> 
>  struct bnxt_db_info;
> 
> -#define CMP_VALID(cmp, raw_cons, ring)
> 	\
> -	(!!(rte_le_to_cpu_32(((struct cmpl_base *)(cmp))->info3_v) &
> 	\
> -	    CMPL_BASE_V) == !((raw_cons) & ((ring)->ring_size)))
> -
>  #define CMP_TYPE(cmp)						\
>  	(((struct cmpl_base *)cmp)->type & CMPL_BASE_TYPE_MASK)
> 
> @@ -121,4 +118,35 @@ bool bnxt_is_recovery_enabled(struct bnxt *bp);
>  bool bnxt_is_master_func(struct bnxt *bp);
> 
>  void bnxt_stop_rxtx(struct bnxt *bp);
> +
> +/**
> + * Check validity of a completion ring entry. If the entry is valid, include a
> + * C11 __ATOMIC_ACQUIRE fence to ensure that subsequent loads of fields
> in the
> + * completion are not hoisted by the compiler or by the CPU to come before
> the
> + * loading of the "valid" field.
> + *
> + * Note: the caller must not access any fields in the specified completion
> + * entry prior to calling this function.
> + *
> + * @param cmp
Nit, cmpl

> + *   Pointer to an entry in the completion ring.
> + * @param raw_cons
> + *   Raw consumer index of entry in completion ring.
> + * @param ring_size
> + *   Size of completion ring.
> + */
> +static __rte_always_inline bool
> +bnxt_cpr_cmp_valid(const void *cmpl, uint32_t raw_cons, uint32_t
> ring_size)
> +{
> +	const struct cmpl_base *c = cmpl;
> +	bool expected, valid;
> +
> +	expected = !(raw_cons & ring_size);
> +	valid = !!(rte_le_to_cpu_32(c->info3_v) & CMPL_BASE_V);
> +	if (valid == expected) {
> +		rte_atomic_thread_fence(__ATOMIC_ACQUIRE);
> +		return true;
> +	}
> +	return false;
> +}
>  #endif
> diff --git a/drivers/net/bnxt/bnxt_ethdev.c
> b/drivers/net/bnxt/bnxt_ethdev.c
> index ed09f1bf5..ee6929692 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c

<snip>

> 
>  	/* Check to see if hw has posted a completion for the descriptor. */
> @@ -3327,7 +3327,7 @@ bnxt_tx_descriptor_status_op(void *tx_queue,
> uint16_t offset)
>  		cons = RING_CMPL(ring_mask, raw_cons);
>  		txcmp = (struct tx_cmpl *)&cp_desc_ring[cons];
> 
> -		if (!CMP_VALID(txcmp, raw_cons, cp_ring_struct))
> +		if (!bnxt_cpr_cmp_valid(txcmp, raw_cons, ring_mask + 1))
cpr->cp_ring_struct->ring_size can be used instead of 'ring_mask + 1'?

>  			break;
> 
>  		if (CMP_TYPE(txcmp) == TX_CMPL_TYPE_TX_L2)

<snip>

> diff --git a/drivers/net/bnxt/bnxt_rxtx_vec_neon.c
> b/drivers/net/bnxt/bnxt_rxtx_vec_neon.c
> index 263e6ec3c..13211060c 100644
> --- a/drivers/net/bnxt/bnxt_rxtx_vec_neon.c
> +++ b/drivers/net/bnxt/bnxt_rxtx_vec_neon.c
> @@ -339,7 +339,7 @@ bnxt_handle_tx_cp_vec(struct bnxt_tx_queue *txq)
>  		cons = RING_CMPL(ring_mask, raw_cons);
>  		txcmp = (struct tx_cmpl *)&cp_desc_ring[cons];
> 
> -		if (!CMP_VALID(txcmp, raw_cons, cp_ring_struct))
> +		if (!bnxt_cpr_cmp_valid(txcmp, raw_cons, ring_mask + 1))
Same here. I think cpr->cp_ring_struct->ring_size can be used and it avoids calculation.
Also some places in other vector files.

>  			break;
> 
>  		if (likely(CMP_TYPE(txcmp) == TX_CMPL_TYPE_TX_L2))
> diff --git a/drivers/net/bnxt/bnxt_rxtx_vec_sse.c
> b/drivers/net/bnxt/bnxt_rxtx_vec_sse.c
> index 9a53d1fba..6e5630532 100644
> --- a/drivers/net/bnxt/bnxt_rxtx_vec_sse.c
> +++ b/drivers/net/bnxt/bnxt_rxtx_vec_sse.c
> @@ -320,7 +320,7 @@ bnxt_handle_tx_cp_vec(struct bnxt_tx_queue *txq)
>  		cons = RING_CMPL(ring_mask, raw_cons);
>  		txcmp = (struct tx_cmpl *)&cp_desc_ring[cons];
> 
> -		if (!CMP_VALID(txcmp, raw_cons, cp_ring_struct))
> +		if (!bnxt_cpr_cmp_valid(txcmp, raw_cons, ring_mask + 1))
>  			break;
> 
>  		if (likely(CMP_TYPE(txcmp) == TX_CMPL_TYPE_TX_L2))
> diff --git a/drivers/net/bnxt/bnxt_txr.c b/drivers/net/bnxt/bnxt_txr.c
> index 9a6b96e04..47824334a 100644
> --- a/drivers/net/bnxt/bnxt_txr.c
> +++ b/drivers/net/bnxt/bnxt_txr.c
> @@ -461,7 +461,7 @@ static int bnxt_handle_tx_cp(struct bnxt_tx_queue
> *txq)
>  		cons = RING_CMPL(ring_mask, raw_cons);
>  		txcmp = (struct tx_cmpl *)&cpr->cp_desc_ring[cons];
> 
> -		if (!CMP_VALID(txcmp, raw_cons, cp_ring_struct))
> +		if (!bnxt_cpr_cmp_valid(txcmp, raw_cons, ring_mask + 1))
>  			break;
> 
>  		opaque = rte_le_to_cpu_32(txcmp->opaque);
> --
> 2.25.1