> From: Jason Wang <jasowang@xxxxxxxxxx>
> Sent: Tuesday, August 17, 2021 9:26 AM
>
> On Tue, Aug 17, 2021 at 3:37 AM Michael S. Tsirkin <mst@xxxxxxxxxx>
> wrote:
> >
> > On Mon, Aug 16, 2021 at 07:56:59PM +0300, Eli Cohen wrote:
> > > On Mon, Aug 16, 2021 at 01:58:06PM +0800, Jason Wang wrote:
> > > >
> > > > 在 2021/8/16 下午1:47, Eli Cohen 写道:
> > > > > On Mon, Aug 16, 2021 at 12:16:14PM +0800, Jason Wang wrote:
> > > > > > 在 2021/8/12 下午5:50, Eli Cohen 写道:
> > > > > > > On Thu, Aug 12, 2021 at 03:04:35PM +0800, Jason Wang wrote:
> > > > > > > > 在 2021/8/12 下午3:01, Eli Cohen 写道:
> > > > > > > > > On Thu, Aug 12, 2021 at 02:47:06PM +0800, Jason Wang wrote:
> > > > > > > > > > On Thu, Aug 12, 2021 at 12:55 PM Eli Cohen
> <elic@xxxxxxxxxx> wrote:
> > > > > > > > > > > On Thu, Aug 12, 2021 at 11:19:19AM +0800, Jason Wang
> wrote:
> > > > > > > > > > > > 在 2021/8/11 下午7:04, Eli Cohen 写道:
> > > > > > > > > > > > > On Wed, Aug 11, 2021 at 04:37:44PM +0800, Jason
> Wang wrote:
> > > > > > > > > > > > > > 在 2021/8/11 下午3:53, Eli Cohen 写道:
> > > > > > > > > > > > > > > > One thing need to solve for mq is that the:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > +static u16 ctrl_vq_idx(struct
> > > > > > > > > > > > > > > > > +mlx5_vdpa_dev *mvdev) {
> > > > > > > > > > > > > > > > > + return 2 *
> > > > > > > > > > > > > > > > > +mlx5_vdpa_max_qps(mvdev->max_vqs);
> > > > > > > > > > > > > > > > > +}
> > > > > > > > > > > > > > > > We should handle the case when MQ is
> > > > > > > > > > > > > > > > supported by the device but not the driver.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > E.g in the case when we have 2 queue pairs:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > When MQ is enabled, cvq is queue 4
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > When MQ is not enabled, cvq is queue 2
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > There's some issue with this. I get
> > > > > > > > > > > > > > > callbacks targeting specific virtqueues before
> features negotiation has been completed.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Specifically, I get set_vq_cb() calls. At
> > > > > > > > > > > > > > > this point I must know the control vq index.
> > > > > > > > > > > > > > So I think we need do both:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > 1) At one hand, it's a bug for the userspace
> > > > > > > > > > > > > > to use vq_index before feature is negotiated
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > (looks like a bug in my cvq series that will
> > > > > > > > > > > > > > call SET_VRING_CALL before feature is negotiate,
> which I will look).
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > 2) At the other hand, the driver should be
> > > > > > > > > > > > > > able to deal with that
> > > > > > > > > > > > > >
> > > > > > > > > > > > > All I can do is drop callbacks for VQs before
> > > > > > > > > > > > > features negotation has been completed.
> > > > > > > > > > > > Or just leave queue index 0, 1 work.
> > > > > > > > > > > >
> > > > > > > > > > > > Since it is not expected to be changed.
> > > > > > > > > > > >
> > > > > > > > > > > Right, will do.
> > > > > > > > > > >
> > > > > > > > > > > > > > > I think the CVQ index must not depend on the
> > > > > > > > > > > > > > > negotiated features but rather depend of the
> > > > > > > > > > > > > > > value the device driver provides in the call to
> _vdpa_register_device().
> > > > > > > > > > > > > > At the virtio level, it's too late to change
> > > > > > > > > > > > > > that and it breaks the backward compatibility.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > But at the vDPA level, the under layer device
> > > > > > > > > > > > > > can map virtio cvq to any of it's virtqueue.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > E.g map cvq (index 2) to mlx5 cvq (the last).
> > > > > > > > > > > > > >
> > > > > > > > > > > > > I am not following you here. I still don't know what
> index is cvq.
> > > > > > > > > > > > Right, we still need to wait for the feature being
> > > > > > > > > > > > negotiated in order to proceed.
> > > > > > > > > > > >
> > > > > > > > > > > So to summarise, before feature negotiation
> > > > > > > > > > > complete, I accept calls referring to VQs only for indices 0
> and 1.
> > > > > > > > > > > After feature negotiation complete I know CVQ index
> > > > > > > > > > > and will accept indices 0 to cvq index.
> > > > > > > > > > I don't get this "accept indices 0 to cvq index".
> > > > > > > > > What I meant to say is that there are several callbacks
> > > > > > > > > that refer to specific virtqueues, e.g.
> > > > > > > > > set_vq_address(), set_vq_num() etc. They all accept virtqueue
> index as an argument.
> > > > > > > > >
> > > > > > > > > What we want to do is verify wheather the index provided
> > > > > > > > > is valid or not. If it is not valid, either return error
> > > > > > > > > (if the callback can return a value) or just avoid
> > > > > > > > > processing it. If the index is valid then we process it normally.
> > > > > > > > >
> > > > > > > > > Now we need to decide which index is valid or not. We
> > > > > > > > > need something like this to identifiy valid indexes range:
> > > > > > > > >
> > > > > > > > > CVQ clear: 0 and 1
> > > > > > > > > CVQ set, MQ clear: 0, 1 and 2 (for CVQ).
> > > > > > > > > CVQ set, MQ set: 0..nvq where nvq is whatever provided
> > > > > > > > > to
> > > > > > > > > _vdpa_register_device()
> > > > > > > > Yes.
> > > > > > > >
> > > > > > > Unfortunately it does not work.
> > > > > > > set_vq_cb() for all the multiqueues is called beofre feature
> > > > > > > negotiation. If I apply the above logic, I will lose these settings.
> > > > > > >
> > > > > > > I can make an exception for set_vq_cb(), save callbacks and
> > > > > > > restore them afterwards. This looks too convoluted and maybe
> > > > > > > we should seek another solution.
> > > > > >
> > > > > > I agree.
> > > > > >
> > > > > >
> > > > > > > Let me know what you think.
> > > > > >
> > > > > > Rethink about this issue. It looks to the only issue we face
> > > > > > is the set_vq_cb().
> > > > > >
> > > > > > With the assumption that the userspace can use the index
> > > > > > correctly (even before set_features). I wonder the following works.
> > > > > >
> > > > > > Instead of checking whether the index is cvq in set_vq_cb() how
> about:
> > > > > >
> > > > > > 1) decouple event_cb out of mlx5_vdpa_virtqueue and
> > > > > > mlx5_congro_vq
> > > > > > 2) have a dedicated event_cb array in mlx5_vdpa_net
> > > > > > 3) then we can do
> > > > > >
> > > > > > ndev->event_cbs[index] = *cb;
> > > > > >
> > > > > So actually you're suggesting to save all the callabck
> > > > > configurations in an array and evaluate cvq index after feature
> > > > > negotiation has been completed.
> > > >
> > > >
> > > > Yes.
> > > >
> > > >
> > > > >
> > > > > I think that could work. I will code this and update.
> > > >
> > >
> > > It works fine when I am working with your version of qemu with
> > > support for multi queue.
> > >
> > > The problem is that it is broken on qemu v6.0.0. If I register my
> > > vdpa device with more than 2 data virtqueues, qemu won't even create
> > > a netdevice in the VM.
>
> Qemu should hide MQ feature in this case but looks like it doesn't.
>
> Will have a look.
>
> > >
> > > I am not sure how to handle this. Is there some kind of indication I
> > > can get as to the version of qemu so I can avoid using multiqueue
> > > for versions I know are problematic?
> >
> > No versions ;) This is what feature bits are for ...
>
> Yes.
>
> So does it work if "mq=off" is specified in the command line?
>
We should not add driver module parameter.
We anyway need number of VQs to be driven by the number of vcpus used by the VM.
So why not specify this when creating a vdpa device?
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
