[ofa-general] Re: [PATCH 2/2] IB/mlx4_ib: fix SRQ buffer allocation

[Eli Cohen]

On Thu, 2007-06-07 at 11:59 -0700, Roland Dreier wrote:
> Thanks... I reworked this a lot and right now I plan to push the
> following (although I'm still testing):
> 
>  
>  static int set_rq_size(struct mlx4_ib_dev *dev, struct ib_qp_cap *cap,
> -		       struct mlx4_ib_qp *qp)
> +		       int is_user, int has_srq, struct mlx4_ib_qp *qp)
>  {
>  	/* Sanity check RQ size before proceeding */
>  	if (cap->max_recv_wr  > dev->dev->caps.max_wqes  ||
>  	    cap->max_recv_sge > dev->dev->caps.max_rq_sg)
>  		return -EINVAL;
>  
> -	qp->rq.max = cap->max_recv_wr ? roundup_pow_of_two(cap->max_recv_wr) : 0;
> +	if (has_srq) {
> +		/* QPs attached to an SRQ should have no RQ */
> +		if (cap->max_recv_wr)
> +			return -EINVAL;
>  
> -	qp->rq.wqe_shift = ilog2(roundup_pow_of_two(cap->max_recv_sge *
> -						    sizeof (struct mlx4_wqe_data_seg)));
> -	qp->rq.max_gs    = (1 << qp->rq.wqe_shift) / sizeof (struct mlx4_wqe_data_seg);
> +		qp->rq.max = qp->rq.max_gs = 0;
> +	} else {
> +		/* HW requires >= 1 RQ entry with >= 1 gather entry */
> +		if (is_user && (!cap->max_recv_wr || !cap->max_recv_sge))
> +			return -EINVAL;

I think we may have a problem here: if a user, not being aware of the HW
requirement of none zero length receive queue, creates a QP with zero in
cap->max_recv_sge, the above kernel code will cause a failure since
libmlx4 does not fix the value in this field. So I think this should be
taken care of in libmlx4.



Moreover, I see you did not take the following:

@@ -302,6 +315,10 @@ static int set_kernel_sq_size(struct mlx
 static int set_user_sq_size(struct mlx4_ib_qp *qp,
                            struct mlx4_ib_create_qp *ucmd)
 {
+       /* Sanity check for SQ size */
+       if (ucmd->log_sq_bb_count > 15 || ucmd->log_sq_stride > 11)
+               return -EINVAL;
+

Shouldn't we use a condition like this to prevent misconfiguration of
the QP if libmlx4 passes improper values?