On 18.08.26 07:12, Connor Kite wrote:
By default svq vrings are placed in an anonymous memory map. As svqs
will be leveraged to enable memory isolation in vhost-user, it is useful
to be able to place the vrings in a shared isolation memory region.
Adds the option to specify vring placement by providing a vring base
address before starting the svq.
Signed-off-by: Connor Kite <[email protected]>
---
hw/virtio/vhost-shadow-virtqueue.c | 69 ++++++++++++++++++++++++++++++++------
hw/virtio/vhost-shadow-virtqueue.h | 8 ++++-
2 files changed, 66 insertions(+), 11 deletions(-)
diff --git a/hw/virtio/vhost-shadow-virtqueue.c
b/hw/virtio/vhost-shadow-virtqueue.c
index 496e7e58a3..f54a61439a 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -812,6 +812,13 @@ size_t vhost_svq_device_area_size(const
VhostShadowVirtqueue *svq)
return ROUND_UP(used_size, qemu_real_host_page_size());
}
+size_t vhost_svq_vring_total_size(VirtIODevice *vdev, VirtQueue *vq)
If this function is not used in this patch, it probably should not be
introduced here.
+{
+ VhostShadowVirtqueue svq;
+ svq.vring.num = virtio_queue_get_num(vdev, virtio_get_queue_index(vq));
+ return vhost_svq_driver_area_size(&svq) + vhost_svq_device_area_size(&svq);
I’m not 100 % happy about this (initializing a pseudo svq), might be
better to make it so there are vhost_svq_driver/device_area_size()
variants that take the num directly, and then have
vhost_svq_driver/device_area_size() be wrappers around it, but, well. At
least the implementations are right above it so one can verify that
nothing else in VhostShadowVirtqueue is used. (It’s just that one should
not *have* to verify.)
+}
+
/**
* Set a new file descriptor for the guest to kick the SVQ and notify for
avail
*
@@ -842,6 +849,19 @@ void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq,
int svq_kick_fd)
}
}
+/**
+ * Set vring base address if using fixed locations
+ *
+ * @svq: Shadow Virtqueue
+ * @addr: Points to new base address
+ */
+
+ void vhost_svq_set_base_addr(VhostShadowVirtqueue *svq, void *addr)
+ {
+ svq->base_addr = addr;
+ }
+
+
/**
* Start the shadow virtqueue operation.
*
@@ -849,8 +869,10 @@ void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq,
int svq_kick_fd)
* @vdev: VirtIO device
* @vq: Virtqueue to shadow
* @iova_tree: Tree to perform descriptors translations
+ *
+ * Return 0 on success, -errno on failure
*/
-void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
+int vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
VirtQueue *vq, VhostIOVATree *iova_tree)
{
size_t desc_size;
@@ -868,14 +890,27 @@ void vhost_svq_start(VhostShadowVirtqueue *svq,
VirtIODevice *vdev,
svq->vring.num = virtio_queue_get_num(vdev, virtio_get_queue_index(vq));
svq->num_free = svq->vring.num;
- svq->vring.desc = mmap(NULL, vhost_svq_driver_area_size(svq),
- PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS,
- -1, 0);
desc_size = sizeof(vring_desc_t) * svq->vring.num;
- svq->vring.avail = (void *)((char *)svq->vring.desc + desc_size);
- svq->vring.used = mmap(NULL, vhost_svq_device_area_size(svq),
- PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS,
- -1, 0);
+ if (svq->base_addr == NULL) {
+ svq->vring.desc = mmap(NULL, vhost_svq_driver_area_size(svq),
+ PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS,
+ -1, 0);
+ svq->vring.avail = (void *)((char *)svq->vring.desc + desc_size);
+ svq->vring.used = mmap(NULL, vhost_svq_device_area_size(svq),
+ PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS,
+ -1, 0);
+ } else {
+ svq->vring.desc = (void *)svq->base_addr;
+ svq->vring.avail = (void *)((char *)svq->vring.desc + desc_size);
+ svq->vring.used = (void *)((char *)svq->base_addr +
+ vhost_svq_driver_area_size(svq));
+
+ if ((uint64_t)svq->vring.used + vhost_svq_device_area_size(svq) - 1 <
+ (uint64_t)svq->vring.desc) {
+ error_report("Invalid shadow vring location");
+ return -ENOMEM;
+ }
Why does this check exist?
What needs to be done here is proper bounds checking. That either `.used
+ device_area_size()` or `desc + vhost_svq_vring_total_size()` does not
exceed the end of the memory region that has been allocated for the
vrings. It is clear that such an allocation would never wrap around the
end of memory because no allocation ever does.
+ }
svq->desc_state = g_new0(SVQDescState, svq->vring.num);
if (virtio_vdev_has_feature(svq->vdev, VIRTIO_F_IN_ORDER)) {
svq->batch_last.id = VIRTIO_RING_NOT_IN_BATCH;
@@ -884,6 +919,8 @@ void vhost_svq_start(VhostShadowVirtqueue *svq,
VirtIODevice *vdev,
svq->desc_state[i].next = i + 1;
}
}
+
+ return 0;
}
/**
@@ -920,8 +957,19 @@ void vhost_svq_stop(VhostShadowVirtqueue *svq)
}
svq->vq = NULL;
g_free(svq->desc_state);
- munmap(svq->vring.desc, vhost_svq_driver_area_size(svq));
- munmap(svq->vring.used, vhost_svq_device_area_size(svq));
+
+ if (!svq->base_addr) {
+ munmap(svq->vring.desc, vhost_svq_driver_area_size(svq));
+ munmap(svq->vring.used, vhost_svq_device_area_size(svq));
+ } else{
+ if (svq->vring.desc) {
+ memset(svq->vring.desc, 0, vhost_svq_driver_area_size(svq));
+ }
+ if (svq->vring.used) {
+ memset(svq->vring.used, 0, vhost_svq_device_area_size(svq));
+ }
Why the memset()?
Hanna
+ }
+
event_notifier_set_handler(&svq->hdev_call, NULL);
}
@@ -940,6 +988,7 @@ VhostShadowVirtqueue *vhost_svq_new(const VhostShadowVirtqueueOps *ops,
event_notifier_init_fd(&svq->svq_kick, VHOST_FILE_UNBIND);
svq->ops = ops;
svq->ops_opaque = ops_opaque;
+ svq->base_addr = NULL;
return svq;
}
diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
index fd68319fb7..1e0cc9e5e4 100644
--- a/hw/virtio/vhost-shadow-virtqueue.h
+++ b/hw/virtio/vhost-shadow-virtqueue.h
@@ -150,6 +150,9 @@ typedef struct VhostShadowVirtqueue {
/* Size of SVQ vring free descriptors */
uint16_t num_free;
+
+ /* Location assigned to vrings if not in default anon memory map */
+ void *base_addr;
} VhostShadowVirtqueue;
bool vhost_svq_valid_features(uint64_t features, Error **errp);
@@ -169,8 +172,9 @@ void vhost_svq_get_vring_addr(const VhostShadowVirtqueue
*svq,
struct vhost_vring_addr *addr);
size_t vhost_svq_driver_area_size(const VhostShadowVirtqueue *svq);
size_t vhost_svq_device_area_size(const VhostShadowVirtqueue *svq);
+size_t vhost_svq_vring_total_size(VirtIODevice *vdev, VirtQueue *vq);
-void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
+int vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
VirtQueue *vq, VhostIOVATree *iova_tree);
void vhost_svq_stop(VhostShadowVirtqueue *svq);
@@ -178,6 +182,8 @@ VhostShadowVirtqueue *vhost_svq_new(const VhostShadowVirtqueueOps *ops,
void *ops_opaque);
void vhost_svq_free(gpointer vq);
+void vhost_svq_set_base_addr(VhostShadowVirtqueue *svq, void *addr);
+
G_DEFINE_AUTOPTR_CLEANUP_FUNC(VhostShadowVirtqueue, vhost_svq_free);
#endif
