Message-ID: <DM8PR12MB540086B732FC6CB8A36A9626ABE79@DM8PR12MB5400.namprd12.prod.outlook.com>
Date:   Wed, 6 Apr 2022 08:33:42 +0000
From:   Eli Cohen <elic@...dia.com>
To:     Jason Wang <jasowang@...hat.com>,
        "Michael S. Tsirkin" <mst@...hat.com>
CC:     "hdanton@...a.com" <hdanton@...a.com>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size

> From: Jason Wang <jasowang@...hat.com>
> Sent: Wednesday, April 6, 2022 11:26 AM
> To: Michael S. Tsirkin <mst@...hat.com>; Eli Cohen <elic@...dia.com>
> Cc: hdanton@...a.com; virtualization@...ts.linux-foundation.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size
> 
> 
> 在 2022/4/6 上午10:35, Jason Wang 写道:
> >
> > 在 2022/4/4 下午7:24, Michael S. Tsirkin 写道:
> >> On Mon, Apr 04, 2022 at 11:07:36AM +0000, Eli Cohen wrote:
> >>>> From: Michael S. Tsirkin <mst@...hat.com>
> >>>> Sent: Monday, April 4, 2022 1:35 PM
> >>>> To: Jason Wang <jasowang@...hat.com>
> >>>> Cc: Eli Cohen <elic@...dia.com>; hdanton@...a.com;
> >>>> virtualization@...ts.linux-foundation.org;
> >>>> linux-kernel@...r.kernel.org
> >>>> Subject: Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size
> >>>>
> >>>> On Tue, Mar 29, 2022 at 12:21:09PM +0800, Jason Wang wrote:
> >>>>> From: Eli Cohen <elic@...dia.com>
> >>>>>
> >>>>> The current code evaluates RQT size based on the configured number of
> >>>>> virtqueues. This can raise an issue in the following scenario:
> >>>>>
> >>>>> Assume MQ was negotiated.
> >>>>> 1. mlx5_vdpa_set_map() gets called.
> >>>>> 2. handle_ctrl_mq() is called setting cur_num_vqs to some value,
> >>>>> lower
> >>>>>     than the configured max VQs.
> >>>>> 3. A second set_map gets called, but now a smaller number of VQs
> >>>>> is used
> >>>>>     to evaluate the size of the RQT.
> >>>>> 4. handle_ctrl_mq() is called with a value larger than what the
> >>>>> RQT can
> >>>>>     hold. This will emit errors and the driver state is compromised.
> >>>>>
> >>>>> To fix this, we use a new field in struct mlx5_vdpa_net to hold the
> >>>>> required number of entries in the RQT. This value is evaluated in
> >>>>> mlx5_vdpa_set_driver_features() where we have the negotiated features
> >>>>> all set up.
> >>>>>
> >>>>> In addtion
> >>>> addition?
> >>> Do you need me to send another version?
> >> It's a bit easier that way but I can handle it manually too.
> >
> >
> > Let me send a new version with this fixed.
> 
> 
> Ok, it looks like if I use git-send-email when From:tag is not me. The
> patch will be sent as a attachment as spotted by Maxime.
> 
> Eli, would you please send a v3 with my acked-by? (Since I don't want to
> change the author)
> 

Sure.

> Thanks
> 
> 
> >
> >
> >>
> >>> If so, let's wait for Jason's reply.
> >> Right.
> >>
> >>>>> to that, we take into consideration the max capability of RQT
> >>>>> entries early when the device is added so we don't need to take
> >>>>> consider
> >>>>> it when creating the RQT.
> >>>>>
> >>>>> Last, we remove the use of mlx5_vdpa_max_qps() which just returns the
> >>>>> max_vas / 2 and make the code clearer.
> >>>>>
> >>>>> Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
> >>>>> Signed-off-by: Eli Cohen <elic@...dia.com>
> >>>> Jason I don't have your ack or S.O.B on this one.
> >
> >
> > My bad, for some reason, I miss that.
> >
> > Will fix.
> >
> > Thanks
> >
> >
> >>>>
> >>>>
> >>>>> ---
> >>>>>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 61
> >>>>> +++++++++++--------------------
> >>>>>   1 file changed, 21 insertions(+), 40 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>>>> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>>>> index 53b8c1a68f90..61bec1ed0bc9 100644
> >>>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>>>> @@ -161,6 +161,7 @@ struct mlx5_vdpa_net {
> >>>>>       struct mlx5_flow_handle *rx_rule_mcast;
> >>>>>       bool setup;
> >>>>>       u32 cur_num_vqs;
> >>>>> +    u32 rqt_size;
> >>>>>       struct notifier_block nb;
> >>>>>       struct vdpa_callback config_cb;
> >>>>>       struct mlx5_vdpa_wq_ent cvq_ent;
> >>>>> @@ -204,17 +205,12 @@ static __virtio16 cpu_to_mlx5vdpa16(struct
> >>>>> mlx5_vdpa_dev *mvdev, u16 val)
> >>>>>       return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev),
> >>>>> val);
> >>>>>   }
> >>>>>
> >>>>> -static inline u32 mlx5_vdpa_max_qps(int max_vqs)
> >>>>> -{
> >>>>> -    return max_vqs / 2;
> >>>>> -}
> >>>>> -
> >>>>>   static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
> >>>>>   {
> >>>>>       if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> >>>>>           return 2;
> >>>>>
> >>>>> -    return 2 * mlx5_vdpa_max_qps(mvdev->max_vqs);
> >>>>> +    return mvdev->max_vqs;
> >>>>>   }
> >>>>>
> >>>>>   static bool is_ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev, u16 idx)
> >>>>> @@ -1236,25 +1232,13 @@ static void teardown_vq(struct
> >>>>> mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
> >>>>>   static int create_rqt(struct mlx5_vdpa_net *ndev)
> >>>>>   {
> >>>>>       __be32 *list;
> >>>>> -    int max_rqt;
> >>>>>       void *rqtc;
> >>>>>       int inlen;
> >>>>>       void *in;
> >>>>>       int i, j;
> >>>>>       int err;
> >>>>> -    int num;
> >>>>> -
> >>>>> -    if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> >>>>> -        num = 1;
> >>>>> -    else
> >>>>> -        num = ndev->cur_num_vqs / 2;
> >>>>>
> >>>>> -    max_rqt = min_t(int, roundup_pow_of_two(num),
> >>>>> -            1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> >>>>> -    if (max_rqt < 1)
> >>>>> -        return -EOPNOTSUPP;
> >>>>> -
> >>>>> -    inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + max_rqt *
> >>>>> MLX5_ST_SZ_BYTES(rq_num);
> >>>>> +    inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + ndev->rqt_size *
> >>>>> MLX5_ST_SZ_BYTES(rq_num);
> >>>>>       in = kzalloc(inlen, GFP_KERNEL);
> >>>>>       if (!in)
> >>>>>           return -ENOMEM;
> >>>>> @@ -1263,12 +1247,12 @@ static int create_rqt(struct mlx5_vdpa_net
> >>>>> *ndev)
> >>>>>       rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context);
> >>>>>
> >>>>>       MLX5_SET(rqtc, rqtc, list_q_type,
> >>>>> MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
> >>>>> -    MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
> >>>>> +    MLX5_SET(rqtc, rqtc, rqt_max_size, ndev->rqt_size);
> >>>>>       list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> >>>>> -    for (i = 0, j = 0; i < max_rqt; i++, j += 2)
> >>>>> -        list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);
> >>>>> +    for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
> >>>>> +        list[i] = cpu_to_be32(ndev->vqs[j %
> >>>>> ndev->cur_num_vqs].virtq_id);
> >>>>>
> >>>>> -    MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
> >>>>> +    MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
> >>>>>       err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen,
> >>>>> &ndev->res.rqtn);
> >>>>>       kfree(in);
> >>>>>       if (err)
> >>>>> @@ -1282,19 +1266,13 @@ static int create_rqt(struct mlx5_vdpa_net
> >>>>> *ndev)
> >>>>>   static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
> >>>>>   {
> >>>>>       __be32 *list;
> >>>>> -    int max_rqt;
> >>>>>       void *rqtc;
> >>>>>       int inlen;
> >>>>>       void *in;
> >>>>>       int i, j;
> >>>>>       int err;
> >>>>>
> >>>>> -    max_rqt = min_t(int, roundup_pow_of_two(ndev->cur_num_vqs / 2),
> >>>>> -            1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> >>>>> -    if (max_rqt < 1)
> >>>>> -        return -EOPNOTSUPP;
> >>>>> -
> >>>>> -    inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + max_rqt *
> >>>>> MLX5_ST_SZ_BYTES(rq_num);
> >>>>> +    inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + ndev->rqt_size *
> >>>>> MLX5_ST_SZ_BYTES(rq_num);
> >>>>>       in = kzalloc(inlen, GFP_KERNEL);
> >>>>>       if (!in)
> >>>>>           return -ENOMEM;
> >>>>> @@ -1305,10 +1283,10 @@ static int modify_rqt(struct mlx5_vdpa_net
> >>>>> *ndev, int num)
> >>>>>       MLX5_SET(rqtc, rqtc, list_q_type,
> >>>>> MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
> >>>>>
> >>>>>       list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> >>>>> -    for (i = 0, j = 0; i < max_rqt; i++, j += 2)
> >>>>> +    for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
> >>>>>           list[i] = cpu_to_be32(ndev->vqs[j % num].virtq_id);
> >>>>>
> >>>>> -    MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
> >>>>> +    MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
> >>>>>       err = mlx5_vdpa_modify_rqt(&ndev->mvdev, in, inlen,
> >>>>> ndev->res.rqtn);
> >>>>>       kfree(in);
> >>>>>       if (err)
> >>>>> @@ -1582,7 +1560,7 @@ static virtio_net_ctrl_ack
> >>>>> handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
> >>>>>
> >>>>>           newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
> >>>>>           if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
> >>>>> -            newqps > mlx5_vdpa_max_qps(mvdev->max_vqs))
> >>>>> +            newqps > ndev->rqt_size)
> >>>>>               break;
> >>>>>
> >>>>>           if (ndev->cur_num_vqs == 2 * newqps) {
> >>>>> @@ -1946,7 +1924,7 @@ static int setup_virtqueues(struct
> >>>>> mlx5_vdpa_dev *mvdev)
> >>>>>       int err;
> >>>>>       int i;
> >>>>>
> >>>>> -    for (i = 0; i < 2 * mlx5_vdpa_max_qps(mvdev->max_vqs); i++) {
> >>>>> +    for (i = 0; i < mvdev->max_vqs; i++) {
> >>>>>           err = setup_vq(ndev, &ndev->vqs[i]);
> >>>>>           if (err)
> >>>>>               goto err_vq;
> >>>>> @@ -2017,9 +1995,11 @@ static int
> >>>>> mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
> >>>>>
> >>>>>       ndev->mvdev.actual_features = features &
> >>>>> ndev->mvdev.mlx_features;
> >>>>>       if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
> >>>>> -        ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev,
> >>>>> ndev->config.max_virtqueue_pairs);
> >>>>> +        ndev->rqt_size = mlx5vdpa16_to_cpu(mvdev,
> >>>>> ndev->config.max_virtqueue_pairs);
> >>>>>       else
> >>>>> -        ndev->cur_num_vqs = 2;
> >>>>> +        ndev->rqt_size = 1;
> >>>>> +
> >>>>> +    ndev->cur_num_vqs = 2 * ndev->rqt_size;
> >>>>>
> >>>>>       update_cvq_info(mvdev);
> >>>>>       return err;
> >>>>> @@ -2486,7 +2466,7 @@ static void init_mvqs(struct mlx5_vdpa_net
> >>>>> *ndev)
> >>>>>       struct mlx5_vdpa_virtqueue *mvq;
> >>>>>       int i;
> >>>>>
> >>>>> -    for (i = 0; i < 2 * mlx5_vdpa_max_qps(ndev->mvdev.max_vqs);
> >>>>> ++i) {
> >>>>> +    for (i = 0; i < ndev->mvdev.max_vqs; ++i) {
> >>>>>           mvq = &ndev->vqs[i];
> >>>>>           memset(mvq, 0, offsetof(struct mlx5_vdpa_virtqueue, ri));
> >>>>>           mvq->index = i;
> >>>>> @@ -2606,7 +2586,8 @@ static int mlx5_vdpa_dev_add(struct
> >>>>> vdpa_mgmt_dev *v_mdev, const char *name,
> >>>>>           return -EOPNOTSUPP;
> >>>>>       }
> >>>>>
> >>>>> -    max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev,
> >>>>> max_num_virtio_queues);
> >>>>> +    max_vqs = min_t(int, MLX5_CAP_DEV_VDPA_EMULATION(mdev,
> >>>>> max_num_virtio_queues),
> >>>>> +            1 << MLX5_CAP_GEN(mdev, log_max_rqt_size));
> >>>>>       if (max_vqs < 2) {
> >>>>>           dev_warn(mdev->device,
> >>>>>                "%d virtqueues are supported. At least 2 are
> >>>>> required\n",
> >>>>> @@ -2670,7 +2651,7 @@ static int mlx5_vdpa_dev_add(struct
> >>>>> vdpa_mgmt_dev *v_mdev, const char *name,
> >>>>>           ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MAC);
> >>>>>       }
> >>>>>
> >>>>> -    config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev,
> >>>>> mlx5_vdpa_max_qps(max_vqs));
> >>>>> +    config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev,
> >>>>> max_vqs / 2);
> >>>>>       mvdev->vdev.dma_dev = &mdev->pdev->dev;
> >>>>>       err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
> >>>>>       if (err)
> >>>>> @@ -2697,7 +2678,7 @@ static int mlx5_vdpa_dev_add(struct
> >>>>> vdpa_mgmt_dev *v_mdev, const char *name,
> >>>>>       ndev->nb.notifier_call = event_handler;
> >>>>>       mlx5_notifier_register(mdev, &ndev->nb);
> >>>>>       mvdev->vdev.mdev = &mgtdev->mgtdev;
> >>>>> -    err = _vdpa_register_device(&mvdev->vdev, 2 *
> >>>>> mlx5_vdpa_max_qps(max_vqs) + 1);
> >>>>> +    err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
> >>>>>       if (err)
> >>>>>           goto err_reg;
> >>>>>
> >>>>> --
> >>>>> 2.18.1

Powered by blists - more mailing lists