From: Stefano Garzarella <sgarzare@redhat.com>
To: Halil Pasic <pasic@linux.ibm.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>,
qemu-devel@nongnu.org, Marc Hartmayer <mhartmay@linux.ibm.com>,
Jason Wang <jasowang@redhat.com>,
Christian Borntraeger <borntraeger@linux.ibm.com>,
Boris Fiuczynski <fiuczy@linux.ibm.com>
Subject: Re: [PATCH 1/1] vhost-vsock: add VIRTIO_F_RING_PACKED to feaure_bits
Date: Thu, 16 May 2024 10:39:42 +0200 [thread overview]
Message-ID: <ps5dukcjk6yh3an3hlkynr227r7kcln7b5dxgwope62avz5ceo@decy6vkuu56j> (raw)
In-Reply-To: <20240429113334.2454197-1-pasic@linux.ibm.com>
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
>
>
next prev parent reply other threads:[~2024-05-16 8:40 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-29 11:33 [PATCH 1/1] vhost-vsock: add VIRTIO_F_RING_PACKED to feaure_bits Halil Pasic
2024-05-07 19:26 ` Halil Pasic
2024-05-15 22:41 ` Halil Pasic
2024-05-16 8:39 ` Stefano Garzarella [this message]
2024-05-27 11:27 ` Halil Pasic
2024-05-28 3:25 ` Jason Wang
2024-05-29 12:17 ` Halil Pasic
2024-05-30 2:34 ` Jason Wang
2024-06-03 11:44 ` Halil Pasic
2024-05-28 15:32 ` Stefano Garzarella
2024-05-29 12:49 ` Halil Pasic
2024-05-30 7:56 ` Stefano Garzarella
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ps5dukcjk6yh3an3hlkynr227r7kcln7b5dxgwope62avz5ceo@decy6vkuu56j \
--to=sgarzare@redhat.com \
--cc=borntraeger@linux.ibm.com \
--cc=fiuczy@linux.ibm.com \
--cc=jasowang@redhat.com \
--cc=mhartmay@linux.ibm.com \
--cc=mst@redhat.com \
--cc=pasic@linux.ibm.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).