On Tue, Aug 17, 2021 at 4:57 PM Jason Wang <jasowang@xxxxxxxxxx> wrote:
>
> On Tue, Aug 17, 2021 at 2:01 PM Eli Cohen <elic@xxxxxxxxxx> 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@xxxxxxxxxx> 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@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.
> > > > >
> > > > >
> > > > > 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
> > > > >
> > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
