Re: [PATCH v3] net/sfc: stop misuse of Rx ingress m-port metadata on EF100

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 3/12/2023 10:54 AM, Ivan Malov wrote:
> The driver supports representor functionality. In it,
> packets coming from VFs to the dedicated back-end Rx
> queue get demultiplexed into front-end Rx queues of
> representor ethdevs as per the per-packet metadata
> indicating logical HW ingress ports. On transmit,
> packets are provided with symmetrical metadata
> by front-end Tx queues, and the back-end queue
> transforms the data into so-called Tx override
> descriptors. These let the packets bypass flow
> lookup and go directly to the represented VFs.
> 
> However, in the Rx part, the driver extracts
> the said metadata on every HW Rx queue, that
> is, not just on the one used by representors.
> Doing so leads to a buggy behaviour. It is
> revealed by operating testpmd as follows:
> 
> dpdk-testpmd -a 0000:c6:00.0 -a 0000:c6:00.1 -- -i
> 
> testpmd> flow create 0 transfer pattern port_representor \
>  port_id is 0 / end actions port_representor port_id 1 / end
> Flow rule #0 created
> 
> testpmd> set fwd io
> testpmd> start tx_first
> 
> testpmd> flow destroy 0 rule 0
> Flow rule #0 destroyed
> 
> testpmd> stop
> 
>   ---------------------- Forward statistics for port 0  -----------------
>   RX-packets: 19196498       RX-dropped: 0             RX-total: 19196498
>   TX-packets: 19196535       TX-dropped: 0             TX-total: 19196535
>   -----------------------------------------------------------------------
> 
>   ---------------------- Forward statistics for port 1  -----------------
>   RX-packets: 19196503       RX-dropped: 0             RX-total: 19196503
>   TX-packets: 19196530       TX-dropped: 0             TX-total: 19196530
>   -----------------------------------------------------------------------
> 
> In this scenario, two physical functions of the adapter
> do not have any corresponding "back-to-back" forwarder
> on peer host. Packets transmitted from port 0 can only
> be forwarded to port 1 by means of a special flow rule.
> 
> The flow rule indeed works, but destroying it does not
> stop forwarding. Port statistics carry on incrementing.
> 
> Also, it is apparent that forwarding in the opposite
> direction must not have worked in this case as the
> flow is meant to target only one of the directions.
> 
> Because of the bug, the first 32 mbufs received
> as a result of the flow rule operation have the
> said metadata present. In io mode, testpmd does
> not tamper with mbufs and passes them directly
> to transmit path, so this data remains in them
> instructing the PMD to override destinations
> of the packets via Tx option descriptors.
> 
> Expected behaviour is as follows:
> 
>   ---------------------- Forward statistics for port 0  -----------------
>   RX-packets: 0              RX-dropped: 0             RX-total: 0
>   TX-packets: 15787496       TX-dropped: 0             TX-total: 15787496
>   -----------------------------------------------------------------------
> 
>   ---------------------- Forward statistics for port 1  -----------------
>   RX-packets: 15787464       RX-dropped: 0             RX-total: 15787464
>   TX-packets: 32             TX-dropped: 0             TX-total: 32
>   -----------------------------------------------------------------------
> 
> These figures show the rule work only for one direction.
> Also, removing the flow shall cause forwarding to cease.
> 
> Provided patch fixes the bug accordingly.
> 
> Fixes: d0f981a3efd8 ("net/sfc: handle ingress mport in EF100 Rx prefix")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> ---
> v3: extra rework after review feedback
> v2: address community review notes
> 
>  drivers/net/sfc/sfc_dp_rx.h    |  1 +
>  drivers/net/sfc/sfc_ef100_rx.c | 18 ++++++++++++++----
>  drivers/net/sfc/sfc_rx.c       |  3 +++
>  3 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/sfc/sfc_dp_rx.h b/drivers/net/sfc/sfc_dp_rx.h
> index 246adbd87c..8a504bdcf1 100644
> --- a/drivers/net/sfc/sfc_dp_rx.h
> +++ b/drivers/net/sfc/sfc_dp_rx.h
> @@ -69,6 +69,7 @@ struct sfc_dp_rx_qcreate_info {
>  	/** Receive queue flags initializer */
>  	unsigned int		flags;
>  #define SFC_RXQ_FLAG_RSS_HASH	0x1
> +#define SFC_RXQ_FLAG_INGRESS_MPORT	0x2
>  
>  	/** Rx queue size */
>  	unsigned int		rxq_entries;
> diff --git a/drivers/net/sfc/sfc_ef100_rx.c b/drivers/net/sfc/sfc_ef100_rx.c
> index b7e3397f77..e323156a26 100644
> --- a/drivers/net/sfc/sfc_ef100_rx.c
> +++ b/drivers/net/sfc/sfc_ef100_rx.c
> @@ -823,6 +823,9 @@ sfc_ef100_rx_qcreate(uint16_t port_id, uint16_t queue_id,
>  	if (rxq->nic_dma_info->nb_regions > 0)
>  		rxq->flags |= SFC_EF100_RXQ_NIC_DMA_MAP;
>  
> +	if (info->flags & SFC_RXQ_FLAG_INGRESS_MPORT)
> +		rxq->flags |= SFC_EF100_RXQ_INGRESS_MPORT;
> +
>  	sfc_ef100_rx_debug(rxq, "RxQ doorbell is %p", rxq->doorbell);
>  
>  	*dp_rxqp = &rxq->dp;
> @@ -889,11 +892,18 @@ sfc_ef100_rx_qstart(struct sfc_dp_rxq *dp_rxq, unsigned int evq_read_ptr,
>  	else
>  		rxq->flags &= ~SFC_EF100_RXQ_USER_MARK;
>  
> +
> +	/*
> +	 * At the moment, this feature is used only
> +	 * by the representor proxy Rx queue and is
> +	 * essential for representor support, so if
> +	 * it has been requested but is unsupported,
> +	 * point this inconsistency out to the user.
> +	 */
>  	if ((unsup_rx_prefix_fields &
> -	     (1U << EFX_RX_PREFIX_FIELD_INGRESS_MPORT)) == 0)
> -		rxq->flags |= SFC_EF100_RXQ_INGRESS_MPORT;
> -	else
> -		rxq->flags &= ~SFC_EF100_RXQ_INGRESS_MPORT;
> +	     (1U << EFX_RX_PREFIX_FIELD_INGRESS_MPORT)) &&
> +	    (rxq->flags & SFC_EF100_RXQ_INGRESS_MPORT))
> +		return ENOTSUP;
>  
>  	if ((unsup_rx_prefix_fields &
>  	     (1U << EFX_RX_PREFIX_FIELD_VLAN_STRIP_TCI)) == 0)


Hi Ivan,

The patch doesn't apply cleanly, and I can't find
'EFX_RX_PREFIX_FIELD_VLAN_STRIP_TCI' in the repo.

Can you please rebase the patch on top of latest 'next-net' and send a
new version? So that CI can run.
If there won't be any functional change, feel free to keep Andrew's ack
in next version.