Re: [PATCH 1/1] vhost-vsock: add VIRTIO_F_RING_PACKED to feaure_bits

[Halil Pasic <pasic@linux.ibm.com>]

On Tue, 28 May 2024 11:25:51 +0800
Jason Wang <jasowang@redhat.com> wrote:

> > 5) Based on the following, I would very much prefer a per device list of
> > features with the semantic "hey QEMU can do that feature without any
> > specialized vhost-device support (e.g. like VIRTIO_SCSI_F_CHANGE)"  
> 
> Indeed the current code is kind of tricky and may need better
> documentation. But the problem is some features were datapath related
> and they are supported since the birth of a specific vhost device. For
> example, some GSO related features (actually, it's not a feature of
> vhost-net but TUN/TAP).
> 
> And I've found another interesting thing, for RING_REST, actually we
> don't need to do anything but we have the following commits:
> 
> 313389be06ff6 ("vhost-net: support VIRTIO_F_RING_RESET")
> 2a3552baafb ("vhost: vhost-kernel: enable vq reset feature")
> 
> Technically, they are not necessary as RING_RESET for vhost-kernel
> doesn't require any additional new ioctls. But it's too late to change
> as the kernel commit has been merged.
> 
> > over
> > the current list with the semantics "these are the features that
> > need vhost backend support, and anything else will just work out". That
> > way if an omission is made, we would end up with the usually safer
> > under-indication  instead of the current over-indication.
> >
> >
> > @Michael, Jason: Could you guys chime in?  
> 
> Another issue is that it seems to require a change of the semantic of
> VHOST_GET_FEATURES. If my understanding is true, it seems a
> non-trivial change which I'm not sure it's worth to bother.

I don't quite understand. Would you mind to elaborate on this?

For starters, what is the current semantic of VHOST_GET_FEATURES, and
where is it documented? You mean the ioctl, right?

Then why do you think the semantic of VHOST_GET_FEATURES should change?

IMHO changing the semantic of the VHOST_GET_FEATURES ioctl is not viable,
but also not necessary. What I am proposing is changing the (in QEMU)
logic of processing the features returned by VHOST_GET_FEATURES, while
preserving the outcomes (essentially realize the same function in a
mathematical sense, but with code that is less fragile), modulo bugs like
the one addressed with this patch of course.

Regards,
Halil