On Tue, Aug 17, 2021 at 1:24 PM Eli Cohen <elic at nvidia.com>
wrote:>
> On Tue, Aug 17, 2021 at 11:55:42AM +0800, Jason Wang wrote:
> > 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.
> >
>
> Not sure I understand what do you think is expected from qemu. The
> device publishes MQ, qemu should either accept or decline.
Probably not. It works like disable MQ silently (without cli) in qemu.
Thanks
>
> > 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?
> >
> > Thanks
> >
> >
> > >
> > > > >
> > > > > Thanks.
> > > > >
> > > > >
> > > > > >
> > > > > > > in mlx5_vdpa_set_vq_cb()
> > > > > > >
> > > > > > > 4) in the mlx5_cvq_kick_handler(), we know
the feature is negotiated and we
> > > > > > > can use the correct index there.
> > > > > > >
> > > > > > > In the mean time, I will look at Qemu code to
see if we can guarantee that
> > > > > > > set_features is called before
set_vq_callback. (At first glance, it's not
> > > > > > > trivial but let's see).
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > >
> > > > > > > > > > And while writing this, I
think this logic does not belog in mlx5_vdpa
> > > > > > > > > > but probably in vdpa.c
> > > > > > > > > The problem is that vdpa should be
unaware of a specific device type. E.g
> > > > > > > > > the above indices may work only for
virtio-net but not other.
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > Thanks
> > > > > > > > > > >
> > > > > > > > > > > > > Thanks
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > > >
Thanks
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > >
> > >
> >
>