On Tue, Aug 17, 2021 at 2:01 PM Eli Cohen <[email protected]> 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 <[email protected]> wrote: > > > > > > On Tue, Aug 17, 2021 at 12:19:55PM +0800, Jason Wang wrote: > > > > > > > > 在 2021/8/17 下午12:03, Parav Pandit 写道: > > > > > > From: Jason Wang <[email protected]> > > > > > > Sent: Tuesday, August 17, 2021 9:26 AM > > > > > > > > > > > > On Tue, Aug 17, 2021 at 3:37 AM Michael S. Tsirkin <[email protected]> > > > > > > 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 > > > > > > <[email protected]> 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. Thanks > > > Thanks > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ Virtualization mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/virtualization
