On Tue, Aug 17, 2021 at 4:57 PM Jason Wang <jasowang at redhat.com>
wrote:>
> On Tue, Aug 17, 2021 at 2:01 PM Eli Cohen <elic at nvidia.com> wrote:
> >
> > On Tue, Aug 17, 2021 at 01:48:17PM +0800, Jason Wang wrote:
> > > On Tue, Aug 17, 2021 at 1:26 PM Eli Cohen <elic at
nvidia.com> wrote:
> > > >
> > > > On Tue, Aug 17, 2021 at 12:19:55PM +0800, Jason Wang wrote:
> > > > >
> > > > > ? 2021/8/17 ??12:03, Parav Pandit ??:
> > > > > > > From: Jason Wang <jasowang at
redhat.com>
> > > > > > > Sent: Tuesday, August 17, 2021 9:26 AM
> > > > > > >
> > > > > > > On Tue, Aug 17, 2021 at 3:37 AM Michael S.
Tsirkin <mst at redhat.com>
> > > > > > > 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 at nvidia.com> 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.
> > > > >
> > > > >
> > > > > Note that, it's not a module parameter but a qemu
command line to disable mq
> > > > > feature.
> > > > >
> > > > >
> > > > > > 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?
> > > > >
> > > > >
> > > > > Yes, I think it should work as well.
> > > > >
> > > > > So management need either:
> > > > >
> > > > > 1) disable multiqueue via "mq=off"
> > > > >
> > > > > or
> > > > >
> > > > > 2) using netlink API to create a single queue pair
device
> > > > >
> > > > > for the qemu <=6.1.
> > > > >
> > > >
> > > > Which management entity are you referring to here?
> > >
> > > The one that launches Qemu. (E.g libvirt or other).
> > >
> >
> > But we want to find a solution with existing packages. If modifying
code
> > existing code is the solution, we could fix qemu.
>
> Right.
>
> >
> > As I reported in another email, mq=off avoids this problem. So users
> > will have to use this when using new driver with old qemu.
>
> Yes, so this is actually the option 1) above.
Note that actually, mq=off is the default option. So we are probably fine here.
Thanks
>
> Thanks
>
> >
> > > Thanks
> > >
> > > >
> > > > > Thanks
> > > > >
> > > > >
> > > > > >
> > > > >
> > > >
> > >
> >