Re: [Qemu-devel] [PATCH] hw/pvrdma: Remove max-sge command-line param

[Marcel Apfelbaum <marcel.apfelbaum@gmail.com>]

On 1/9/19 9:41 PM, Yuval Shaia wrote:
> This parameter has no effect, fix it.
>
> The function init_dev_caps sets the front-end's max-sge to MAX_SGE. Then
> it checks backend's max-sge and adjust it accordingly (we can't send
> more than what the device supports).
>
> On send and recv we need to make sure the num_sge in the WQE does not
> exceeds the backend device capability.
> This check is done in pvrdma level so check on rdma level is deleted.

I think it makes sense to match max-sge with the underlying device
capability.

Reviewed-by: Marcel Apfelbaum<marcel.apfelbaum@gmail.com>
Thanks,
Marcel



> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
> ---
>   docs/pvrdma.txt             |  1 -
>   hw/rdma/rdma_backend.c      | 23 ++---------------------
>   hw/rdma/rdma_backend.h      | 11 +++++++++++
>   hw/rdma/vmw/pvrdma_main.c   | 10 +++++-----
>   hw/rdma/vmw/pvrdma_qp_ops.c | 24 ++++++++++++++++++++++++
>   5 files changed, 42 insertions(+), 27 deletions(-)
>
> diff --git a/docs/pvrdma.txt b/docs/pvrdma.txt
> index 5175251b47..5973c0c68b 100644
> --- a/docs/pvrdma.txt
> +++ b/docs/pvrdma.txt
> @@ -153,7 +153,6 @@ Ethernet function can be used for other Ethernet purposes such as IP.
>     specify the port to use. If not set 1 will be used.
>   - dev-caps-max-mr-size: The maximum size of MR.
>   - dev-caps-max-qp:      Maximum number of QPs.
> -- dev-caps-max-sge:     Maximum number of SGE elements in WR.
>   - dev-caps-max-cq:      Maximum number of CQs.
>   - dev-caps-max-mr:      Maximum number of MRs.
>   - dev-caps-max-pd:      Maximum number of PDs.
> diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
> index c28bfbd44d..16dca69ee9 100644
> --- a/hw/rdma/rdma_backend.c
> +++ b/hw/rdma/rdma_backend.c
> @@ -32,17 +32,6 @@
>   #include "rdma_rm.h"
>   #include "rdma_backend.h"
>   
> -/* Vendor Errors */
> -#define VENDOR_ERR_FAIL_BACKEND     0x201
> -#define VENDOR_ERR_TOO_MANY_SGES    0x202
> -#define VENDOR_ERR_NOMEM            0x203
> -#define VENDOR_ERR_QP0              0x204
> -#define VENDOR_ERR_INV_NUM_SGE      0x205
> -#define VENDOR_ERR_MAD_SEND         0x206
> -#define VENDOR_ERR_INVLKEY          0x207
> -#define VENDOR_ERR_MR_SMALL         0x208
> -#define VENDOR_ERR_INV_MAD_BUFF     0x209
> -
>   #define THR_NAME_LEN 16
>   #define THR_POLL_TO  5000
>   
> @@ -475,11 +464,6 @@ void rdma_backend_post_send(RdmaBackendDev *backend_dev,
>       }
>   
>       pr_dbg("num_sge=%d\n", num_sge);
> -    if (!num_sge || num_sge > MAX_SGE) {
> -        pr_dbg("invalid num_sge=%d\n", num_sge);
> -        complete_work(IBV_WC_GENERAL_ERR, VENDOR_ERR_INV_NUM_SGE, ctx);
> -        return;
> -    }
>   
>       bctx = g_malloc0(sizeof(*bctx));
>       bctx->up_ctx = ctx;
> @@ -602,11 +586,6 @@ void rdma_backend_post_recv(RdmaBackendDev *backend_dev,
>       }
>   
>       pr_dbg("num_sge=%d\n", num_sge);
> -    if (!num_sge || num_sge > MAX_SGE) {
> -        pr_dbg("invalid num_sge=%d\n", num_sge);
> -        complete_work(IBV_WC_GENERAL_ERR, VENDOR_ERR_INV_NUM_SGE, ctx);
> -        return;
> -    }
>   
>       bctx = g_malloc0(sizeof(*bctx));
>       bctx->up_ctx = ctx;
> @@ -942,6 +921,8 @@ static int init_device_caps(RdmaBackendDev *backend_dev,
>           return -EIO;
>       }
>   
> +    dev_attr->max_sge = MAX_SGE;
> +
>       CHK_ATTR(dev_attr, backend_dev->dev_attr, max_mr_size, "%" PRId64);
>       CHK_ATTR(dev_attr, backend_dev->dev_attr, max_qp, "%d");
>       CHK_ATTR(dev_attr, backend_dev->dev_attr, max_sge, "%d");
> diff --git a/hw/rdma/rdma_backend.h b/hw/rdma/rdma_backend.h
> index 8cae40f827..a9ba40ae48 100644
> --- a/hw/rdma/rdma_backend.h
> +++ b/hw/rdma/rdma_backend.h
> @@ -22,6 +22,17 @@
>   #include "rdma_rm_defs.h"
>   #include "rdma_backend_defs.h"
>   
> +/* Vendor Errors */
> +#define VENDOR_ERR_FAIL_BACKEND     0x201
> +#define VENDOR_ERR_TOO_MANY_SGES    0x202
> +#define VENDOR_ERR_NOMEM            0x203
> +#define VENDOR_ERR_QP0              0x204
> +#define VENDOR_ERR_INV_NUM_SGE      0x205
> +#define VENDOR_ERR_MAD_SEND         0x206
> +#define VENDOR_ERR_INVLKEY          0x207
> +#define VENDOR_ERR_MR_SMALL         0x208
> +#define VENDOR_ERR_INV_MAD_BUFF     0x209
> +
>   /* Add definition for QP0 and QP1 as there is no userspace enums for them */
>   enum ibv_special_qp_type {
>       IBV_QPT_SMI = 0,
> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> index 838ad8a949..d2bdb5ba8c 100644
> --- a/hw/rdma/vmw/pvrdma_main.c
> +++ b/hw/rdma/vmw/pvrdma_main.c
> @@ -43,7 +43,6 @@ static Property pvrdma_dev_properties[] = {
>       DEFINE_PROP_UINT64("dev-caps-max-mr-size", PVRDMADev, dev_attr.max_mr_size,
>                          MAX_MR_SIZE),
>       DEFINE_PROP_INT32("dev-caps-max-qp", PVRDMADev, dev_attr.max_qp, MAX_QP),
> -    DEFINE_PROP_INT32("dev-caps-max-sge", PVRDMADev, dev_attr.max_sge, MAX_SGE),
>       DEFINE_PROP_INT32("dev-caps-max-cq", PVRDMADev, dev_attr.max_cq, MAX_CQ),
>       DEFINE_PROP_INT32("dev-caps-max-mr", PVRDMADev, dev_attr.max_mr, MAX_MR),
>       DEFINE_PROP_INT32("dev-caps-max-pd", PVRDMADev, dev_attr.max_pd, MAX_PD),
> @@ -549,8 +548,9 @@ static void init_dev_caps(PVRDMADev *dev)
>                          sizeof(struct pvrdma_rq_wqe_hdr));
>   
>       dev->dev_attr.max_qp_wr = pg_tbl_bytes /
> -                              (wr_sz + sizeof(struct pvrdma_sge) * MAX_SGE) -
> -                              TARGET_PAGE_SIZE; /* First page is ring state */
> +                              (wr_sz + sizeof(struct pvrdma_sge) *
> +                              dev->dev_attr.max_sge) - TARGET_PAGE_SIZE;
> +                              /* First page is ring state  ^^^^ */
>       pr_dbg("max_qp_wr=%d\n", dev->dev_attr.max_qp_wr);
>   
>       dev->dev_attr.max_cqe = pg_tbl_bytes / sizeof(struct pvrdma_cqe) -
> @@ -626,8 +626,6 @@ static void pvrdma_realize(PCIDevice *pdev, Error **errp)
>   
>       init_regs(pdev);
>   
> -    init_dev_caps(dev);
> -
>       rc = init_msix(pdev, errp);
>       if (rc) {
>           goto out;
> @@ -640,6 +638,8 @@ static void pvrdma_realize(PCIDevice *pdev, Error **errp)
>           goto out;
>       }
>   
> +    init_dev_caps(dev);
> +
>       rc = rdma_rm_init(&dev->rdma_dev_res, &dev->dev_attr, errp);
>       if (rc) {
>           goto out;
> diff --git a/hw/rdma/vmw/pvrdma_qp_ops.c b/hw/rdma/vmw/pvrdma_qp_ops.c
> index 300471a4c9..465bee8641 100644
> --- a/hw/rdma/vmw/pvrdma_qp_ops.c
> +++ b/hw/rdma/vmw/pvrdma_qp_ops.c
> @@ -121,6 +121,16 @@ static void pvrdma_qp_ops_comp_handler(void *ctx, struct ibv_wc *wc)
>       g_free(ctx);
>   }
>   
> +static void complete_with_error(uint32_t vendor_err, void *ctx)
> +{
> +    struct ibv_wc wc = {0};
> +
> +    wc.status = IBV_WC_GENERAL_ERR;
> +    wc.vendor_err = vendor_err;
> +
> +    pvrdma_qp_ops_comp_handler(ctx, &wc);
> +}
> +
>   void pvrdma_qp_ops_fini(void)
>   {
>       rdma_backend_unregister_comp_handler();
> @@ -182,6 +192,13 @@ int pvrdma_qp_send(PVRDMADev *dev, uint32_t qp_handle)
>               return -EIO;
>           }
>   
> +        if (wqe->hdr.num_sge > dev->dev_attr.max_sge) {
> +            pr_dbg("Invalid num_sge=%d (max %d)\n", wqe->hdr.num_sge,
> +                   dev->dev_attr.max_sge);
> +            complete_with_error(VENDOR_ERR_INV_NUM_SGE, comp_ctx);
> +            continue;
> +        }
> +
>           rdma_backend_post_send(&dev->backend_dev, &qp->backend_qp, qp->qp_type,
>                                  (struct ibv_sge *)&wqe->sge[0], wqe->hdr.num_sge,
>                                  sgid_idx, sgid,
> @@ -227,6 +244,13 @@ int pvrdma_qp_recv(PVRDMADev *dev, uint32_t qp_handle)
>           comp_ctx->cqe.qp = qp_handle;
>           comp_ctx->cqe.opcode = IBV_WC_RECV;
>   
> +        if (wqe->hdr.num_sge > dev->dev_attr.max_sge) {
> +            pr_dbg("Invalid num_sge=%d (max %d)\n", wqe->hdr.num_sge,
> +                   dev->dev_attr.max_sge);
> +            complete_with_error(VENDOR_ERR_INV_NUM_SGE, comp_ctx);
> +            continue;
> +        }
> +
>           rdma_backend_post_recv(&dev->backend_dev, &dev->rdma_dev_res,
>                                  &qp->backend_qp, qp->qp_type,
>                                  (struct ibv_sge *)&wqe->sge[0], wqe->hdr.num_sge,