[dpdk-dev] [PATCH v5 1/2] rte_flow: add eCPRI key fields to flow API
Olivier Matz
olivier.matz at 6wind.com
Fri Jul 10 16:31:23 CEST 2020
Hi Bing,
On Fri, Jul 10, 2020 at 04:45:22PM +0800, Bing Zhao wrote:
> Add a new item "rte_flow_item_ecpri" in order to match eCRPI header.
>
> eCPRI is a packet based protocol used in the fronthaul interface of
> 5G networks. Header format definition could be found in the
> specification via the link below:
> https://www.gigalight.com/downloads/standards/ecpri-specification.pdf
>
> eCPRI message can be over Ethernet layer (.1Q supported also) or over
> UDP layer. Message header formats are the same in these two variants.
>
> Signed-off-by: Bing Zhao <bingz at mellanox.com>
> Acked-by: Ori Kam <orika at mellanox.com>
> ---
> doc/guides/prog_guide/rte_flow.rst | 8 ++
> doc/guides/rel_notes/release_20_08.rst | 5 +
> lib/librte_ethdev/rte_flow.c | 1 +
> lib/librte_ethdev/rte_flow.h | 31 ++++++
> lib/librte_net/Makefile | 1 +
> lib/librte_net/meson.build | 3 +-
> lib/librte_net/rte_ecpri.h | 182 +++++++++++++++++++++++++++++++++
> lib/librte_net/rte_ether.h | 1 +
> 8 files changed, 231 insertions(+), 1 deletion(-)
> create mode 100644 lib/librte_net/rte_ecpri.h
>
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index d5dd18c..669d519 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1362,6 +1362,14 @@ Matches a PFCP Header.
> - ``seid``: session endpoint identifier.
> - Default ``mask`` matches s_field and seid.
>
> +Item: ``ECPRI``
> +^^^^^^^^^^^^^
> +
> +Matches a eCPRI header.
> +
> +- ``hdr``: eCPRI header definition (``rte_ecpri.h``).
> +- Default ``mask`` matches message type of common header only.
> +
> Actions
> ~~~~~~~
>
> diff --git a/doc/guides/rel_notes/release_20_08.rst b/doc/guides/rel_notes/release_20_08.rst
> index 988474c..19feb68 100644
> --- a/doc/guides/rel_notes/release_20_08.rst
> +++ b/doc/guides/rel_notes/release_20_08.rst
> @@ -184,6 +184,11 @@ New Features
> which are used to access packet data in a safe manner. Currently JIT support
> for these instructions is implemented for x86 only.
>
> +* **Added eCPRI protocol support in rte_flow.**
> +
> + The ``ECPRI`` item have been added to support eCPRI packet offloading for
> + 5G network.
> +
> * **Added flow performance test application.**
>
> Added new application to test ``rte_flow`` performance, including:
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index 1685be5..f8fdd68 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -95,6 +95,7 @@ struct rte_flow_desc_data {
> MK_FLOW_ITEM(HIGIG2, sizeof(struct rte_flow_item_higig2_hdr)),
> MK_FLOW_ITEM(L2TPV3OIP, sizeof(struct rte_flow_item_l2tpv3oip)),
> MK_FLOW_ITEM(PFCP, sizeof(struct rte_flow_item_pfcp)),
> + MK_FLOW_ITEM(ECPRI, sizeof(struct rte_flow_item_ecpri)),
> };
>
> /** Generate flow_action[] entry. */
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index b0e4199..8a90226 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -28,6 +28,7 @@
> #include <rte_byteorder.h>
> #include <rte_esp.h>
> #include <rte_higig.h>
> +#include <rte_ecpri.h>
> #include <rte_mbuf.h>
> #include <rte_mbuf_dyn.h>
>
> @@ -527,6 +528,15 @@ enum rte_flow_item_type {
> */
> RTE_FLOW_ITEM_TYPE_PFCP,
>
> + /**
> + * Matches eCPRI Header.
> + *
> + * Configure flow for eCPRI over ETH or UDP packets.
> + *
> + * See struct rte_flow_item_ecpri.
> + */
> + RTE_FLOW_ITEM_TYPE_ECPRI,
> +
> };
>
> /**
> @@ -1547,6 +1557,27 @@ struct rte_flow_item_pfcp {
> #endif
>
> /**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * RTE_FLOW_ITEM_TYPE_ECPRI
> + *
> + * Match eCPRI Header
> + */
> +struct rte_flow_item_ecpri {
> + struct rte_ecpri_msg_hdr hdr;
> +};
> +
> +/** Default mask for RTE_FLOW_ITEM_TYPE_ECPRI. */
> +#ifndef __cplusplus
> +static const struct rte_flow_item_ecpri rte_flow_item_ecpri_mask = {
> + .hdr = {
> + .dw0 = 0x0,
> + },
> +};
> +#endif
> +
> +/**
> * Matching pattern item definition.
> *
> * A pattern is formed by stacking items starting from the lowest protocol
> diff --git a/lib/librte_net/Makefile b/lib/librte_net/Makefile
> index aa1d6fe..9830e77 100644
> --- a/lib/librte_net/Makefile
> +++ b/lib/librte_net/Makefile
> @@ -20,5 +20,6 @@ SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include += rte_sctp.h rte_icmp.h rte_arp.h
> SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include += rte_ether.h rte_gre.h rte_net.h
> SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include += rte_net_crc.h rte_mpls.h rte_higig.h
> SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include += rte_gtp.h rte_vxlan.h
> +SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include += rte_ecpri.h
>
> include $(RTE_SDK)/mk/rte.lib.mk
> diff --git a/lib/librte_net/meson.build b/lib/librte_net/meson.build
> index f799349..24ed825 100644
> --- a/lib/librte_net/meson.build
> +++ b/lib/librte_net/meson.build
> @@ -15,7 +15,8 @@ headers = files('rte_ip.h',
> 'rte_net.h',
> 'rte_net_crc.h',
> 'rte_mpls.h',
> - 'rte_higig.h')
> + 'rte_higig.h',
> + 'rte_ecpri.h')
>
> sources = files('rte_arp.c', 'rte_ether.c', 'rte_net.c', 'rte_net_crc.c')
> deps += ['mbuf']
> diff --git a/lib/librte_net/rte_ecpri.h b/lib/librte_net/rte_ecpri.h
> new file mode 100644
> index 0000000..60fb4f7
> --- /dev/null
> +++ b/lib/librte_net/rte_ecpri.h
> @@ -0,0 +1,182 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2020 Mellanox Technologies, Ltd
> + */
> +
> +#ifndef _RTE_ECPRI_H_
> +#define _RTE_ECPRI_H_
> +
> +#include <stdint.h>
> +#include <rte_byteorder.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/**
> + * eCPRI Protocol Revision 1.0, 1.1, 1.2, 2.0: 0001b
> + * Other values are reserved for future
> + */
> +#define RTE_ECPRI_REV_UP_TO_20 1
> +
> +/**
> + * eCPRI message types in specifications
> + * IWF* types will only be supported from rev.2
> + * 12-63: Reserved for future revision
> + * 64-255: Vendor Specific
> + */
> +#define RTE_ECPRI_MSG_TYPE_IQ_DATA 0
Here the doxygen comment applies to RTE_ECPRI_MSG_TYPE_IQ_DATA.
I think it should either be a standard comment (no doxygen), or
something more complex should be done, like grouping:
https://stackoverflow.com/questions/30803156/group-level-documentation-for-preprocessor-defines-in-doxygen
(I didn't try)
> +#define RTE_ECPRI_MSG_TYPE_BIT_SEQ 1
> +#define RTE_ECPRI_MSG_TYPE_RTC_CTRL 2
> +#define RTE_ECPRI_MSG_TYPE_GEN_DATA 3
> +#define RTE_ECPRI_MSG_TYPE_RM_ACC 4
> +#define RTE_ECPRI_MSG_TYPE_DLY_MSR 5
> +#define RTE_ECPRI_MSG_TYPE_RMT_RST 6
> +#define RTE_ECPRI_MSG_TYPE_EVT_IND 7
> +#define RTE_ECPRI_MSG_TYPE_IWF_UP 8
> +#define RTE_ECPRI_MSG_TYPE_IWF_OPT 9
> +#define RTE_ECPRI_MSG_TYPE_IWF_MAP 10
> +#define RTE_ECPRI_MSG_TYPE_IWF_DCTRL 11
> +
> +/**
> + * Event Type of Message Type #7: Event Indication
> + * 0x00: Fault(s) Indication
> + * 0x01: Fault(s) Indication Acknowledge
> + * 0x02: Notification(s) Indication
> + * 0x03: Synchronization Request
> + * 0x04: Synchronization Acknowledge
> + * 0x05: Synchronization End Indication
> + * 0x06...0xFF: Reserved
> + */
> +#define RTE_ECPRI_EVT_IND_FAULT_IND 0
> +#define RTE_ECPRI_EVT_IND_FAULT_ACK 1
> +#define RTE_ECPRI_EVT_IND_NTFY_IND 2
> +#define RTE_ECPRI_EVT_IND_SYNC_REQ 3
> +#define RTE_ECPRI_EVT_IND_SYNC_ACK 4
> +#define RTE_ECPRI_EVT_IND_SYNC_END 5
> +
> +/**
> + * eCPRI Common Header
> + */
> +RTE_STD_C11
> +struct rte_ecpri_common_hdr {
> +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> + uint32_t size:16; /**< Payload Size */
> + uint32_t type:8; /**< Message Type */
> + uint32_t c:1; /**< Concatenation Indicator */
> + uint32_t res:3; /**< Reserved */
> + uint32_t revision:4; /**< Protocol Revision */
> +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> + uint32_t revision:4; /**< Protocol Revision */
> + uint32_t res:3; /**< Reserved */
> + uint32_t c:1; /**< Concatenation Indicator */
> + uint32_t type:8; /**< Message Type */
> + uint32_t size:16; /**< Payload Size */
> +#endif
> +} __rte_packed;
Does it really need to be packed? Why next types do not need it?
It looks only those which have bitfields are.
I wonder if the 'dw0' could be in this definition instead of in
struct rte_ecpri_msg_hdr?
Something like this:
struct rte_ecpri_common_hdr {
union {
uint32_t u32;
struct {
...
};
};
};
I see 2 advantages:
- someone that only uses the common_hdr struct can use the .u32
in its software
- when using it in messages, it looks clearer to me:
msg.common_hdr.u32 = value;
instead of:
msg.dw0 = value;
What do you think?
> +
> +/**
> + * eCPRI Message Header of Type #0: IQ Data
> + */
> +struct rte_ecpri_msg_iq_data {
> + rte_be16_t pc_id; /**< Physical channel ID */
> + rte_be16_t seq_id; /**< Sequence ID */
> +};
> +
> +/**
> + * eCPRI Message Header of Type #1: Bit Sequence
> + */
> +struct rte_ecpri_msg_bit_seq {
> + rte_be16_t pc_id; /**< Physical channel ID */
> + rte_be16_t seq_id; /**< Sequence ID */
> +};
> +
> +/**
> + * eCPRI Message Header of Type #2: Real-Time Control Data
> + */
> +struct rte_ecpri_msg_rtc_ctrl {
> + rte_be16_t rtc_id; /**< Real-Time Control Data ID */
> + rte_be16_t seq_id; /**< Sequence ID */
> +};
> +
> +/**
> + * eCPRI Message Header of Type #3: Generic Data Transfer
> + */
> +struct rte_ecpri_msg_gen_data {
> + rte_be32_t pc_id; /**< Physical channel ID */
> + rte_be32_t seq_id; /**< Sequence ID */
> +};
> +
> +/**
> + * eCPRI Message Header of Type #4: Remote Memory Access
> + */
> +RTE_STD_C11
> +struct rte_ecpri_msg_rm_access {
> +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> + uint32_t ele_id:16; /**< Element ID */
> + uint32_t rr:4; /**< Req/Resp */
> + uint32_t rw:4; /**< Read/Write */
> + uint32_t rma_id:8; /**< Remote Memory Access ID */
> +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> + uint32_t rma_id:8; /**< Remote Memory Access ID */
> + uint32_t rw:4; /**< Read/Write */
> + uint32_t rr:4; /**< Req/Resp */
> + uint32_t ele_id:16; /**< Element ID */
> +#endif
> + rte_be16_t addr_m; /**< 48-bits address (16 MSB) */
> + rte_be32_t addr_l; /**< 48-bits address (32 LSB) */
> + rte_be16_t length; /**< number of bytes */
> +} __rte_packed;
> +
> +/**
> + * eCPRI Message Header of Type #5: One-Way Delay Measurement
> + */
> +struct rte_ecpri_msg_delay_measure {
> + uint8_t msr_id; /**< Measurement ID */
> + uint8_t act_type; /**< Action Type */
> +};
> +
> +/**
> + * eCPRI Message Header of Type #6: Remote Reset
> + */
> +struct rte_ecpri_msg_remote_reset {
> + rte_be16_t rst_id; /**< Reset ID */
> + uint8_t rst_op; /**< Reset Code Op */
> +};
> +
> +/**
> + * eCPRI Message Header of Type #7: Event Indication
> + */
> +struct rte_ecpri_msg_event_ind {
> + uint8_t evt_id; /**< Event ID */
> + uint8_t evt_type; /**< Event Type */
> + uint8_t seq; /**< Sequence Number */
> + uint8_t number; /**< Number of Faults/Notif */
> +};
> +
> +/**
> + * eCPRI Message Header Format: Common Header + Message Types
> + */
> +RTE_STD_C11
> +struct rte_ecpri_msg_hdr {
> + union {
> + struct rte_ecpri_common_hdr common;
> + uint32_t dw0;
> + };
> + union {
> + struct rte_ecpri_msg_iq_data type0;
> + struct rte_ecpri_msg_bit_seq type1;
> + struct rte_ecpri_msg_rtc_ctrl type2;
> + struct rte_ecpri_msg_bit_seq type3;
> + struct rte_ecpri_msg_rm_access type4;
> + struct rte_ecpri_msg_delay_measure type5;
> + struct rte_ecpri_msg_remote_reset type6;
> + struct rte_ecpri_msg_event_ind type7;
> + uint32_t dummy[3];
> + };
> +};
What is the point in having this struct?
>From a software point of view, I think it is a bit risky, because
its size is the size of the largest message. This is probably what
you want in your case, but when a software will rx or tx such
packet, I think they shouldn't use this one. My understanding is
that you only need this structure for the mask in rte_flow.
Also, I'm not sure to understand the purpose of dummy[3], even after reading
your answer to Akhil's question.
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_ECPRI_H_ */
> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> index 0ae4e75..184a3f9 100644
> --- a/lib/librte_net/rte_ether.h
> +++ b/lib/librte_net/rte_ether.h
> @@ -304,6 +304,7 @@ struct rte_vlan_hdr {
> #define RTE_ETHER_TYPE_LLDP 0x88CC /**< LLDP Protocol. */
> #define RTE_ETHER_TYPE_MPLS 0x8847 /**< MPLS ethertype. */
> #define RTE_ETHER_TYPE_MPLSM 0x8848 /**< MPLS multicast ethertype. */
> +#define RTE_ETHER_TYPE_ECPRI 0xAEFE /**< eCPRI ethertype (.1Q supported). */
>
> /**
> * Extract VLAN tag information into mbuf
> --
> 1.8.3.1
>
More information about the dev
mailing list