-----"Jason Gunthorpe" <jgg@xxxxxxxx> wrote: -----
>To: bmt@xxxxxxxxxxxxxx
>From: "Jason Gunthorpe" <jgg@xxxxxxxx>
>Date: 01/30/2019 11:41PM
>Cc: linux-rdma@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH v4 06/13] SIW application interface
>
>On Wed, Jan 30, 2019 at 06:21:29PM +0100, bmt@xxxxxxxxxxxxxx wrote:
>
>> diff --git a/include/uapi/rdma/siw_user.h
>b/include/uapi/rdma/siw_user.h
>> new file mode 100644
>> index 000000000000..25d1005a0b20
>> +++ b/include/uapi/rdma/siw_user.h
>> @@ -0,0 +1,216 @@
>> +/*
>> + * Software iWARP device driver for Linux
>> + *
>> + * Authors: Bernard Metzler <bmt@xxxxxxxxxxxxxx>
>> + *
>> + * Copyright (c) 2008-2017, IBM Corporation
>> + *
>> + * This software is available to you under a choice of one of two
>> + * licenses. You may choose to be licensed under the terms of the
>GNU
>> + * General Public License (GPL) Version 2, available from the file
>> + * COPYING in the main directory of this source tree, or the
>> + * BSD license below:
>> + *
>> + * Redistribution and use in source and binary forms, with or
>> + * without modification, are permitted provided that the
>following
>> + * conditions are met:
>> + *
>> + * - Redistributions of source code must retain the above
>copyright notice,
>> + * this list of conditions and the following disclaimer.
>> + *
>> + * - Redistributions in binary form must reproduce the above
>copyright
>> + * notice, this list of conditions and the following
>disclaimer in the
>> + * documentation and/or other materials provided with the
>distribution.
>> + *
>> + * - Neither the name of IBM nor the names of its contributors
>may be
>> + * used to endorse or promote products derived from this
>software without
>> + * specific prior written permission.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
>OF
>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
>> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
>HOLDERS
>> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
>AN
>> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR
>IN
>> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>THE
>> + * SOFTWARE.
>> + */
>
>I think this is the wrong license for uapi stuff - follow the SPDX
>headers from the other files please
Okay thanks.
>
>> +
>> +#ifndef _SIW_USER_H
>> +#define _SIW_USER_H
>> +
>> +#include <linux/types.h>
>> +
>> +/*Common string that is matched to accept the device by the user
>library*/
>> +#define SIW_NODE_DESC_COMMON "Software iWARP stack"
>> +
>> +#define SIW_IBDEV_PREFIX "siw_"
>> +
>> +#define VERSION_ID_SOFTIWARP 2
>> +
>> +#define SIW_MAX_SGE 6
>> +#define SIW_MAX_UOBJ_KEY 0xffffff
>> +#define SIW_INVAL_UOBJ_KEY (SIW_MAX_UOBJ_KEY + 1)
>> +
>> +struct siw_uresp_create_cq {
>> + __u32 cq_id;
>> + __u32 num_cqe;
>> + __u32 cq_key;
>> +};
>
>All command structs should be a multiple of 8 bytes.. Otherwise they
>cannot be used on the _ex interface side.
>
Thanks,
will do.
>> +
>> +struct siw_uresp_create_qp {
>> + __u32 qp_id;
>> + __u32 num_sqe;
>> + __u32 num_rqe;
>> + __u32 sq_key;
>> + __u32 rq_key;
>> +};
>> +
>> +struct siw_ureq_reg_mr {
>> + __u8 stag_key;
>> + __u8 reserved[3];
>> +};
>> +
>> +struct siw_uresp_reg_mr {
>> + __u32 stag;
>> +};
>> +
>> +struct siw_uresp_create_srq {
>> + __u32 num_rqe;
>> + __u32 srq_key;
>> +};
>> +
>> +struct siw_uresp_alloc_ctx {
>> + __u32 dev_id;
>> +};
>> +
>> +enum siw_opcode {
>> + SIW_OP_WRITE = 0,
>> + SIW_OP_READ = 1,
>> + SIW_OP_READ_LOCAL_INV = 2,
>> + SIW_OP_SEND = 3,
>> + SIW_OP_SEND_WITH_IMM = 4,
>> + SIW_OP_SEND_REMOTE_INV = 5,
>> +
>> + /* Unsupported */
>> + SIW_OP_FETCH_AND_ADD = 6,
>> + SIW_OP_COMP_AND_SWAP = 7,
>> +
>> + SIW_OP_RECEIVE = 8,
>> + /* provider internal SQE */
>> + SIW_OP_READ_RESPONSE = 9,
>> + /*
>> + * below opcodes valid for
>> + * in-kernel clients only
>> + */
>> + SIW_OP_INVAL_STAG = 10,
>> + SIW_OP_REG_MR = 11,
>> + SIW_NUM_OPCODES = 12
>> +};
>> +
>> +/* Keep it same as ibv_sge to allow for memcpy */
>> +struct siw_sge {
>> + __u64 laddr;
>
>__aligned_u64 in all places
>
OK.
>> +struct siw_cqe {
>> + __u64 id;
>> + __u8 flags;
>> + __u8 opcode;
>> + __u16 status;
>> + __u32 bytes;
>> + __u64 imm_data;
>> + /* QP number or QP pointer */
>> + union {
>> + void *qp;
>> + __u64 qp_id;
>
>void * in this header is almost certainly wrong
Depending on application location (user/kernel),
we keep the QP identity of that CQE here. User land
gets a unique QP number in the WC, Kernel user get a
pointer to the QP.
Would that be OK/better?
struct siw_qp;
struct siw_cqe {
__aligned_u64 id;
__u8 flags;
__u8 opcode;
__u16 status;
__u32 bytes;
__aligned_u64 imm_data;
/* QP number or QP pointer */
union {
struct siw_qp *qp;
__aligned_u64 qp_id;
};
};
>
>> + };
>> +};
>> +
>> +/*
>> + * Shared structure between user and kernel
>> + * to control CQ arming.
>> + */
>> +struct siw_cq_ctrl {
>> + enum siw_notify_flags notify;
>
>Do not use 'enum' in any of these structs. Only fixed size memory.
>
Right. Will fix that.
>Is this shared memory?
>
Yes, it is shared between provider and user land library
in case of user land app. This avoids trapping into the kernel
for CQ re-arming.
Thanks!
Bernard.
