From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4153742C10; Fri, 2 Jun 2023 10:32:21 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2DD7440ED8; Fri, 2 Jun 2023 10:32:21 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 974A2406B8 for ; Fri, 2 Jun 2023 10:32:19 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 16CC850; Fri, 2 Jun 2023 11:32:18 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 16CC850 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1685694738; bh=yzA823WQCADAfvfVoxbYXIkp11KHzLywuadT7qtdpFY=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=HOabOCjcA7dDAHtJfG+yYuHIaJO/h6+oxxieKtUa6+nZG2Gsboi2eGXkVp/r4P8CX 9pvXjIj5YrJ0Hy6VHlA8EFo3gIQgrL7xJ/Pg3tTRMEcOm3RIz7+2Otkt3X1/tD+ewu 0mG6F8+DZcI7A+5ZsPBu7qSo/jGcRfgilYr41B4I= Message-ID: Date: Fri, 2 Jun 2023 11:32:17 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCH v4 2/3] common/sfc_efx/base: add support to enable VLAN stripping Content-Language: en-US To: Artemii Morozov , dev@dpdk.org Cc: Ivan Malov , Viacheslav Galaktionov , Andy Moreton References: <20230531134122.119508-1-artemii.morozov@arknetworks.am> <20230601153022.99634-1-artemii.morozov@arknetworks.am> <20230601153022.99634-3-artemii.morozov@arknetworks.am> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20230601153022.99634-3-artemii.morozov@arknetworks.am> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 6/1/23 18:30, Artemii Morozov wrote: > To enable VLAN stripping, two conditions must be met: > the corresponding flag must be set and the appropriate > RX prefix should be requested. RX -> Rx > VLAN stripping is supported for ef100 datapath only. When you read below notes carefully, you'll understand that the patch is very raw yet. There are too many questions below. The major decision to be made is if libefx API should enforce device level offload or should just provide transparent API and driver must be responsible for consistency. If driver must be responsible for consistency, don't try to enforce VLAN stripping enable via filter table. RxQ flags should control stripped VLAN delivery. Filter flag and default RxQ set flag (API should be extended) should control VLAN stripping enabling. If libefx must guarantee consistency, we need a new way to enable device level offload early (efx_rx_init()-like, may be a new API). If so, it should be impossible to control VLAN stripping per Rx filter. It still could be possible to control stripped VLAN delivery per RxQ. Anyway current solution is really inconsistent. > > Signed-off-by: Artemii Morozov > Reviewed-by: Ivan Malov > Reviewed-by: Andy Moreton > --- > drivers/common/sfc_efx/base/ef10_filter.c | 21 +++++++++++++++++++++ > drivers/common/sfc_efx/base/ef10_impl.h | 1 + > drivers/common/sfc_efx/base/efx.h | 9 +++++++++ > drivers/common/sfc_efx/base/efx_filter.c | 3 ++- > drivers/common/sfc_efx/base/efx_impl.h | 1 + > drivers/common/sfc_efx/base/efx_rx.c | 17 +++++++++++++++++ > drivers/common/sfc_efx/base/rhead_rx.c | 3 +++ > 7 files changed, 54 insertions(+), 1 deletion(-) > > diff --git a/drivers/common/sfc_efx/base/ef10_filter.c b/drivers/common/sfc_efx/base/ef10_filter.c > index 6d19797d16..6371d8d489 100644 > --- a/drivers/common/sfc_efx/base/ef10_filter.c > +++ b/drivers/common/sfc_efx/base/ef10_filter.c > @@ -338,6 +338,11 @@ efx_mcdi_filter_op_add( > MC_CMD_FILTER_OP_V3_IN_MATCH_ACTION_FLAG); > } > > + if (spec->efs_flags & EFX_FILTER_FLAG_VLAN_STRIP) { > + MCDI_IN_SET_DWORD_FIELD(req, FILTER_OP_V3_IN_MATCH_ACTION_FLAGS, > + FILTER_OP_V3_IN_MATCH_STRIP_VLAN, 1); > + } > + > efx_mcdi_execute(enp, &req); > > if (req.emr_rc != 0) { > @@ -852,6 +857,16 @@ ef10_filter_add_internal( > > hash = ef10_filter_hash(spec); > > + /* > + * VLAN stripping is offload at the device level, and it should be > + * applied to all filters if it has been applied at least once before. > + * Knowledge of device level vlan strip offload being enabled comes from vlan -> VLAN > + * the default RxQ flags, albeit the flag may be set on any queue. > + * But only default RxQ affects this 'eft_strip_vlan' decision. > + */ > + if (eftp->eft_strip_vlan) > + spec->efs_flags |= EFX_FILTER_FLAG_VLAN_STRIP; > + > /* > * FIXME: Add support for inserting filters of different priorities > * and removing lower priority multicast filters (bug 42378) > @@ -2010,6 +2025,9 @@ ef10_filter_reconfigure( > else > filter_flags = 0; > > + if (table->eft_strip_vlan) > + filter_flags |= EFX_FILTER_FLAG_VLAN_STRIP; > + > /* Mark old filters which may need to be removed */ > ef10_filter_mark_old_filters(enp); > > @@ -2142,6 +2160,8 @@ ef10_filter_default_rxq_set( > EFSYS_ASSERT(using_rss == B_FALSE); > table->eft_using_rss = B_FALSE; > #endif > + if (erp->er_flags & EFX_RXQ_FLAG_VLAN_STRIP) > + table->eft_strip_vlan = B_TRUE; How to enable VLAN stripping in isolated mode where API to set default RxQ is not called? > table->eft_default_rxq = erp; > } > > @@ -2153,6 +2173,7 @@ ef10_filter_default_rxq_clear( > > table->eft_default_rxq = NULL; > table->eft_using_rss = B_FALSE; > + table->eft_strip_vlan = B_FALSE; > } > > > diff --git a/drivers/common/sfc_efx/base/ef10_impl.h b/drivers/common/sfc_efx/base/ef10_impl.h > index 877aedad45..017e561f19 100644 > --- a/drivers/common/sfc_efx/base/ef10_impl.h > +++ b/drivers/common/sfc_efx/base/ef10_impl.h > @@ -1287,6 +1287,7 @@ typedef struct ef10_filter_table_s { > ef10_filter_entry_t eft_entry[EFX_EF10_FILTER_TBL_ROWS]; > efx_rxq_t *eft_default_rxq; > boolean_t eft_using_rss; > + boolean_t eft_strip_vlan; > uint32_t eft_unicst_filter_indexes[ > EFX_EF10_FILTER_UNICAST_FILTERS_MAX]; > uint32_t eft_unicst_filter_count; > diff --git a/drivers/common/sfc_efx/base/efx.h b/drivers/common/sfc_efx/base/efx.h > index ac89b418e0..508d0a802d 100644 > --- a/drivers/common/sfc_efx/base/efx.h > +++ b/drivers/common/sfc_efx/base/efx.h > @@ -3101,6 +3101,10 @@ typedef enum efx_rxq_type_e { > * Request user flag field in the Rx prefix of a queue. > */ > #define EFX_RXQ_FLAG_USER_FLAG 0x20 > +/* > + * Request vlan tci field in the Rx prefix of a queue. vlan -> VLAN, tci -> TCI The name and comment are a bit misleading. Name sounds like it controls VLAN stripping on the RxQ. Consider EFX_RXQ_FLAG_STRIPPED_VLAN to highlight that it just requests stripped VLAN TCI. Moreover, comment should say that it does *not* control VLAN stripping. It just controls delivery of the stripped VLAN TCI if VLAN stripping is enabled and done for a packet. However, as far as I can see finally the flag is used to enable VLAN stripping as well and it is terribly bad since it adds false expectations on per-RxQ control for the feature. It the flag should really control Rx VLAN stripping, comment should say so and explain limitations. > + */ > +#define EFX_RXQ_FLAG_VLAN_STRIP 0x40 > > LIBEFX_API > extern __checkReturn efx_rc_t > @@ -3455,6 +3459,11 @@ efx_tx_qdestroy( > #define EFX_FILTER_FLAG_ACTION_FLAG 0x20 > /* Set match mark on the received packet */ > #define EFX_FILTER_FLAG_ACTION_MARK 0x40 > +/* > + * Request that the first tag in the outer L2 header be stripped > + * and TCI be indicated in Rx prefix. > + */ > +#define EFX_FILTER_FLAG_VLAN_STRIP 0x80 The flag allows driver to request Rx VLAN stripping in some filters only. Is it really intended? > > typedef uint8_t efx_filter_flags_t; > > diff --git a/drivers/common/sfc_efx/base/efx_filter.c b/drivers/common/sfc_efx/base/efx_filter.c > index 83c37ff859..778ed0c370 100644 > --- a/drivers/common/sfc_efx/base/efx_filter.c > +++ b/drivers/common/sfc_efx/base/efx_filter.c > @@ -322,7 +322,8 @@ efx_filter_spec_init_rx( > EFSYS_ASSERT3P(spec, !=, NULL); > EFSYS_ASSERT3P(erp, !=, NULL); > EFSYS_ASSERT((flags & ~(EFX_FILTER_FLAG_RX_RSS | > - EFX_FILTER_FLAG_RX_SCATTER)) == 0); > + EFX_FILTER_FLAG_RX_SCATTER | > + EFX_FILTER_FLAG_VLAN_STRIP)) == 0); > > memset(spec, 0, sizeof (*spec)); > spec->efs_priority = priority; > diff --git a/drivers/common/sfc_efx/base/efx_impl.h b/drivers/common/sfc_efx/base/efx_impl.h > index 45e99d01c5..c45669e077 100644 > --- a/drivers/common/sfc_efx/base/efx_impl.h > +++ b/drivers/common/sfc_efx/base/efx_impl.h > @@ -1045,6 +1045,7 @@ struct efx_rxq_s { > efsys_mem_t *er_esmp; > efx_evq_rxq_state_t *er_ev_qstate; > efx_rx_prefix_layout_t er_prefix_layout; > + uint16_t er_flags; Why is it 16-bit width. It shares typedefs with RxQ create flags which are unsigned int? > }; > > #define EFX_RXQ_MAGIC 0x15022005 > diff --git a/drivers/common/sfc_efx/base/efx_rx.c b/drivers/common/sfc_efx/base/efx_rx.c > index 68f42f5cac..5726085953 100644 > --- a/drivers/common/sfc_efx/base/efx_rx.c > +++ b/drivers/common/sfc_efx/base/efx_rx.c > @@ -925,6 +925,7 @@ efx_rx_qcreate_internal( > erp->er_index = index; > erp->er_mask = ndescs - 1; > erp->er_esmp = esmp; > + erp->er_flags = 0; > > if ((rc = erxop->erxo_qcreate(enp, index, label, type, type_data, esmp, > ndescs, id, flags, eep, erp)) != 0) > @@ -941,11 +942,27 @@ efx_rx_qcreate_internal( As far as I can see it is a bug here since rc is 0, but the function should fail. Please, submit separate patch with appropriate Fixes tag. > goto fail5; > } > > + if (flags & EFX_RXQ_FLAG_VLAN_STRIP) { > + const efx_rx_prefix_layout_t *erplp = &erp->er_prefix_layout; > + const efx_rx_prefix_field_info_t *vlan_tci_field; > + > + vlan_tci_field = > + &erplp->erpl_fields[EFX_RX_PREFIX_FIELD_VLAN_STRIP_TCI]; > + if (vlan_tci_field->erpfi_width_bits == 0) { > + rc = ENOTSUP; > + goto fail6; > + } IMHO support for Rx VLAN stripping capability should be checked here as well. It is different aspects of the HW support: 1. Support for Rx VLAN stripping 2. Delivery of the stripped VLAN tag > + > + erp->er_flags |= EFX_RXQ_FLAG_VLAN_STRIP; > + } > + > enp->en_rx_qcount++; > *erpp = erp; > > return (0); > > +fail6: > + EFSYS_PROBE(fail6); > fail5: > EFSYS_PROBE(fail5); >