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

Not having VIRTIO_F_RING_PACKED in feature_bits[] is a problem when the
vhost-vsock device does not offer the feature bit VIRTIO_F_RING_PACKED
but the in QEMU device is configured to try to use the packed layout
(the virtio property "packed" is on).

As of today, the  Linux kernel vhost-vsock device does not support the
packed queue layout (as vhost does not support packed), and does not
offer VIRTIO_F_RING_PACKED. Thus when for example a vhost-vsock-ccw is
used with packed=on, VIRTIO_F_RING_PACKED ends up being negotiated,
despite the fact that the device does not actually support it, and
one gets to keep the pieces.

Fixes: 74b3e46630 ("virtio: add property to enable packed virtqueue")
Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
---

This is a minimal fix, that follows the current patterns in the
codebase, and not necessarily the best one.

I don't quite understand why vhost_get_features() works the way
it works. Fortunately it is documented, so let me quote the
documentation.

"""
/**
 * vhost_get_features() - return a sanitised set of feature bits
 * @hdev: common vhost_dev structure
 * @feature_bits: pointer to terminated table of feature bits
 * @features: original feature set
 *
 * This returns a set of features bits that is an intersection of what
 * is supported by the vhost backend (hdev->features), the supported
 * feature_bits and the requested feature set.
 */
uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
                            uint64_t features);
"""

Based on this I would expect the following statement to be true: if a
feature bit is not in feature_bits then the corresponding bit in the
return value is guaranteed to be not set (regardless of the values of
the 3rd arguments and hdev->features).

The implementation however does the following: if the feature bit is not
listed in feature_bits (2nd argument) then the corresponding bit in the
return value is set iff the corresponding bit in the 3rd argument
(features) is set (i.e. it does not matter what hdev->features and thus
the vhost backend says).

The documentation however does kind of state, that feature_bits is
supposed to contain the supported features. And under the assumption
that feature bit not in feature_bits implies that the corresponding bit
must not be set in the 3rd argument (features), then even with the
current implementation we do end up with the intersection of the three
as stated. And then vsock would be at fault for violating that
assumption, and my fix would be the best thing to do -- I guess.

Is the implementation the way it is for a good reason, I can't judge
that with certainty for myself.

But I'm pretty convinced that the current approach is fragile,
especially for the feature bits form the range 24 to 40, as those are
not specific to a device.

BTW vsock also lacks VIRTIO_F_ACCESS_PLATFORM, and VIRTIO_F_RING_RESET
as well while vhost-net has both.

If our design is indeed to make the individual devices responsible for
having a complete list of possible features in feature_bits, then at
least having a common macro for the non-device specific features would
make sense to me.

On the other hand, I'm also very happy to send a patch which changes the
behavior of vhost_get_features(), should the community decide that the
current behavior does not make all that much sense -- I lean towards:
probably it does not make much sense, but things like
VIRTIO_F_ACCESS_PLATFORM, which are mandatory feature bits, need careful
consideration, because there vhost can't do so we just won't offer it
and proceed on our merry way is not the right behavior.

Please comment!

Regards,
Halil
---
 hw/virtio/vhost-vsock-common.c | 1 +
 1 file changed, 1 insertion(+)


base-commit: fd87be1dada5672f877e03c2ca8504458292c479

On Mon, 29 Apr 2024 13:33:34 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> Not having VIRTIO_F_RING_PACKED in feature_bits[] is a problem when the
> vhost-vsock device does not offer the feature bit VIRTIO_F_RING_PACKED
> but the in QEMU device is configured to try to use the packed layout
> (the virtio property "packed" is on).

polite ping

On Tue, 7 May 2024 21:26:30 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> > Not having VIRTIO_F_RING_PACKED in feature_bits[] is a problem when the
> > vhost-vsock device does not offer the feature bit VIRTIO_F_RING_PACKED
> > but the in QEMU device is configured to try to use the packed layout
> > (the virtio property "packed" is on).  
> 
> polite ping

ping

On Mon, Apr 29, 2024 at 01:33:34PM GMT, Halil Pasic wrote:
>Not having VIRTIO_F_RING_PACKED in feature_bits[] is a problem when the
>vhost-vsock device does not offer the feature bit VIRTIO_F_RING_PACKED
>but the in QEMU device is configured to try to use the packed layout
>(the virtio property "packed" is on).
>
>As of today, the  Linux kernel vhost-vsock device does not support the
>packed queue layout (as vhost does not support packed), and does not
>offer VIRTIO_F_RING_PACKED. Thus when for example a vhost-vsock-ccw is
>used with packed=on, VIRTIO_F_RING_PACKED ends up being negotiated,
>despite the fact that the device does not actually support it, and
>one gets to keep the pieces.
>
>Fixes: 74b3e46630 ("virtio: add property to enable packed virtqueue")
>Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
>---
>
>This is a minimal fix, that follows the current patterns in the
>codebase, and not necessarily the best one.

Yeah, I did something similar with commit 562a7d23bf ("vhost: mask 
VIRTIO_F_RING_RESET for vhost and vhost-user devices") so I think for 
now is the right approach.

I suggest to check also other devices like we did in that commit (e.g.  
hw/scsi/vhost-scsi.c, hw/scsi/vhost-user-scsi.c, etc. )

>
>I don't quite understand why vhost_get_features() works the way
>it works. Fortunately it is documented, so let me quote the
>documentation.
>
>"""
>/**
> * vhost_get_features() - return a sanitised set of feature bits
> * @hdev: common vhost_dev structure
> * @feature_bits: pointer to terminated table of feature bits
> * @features: original feature set
> *
> * This returns a set of features bits that is an intersection of what
> * is supported by the vhost backend (hdev->features), the supported
> * feature_bits and the requested feature set.
> */
>uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
>                            uint64_t features);
>"""
>
>Based on this I would expect the following statement to be true: if a
>feature bit is not in feature_bits then the corresponding bit in the
>return value is guaranteed to be not set (regardless of the values of
>the 3rd arguments and hdev->features).
>
>The implementation however does the following: if the feature bit is not
>listed in feature_bits (2nd argument) then the corresponding bit in the
>return value is set iff the corresponding bit in the 3rd argument
>(features) is set (i.e. it does not matter what hdev->features and thus
>the vhost backend says).
>
>The documentation however does kind of state, that feature_bits is
>supposed to contain the supported features. And under the assumption
>that feature bit not in feature_bits implies that the corresponding bit
>must not be set in the 3rd argument (features), then even with the
>current implementation we do end up with the intersection of the three
>as stated. And then vsock would be at fault for violating that
>assumption, and my fix would be the best thing to do -- I guess.
>
>Is the implementation the way it is for a good reason, I can't judge
>that with certainty for myself.

Yes, I think we should fix the documentation, and after a few years of 
not looking at it I'm confused again about what it does.

But re-reading my commit for VIRTIO_F_RING_RESET, it seems that I had 
interpreted `feature_bits` (2nd argument) as a list of features that 
QEMU doesn't know how to emulate and therefore are required by the 
backend (vhost/vhost-user/vdpa). Because the problem is that `features` 
(3rd argument) is a set of features required by the driver that can be 
provided by both QEMU and the backend.

>
>But I'm pretty convinced that the current approach is fragile,
>especially for the feature bits form the range 24 to 40, as those are
>not specific to a device.
>
>BTW vsock also lacks VIRTIO_F_ACCESS_PLATFORM, and VIRTIO_F_RING_RESET
>as well while vhost-net has both.

VIRTIO_F_RING_RESET is just above the line added by this patch.

>
>If our design is indeed to make the individual devices responsible for
>having a complete list of possible features in feature_bits, then at
>least having a common macro for the non-device specific features would
>make sense to me.

Yeah, I agree on this!

>
>On the other hand, I'm also very happy to send a patch which changes the
>behavior of vhost_get_features(), should the community decide that the
>current behavior does not make all that much sense -- I lean towards:
>probably it does not make much sense, but things like
>VIRTIO_F_ACCESS_PLATFORM, which are mandatory feature bits, need careful
>consideration, because there vhost can't do so we just won't offer it
>and proceed on our merry way is not the right behavior.
>
>Please comment!

Maybe we should discuss it in another thread, but I agree that we should 
fix it in someway. Thank you for raising this discussion!

>
>Regards,
>Halil
>---
> hw/virtio/vhost-vsock-common.c | 1 +
> 1 file changed, 1 insertion(+)

This patch LGTM, but as I mention we should fix other devices as well,
but maybe we can do with the common macro you suggested in another 
patch.

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
>index 12ea87d7a7..fd88df2560 100644
>--- a/hw/virtio/vhost-vsock-common.c
>+++ b/hw/virtio/vhost-vsock-common.c
>@@ -22,6 +22,7 @@
> const int feature_bits[] = {
>     VIRTIO_VSOCK_F_SEQPACKET,
>     VIRTIO_F_RING_RESET,
>+    VIRTIO_F_RING_PACKED,
>     VHOST_INVALID_FEATURE_BIT
> };
>
>
>base-commit: fd87be1dada5672f877e03c2ca8504458292c479
>-- 
>2.40.1
>
>