On Tue, Jul 28, 2026 at 10:59 AM Stefan Hajnoczi <[email protected]> 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 thinking:
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 >= 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 = {
> > + .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())?
>
Yes, I suppose this doesn't need to be assigned here. As it currently stands,
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
