From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id C0387A052A; Fri, 10 Jul 2020 16:31:27 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 573101DB08; Fri, 10 Jul 2020 16:31:27 +0200 (CEST) Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by dpdk.org (Postfix) with ESMTP id 6BCEF1DAFB for ; Fri, 10 Jul 2020 16:31:25 +0200 (CEST) Received: by mail-wm1-f66.google.com with SMTP id q15so6391477wmj.2 for ; Fri, 10 Jul 2020 07:31:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=Gq7moUAy4SucJS38PePAcfTsCjSD9f9DhpsygQj1Tiw=; b=Wxg3iN3CD6eoWj1qfeIcf2ukBGLSSga4ruzBuzQnYPsNfWKV/8rUf9GtTaxEcAGHLf LWmS/dgyeFWPDtrQTQ7FGv9KEb1ArIvQHTa8KIRasceWz1gQ3mK3pwydnE21IUa3eFnx Zciw9UK4F3+lSjGzg1XZ3IriD5Qqv8Z1sEYxiij81YR1ycrWzBKVcXCuk/mtRQONAg3S dtPHotCiBlfaak2XT4NjbutNsongyjgpyN+/MmYzhEuxPaDhXBJq90w6CuGEOAR9zvb0 IT6pR9afc7EV0hb+DjA7Y2knv+CaM2+/AlspagjiSr1NnksYOs1wIPmGhyqtoI6j4E0F WJrQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=Gq7moUAy4SucJS38PePAcfTsCjSD9f9DhpsygQj1Tiw=; b=V1aw33KwO7SB6pt1JcjDYoIqjbg7nz7wYq7Q0roCvt2eMz3AfR1AR/WfNO+bGeKH9S fuZG7JkwuDhVg6zNqGbQ9KM+wpgvO/30zAxkfXlD6HQtIE3JiMw8SLOWFnM42MaLtLjD AZ1rgqWGjPWWc/F4feKFQSkdGDUTyogpf7ODFOsGYLk8yBlah9BQNpGdaP3mgKT48x3s 7yzw6BJaT0QC5YqEpl9Al0pN7aibTarJxR8DiXJjnHXmtIKOQGSTom6+Y+1TzLUrXC+C q8YwGmlqZyHgPrZTeywxvZ6Wo9TmF/p7tYWvt3jlpsOkfdUkdvtmZVA+/b4y+PS3coh7 JJFA== X-Gm-Message-State: AOAM5307KOJoGyUnbIo2pfHxXj9B2qjiwwBAahmrtSlukM2PlJ7yKFxS 5gj/Dq4pk4C1Bu4J9/4OMYaPBg== X-Google-Smtp-Source: ABdhPJywvRMdCFITlMKWK1tEcpyQyqYDymlGc6833zFzNgGLvrg+QlsHsj8TlEPl/LZ+fPMQISlFWQ== X-Received: by 2002:a1c:4846:: with SMTP id v67mr5722920wma.175.1594391485068; Fri, 10 Jul 2020 07:31:25 -0700 (PDT) Received: from 6wind.com (2a01cb0c0005a600345636f7e65ed1a0.ipv6.abo.wanadoo.fr. [2a01:cb0c:5:a600:3456:36f7:e65e:d1a0]) by smtp.gmail.com with ESMTPSA id j24sm11101537wrd.43.2020.07.10.07.31.24 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 10 Jul 2020 07:31:24 -0700 (PDT) Date: Fri, 10 Jul 2020 16:31:23 +0200 From: Olivier Matz To: Bing Zhao Cc: orika@mellanox.com, john.mcnamara@intel.com, marko.kovacevic@intel.com, thomas@monjalon.net, ferruh.yigit@intel.com, arybchenko@solarflare.com, akhil.goyal@nxp.com, dev@dpdk.org, wenzhuo.lu@intel.com, beilei.xing@intel.com, bernard.iremonger@intel.com Message-ID: <20200710143123.GE5869@platinum> References: <1594136219-133336-1-git-send-email-bingz@mellanox.com> <1594370723-343354-1-git-send-email-bingz@mellanox.com> <1594370723-343354-2-git-send-email-bingz@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1594370723-343354-2-git-send-email-bingz@mellanox.com> User-Agent: Mutt/1.10.1 (2018-07-13) Subject: Re: [dpdk-dev] [PATCH v5 1/2] rte_flow: add eCPRI key fields to flow API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > Acked-by: Ori Kam > --- > 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 > #include > #include > +#include > #include > #include > > @@ -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 > +#include > + > +#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 >