Re: [PATCH RFC 11/15] hw/virtio/vhost-user: create isolation region

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 8880 bytes --]

On Thu, Jul 23, 2026 at 03:30:10PM -0700, Connor Kite wrote:
> If memory isolation mode is active for the vhost-user device adds
> features to:
> - Gather the size required for bounce buffers and vrings in shared
>   isolation region
> - Allocate the required space in an anonymous file
> - Create a vhost-iova-tree with space to map entire isolation region
> - Map guest memory regions and shared vrings into the tree
> - Release these resources upon backend cleanup
> 
> Signed-off-by: Connor Kite <connorkite@gmail.com>
> ---
>  hw/virtio/vhost-user.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 129 insertions(+)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index f296b63fb9..710cf966f8 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -18,6 +18,7 @@
>  #include "hw/virtio/vhost-backend.h"
>  #include "hw/virtio/virtio.h"
>  #include "hw/virtio/virtio-net.h"
> +#include "hw/virtio/vhost-iova-tree.h"
>  #include "chardev/char-fe.h"
>  #include "io/channel-socket.h"
>  #include "system/kvm.h"
> @@ -25,6 +26,7 @@
>  #include "qemu/main-loop.h"
>  #include "qemu/uuid.h"
>  #include "qemu/sockets.h"
> +#include "qemu/memfd.h"
>  #include "system/runstate.h"
>  #include "system/cryptodev.h"
>  #include "migration/postcopy-ram.h"
> @@ -320,6 +322,13 @@ static VhostUserMsg m __attribute__ ((unused));
>  /* The version of the protocol we support */
>  #define VHOST_USER_VERSION    (0x1)
>  
> +typedef struct IsolationRegion {
> +    uint64_t base_addr;
> +    uint64_t vring_base_addr;
> +    uint64_t size;
> +    int iso_fd;
> +} IsolationRegion;

The purpose of the base_addr and vring_base_addr fields is not obvious.
I suggest adjusting the types, names, and adding comments to make the
purpose clearer:

  typedef struct {
      void *mem;           /* mapped shared memory */
      size_t size;
      uint64_t vring_iova;
      int fd;              /* shared memory fd */
  } IsolationRegion;

> +
>  struct vhost_user {
>      struct vhost_dev *dev;
>      /* Shared between vhost devs of the same virtio device */
> @@ -353,6 +362,10 @@ struct vhost_user {
>       * by the backend (see @features).
>       */
>      uint64_t protocol_features;
> +
> +    /* Isolated memory data*/
> +    struct IsolationRegion iso_memory;

struct is not necessary since there is a typedef:

  IsolationRegion iso_memory;

> +    VhostIOVATree *iso_iova_tree;

The IOVA tree seems to be closely used with IsolationRegion. Maybe this
field should move into IsolationRegion?

>  };
>  
>  struct scrub_regions {
> @@ -1109,6 +1122,121 @@ static int vhost_user_set_mem_table_postcopy(struct vhost_dev *dev,
>      return 0;
>  }
>  
> +/* TODO: Is there any notifier cleanup required here?*/
> +static void cleanup_isolation_regions(struct vhost_dev *dev)
> +{
> +    struct vhost_user *u = dev->opaque;
> +    if (u->iso_memory.base_addr) {
> +        vhost_iova_tree_delete(u->iso_iova_tree);
> +        u->iso_iova_tree = NULL;
> +        memset(&u->iso_memory, 0, sizeof(IsolationRegion));
> +        qemu_memfd_free((gpointer) u->iso_memory.base_addr, u->iso_memory.size,
> +                         u->iso_memory.iso_fd);
> +        u->iso_memory.base_addr = 0;
> +    }
> +}
> +
> +__attribute__((unused))
> +static int init_isolation_regions(struct vhost_dev *dev,
> +                                  VhostUserMsg *msg,
> +                                  int *fds, size_t *fd_num)
> +{
> +    Error *err = NULL;
> +    struct vhost_user *u = dev->opaque;
> +    uint32_t nregions = dev->mem->nregions;
> +    uint64_t buffer_reg_size = 0;
> +    DMAMap newEntry = {
> +        .perm = IOMMU_RW
> +    };
> +    g_autoptr(GArray) buffer_regions =
> +        g_array_new(FALSE, TRUE, sizeof(DMAMap));
> +
> +    msg->hdr.request = VHOST_USER_SET_MEM_TABLE;
> +
> +    /* In case of reset, clear old regions*/
> +    if (u->iso_memory.base_addr != 0) {
> +        cleanup_isolation_regions(dev);
> +        vhost_iova_tree_delete(u->iso_iova_tree);
> +    }
> +
> +    /* Gather information for bounce buffers to be mapped */
> +    for (int i = 0; i < nregions; i++) {

nregions is uint32_t, so i should also be uint32_t to avoid
signed/unsigned comparisons.

> +        struct vhost_memory_region *dev_region = &dev->mem->regions[i];
> +        hwaddr size = ROUND_UP(dev_region->memory_size,
> +                      qemu_real_host_page_size());
> +        newEntry.translated_addr = dev_region->guest_phys_addr;
> +        newEntry.size = size - 1;
> +        buffer_reg_size += size;
> +        g_array_append_val(buffer_regions, newEntry);
> +    }
> +
> +    int num;
> +    size_t desc_size;
> +    size_t avail_size;
> +    size_t driver_area_size;
> +    size_t device_area_size;
> +    size_t total_vring_size = 0;
> +    size_t total_mmap_size;
> +
> +    /* Get space required for all vrings */
> +    for (int j = 0; j < dev->nvqs; j++) {
> +        num = virtio_queue_get_num(dev->vdev, dev->vq_index + j);
> +        desc_size = sizeof(vring_desc_t) * num;
> +        avail_size = offsetof(vring_avail_t, ring[num]) +
> +                        sizeof(uint16_t);
> +        driver_area_size = ROUND_UP(desc_size + avail_size,
> +                            qemu_real_host_page_size());
> +        device_area_size = ROUND_UP(offsetof(vring_used_t, ring[num]) +
> +                                    sizeof(uint16_t),
> +                                    qemu_real_host_page_size());
> +        total_vring_size += driver_area_size + device_area_size;

Please avoid duplicating the memory layout calculations. When packed
vring support is added to vhost-shadow-virtqueue.c this will become more
complex and it should be done in a single place. vhost-shadow-virtque.c
should expose an API for the size calculation.

> +    }
> +
> +    total_mmap_size = buffer_reg_size + total_vring_size;
> +
> +    /* Allocate and map an anonymous file to hold the isolation region */
> +    u->iso_memory.base_addr = (uint64_t) qemu_memfd_alloc("iso_r",

Please make the name unique (e.g. using dev->vdev->name).

> +                              total_mmap_size,
> +                              F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
> +                              &u->iso_memory.iso_fd, &err);
> +    u->iso_memory.size = total_mmap_size;
> +
> +    if (err) {
> +        error_report_err(err);
> +        cleanup_isolation_regions(dev);
> +        return -1;
> +    }
> +
> +    uint64_t last_addr = int128_get64(int128_add(u->iso_memory.base_addr,
> +                                                 total_mmap_size - 1));
> +
> +    /*
> +     * Instantiates iova tree sized to map bounce buffers and vrings to the
> +     * isolation region in host va.
> +     */
> +    u->iso_iova_tree = vhost_iova_tree_new(u->iso_memory.base_addr, last_addr);

Is it possible to use 0 as the IOVA base address so that QEMU's
addresses aren't leaked to the vhost-user back-end? It's good security
practice not to reveal memory addresses to the outside world because
that information can be used to defeat address space randomization or
infer memory addresses of other data structures.

> +
> +    assert(&u->iso_memory.iso_fd >= 0);

The dereference operator should not be used here, it's the iso_fd value
that is being tested.

> +    DMAMap *map;
> +    DMAMap vring_map = {
> +        .perm = IOMMU_RW,
> +        .size = total_vring_size - 1,
> +        /*vrings are allocated on tree first, so will be assigned base addr*/
> +        .translated_addr = u->iso_memory.base_addr

Why is this field assigned here, I think this field is used as the
output of vhost_iova_tree_map_alloc() rather than an input (e.g. see
vhost_vdpa_svq_map_rings())?

> +    };
> +
> +    vhost_iova_tree_map_alloc(u->iso_iova_tree, &vring_map,
> +                              vring_map.translated_addr);
> +    u->iso_memory.vring_base_addr = vring_map.iova;
> +    for (int i = 0; i < buffer_regions->len; i++) {
> +        map = &g_array_index(buffer_regions, DMAMap, i);
> +        vhost_iova_tree_map_alloc_gpa(u->iso_iova_tree, map,
> +                                      map->translated_addr);
> +    }
> +
> +    return 0;
> +}
> +
>  static int vhost_user_set_mem_table(struct vhost_dev *dev,
>                                      struct vhost_memory *mem)
>  {
> @@ -2681,6 +2809,7 @@ static int vhost_user_backend_cleanup(struct vhost_dev *dev)
>      g_free(u->region_rb_offset);
>      u->region_rb_offset = NULL;
>      u->region_rb_len = 0;
> +    cleanup_isolation_regions(dev);
>      g_free(u);
>      dev->opaque = 0;
>  
> 
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]