[v4,2/3] common/sfc_efx/base: add support to enable VLAN stripping
Checks
Commit Message
To enable VLAN stripping, two conditions must be met:
the corresponding flag must be set and the appropriate
RX prefix should be requested.
VLAN stripping is supported for ef100 datapath only.
Signed-off-by: Artemii Morozov <artemii.morozov@arknetworks.am>
Reviewed-by: Ivan Malov <ivan.malov@arknetworks.am>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
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(-)
Comments
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 <artemii.morozov@arknetworks.am>
> Reviewed-by: Ivan Malov <ivan.malov@arknetworks.am>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> ---
> 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);
>
Hello,
On 6/2/23 12:32, Andrew Rybchenko wrote:
> 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.
How about the following concept:
1. Add a new negotiated flag for metadata: RTE_ETH_RX_METADATA_VLAN_INFO.
2. Extend the driver to allow requesting the delivery of stripped VLAN
TCI through the metadata
negotiation method. This request will internally affect the Rx
prefix configuration via
libefx EFX_RXQ_FLAG_VLAN_TCI (or something like that).
3. Set the corresponding boolean flag in the sfc_port(strip_vlan =
something like that please advice)
structure if RTE_ETH_RX_OFFLOAD_VLAN_STRIP is received. Map this
flag to the corresponding flag in
efx_nic_t, Then, when generating specs for filters, use the
boolean flag from efx_nic_t to determine
the VLAN stripping behavior.
4. Users can request VLAN strip action. If a user does not request this
action, but the driver detects that
device-level stripping has been requested, it may implicitly enable
VLAN stripping for that specific flow
within the efx_mcdi_filter_op_add function.
In this concept, we allow the enabling of device-level VLAN stripping.
Points 1 and 2 concern the delivery of stripped VLAN TCI, while points 3
and 4 pertain to the request for VLAN tag stripping.
On 6/8/23 14:16, Artemii Morozov wrote:
> Hello,
>
> On 6/2/23 12:32, Andrew Rybchenko wrote:
>> 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.
>
> How about the following concept:
>
> 1. Add a new negotiated flag for metadata: RTE_ETH_RX_METADATA_VLAN_INFO.
>
> 2. Extend the driver to allow requesting the delivery of stripped VLAN
> TCI through the metadata
> negotiation method. This request will internally affect the Rx
> prefix configuration via
> libefx EFX_RXQ_FLAG_VLAN_TCI (or something like that).
>
> 3. Set the corresponding boolean flag in the sfc_port(strip_vlan =
> something like that please advice)
> structure if RTE_ETH_RX_OFFLOAD_VLAN_STRIP is received. Map this
> flag to the corresponding flag in
> efx_nic_t, Then, when generating specs for filters, use the
> boolean flag from efx_nic_t to determine
> the VLAN stripping behavior.
>
> 4. Users can request VLAN strip action. If a user does not request this
> action, but the driver detects that
> device-level stripping has been requested, it may implicitly enable
> VLAN stripping for that specific flow
> within the efx_mcdi_filter_op_add function.
>
> In this concept, we allow the enabling of device-level VLAN stripping.
> Points 1 and 2 concern the delivery of stripped VLAN TCI, while points 3
> and 4 pertain to the request for VLAN tag stripping.
>
It does not reply my questions and I see no single point to
complicate VLAN stripping configuration and introduce
corresponding Rx metadata control. Device-level offload is
sufficient and configured early enough to do its job.
@@ -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
+ * 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;
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;
}
@@ -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;
@@ -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.
+ */
+#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
typedef uint8_t efx_filter_flags_t;
@@ -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;
@@ -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;
};
#define EFX_RXQ_MAGIC 0x15022005
@@ -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(
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;
+ }
+
+ erp->er_flags |= EFX_RXQ_FLAG_VLAN_STRIP;
+ }
+
enp->en_rx_qcount++;
*erpp = erp;
return (0);
+fail6:
+ EFSYS_PROBE(fail6);
fail5:
EFSYS_PROBE(fail5);
@@ -640,6 +640,9 @@ rhead_rx_qcreate(
if (flags & EFX_RXQ_FLAG_USER_FLAG)
fields_mask |= 1U << EFX_RX_PREFIX_FIELD_USER_FLAG;
+ if (flags & EFX_RXQ_FLAG_VLAN_STRIP)
+ fields_mask |= 1U << EFX_RX_PREFIX_FIELD_VLAN_STRIP_TCI;
+
/*
* LENGTH is required in EF100 host interface, as receive events
* do not include the packet length.