On Wed, Feb 7, 2024 at 4:47 PM Stefano Garzarella <sgarz...@redhat.com> wrote: > > On Wed, Feb 07, 2024 at 11:17:34AM +0800, Jason Wang wrote: > >On Tue, Feb 6, 2024 at 4:31 PM Stefano Garzarella <sgarz...@redhat.com> > >wrote: > >> > >> On Tue, Feb 06, 2024 at 10:47:40AM +0800, Jason Wang wrote: > >> >On Mon, Feb 5, 2024 at 6:51 PM Stefano Garzarella <sgarz...@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. > > > >Right. > > > >> > >> >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? > > > >Kind of, but as stated, it's just because spec is unclear about the > >behaviour. There's a chance that spec will explicitly support it in > >the future. > > > >> > >> > > >> >> > >> >> 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? > > > >I'm not sure if this can break some setups or not. It might be better > >to leave it as is? > > As I also answered in the kernel patch, IMHO we have to return an error, > because any setups are broken anyway (see the problem we're fixing with is > this patch),
For VDUSE probably yes, but not for other parents. It's easy to mandate on day 0 but might be hard for now. For mlx5e, it supports the semantic VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK even before the feature bit is introduced. And we don't do other checks like for example setting vq address, size after DRIVER_OK. We can hear from others. > so at this point it's better to return an error, so they > don't spend time figuring out why the VDUSE device doesn't work. > > > > >Without VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK, we don't know if > >parent support vq_ready after driver_ok. > > So we have to assume that it doesn't support it, to be more > conservative, right? > > >With VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK, we know parent support > >vq_ready after driver_ok. > > > >> > >> >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? > > > >Yes, since in this case the parent is in the userspace, there's no way > >for VDUSE to know if user space supports vq_ready after driver_ok or > >not. > > > >As you may have noticed, we don't have a message for vq_ready which > >implies that vq_ready after driver_ok can't be supported. > > Yep, I see. > > > > >> > >> >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. > > > >Yes, the problem is assuming we have a message for vq_ready, there > >could be a "legacy" userspace that doesn't support that. So in that > >case, VDUSE needs to know if the userspace parent can support that or > >not. > > I think VDUSE needs to negotiate features (if it doesn't already) as > vhost-user does with the backend. Also for new future functionality. It negotiates virtio features but not vhost backend features. Thanks
