Re: Re: [PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility

[Stefano Garzarella]

On Tue, Feb 06, 2024 at 10:47:40AM +0800, Jason Wang wrote:
> On Mon, Feb 5, 2024 at 6:51 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > On Fri, Feb 02, 2024 at 02:25:21PM +0100, Kevin Wolf wrote:
> > >VDUSE requires that virtqueues are first enabled before the DRIVER_OK
> > >status flag is set; with the current API of the kernel module, it is
> > >impossible to enable the opposite order in our block export code because
> > >userspace is not notified when a virtqueue is enabled.
>
> Did this mean virtio-blk will enable a virtqueue after DRIVER_OK?

It's not specific to virtio-blk, but to the generic vdpa device we have 
in QEMU (i.e. vhost-vdpa-device). Yep, after commit 
6c4825476a4351530bcac17abab72295b75ffe98, virtqueues are enabled after 
DRIVER_OK.

> Sepc is not clear about this and that's why we introduce
> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK.

Ah, I didn't know about this new feature. So after commit 
6c4825476a4351530bcac17abab72295b75ffe98 the vhost-vdpa-device is not 
complying with the specification, right?

> > Yeah, IMHO the VDUSE protocol is missing a VDUSE_SET_VQ_READY message,
>
> I think you meant when VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is
> negotiated.

At this point yes. But if VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not 
negotiated, should we return an error in vhost-vdpa kernel module if 
VHOST_VDPA_SET_VRING_ENABLE is called when DRIVER_OK is already set?

> If this is truth, it seems a little more complicated, for
> example the get_backend_features needs to be forward to the userspace?

I'm not understanding, don't we already have VHOST_GET_BACKEND_FEATURES 
for this? Or do you mean userspace on the VDUSE side?

> This seems suboptimal to implement this in the spec first and then we
> can leverage the features. Or we can have another parameter for the
> ioctl that creates the vduse device.

I got a little lost, though in vhost-user, the device can always expect 
a vring_enable/disable, so I thought it was not complicated in VDUSE.

> > I'll start another thread about that, but in the meantime I agree that
> > we should fix QEMU since we need to work properly with old kernels as
> > well.
> >
> > >
> > >This requirement also mathces the normal initialisation order as done by
> > >the generic vhost code in QEMU. However, commit 6c482547 accidentally
> > >changed the order for vdpa-dev and broke access to VDUSE devices with
> > >this.
> > >
> > >This changes vdpa-dev to use the normal order again and use the standard
> > >vhost callback .vhost_set_vring_enable for this. VDUSE devices can be
> > >used with vdpa-dev again after this fix.
> >
> > I like this approach and the patch LGTM, but I'm a bit worried about
> > this function in hw/net/vhost_net.c:
> >
> >      int vhost_set_vring_enable(NetClientState *nc, int enable)
> >      {
> >          VHostNetState *net = get_vhost_net(nc);
> >          const VhostOps *vhost_ops = net->dev.vhost_ops;
> >
> >          nc->vring_enable = enable;
> >
> >          if (vhost_ops && vhost_ops->vhost_set_vring_enable) {
> >              return vhost_ops->vhost_set_vring_enable(&net->dev, enable);
> >          }
> >
> >          return 0;
> >      }
> >
> > @Eugenio, @Jason, should we change some things there if vhost-vdpa
> > implements the vhost_set_vring_enable callback?
>
> Eugenio may know more, I remember we need to enable cvq first for
> shadow virtqueue to restore some states.
>
> > Do you remember why we didn't implement it from the beginning?
>
> It seems the vrings parameter is introduced after vhost-vdpa is 
> implemented.

Sorry, I mean why we didn't implement the vhost_set_vring_enable 
callback for vhost-vdpa from the beginning.

Thanks,
Stefano