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

On Tue, Jul 28, 2026 at 10:59=E2=80=AFAM Stefan Hajnoczi <[email protected]=
om> wrote:
> >
> > +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;
>

Agreed on adding clarification.  Is there a reason to change from uint64_t
to void * for referencing the shared memory region address?  Most uses of
that variable expect uint64_t currently.  Here's what I am currently thinki=
ng:

typedef struct IsolationRegion {
    uint64_t shared_mem_addr; /* mapped shared memory */
    uint64_t vring_iova_addr; /* beginning of vring region in shared memory=
 */
    size_t size;
    int fd; /* shared memory iova */
} IsolationRegion;

...
> > +
> > +    /* Isolated memory data*/
> > +    struct IsolationRegion iso_memory;
>
> struct is not necessary since there is a typedef:
>
>   IsolationRegion iso_memory;
>

Agreed!  This will be fixed.

> > +    VhostIOVATree *iso_iova_tree;
>
> The IOVA tree seems to be closely used with IsolationRegion. Maybe this
> field should move into IsolationRegion?
>

Agreed.  Since the IOVA tree is basically tracking mappings to the
isolation regions, these can comfortably be grouped together.

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

Agreed.  Will fix this.

...

>
> 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.
>

I think I originally did this because the svqs weren't initialized yet
at this stage.
However, I can cut down on the duplication with a new function in
vhost-shadow-virtqueue that makes a dummy svq with the correct num value
and calls the existing functions to get device and driver area sizes.
...

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

I will do this by prepending "iso_mem_" to dev->vdev->name.

...

>
> 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.
>

That should be possible.

> > +
> > +    assert(&u->iso_memory.iso_fd >=3D 0);
>
> The dereference operator should not be used here, it's the iso_fd value
> that is being tested.
>

Got it.  This will be fixed.

> > +    DMAMap *map;
> > +    DMAMap vring_map =3D {
> > +        .perm =3D IOMMU_RW,
> > +        .size =3D total_vring_size - 1,
> > +        /*vrings are allocated on tree first, so will be assigned base=
 addr*/
> > +        .translated_addr =3D 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())?
>

Yes, I suppose this doesn't need to be assigned here.  As it currently stan=
ds,
that value is just getting assigned back to itself in
vhost_iova_tree_map_alloc().  I'll fix this for the values in
`buffer_regions`, too