From: Halil Pasic <pasic@linux.ibm.com>
To: "Michael S. Tsirkin" <mst@redhat.com>, qemu-devel@nongnu.org
Cc: Halil Pasic <pasic@linux.ibm.com>,
Marc Hartmayer <mhartmay@linux.ibm.com>,
Jason Wang <jasowang@redhat.com>,
Christian Borntraeger <borntraeger@linux.ibm.com>,
Boris Fiuczynski <fiuczy@linux.ibm.com>
Subject: [PATCH 1/1] vhost-vsock: add VIRTIO_F_RING_PACKED to feaure_bits
Date: Mon, 29 Apr 2024 13:33:34 +0200 [thread overview]
Message-ID: <20240429113334.2454197-1-pasic@linux.ibm.com> (raw)
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(+)
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 reply other threads:[~2024-04-29 11:34 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-29 11:33 Halil Pasic [this message]
2024-05-07 19:26 ` [PATCH 1/1] vhost-vsock: add VIRTIO_F_RING_PACKED to feaure_bits Halil Pasic
2024-05-15 22:41 ` Halil Pasic
2024-05-16 8:39 ` 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=20240429113334.2454197-1-pasic@linux.ibm.com \
--to=pasic@linux.ibm.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=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).