mbox series

[v6,00/30] fix packing of structs when building with MSVC

Message ID 1732668761-5556-1-git-send-email-andremue@linux.microsoft.com (mailing list archive)
Headers
Series fix packing of structs when building with MSVC |

Message

Andre Muezerie Nov. 27, 2024, 12:52 a.m. UTC
MSVC struct packing is not compatible with GCC. Provide a macro
(__rte_packed_begin) that can be used to push existing pack value
and sets packing to 1-byte. The existing __rte_packed macro is
replaced with __rte_packed_end and restores the pack value
prior to the push.

Instead of providing macros exclusively for MSVC and for GCC,
macro __rte_packed_end is deliberately utilized to trigger a
MSVC compiler warning if no existing packing has been pushed allowing
easy identification of locations where the __rte_packed_begin is
missing.

Macro __rte_packed is removed and the two new macros represent the
new way to enable packing in the DPDK code.

Script checkpatches.sh was enhanced to ensure __rte_packed_begin and
__rte_packed_end show up in pairs when checking patches.

If as a part of review maintainers identify structs they believe
don't require packing so long as they are explicitly identified
I'll remove the __rte_packed as a part of this series.

v6:
  * replace __rte_msvc_pack with __rte_packed_begin
  * replace __rte_packed with __rte_packed_end
  * update checkpatches.sh to ensure __rte_packed_begin and
    __rte_packed_end are used in pairs
  * remove __rte_packed

v5:
  * rebase on top of latest main

v4:
  * add another missing __rte_msvc_pack to crypto/mlx5 patch
  * correct commit message for duplicated packing packing in
    crypto/mlx5 patch

v3:
  * add missing __rte_msvc_pack to crypto/mlx5
  * fix commit messages to reference __rte_msvc_pack macro instead
    of __rte_msvc_pushpack(1)

v2:
  * app/testpmd, remove packing from simple_gre_hdr
  * net/iavf, remove packing from iavf_ipsec_crypto_pkt_metadata,
    simple_gre_hdr
  * examples, remove packing from pkt_key_qinq, pkt_key_ipv4_5tuple,
    pkt_key_ipv6_5tuple, pkt_key_ipv4_addr, pkt_key_ipv6_addr
  * eal, remove packing from rte_config, __rte_trace_stream_header

Andre Muezerie (30):
  devtools: check packed attributes
  eal/include: add new packing macros
  app/test-pmd: remove unnecessary packed attributes
  app/test: replace packed attributes
  doc/guides: replace packed attributes
  drivers/baseband: replace packed attributes
  drivers/bus: replace packed attributes
  drivers/common: replace packed attributes
  drivers/compress: replace packed attributes
  drivers/crypto: replace packed attributes
  drivers/dma: replace packed attributes
  drivers/event: replace packed attributes
  drivers/mempool: replace packed attributes
  drivers/net: replace packed attributes
  drivers/raw: replace packed attributes
  drivers/regex: replace packed attributes
  drivers/vdpa: replace packed attributes
  examples/common: replace packed attributes
  examples/ip-pipeline: remove packed attributes
  examples/ipsec_secgw: replace packed attributes
  examples/l3fwd-power: replace packed attributes
  examples/l3fwd: replace packed attributes
  examples/ptpclient: replace packed attributes
  examples/vhost_blk: replace packed attributes
  lib/eal: replace packed attributes
  lib/ipsec: replace packed attributes
  lib/net: replace packed attributes
  lib/pipeline: replace packed attributes
  lib/vhost: replace packed attributes
  lib/eal: remove __rte_packed

 app/test-pmd/csumonly.c                       |    2 +-
 app/test/test_efd.c                           |    3 +-
 app/test/test_hash.c                          |    3 +-
 app/test/test_member.c                        |    3 +-
 devtools/checkpatches.sh                      |   23 +
 doc/guides/nics/ark.rst                       |    3 +-
 .../prog_guide/packet_classif_access_ctrl.rst |    3 +-
 drivers/baseband/acc/acc_common.h             |   59 +-
 drivers/baseband/fpga_5gnr_fec/agx100_pmd.h   |   16 +-
 .../baseband/fpga_5gnr_fec/fpga_5gnr_fec.h    |    4 +-
 drivers/baseband/fpga_5gnr_fec/vc_5gnr_pmd.h  |    8 +-
 drivers/baseband/fpga_lte_fec/fpga_lte_fec.c  |   12 +-
 drivers/baseband/la12xx/bbdev_la12xx_ipc.h    |   32 +-
 drivers/bus/dpaa/include/fsl_bman.h           |   15 +-
 drivers/bus/dpaa/include/fsl_fman.h           |    4 +-
 drivers/bus/dpaa/include/fsl_qman.h           |  158 +-
 drivers/bus/ifpga/bus_ifpga_driver.h          |    8 +-
 drivers/bus/vmbus/rte_vmbus_reg.h             |  108 +-
 drivers/common/cnxk/hw/sdp.h                  |    4 +-
 drivers/common/cnxk/roc_npc.h                 |   16 +-
 drivers/common/cnxk/roc_npc_mcam_dump.c       |    4 +-
 drivers/common/cnxk/roc_platform.h            |    3 +-
 drivers/common/dpaax/compat.h                 |    3 -
 drivers/common/iavf/iavf_osdep.h              |    8 +-
 drivers/common/iavf/virtchnl_inline_ipsec.h   |   44 +-
 drivers/common/idpf/base/idpf_osdep.h         |    8 +-
 drivers/common/mlx5/mlx5_common_mr.h          |   12 +-
 drivers/common/mlx5/mlx5_common_utils.h       |    3 +-
 drivers/common/mlx5/mlx5_prm.h                |   90 +-
 drivers/common/qat/qat_adf/icp_qat_fw_la.h    |    8 +-
 drivers/common/qat/qat_common.h               |    8 +-
 drivers/compress/qat/qat_comp.h               |    4 +-
 drivers/crypto/caam_jr/caam_jr.c              |    4 +-
 drivers/crypto/caam_jr/caam_jr_desc.h         |   64 +-
 drivers/crypto/caam_jr/caam_jr_hw_specific.h  |   48 +-
 drivers/crypto/dpaa_sec/dpaa_sec.h            |   12 +-
 drivers/crypto/ionic/ionic_crypto_if.h        |   36 +-
 drivers/crypto/mlx5/mlx5_crypto.h             |    6 +-
 drivers/crypto/mlx5/mlx5_crypto_gcm.c         |    3 +-
 drivers/crypto/qat/qat_sym.h                  |    7 +-
 drivers/crypto/qat/qat_sym_session.h          |    4 +-
 drivers/dma/dpaa/dpaa_qdma.h                  |   20 +-
 drivers/dma/dpaa2/dpaa2_qdma.h                |   16 +-
 drivers/dma/ioat/ioat_hw_defs.h               |    3 +-
 drivers/event/octeontx/timvf_evdev.c          |    4 +-
 drivers/event/octeontx/timvf_evdev.h          |   12 +-
 drivers/mempool/octeontx/octeontx_fpavf.c     |   16 +-
 drivers/net/ark/ark_ddm.h                     |    4 +-
 drivers/net/ark/ark_pktchkr.h                 |    8 +-
 drivers/net/ark/ark_pktdir.h                  |    5 +-
 drivers/net/ark/ark_pktgen.h                  |    4 +-
 drivers/net/ark/ark_udm.h                     |    4 +-
 drivers/net/atlantic/hw_atl/hw_atl_utils.h    |  120 +-
 .../net/atlantic/hw_atl/hw_atl_utils_fw2x.c   |    8 +-
 drivers/net/avp/rte_avp_common.h              |   12 +-
 drivers/net/bnxt/bnxt.h                       |    8 +-
 drivers/net/bnxt/hsi_struct_def_dpdk.h        | 3344 ++++++++---------
 drivers/net/bnxt/tf_core/tf_resources.h       |   32 +-
 drivers/net/bnxt/tf_core/v3/tfc_mpc_table.c   |   20 +-
 drivers/net/bonding/rte_eth_bond_8023ad.h     |   32 +-
 drivers/net/cnxk/cn10k_rxtx.h                 |    4 +-
 drivers/net/cnxk/cn20k_rxtx.h                 |    4 +-
 drivers/net/cnxk/cn9k_ethdev.h                |    4 +-
 drivers/net/cnxk/cnxk_rep_msg.h               |   64 +-
 drivers/net/dpaa/dpaa_rxtx.h                  |   28 +-
 drivers/net/dpaa/fmlib/fm_ext.h               |    4 +-
 drivers/net/dpaa2/base/dpaa2_hw_dpni_annot.h  |    4 +-
 drivers/net/dpaa2/dpaa2_recycle.c             |   16 +-
 drivers/net/enic/base/vnic_devcmd.h           |   40 +-
 drivers/net/enic/base/vnic_flowman.h          |  120 +-
 drivers/net/gve/base/gve_desc.h               |   16 +-
 drivers/net/gve/base/gve_desc_dqo.h           |   32 +-
 drivers/net/gve/base/gve_osdep.h              |    3 -
 drivers/net/hns3/hns3_mbx.h                   |    8 +-
 drivers/net/hns3/hns3_rxtx.h                  |    4 +-
 drivers/net/i40e/base/i40e_osdep.h            |    6 +-
 drivers/net/iavf/iavf_ipsec_crypto.h          |    8 +-
 drivers/net/iavf/iavf_rxtx.c                  |    2 +-
 drivers/net/ice/base/ice_osdep.h              |    9 +-
 drivers/net/ionic/ionic_if.h                  |   72 +-
 drivers/net/memif/memif.h                     |   36 +-
 drivers/net/mlx4/mlx4_mr.h                    |   12 +-
 drivers/net/mlx5/hws/mlx5dr.h                 |    3 +-
 drivers/net/mlx5/mlx5.h                       |    3 +-
 drivers/net/mlx5/mlx5_flow.h                  |   12 +-
 drivers/net/mlx5/mlx5_hws_cnt.h               |    3 +-
 drivers/net/mlx5/mlx5_utils.h                 |   12 +-
 drivers/net/netvsc/hn_nvs.h                   |   72 +-
 drivers/net/netvsc/ndis.h                     |    8 +-
 drivers/net/nfp/flower/nfp_flower_cmsg.h      |    4 +-
 drivers/net/nfp/flower/nfp_flower_flow.h      |    4 +-
 drivers/net/nfp/nfd3/nfp_nfd3.h               |    4 +-
 drivers/net/nfp/nfp_rxtx.h                    |    8 +-
 drivers/net/nfp/nfpcore/nfp_nsp.c             |    4 +-
 drivers/net/ntnic/dbsconfig/ntnic_dbsconfig.c |   12 +-
 drivers/net/octeon_ep/otx_ep_mbox.h           |    3 +-
 drivers/net/octeontx/base/octeontx_pki_var.h  |    4 +-
 drivers/net/pfe/pfe_hif.h                     |    4 +-
 drivers/net/virtio/virtio.h                   |    4 +-
 drivers/net/virtio/virtio_cvq.h               |    8 +-
 drivers/net/virtio/virtio_user/vhost_user.c   |    4 +-
 drivers/net/zxdh/zxdh_common.c                |    8 +-
 drivers/net/zxdh/zxdh_msg.h                   |   16 +-
 drivers/net/zxdh/zxdh_pci.h                   |    4 +-
 drivers/net/zxdh/zxdh_queue.h                 |   64 +-
 drivers/net/zxdh/zxdh_rxtx.h                  |    8 +-
 drivers/raw/ifpga/afu_pmd_n3000.h             |    8 +-
 drivers/raw/ifpga/base/opae_hw_api.h          |    4 +-
 drivers/regex/cn9k/cn9k_regexdev.c            |    4 +-
 drivers/regex/mlx5/mlx5_rxp.h                 |   16 +-
 drivers/vdpa/ifc/base/ifcvf.h                 |    4 +-
 drivers/vdpa/mlx5/mlx5_vdpa.h                 |    4 +-
 examples/common/neon/port_group.h             |    4 +-
 examples/ip_pipeline/cli.c                    |   10 +-
 examples/ipsec-secgw/ipsec.h                  |    3 +-
 examples/l3fwd-power/main.c                   |    6 +-
 examples/l3fwd/l3fwd_route.h                  |    6 +-
 examples/ptpclient/ptpclient.c                |   24 +-
 examples/vhost_blk/blk_spec.h                 |    3 +-
 lib/eal/common/eal_private.h                  |    2 +-
 lib/eal/include/rte_common.h                  |    6 +-
 lib/eal/include/rte_memory.h                  |    3 +-
 lib/eal/include/rte_memzone.h                 |    3 +-
 lib/eal/include/rte_trace_point.h             |    2 +-
 lib/eal/x86/include/rte_memcpy.h              |    9 +-
 lib/ipsec/crypto.h                            |   44 +-
 lib/net/rte_arp.h                             |    6 +-
 lib/net/rte_dtls.h                            |    3 +-
 lib/net/rte_esp.h                             |    6 +-
 lib/net/rte_geneve.h                          |    3 +-
 lib/net/rte_gre.h                             |   12 +-
 lib/net/rte_gtp.h                             |   15 +-
 lib/net/rte_ib.h                              |    3 +-
 lib/net/rte_icmp.h                            |    9 +-
 lib/net/rte_ip4.h                             |    3 +-
 lib/net/rte_ip6.h                             |   12 +-
 lib/net/rte_l2tpv2.h                          |   12 +-
 lib/net/rte_macsec.h                          |    6 +-
 lib/net/rte_mpls.h                            |    3 +-
 lib/net/rte_pdcp_hdr.h                        |   12 +-
 lib/net/rte_ppp.h                             |    3 +-
 lib/net/rte_sctp.h                            |    3 +-
 lib/net/rte_tcp.h                             |    3 +-
 lib/net/rte_tls.h                             |    3 +-
 lib/net/rte_udp.h                             |    3 +-
 lib/net/rte_vxlan.h                           |   21 +-
 lib/pipeline/rte_table_action.c               |   64 +-
 lib/vhost/vhost_user.h                        |    8 +-
 148 files changed, 2926 insertions(+), 2759 deletions(-)

--
2.34.1
  

Comments

David Marchand Dec. 23, 2024, 11:03 a.m. UTC | #1
Hello Andre,

On Wed, Nov 27, 2024 at 1:53 AM Andre Muezerie
<andremue@linux.microsoft.com> wrote:
>
> MSVC struct packing is not compatible with GCC. Provide a macro
> (__rte_packed_begin) that can be used to push existing pack value
> and sets packing to 1-byte. The existing __rte_packed macro is
> replaced with __rte_packed_end and restores the pack value
> prior to the push.
>
> Instead of providing macros exclusively for MSVC and for GCC,
> macro __rte_packed_end is deliberately utilized to trigger a
> MSVC compiler warning if no existing packing has been pushed allowing
> easy identification of locations where the __rte_packed_begin is
> missing.
>
> Macro __rte_packed is removed and the two new macros represent the
> new way to enable packing in the DPDK code.
>
> Script checkpatches.sh was enhanced to ensure __rte_packed_begin and
> __rte_packed_end show up in pairs when checking patches.
>
> If as a part of review maintainers identify structs they believe
> don't require packing so long as they are explicitly identified
> I'll remove the __rte_packed as a part of this series.
>
> v6:
>   * replace __rte_msvc_pack with __rte_packed_begin
>   * replace __rte_packed with __rte_packed_end
>   * update checkpatches.sh to ensure __rte_packed_begin and
>     __rte_packed_end are used in pairs

I had mentionned this in a separate thread.
Why not do like OVS and have a RTE_PACKED() macro?

#ifdef RTE_TOOLCHAIN_MSVC
#define RTE_PACKED(...) __pragma(pack(push, 1)) __VA_ARGS__ __pragma(pack(pop))
#else
#define RTE_PACKED(...) __VA_ARGS__ __attribute__((__packed__))
#endif

This removes the need for updating checkpatch.
Plus, builds on Linux will catch issues (hopefully by the author of
the change, before submitting).


>   * remove __rte_packed

Please mark it deprecated for now (see RTE_DEPRECATED / add a
deprecation notice) and we will remove it in 25.11.
  
David Marchand Dec. 23, 2024, 11:46 a.m. UTC | #2
On Mon, Dec 23, 2024 at 12:03 PM David Marchand
<david.marchand@redhat.com> wrote:
> > v6:
> >   * replace __rte_msvc_pack with __rte_packed_begin
> >   * replace __rte_packed with __rte_packed_end
> >   * update checkpatches.sh to ensure __rte_packed_begin and
> >     __rte_packed_end are used in pairs
>
> I had mentionned this in a separate thread.
> Why not do like OVS and have a RTE_PACKED() macro?
>
> #ifdef RTE_TOOLCHAIN_MSVC
> #define RTE_PACKED(...) __pragma(pack(push, 1)) __VA_ARGS__ __pragma(pack(pop))
> #else
> #define RTE_PACKED(...) __VA_ARGS__ __attribute__((__packed__))
> #endif

Mm, in practice, this would be problematic with struct where
endianness matters (for example).

In file included from ../../../git/pub/dpdk.org/main/lib/net/rte_ip.h:9,
                 from ../../../git/pub/dpdk.org/main/lib/ethdev/rte_flow.h:25,
                 from
../../../git/pub/dpdk.org/main/lib/ethdev/rte_eth_ctrl.h:11,
                 from
../../../git/pub/dpdk.org/main/lib/ethdev/rte_ethdev.h:1472,
                 from
../../../git/pub/dpdk.org/main/lib/ethdev/ethdev_driver.h:21,
                 from
../../../git/pub/dpdk.org/main/drivers/net/failsafe/failsafe_ops.c:15:
../../../git/pub/dpdk.org/main/lib/net/rte_ip4.h:48:1: error:
embedding a directive within macro arguments is not portable [-Werror]
   48 | #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
      | ^
../../../git/pub/dpdk.org/main/lib/net/rte_ip4.h:51:1: error:
embedding a directive within macro arguments is not portable [-Werror]
   51 | #elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
      | ^
../../../git/pub/dpdk.org/main/lib/net/rte_ip4.h:54:1: error:
embedding a directive within macro arguments is not portable [-Werror]
   54 | #endif
      | ^


>
> This removes the need for updating checkpatch.
> Plus, builds on Linux will catch issues (hopefully by the author of
> the change, before submitting).
>
>
> >   * remove __rte_packed
>
> Please mark it deprecated for now (see RTE_DEPRECATED / add a
> deprecation notice) and we will remove it in 25.11.

Still, please rework this part.
  
Andre Muezerie Dec. 23, 2024, 7:11 p.m. UTC | #3
On Mon, Dec 23, 2024 at 12:46:34PM +0100, David Marchand wrote:
> On Mon, Dec 23, 2024 at 12:03 PM David Marchand
> <david.marchand@redhat.com> wrote:
> > > v6:
> > >   * replace __rte_msvc_pack with __rte_packed_begin
> > >   * replace __rte_packed with __rte_packed_end
> > >   * update checkpatches.sh to ensure __rte_packed_begin and
> > >     __rte_packed_end are used in pairs
> >
> > I had mentionned this in a separate thread.
> > Why not do like OVS and have a RTE_PACKED() macro?
> >
> > #ifdef RTE_TOOLCHAIN_MSVC
> > #define RTE_PACKED(...) __pragma(pack(push, 1)) __VA_ARGS__ __pragma(pack(pop))
> > #else
> > #define RTE_PACKED(...) __VA_ARGS__ __attribute__((__packed__))
> > #endif
> 
> Mm, in practice, this would be problematic with struct where
> endianness matters (for example).
> 
> In file included from ../../../git/pub/dpdk.org/main/lib/net/rte_ip.h:9,
>                  from ../../../git/pub/dpdk.org/main/lib/ethdev/rte_flow.h:25,
>                  from
> ../../../git/pub/dpdk.org/main/lib/ethdev/rte_eth_ctrl.h:11,
>                  from
> ../../../git/pub/dpdk.org/main/lib/ethdev/rte_ethdev.h:1472,
>                  from
> ../../../git/pub/dpdk.org/main/lib/ethdev/ethdev_driver.h:21,
>                  from
> ../../../git/pub/dpdk.org/main/drivers/net/failsafe/failsafe_ops.c:15:
> ../../../git/pub/dpdk.org/main/lib/net/rte_ip4.h:48:1: error:
> embedding a directive within macro arguments is not portable [-Werror]
>    48 | #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
>       | ^
> ../../../git/pub/dpdk.org/main/lib/net/rte_ip4.h:51:1: error:
> embedding a directive within macro arguments is not portable [-Werror]
>    51 | #elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
>       | ^
> ../../../git/pub/dpdk.org/main/lib/net/rte_ip4.h:54:1: error:
> embedding a directive within macro arguments is not portable [-Werror]
>    54 | #endif
>       | ^
> 
> 
> >
> > This removes the need for updating checkpatch.
> > Plus, builds on Linux will catch issues (hopefully by the author of
> > the change, before submitting).
> >
> >
> > >   * remove __rte_packed
> >
> > Please mark it deprecated for now (see RTE_DEPRECATED / add a
> > deprecation notice) and we will remove it in 25.11.
> 
> Still, please rework this part.
> 
> 
> -- 
> David Marchand

Yes, I'll send out an updated series shortly.
--
Andre Muezerie