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? 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
> 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. 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?
? 2021/8/17 ??11:55, Jason Wang ??:>>>>> 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.So it looks to me current Qemu code doesn't take care about the case when a feature (e.g MQ): 1) supported by the vhost 2) enabled via the command line 3) but not supported by the Qemu Not sure it's worth to bother. Thanks