On Wed, Mar 21, 2018 at 04:38:45PM +0200, Matan Barak wrote:
> On 21/03/2018 03:44, Jason Gunthorpe wrote:
> >On Tue, Mar 20, 2018 at 03:01:36PM +0200, Leon Romanovsky wrote:
> >
> >>+struct ib_uverbs_flow_action_esp_encap {
> >>+ /* This struct represents a list of pointers to flow_xxxx_filter that
> >>+ * encapsulates the payload in ESP tunnel mode.
> >>+ */
> >>+ RDMA_UAPI_PTR(void *, val_ptr); /* pointer to a flow_xxxx_filter */
> >>+ RDMA_UAPI_PTR(struct ib_uverbs_flow_action_esp_encap *, next_ptr);
> >>+ __u16 len; /* Len of the filter struct val_ptr points to */
> >>+ __u16 type; /* Use flow_spec_type enum */
> >>+};
> >
> >Did you test this Matan? It doesn't work the way you think:
> >
> >#include <linux/types.h>
> >#include <assert.h>
> >
> >#define RDMA_UAPI_PTR(_type, _name) _type __attribute__((aligned(8))) _name
> >
> >struct ib_uverbs_flow_action_esp_encap {
> > /* This struct represents a list of pointers to flow_xxxx_filter that
> > * encapsulates the payload in ESP tunnel mode.
> > */
> > RDMA_UAPI_PTR(void *, val_ptr); /* pointer to a flow_xxxx_filter */
> > RDMA_UAPI_PTR(struct ib_uverbs_flow_action_esp_encap *, next_ptr);
> > __u16 len; /* Len of the filter struct val_ptr points to */
> > __u16 type; /* Use flow_spec_type enum */
> >};
> >
> >static_assert(sizeof(struct ib_uverbs_flow_action_esp_encap) == 8*3, "Bad size");
> >static_assert(__alignof__(struct ib_uverbs_flow_action_esp_encap) == 8, "Bad Align");
> >
> >$ gcc -std=c11 -Wall -c /tmp/t.c -m32
> >In file included from /tmp/t.c:2:0:
> >/tmp/t.c:16:1: error: static assertion failed: "Bad size"
> > static_assert(sizeof(struct ib_uverbs_flow_action_esp_encap) == 8*3, "Bad size");
> >
> >$ pahole32 t.o
> >struct ib_uverbs_flow_action_esp_encap {
> > void * val_ptr; /* 0 4 */
> >
> > /* XXX 4 bytes hole, try to pack */
> >
> > struct ib_uverbs_flow_action_esp_encap * next_ptr; /* 8 4 */
> > __u16 len; /* 12 2 */
> > __u16 type; /* 14 2 */
> >
> > /* size: 16, cachelines: 1, members: 4 */
> > /* sum members: 12, holes: 1, sum holes: 4 */
> > /* last cacheline: 16 bytes */
> >};
> >
> >And why is a pointer in a user struct?
> >
> >The kernel default for RDMA_UAPI_PTR() should simply be
> >
> > #define RDMA_UAPI_PTR(_type, _name) __aligned_u64 _name
> >
> >Because we always have to use u64_to_user_ptr() anyhow.
> >
> >Userspace needs to redefine it to
> >
> > #define RDMA_UAPI_PTR(_type, _name) union {_type _name; __aligned_u64 _dummy_#_name;}
> >or
> > #define RDMA_UAPI_PTR(_type, _name) union {struct {__u32 _dummy2_##_name; _type _name; __aligned_u64 _dummy_##_name;}
> >
> >Depending on endian
> >
>
> This is redefined in the user-space code at libibverbs/verbs_api.h by:
> #if UINTPTR_MAX == UINT32_MAX
> #if __BYTE_ORDER == __LITTLE_ENDIAN
> #define RDMA_UAPI_PTR(_type, _name) union { \
> struct {_type _name; \
> __u32 _name ##_reserved;\
> }; \
> __u64 _name ## _data_u64; } \
> __attribute__((aligned(8)))
> #else
> #define RDMA_UAPI_PTR(_type, _name) union { \
> struct {__u32 _name ##_reserved;\
> _type _name; \
> }; \
> __u64 _name ## _data_u64; } \
> __attribute__((aligned(8)))
Should be __aligned_u64, but yes, that is fine.
The 64 bit version should be
union {_type _name; __aligned_u64 _name ## _data_u64; }
Though, just to make sure the alignment is right.
So only the kernel macro is wrong, please fix it to
#define RDMA_UAPI_PTR(_type, _name) __aligned_u64 _name
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
