| Message ID | 20200520070415.3392210-15-jeffrey.t.kirsher@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Intel RDMA Driver Updates 2020-05-19 | expand |
On 20/05/2020 10:04, Jeff Kirsher wrote: > +struct i40iw_create_qp_resp { > + __u32 qp_id; > + __u32 actual_sq_size; > + __u32 actual_rq_size; > + __u32 i40iw_drv_opt; > + __u16 push_idx; > + __u8 lsmm; > + __u8 rsvd; > +}; This struct size should be 8 bytes aligned.
On Wed, May 20, 2020 at 10:54:25AM +0300, Gal Pressman wrote: > On 20/05/2020 10:04, Jeff Kirsher wrote: > > +struct i40iw_create_qp_resp { > > + __u32 qp_id; > > + __u32 actual_sq_size; > > + __u32 actual_rq_size; > > + __u32 i40iw_drv_opt; > > + __u16 push_idx; > > + __u8 lsmm; > > + __u8 rsvd; > > +}; > > This struct size should be 8 bytes aligned. Aligned in what way? Seems sane to me, what would you want it to look like instead? thanks, greg k-h
On 20/05/2020 11:52, Greg KH wrote: > On Wed, May 20, 2020 at 10:54:25AM +0300, Gal Pressman wrote: >> On 20/05/2020 10:04, Jeff Kirsher wrote: >>> +struct i40iw_create_qp_resp { >>> + __u32 qp_id; >>> + __u32 actual_sq_size; >>> + __u32 actual_rq_size; >>> + __u32 i40iw_drv_opt; >>> + __u16 push_idx; >>> + __u8 lsmm; >>> + __u8 rsvd; >>> +}; >> >> This struct size should be 8 bytes aligned. > > Aligned in what way? Seems sane to me, what would you want it to look > like instead? The uverbs ABI structs sizes are assumed to be padded to 8 bytes alignment, I would expect the reserved field to be an array of 5 bytes as done in other structs in this file (irdma_modify_qp_req for example). Jason could correct me if I'm wrong?
On Wed, May 20, 2020 at 12:02:35PM +0300, Gal Pressman wrote: > On 20/05/2020 11:52, Greg KH wrote: > > On Wed, May 20, 2020 at 10:54:25AM +0300, Gal Pressman wrote: > >> On 20/05/2020 10:04, Jeff Kirsher wrote: > >>> +struct i40iw_create_qp_resp { > >>> + __u32 qp_id; > >>> + __u32 actual_sq_size; > >>> + __u32 actual_rq_size; > >>> + __u32 i40iw_drv_opt; > >>> + __u16 push_idx; > >>> + __u8 lsmm; > >>> + __u8 rsvd; > >>> +}; > >> > >> This struct size should be 8 bytes aligned. > > > > Aligned in what way? Seems sane to me, what would you want it to look > > like instead? > > The uverbs ABI structs sizes are assumed to be padded to 8 bytes alignment, I > would expect the reserved field to be an array of 5 bytes as done in other > structs in this file (irdma_modify_qp_req for example). > Jason could correct me if I'm wrong? "it is complicated" The udata structs must have alignment that is compatible with the core struct that prefixes them. Of course we have a mess here, and nothing is uniform.. In this case struct ib_uverbs_create_qp_resp has a '__u32 driver_data[0]' aligned to 8 bytes thus the alignment of this struct can be 4 or 8. I generally don't recommend relying on this weird side effect, and encourage explicit padding when possible, but since the intent of this new driver is to be ABI compatible with the old driver, it should be kept the same. The userspace has a number of static_asserts which are designed to automatically check these various cases. I assume Intel has revised the userspace to use the new struct names and tested it.. Jason
> Subject: Re: [RDMA RFC v6 14/16] RDMA/irdma: Add ABI definitions > > On Wed, May 20, 2020 at 12:02:35PM +0300, Gal Pressman wrote: > > On 20/05/2020 11:52, Greg KH wrote: > > > On Wed, May 20, 2020 at 10:54:25AM +0300, Gal Pressman wrote: > > >> On 20/05/2020 10:04, Jeff Kirsher wrote: > > >>> +struct i40iw_create_qp_resp { > > >>> + __u32 qp_id; > > >>> + __u32 actual_sq_size; > > >>> + __u32 actual_rq_size; > > >>> + __u32 i40iw_drv_opt; > > >>> + __u16 push_idx; > > >>> + __u8 lsmm; > > >>> + __u8 rsvd; > > >>> +}; > > >> > > >> This struct size should be 8 bytes aligned. > > > > > > Aligned in what way? Seems sane to me, what would you want it to > > > look like instead? > > > > The uverbs ABI structs sizes are assumed to be padded to 8 bytes > > alignment, I would expect the reserved field to be an array of 5 bytes > > as done in other structs in this file (irdma_modify_qp_req for example). > > Jason could correct me if I'm wrong? > > "it is complicated" > > The udata structs must have alignment that is compatible with the core struct that > prefixes them. Of course we have a mess here, and nothing is uniform.. > > In this case struct ib_uverbs_create_qp_resp has a '__u32 driver_data[0]' aligned > to 8 bytes thus the alignment of this struct can be 4 or 8. > > I generally don't recommend relying on this weird side effect, and encourage > explicit padding when possible, but since the intent of this new driver is to be ABI > compatible with the old driver, it should be kept the same. > > The userspace has a number of static_asserts which are designed to automatically > check these various cases. I assume Intel has revised the userspace to use the > new struct names and tested it.. > Thanks Jason for the explanation! Yes these abi structs are kept the same for old user-space compatibility. And yes its been tested with old user-space.
diff --git a/include/uapi/rdma/irdma-abi.h b/include/uapi/rdma/irdma-abi.h new file mode 100644 index 000000000000..2eb253220161 --- /dev/null +++ b/include/uapi/rdma/irdma-abi.h @@ -0,0 +1,140 @@ +/* SPDX-License-Identifier: (GPL-2.0 WITH Linux-syscall-note) OR Linux-OpenIB) */ +/* + * Copyright (c) 2006 - 2019 Intel Corporation. All rights reserved. + * Copyright (c) 2005 Topspin Communications. All rights reserved. + * Copyright (c) 2005 Cisco Systems. All rights reserved. + * Copyright (c) 2005 Open Grid Computing, Inc. All rights reserved. + */ + +#ifndef IRDMA_ABI_H +#define IRDMA_ABI_H + +#include <linux/types.h> + +/* irdma must support legacy GEN_1 i40iw kernel + * and user-space whose last ABI ver is 5 + */ +#define IRDMA_ABI_VER 6 + +enum irdma_memreg_type { + IW_MEMREG_TYPE_MEM = 0, + IW_MEMREG_TYPE_QP = 1, + IW_MEMREG_TYPE_CQ = 2, + IW_MEMREG_TYPE_RSVD = 3, + IW_MEMREG_TYPE_MW = 4, +}; + +struct irdma_alloc_ucontext_req { + __u32 rsvd32; + __u8 userspace_ver; + __u8 rsvd8[3]; +}; + +struct i40iw_alloc_ucontext_req { + __u32 rsvd32; + __u8 userspace_ver; + __u8 rsvd8[3]; +}; + +struct irdma_alloc_ucontext_resp { + __aligned_u64 feature_flags; + __aligned_u64 db_mmap_key; + __u32 max_hw_wq_frags; + __u32 max_hw_read_sges; + __u32 max_hw_inline; + __u32 max_hw_rq_quanta; + __u32 max_hw_wq_quanta; + __u32 min_hw_cq_size; + __u32 max_hw_cq_size; + __u32 rsvd1[7]; + __u16 max_hw_sq_chunk; + __u16 rsvd2[11]; + __u8 kernel_ver; + __u8 hw_rev; + __u8 rsvd3[6]; +}; + +struct i40iw_alloc_ucontext_resp { + __u32 max_pds; + __u32 max_qps; + __u32 wq_size; /* size of the WQs (SQ+RQ) in the mmaped area */ + __u8 kernel_ver; + __u8 rsvd[3]; +}; + +struct irdma_alloc_pd_resp { + __u32 pd_id; + __u8 rsvd[4]; +}; + +struct irdma_resize_cq_req { + __aligned_u64 user_cq_buffer; +}; + +struct irdma_create_cq_req { + __aligned_u64 user_cq_buf; + __aligned_u64 user_shadow_area; +}; + +struct irdma_create_qp_req { + __aligned_u64 user_wqe_bufs; + __aligned_u64 user_compl_ctx; +}; + +struct i40iw_create_qp_req { + __aligned_u64 user_wqe_bufs; + __aligned_u64 user_compl_ctx; +}; + +struct irdma_mem_reg_req { + __u16 reg_type; /* Memory, QP or CQ */ + __u16 cq_pages; + __u16 rq_pages; + __u16 sq_pages; +}; + +struct irdma_modify_qp_req { + __u8 sq_flush; + __u8 rq_flush; + __u8 rsvd[6]; +}; + +struct irdma_create_cq_resp { + __u32 cq_id; + __u32 cq_size; +}; + +struct irdma_create_qp_resp { + __u32 qp_id; + __u32 actual_sq_size; + __u32 actual_rq_size; + __u32 irdma_drv_opt; + __u32 qp_caps; + __u16 rsvd1; + __u8 lsmm; + __u8 rsvd2; +}; + +struct i40iw_create_qp_resp { + __u32 qp_id; + __u32 actual_sq_size; + __u32 actual_rq_size; + __u32 i40iw_drv_opt; + __u16 push_idx; + __u8 lsmm; + __u8 rsvd; +}; + +struct irdma_modify_qp_resp { + __aligned_u64 push_wqe_mmap_key; + __aligned_u64 push_db_mmap_key; + __u16 push_offset; + __u8 push_valid; + __u8 rsvd[5]; +}; + +struct irdma_create_ah_resp { + __u32 ah_id; + __u8 rsvd[4]; +}; +#endif /* IRDMA_ABI_H */