On Wed, Jun 04, 2025 at 05:28:57PM +0200, Dariusz Sosnowski wrote:
> Hi,
>
> On Fri, May 30, 2025 at 05:21:57PM -0700, Andre Muezerie wrote:
> > When compiling with MSVC the error below is hit:
> >
> > drivers\net\mlx5\mlx5_tx.h(1148): error C2065: 'rte_v128u32_t':
> > undeclared identifier
> >
> > Turns out that with MSVC the data type rte_v128u32_t is not used, but
> > its size needs to be known. This patch defines a macro to store that
> > size and replaces instances of sizeof(rte_v128u32_t) with that macro.
> >
> > A static assert is added to ensure the new macro never diverges from
> > sizeof(rte_v128u32_t).
> >
> > Signed-off-by: Andre Muezerie <andre...@linux.microsoft.com>
> > ---
> > drivers/net/mlx5/mlx5_defs.h | 2 ++
> > drivers/net/mlx5/mlx5_rxtx.c | 11 ++++++++---
> > drivers/net/mlx5/mlx5_tx.h | 2 +-
> > 3 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
> > index 9c454983be..c5e2c59309 100644
> > --- a/drivers/net/mlx5/mlx5_defs.h
> > +++ b/drivers/net/mlx5/mlx5_defs.h
> > @@ -196,4 +196,6 @@
> >
> > #define MLX5_CNT_SVC_CYCLE_TIME_DEFAULT 500
> >
> > +#define MLX5_SIZEOF_V128U32_T 16
> > +
> > #endif /* RTE_PMD_MLX5_DEFS_H_ */
> > diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
> > index 9c075f6a56..3f4942e225 100644
> > --- a/drivers/net/mlx5/mlx5_rxtx.c
> > +++ b/drivers/net/mlx5/mlx5_rxtx.c
> > @@ -28,11 +28,16 @@
> > #include "mlx5_tx.h"
> >
> > /* static asserts */
> > +#ifndef RTE_TOOLCHAIN_MSVC
> > +static_assert(sizeof(rte_v128u32_t) == MLX5_SIZEOF_V128U32_T,
> > + "MLX5_SIZEOF_V128U32_T must hold sizeof(rte_v128u32_t)");
> > +#endif
>
> Thank you for the patch.
> This proposal is ok, but I think it can be slightly improved.
>
> The reference to rte_v128u32_t (in code and in names) can be removed, because
> the code which relies on MLX5_ESEG_MIN_INLINE_SIZE does not really use
> rte_v128u32_t type. The relevant code e.g., in mlx5_tx_eseg_dmin(),
> always copies first 2 bytes of the packet (through uint16_t pointer,
> these would be first 2 bytes of destination MAC address)
> and then does one of the two:
>
> 1. Copies following 16 bytes using rte_mov16().
> (Corresponds to first static_assert for MLX5_ESEG_MIN_INLINE_SIZE).
> 2. If there is a VLAN defined in mbuf, then:
> - the rest of destination MAC address is copied,
> - source MAC address is copied,
> - VLAN is inserted,
> - 2 bytes appearing after VLAN header are copied.
> (Corresponds to 2nd static_assert for MLX5_ESEG_MIN_INLINE_SIZE).
>
> Amount of data copied is inferred from HW arch and packet descriptor
> layout, not rte_v128u32_t.
>
> Since rte_mov16() is used in real code and rte_v128u32_t is only used to
> carry over the length of the copy, I would propose to:
>
> - Replace instances of sizeof(rte_v128u32_t) with a macro named
> MLX5_SIZE_MOV16
> to better reflect the real usage in mlx5 PMD.
> - Remove static_assert for
> "sizeof(rte_v128u32_t) == MLX5_SIZEOF_V128U32_T".
>
> Would you be able to apply these changes?
Yes, I'll send a new patch with the changes suggested.
Thanks,
Andre Muezerie
>
> > +
> > static_assert(MLX5_CQE_STATUS_HW_OWN < 0, "Must be negative value");
> > static_assert(MLX5_CQE_STATUS_SW_OWN < 0, "Must be negative value");
> > static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
> > (sizeof(uint16_t) +
> > - sizeof(rte_v128u32_t)),
> > + MLX5_SIZEOF_V128U32_T),
> > "invalid Ethernet Segment data size");
> > static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
> > (sizeof(uint16_t) +
> > @@ -41,7 +46,7 @@ static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
> > "invalid Ethernet Segment data size");
> > static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
> > (sizeof(uint16_t) +
> > - sizeof(rte_v128u32_t)),
> > + MLX5_SIZEOF_V128U32_T),
> > "invalid Ethernet Segment data size");
> > static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
> > (sizeof(uint16_t) +
> > @@ -50,7 +55,7 @@ static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
> > "invalid Ethernet Segment data size");
> > static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
> > (sizeof(uint16_t) +
> > - sizeof(rte_v128u32_t)),
> > + MLX5_SIZEOF_V128U32_T),
> > "invalid Ethernet Segment data size");
> > static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
> > (sizeof(uint16_t) +
> > diff --git a/drivers/net/mlx5/mlx5_tx.h b/drivers/net/mlx5/mlx5_tx.h
> > index 55568c41b1..5647f6a37d 100644
> > --- a/drivers/net/mlx5/mlx5_tx.h
> > +++ b/drivers/net/mlx5/mlx5_tx.h
> > @@ -1145,7 +1145,7 @@ mlx5_tx_eseg_data(struct mlx5_txq_data
> > *__rte_restrict txq,
> > } else {
> > /* Fill the gap in the title WQEBB with inline data. */
> > rte_mov16(pdst, psrc);
> > - psrc += sizeof(rte_v128u32_t);
> > + psrc += MLX5_SIZEOF_V128U32_T;
> > }
> > pdst = (uint8_t *)(es + 2);
> > MLX5_ASSERT(inlen >= MLX5_ESEG_MIN_INLINE_SIZE);
> > --
> > 2.49.0.vfs.0.3
> >
>
> Best regards,
> Dariusz Sosnowski
