On 24.07.26 00:30, Connor Kite wrote:
- Add start logic for shadow virtqueues, which sets vring addresses.
- Update logic for sending vring addresses to backend to point
to the shadow vrings when isolation mode is active.
- Implement handlers for intercepted avail and used descriptors. These
handlers copy buffer contents between bounce buffers in the isolation
region and the buffers made available by the guest
- Implement logic to stop svqs
Signed-off-by: Connor Kite <[email protected]>
---
hw/virtio/vhost-user.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 166 insertions(+), 1 deletion(-)
In general, I find it strange to use two completely separate data
structures for the address translation in either direction (avail/used).
I suppose we could have a map of in-flight translations, adding entries
in avail, and looking for them in used, removing them from the map.
I mean, sure, right now, we can calculate the addresses, because we
“duplicate” the whole range of guest memory for each device, but that is
clearly a waste and it would be better to use a smaller size in the
future; and then each request will need to allocate from that smaller
device memory, get a random address, and that will need such a map for
tracking.
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 75858289a2..e65f877f9a 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1299,6 +1299,97 @@ static int init_isolation_regions(struct vhost_dev *dev,
return 0;
}
+static int vhost_user_memory_lookup(struct vhost_dev *dev, hwaddr gpa,
+ hwaddr *hva)
+{
+ int i;
+ hwaddr offset;
+
+ for (i = 0; i < dev->mem->nregions; i++) {
+ struct vhost_memory_region *reg = dev->mem->regions + i;
+
+ if (gpa >= reg->guest_phys_addr &&
+ reg->guest_phys_addr + reg->memory_size > gpa) {
+ offset = gpa - reg->guest_phys_addr;
+ *hva = reg->userspace_addr + offset;
+ return 0;
+ }
+ }
+
+ return -EFAULT;
+}
+
+static int vhost_user_svq_handle_used(VhostShadowVirtqueue *svq,
+ VirtQueueElement *elem,
+ void *opaque)
+{
+ hwaddr hva;
+ int r;
+ struct vhost_dev *dev = opaque;
+
+ for (int i = 0; i < elem->in_num; i++) {
+ r = vhost_user_memory_lookup(dev, elem->in_addr[i], &hva);
This function only looks for the starting address, so it is not
guaranteed that the whole buffer is within one memory region.
+ if (r < 0) {
+ return r;
+ }
+
+ memcpy((void *) hva, elem->in_sg[i].iov_base, elem->in_sg[i].iov_len);
+ elem->in_sg[i].iov_base = (void *) hva;
+ }
+
+ for (int i = 0; i < elem->out_num; i++) {
+ r = vhost_user_memory_lookup(dev, elem->out_addr[i], &hva);
+ if (r < 0) {
+ return r;
+ }
+
+ memcpy((void *) hva, elem->out_sg[i].iov_base,
elem->out_sg[i].iov_len);
I don’t think we need this `memcpy()` here, as the device may only write
to IN buffers, not OUT buffers.
+ elem->out_sg[i].iov_base = (void *) hva;
+ }
+
+ return 0;
+}
+
+static int vhost_user_svq_handle_avail(VhostShadowVirtqueue *svq,
+ VirtQueueElement *elem,
+ void *opaque)
+{
+ hwaddr offset;
+ const DMAMap *map;
+ DMAMap needle;
+ hwaddr *iova_base;
+
+ for (int i = 0; i < elem->out_num; i++) {
+ needle.translated_addr = elem->out_addr[i];
+ needle.size = elem->out_sg[i].iov_len - 1;
+ map = vhost_iova_tree_find_gpa(svq->iova_tree, &needle);
Is it guaranteed that the returned `map` will fully contain `needle`?
+ offset = needle.translated_addr - map->translated_addr;
+ iova_base = (void *)(map->iova + offset);
+
+ elem->out_sg[i].iov_base = iova_base;
+ memcpy(iova_base, elem->out_sg[i].iov_base, needle.size + 1);
+ }
+
+ for (int i = 0; i < elem->in_num; i++) {
+ needle.translated_addr = elem->in_addr[i];
+ needle.size = elem->in_sg[i].iov_len - 1;
+ map = vhost_iova_tree_find_gpa(svq->iova_tree, &needle);
+ offset = needle.translated_addr - map->translated_addr;
+ iova_base = (void *)(map->iova + offset);
+
+ elem->in_sg[i].iov_base = iova_base;
+ memcpy(iova_base, elem->in_sg[i].iov_base, needle.size + 1);
I don’t think we need this `memcpy()` here as the device is to strictly
write to IN buffers, not read from them.
+ }
+
+ vhost_svq_add(svq, elem->out_sg, elem->out_num, elem->out_addr,
+ elem->in_sg, elem->in_num, elem->in_addr, elem);
Might make sense to make vhost_svq_add_element() public and use it here.
+
+ return 0;
+}
+
+
+
+
static int vhost_user_set_mem_table(struct vhost_dev *dev,
struct vhost_memory *mem)
{
@@ -1772,6 +1863,8 @@ static int vhost_user_set_vring_err(struct vhost_dev *dev,
static int vhost_user_set_vring_addr(struct vhost_dev *dev,
struct vhost_vring_addr *addr)
{
+ struct vhost_user *u = dev->opaque;
+
VhostUserMsg msg = {
.hdr.request = VHOST_USER_SET_VRING_ADDR,
.hdr.flags = VHOST_USER_VERSION,
@@ -1779,6 +1872,25 @@ static int vhost_user_set_vring_addr(struct vhost_dev
*dev,
.hdr.size = sizeof(msg.payload.addr),
};
+ if (u->user->memory_isolation) {
+ if (!u->svqs_allocated) {
+ return 0;
+ }
+
+ int svq_idx = addr->index - dev->vq_index;
+ VhostShadowVirtqueue *svq = g_ptr_array_index(u->shadow_vqs,
+ svq_idx);
Again, bounds checking via `vhost_user_get_vq_index()` would be good.
+
+ struct vhost_vring_addr svq_addr = {
+ .avail_user_addr = (uint64_t)(uintptr_t)svq->vring.avail,
+ .desc_user_addr = (uint64_t)(uintptr_t)svq->vring.desc,
+ .used_user_addr = (uint64_t)(uintptr_t)svq->vring.used,
+ .index = addr->index,
+ };
+
+ msg.payload.addr = svq_addr;
+ }
+
/*
* wait for a reply if logging is enabled to make sure
* backend is actually logging changes
@@ -2754,13 +2866,18 @@ static int
vhost_user_postcopy_notifier(NotifierWithReturn *notifier,
return 0;
}
+static const VhostShadowVirtqueueOps vhost_user_svq_ops = {
+ .avail_handler = vhost_user_svq_handle_avail,
+ .used_handler = vhost_user_svq_handle_used
+};
+
static void vhost_user_init_svq(struct vhost_dev *dev, struct vhost_user *u)
{
/*Modified from vhost-vdpa*/
u->shadow_vqs = g_ptr_array_new_full(dev->nvqs, vhost_svq_free);
for (int i = 0; i < dev->nvqs; i++) {
VhostShadowVirtqueue *svq;
- svq = vhost_svq_new(NULL, NULL);
+ svq = vhost_svq_new(&vhost_user_svq_ops, dev);
g_ptr_array_add(u->shadow_vqs, svq);
}
}
@@ -3466,8 +3583,56 @@ void vhost_user_async_close(DeviceState *d,
}
}
+static bool vhost_user_svqs_start(struct vhost_dev *dev)
+{
+ struct vhost_user *u = dev->opaque;
+ uint64_t vring_base = u->iso_memory.vring_base_addr;
+ u->svqs_allocated = true;
I don’t like setting this before it actually being the case very much. I
understand that it is necessary for how you decided to set up
`vhost_user_set_vring_addr()`, but there should at least be a comment
(because reading this code, there is the obvious question: Why is this
set when it is not true?); or the code should be different. I.e. having
a `vhost_user_set_shadow_vring_addr()` function, maybe, which would not
even take an `addr` argument and could be used both below and in
`vhost_user_set_vring_addr()`.
+
+ for (int i = 0; i < u->shadow_vqs->len; i++) {
+ VirtQueue *vq = virtio_get_queue(dev->vdev, dev->vq_index + i);
+ VhostShadowVirtqueue *svq = g_ptr_array_index(u->shadow_vqs, i);
+ svq->base_addr = (hwaddr *) vring_base;
I really do not like the lack of bounds checking here again. There
should be something, somewhere (i.e. in the “Isolated memory data”
portion, maybe in `struct IsolationRegion`), that tells how much memory
has been allocated for the shadow virtqueues, and we need to check that
we do not overflow.
+ vhost_svq_start(svq, dev->vdev, vq, u->iso_iova_tree);
+
+ struct vhost_vring_addr addr = {
+ .index = dev->vq_index + i,
+ .desc_user_addr = vring_base,
+ .avail_user_addr = vring_base + sizeof(vring_desc_t) *
+ svq->vring.num,
+ .used_user_addr = vring_base + vhost_svq_driver_area_size(svq)
All values set here are effectively ignored, right? Because
`vhost_user_set_vring_addr()` just overwrites them. (Having a
`vhost_user_set_shadow_vring_addr()` could maybe save us from having to
set this up.)
Not that it’s currently wrong to set them this way, just asking.
Hanna
+ };
+
+ vhost_user_set_vring_addr(dev, &addr);
+
+ vring_base += vhost_svq_device_area_size(svq) +
+ vhost_svq_driver_area_size(svq);
+ }
+
+ return false;
+}
+
+static void vhost_user_svqs_stop(struct vhost_dev *dev)
+{
+ struct vhost_user *u = dev->opaque;
+
+ for (int i = 0; i < u->shadow_vqs->len; i++) {
+ vhost_svq_stop(g_ptr_array_index(u->shadow_vqs, i));
+ }
+}
+
+
static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
{
+ struct vhost_user *u = dev->opaque;
+ if (u->user->memory_isolation) {
+ if (started) {
+ vhost_user_svqs_start(dev);
+ } else {
+ vhost_user_svqs_stop(dev);
+ }
+ }
+
if (!vhost_user_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_STATUS)) {
return 0;
}
