在 2022/2/1 下午6:57, Eugenio Perez Martin 写道:
On Mon, Jan 31, 2022 at 4:49 PM Eugenio Perez Martin
<eperezma@xxxxxxxxxx> wrote:
On Sat, Jan 29, 2022 at 9:11 AM Jason Wang <jasowang@xxxxxxxxxx> wrote:
在 2022/1/22 上午4:27, Eugenio Pérez 写道:
This allows SVQ to negotiate features with the device. For the device,
SVQ is a driver. While this function needs to bypass all non-transport
features, it needs to disable the features that SVQ does not support
when forwarding buffers. This includes packed vq layout, indirect
descriptors or event idx.
Signed-off-by: Eugenio Pérez <eperezma@xxxxxxxxxx>
---
hw/virtio/vhost-shadow-virtqueue.h | 2 ++
hw/virtio/vhost-shadow-virtqueue.c | 44 ++++++++++++++++++++++++++++++
hw/virtio/vhost-vdpa.c | 21 ++++++++++++++
3 files changed, 67 insertions(+)
diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
index c9ffa11fce..d963867a04 100644
--- a/hw/virtio/vhost-shadow-virtqueue.h
+++ b/hw/virtio/vhost-shadow-virtqueue.h
@@ -15,6 +15,8 @@
typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
+bool vhost_svq_valid_device_features(uint64_t *features);
+
void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd);
void vhost_svq_set_guest_call_notifier(VhostShadowVirtqueue *svq, int call_fd);
const EventNotifier *vhost_svq_get_dev_kick_notifier(
diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index 9619c8082c..51442b3dbf 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -45,6 +45,50 @@ const EventNotifier *vhost_svq_get_dev_kick_notifier(
return &svq->hdev_kick;
}
+/**
+ * Validate the transport device features that SVQ can use with the device
+ *
+ * @dev_features The device features. If success, the acknowledged features.
+ *
+ * Returns true if SVQ can go with a subset of these, false otherwise.
+ */
+bool vhost_svq_valid_device_features(uint64_t *dev_features)
+{
+ bool r = true;
+
+ for (uint64_t b = VIRTIO_TRANSPORT_F_START; b <= VIRTIO_TRANSPORT_F_END;
+ ++b) {
+ switch (b) {
+ case VIRTIO_F_NOTIFY_ON_EMPTY:
+ case VIRTIO_F_ANY_LAYOUT:
+ continue;
+
+ case VIRTIO_F_ACCESS_PLATFORM:
+ /* SVQ does not know how to translate addresses */
I may miss something but any reason that we need to disable
ACCESS_PLATFORM? I'd expect the vring helper we used for shadow
virtqueue can deal with vIOMMU perfectly.
This function is validating SVQ <-> Device communications features,
that may or may not be the same as guest <-> SVQ. These feature flags
are valid for guest <-> SVQ communication, same as with indirect
descriptors one.
Having said that, there is a point in the series where
VIRTIO_F_ACCESS_PLATFORM is actually mandatory, so I think we could
use the latter addition of x-svq cmdline parameter and delay the
feature validations where it makes more sense.
+ if (*dev_features & BIT_ULL(b)) {
+ clear_bit(b, dev_features);
+ r = false;
+ }
+ break;
+
+ case VIRTIO_F_VERSION_1:
I had the same question here.
For VERSION_1 it's easier to assume that guest is little endian at
some points, but we could try harder to support both endianness if
needed.
Re-thinking the SVQ feature isolation stuff for this first iteration
based on your comments.
Maybe it's easier to simply fail if the device does not *match* the
expected feature set, and add all of the "feature isolation" later.
While a lot of guest <-> SVQ communication details are already solved
for free with qemu's VirtQueue (indirect, packed, ...), we may
simplify this series in particular and add the support for it later.
For example, at this moment would be valid for the device to export
indirect descriptors feature flag, and SVQ simply forward that feature
flag offering to the guest. So the guest <-> SVQ communication could
have indirect descriptors (qemu's VirtQueue code handles it for free),
but SVQ would not acknowledge it for the device. As a side note, to
negotiate it would have been harmless actually, but it's not the case
of packed vq.
So maybe for the v2 we can simply force the device to just export the
strictly needed features and nothing else with qemu cmdline, and then
enable the feature negotiation isolation for each side of SVQ?
Yes, that's exactly my point.
Thanks
Thanks!
Thanks!
Thanks
+ /* SVQ trust that guest vring is little endian */
+ if (!(*dev_features & BIT_ULL(b))) {
+ set_bit(b, dev_features);
+ r = false;
+ }
+ continue;
+
+ default:
+ if (*dev_features & BIT_ULL(b)) {
+ clear_bit(b, dev_features);
+ }
+ }
+ }
+
+ return r;
+}
+
/* Forward guest notifications */
static void vhost_handle_guest_kick(EventNotifier *n)
{
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index bdb45c8808..9d801cf907 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -855,10 +855,31 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
size_t n_svqs = v->shadow_vqs_enabled ? hdev->nvqs : 0;
g_autoptr(GPtrArray) shadow_vqs = g_ptr_array_new_full(n_svqs,
vhost_psvq_free);
+ uint64_t dev_features;
+ uint64_t svq_features;
+ int r;
+ bool ok;
+
if (!v->shadow_vqs_enabled) {
goto out;
}
+ r = vhost_vdpa_get_features(hdev, &dev_features);
+ if (r != 0) {
+ error_setg(errp, "Can't get vdpa device features, got (%d)", r);
+ return r;
+ }
+
+ svq_features = dev_features;
+ ok = vhost_svq_valid_device_features(&svq_features);
+ if (unlikely(!ok)) {
+ error_setg(errp,
+ "SVQ Invalid device feature flags, offer: 0x%"PRIx64", ok: 0x%"PRIx64,
+ hdev->features, svq_features);
+ return -1;
+ }
+
+ shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_psvq_free);
for (unsigned n = 0; n < hdev->nvqs; ++n) {
VhostShadowVirtqueue *svq = vhost_svq_new();
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
