Re: [PATCH 09/15] net/iavf: pack structures when building with MSVC

From: Bruce Richardson <bruce.richardson@intel.com>
To: Tyler Retzlaff <roretzla@linux.microsoft.com>
Cc: <dev@dpdk.org>, Akhil Goyal <gakhil@marvell.com>,
	Aman Singh <aman.deep.singh@intel.com>,
	Anatoly Burakov <anatoly.burakov@intel.com>,
	Byron Marohn <byron.marohn@intel.com>,
	Conor Walsh <conor.walsh@intel.com>,
	Cristian Dumitrescu <cristian.dumitrescu@intel.com>,
	Dariusz Sosnowski <dsosnowski@nvidia.com>,
	David Hunt <david.hunt@intel.com>,
	Jerin Jacob <jerinj@marvell.com>,
	Jingjing Wu <jingjing.wu@intel.com>,
	Kirill Rybalchenko <kirill.rybalchenko@intel.com>,
	Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>,
	Matan Azrad <matan@nvidia.com>, Ori Kam <orika@nvidia.com>,
	Radu Nicolau <radu.nicolau@intel.com>,
	Ruifeng Wang <ruifeng.wang@arm.com>,
	Sameh Gobriel <sameh.gobriel@intel.com>,
	"Sivaprasad Tummala" <sivaprasad.tummala@amd.com>,
	Suanming Mou <suanmingm@nvidia.com>,
	Sunil Kumar Kori <skori@marvell.com>,
	Vamsi Attunuru <vattunuru@marvell.com>,
	 Viacheslav Ovsiienko <viacheslavo@nvidia.com>,
	Vladimir Medvedkin <vladimir.medvedkin@intel.com>,
	Yipeng Wang <yipeng1.wang@intel.com>,
	"Yuying Zhang" <Yuying.Zhang@intel.com>
Subject: Re: [PATCH 09/15] net/iavf: pack structures when building with MSVC
Date: Thu, 21 Mar 2024 16:26:09 +0000	[thread overview]
Message-ID: <Zfxfoa5_Utk523Gq@bricha3-mobl1.ger.corp.intel.com> (raw)
In-Reply-To: <1710968771-16435-10-git-send-email-roretzla@linux.microsoft.com>

On Wed, Mar 20, 2024 at 02:06:05PM -0700, Tyler Retzlaff wrote:
> Add __rte_msvc_pushpack(1) to all __rte_packed structs to cause packing
> when building with MSVC.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  drivers/net/iavf/iavf_ipsec_crypto.h | 3 +++
>  drivers/net/iavf/iavf_rxtx.c         | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/net/iavf/iavf_ipsec_crypto.h b/drivers/net/iavf/iavf_ipsec_crypto.h
> index 49f9202..36ced50 100644
> --- a/drivers/net/iavf/iavf_ipsec_crypto.h
> +++ b/drivers/net/iavf/iavf_ipsec_crypto.h
> @@ -11,12 +11,14 @@
>  
>  
>  
> +__rte_msvc_pack
>  struct iavf_tx_ipsec_desc {
>  	union {
>  		struct {
>  			__le64 qw0;
>  			__le64 qw1;
>  		};
> +		__rte_msvc_pack
>  		struct {
>  			__le16 l4payload_length;
>  			__le32 esn;

Looking at the structure, I believe only the inner packed attribute is
necessary. However, given that we need packed attributes, for safety, let's
keep them both.

> @@ -84,6 +86,7 @@ enum iavf_ipsec_iv_len {
>   * transmit data path. Parameters set for session by calling
>   * rte_security_set_pkt_metadata() API.
>   */
> +__rte_msvc_pack
>  struct iavf_ipsec_crypto_pkt_metadata {
>  	uint32_t sa_idx;                /* SA hardware index (20b/4B) */
>  

This is exactly 16-bytes in size, with all members explicitly sized and
properly aligned. Therefore we can drop the packed atttribute.

> diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
> index 0a5246d..ba0fa6f 100644
> --- a/drivers/net/iavf/iavf_rxtx.c
> +++ b/drivers/net/iavf/iavf_rxtx.c
> @@ -50,6 +50,7 @@
>  static uint16_t vxlan_gpe_udp_port = RTE_VXLAN_GPE_DEFAULT_PORT;
>  static uint16_t geneve_udp_port = RTE_GENEVE_DEFAULT_PORT;
>  
> +__rte_msvc_pack
>  struct simple_gre_hdr {
>  	uint16_t flags;
>  	uint16_t proto;

Two 16-bit fields only, so no need to pack this struct either.

/Bruce