diff mbox series

[v6,2/2] net: introduce IPv4 ihl and version fields

Message ID 20211013171354.27817-3-getelson@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series net: introduce IPv4 ihl and version fields |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/github-robot: build success github build: passed
ci/iol-spell-check-testing warning Testing issues
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing fail Testing issues
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional fail Functional Testing issues

Commit Message

Gregory Etelson Oct. 13, 2021, 5:13 p.m. UTC 
  RTE IPv4 header definition combines the `version' and `ihl'  fields
into a single structure member.
This patch introduces dedicated structure members for both `version'
and `ihl' IPv4 fields. Separated header fields definitions allow to
create simplified code to match on the IHL value in a flow rule.
The original `version_ihl' structure member is kept for backward
compatibility.

Signed-off-by: Gregory Etelson <getelson@nvidia.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
Acked-by: Ray Kinsella <mdr@ashroe.eu>
---
v2: Add dependency.
v3: Add comments.
v4: Update release notes.
v5: Remove deprecation notice.
    Update the patch comment.
v6: split the patch into 2 parts:
    1: update the announce.
    2: implement the IPv4 header changes.    
---
 app/test/test_flow_classify.c          |  8 ++++----
 doc/guides/rel_notes/deprecation.rst   |  3 ---
 doc/guides/rel_notes/release_21_11.rst |  3 +++
 lib/net/rte_ip.h                       | 16 +++++++++++++++-
 4 files changed, 22 insertions(+), 8 deletions(-)

Comments

Ferruh Yigit Oct. 14, 2021, 3:11 p.m. UTC | #1 
On 10/13/2021 6:13 PM, Gregory Etelson wrote:
>   struct rte_ipv4_hdr {
> -	uint8_t  version_ihl;		/**< version and header length */
> +	__extension__
> +	union {
> +		uint8_t version_ihl;    /**< version and header length */
> +		struct {
> +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> +			uint8_t ihl:4;     /**< header length */
> +			uint8_t version:4; /**< version */
> +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> +			uint8_t version:4; /**< version */
> +			uint8_t ihl:4;     /**< header length */
> +#else
> +#error "setup endian definition"
> +#endif

Do we need the last 'else' part?
Although it is harmless to have it, other protocol headers for endianness
check doesn't have this part, so I think better to be consistent.
Thomas Monjalon Oct. 14, 2021, 4:04 p.m. UTC | #2 
14/10/2021 17:11, Ferruh Yigit:
> On 10/13/2021 6:13 PM, Gregory Etelson wrote:
> >   struct rte_ipv4_hdr {
> > -	uint8_t  version_ihl;		/**< version and header length */
> > +	__extension__
> > +	union {
> > +		uint8_t version_ihl;    /**< version and header length */
> > +		struct {
> > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > +			uint8_t ihl:4;     /**< header length */
> > +			uint8_t version:4; /**< version */
> > +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> > +			uint8_t version:4; /**< version */
> > +			uint8_t ihl:4;     /**< header length */
> > +#else
> > +#error "setup endian definition"
> > +#endif
> 
> Do we need the last 'else' part?
> Although it is harmless to have it, other protocol headers for endianness
> check doesn't have this part, so I think better to be consistent.

In lib/eal/include/generic/rte_byteorder.h we already have
#if !defined(RTE_BYTE_ORDER)                                                                         
#error Unknown endianness.
#endif

So indeed we don't need this last else part.
Gregory Etelson Oct. 14, 2021, 5:42 p.m. UTC | #3 
Hello Thomas,

> > >   struct rte_ipv4_hdr {
> > > -   uint8_t  version_ihl;           /**< version
> and header length */
> > > +   __extension__
> > > +   union {
> > > +           uint8_t version_ihl;    /**< version
> and header length */
> > > +           struct {
> > > +#if RTE_BYTE_ORDER ==
> RTE_LITTLE_ENDIAN
> > > +                   uint8_t ihl:4;     /**< header
> length */
> > > +                   uint8_t version:4; /**< version
> */
> > > +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> > > +                   uint8_t version:4; /**< version
> */
> > > +                   uint8_t ihl:4;     /**< header
> length */
> > > +#else
> > > +#error "setup endian definition"
> > > +#endif
> >
> > Do we need the last 'else' part?
> > Although it is harmless to have it, other
> protocol headers for endianness
> > check doesn't have this part, so I think better
> to be consistent.
> 
> In lib/eal/include/generic/rte_byteorder.h we
> already have
> #if !defined(RTE_BYTE_ORDER)
> #error Unknown endianness.
> #endif
> 
> So indeed we don't need this last else part.
> 
> 

I updated the patch in v7.

Regards,
Gregory
diff mbox series

Patch

diff --git a/app/test/test_flow_classify.c b/app/test/test_flow_classify.c
index 951606f248..4f64be5357 100644
--- a/app/test/test_flow_classify.c
+++ b/app/test/test_flow_classify.c
@@ -95,7 +95,7 @@  static struct rte_acl_field_def ipv4_defs[NUM_FIELDS_IPV4] = {
  *  dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
  */
 static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
-	{ 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
+	{ { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
 	  RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}
 };
 static const struct rte_flow_item_ipv4 ipv4_mask_24 = {
@@ -131,7 +131,7 @@  static struct rte_flow_item  end_item = { RTE_FLOW_ITEM_TYPE_END,
  *  dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end"
  */
 static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
-	{ 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
+	{ { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
 	  RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)}
 };
 
@@ -150,8 +150,8 @@  static struct rte_flow_item  tcp_item_1 = { RTE_FLOW_ITEM_TYPE_TCP,
  *  dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
  */
 static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
-	{ 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14),
-	RTE_IPV4(15, 16, 17, 18)}
+	{ { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
+	RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
 };
 
 static struct rte_flow_item_sctp sctp_spec_1 = {
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 841653fe30..d1f3faac39 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -158,9 +158,6 @@  Deprecation Notices
   consistent with existing outer header checksum status flag naming, which
   should help in reducing confusion about its usage.
 
-* net: The structure ``rte_ipv4_hdr`` will have a union for
-  existing ``version_ihl`` byte and new bitfield for ``version`` and ``ihl``.
-
 * vhost: ``rte_vdpa_register_device``, ``rte_vdpa_unregister_device``,
   ``rte_vhost_host_notifier_ctrl`` and ``rte_vdpa_relay_vring_used`` vDPA
   driver interface will be marked as internal in DPDK v21.11.
diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
index e1726774ba..fa60fd9a85 100644
--- a/doc/guides/rel_notes/release_21_11.rst
+++ b/doc/guides/rel_notes/release_21_11.rst
@@ -216,6 +216,9 @@  API Changes
   the crypto/security operation. This field will be used to communicate
   events such as soft expiry with IPsec in lookaside mode.
 
+* net: Add ``version`` and ``ihl`` bit-fields to ``struct rte_ipv4_hdr``.
+  Existing ``version_ihl`` field was kept for backward compatibility.
+
 
 ABI Changes
 -----------
diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
index b3d45e85db..1f3660e8b4 100644
--- a/lib/net/rte_ip.h
+++ b/lib/net/rte_ip.h
@@ -39,7 +39,21 @@  extern "C" {
  * IPv4 Header
  */
 struct rte_ipv4_hdr {
-	uint8_t  version_ihl;		/**< version and header length */
+	__extension__
+	union {
+		uint8_t version_ihl;    /**< version and header length */
+		struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+			uint8_t ihl:4;     /**< header length */
+			uint8_t version:4; /**< version */
+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+			uint8_t version:4; /**< version */
+			uint8_t ihl:4;     /**< header length */
+#else
+#error "setup endian definition"
+#endif
+		};
+	};
 	uint8_t  type_of_service;	/**< type of service */
 	rte_be16_t total_length;	/**< length of packet */
 	rte_be16_t packet_id;		/**< packet ID */