Re: [PATCH RFC 12/15] hw/virtio/vhost-user: send isolation regions to device

On Mon, Aug 3, 2026 at 6:46 AM Hanna Czenczek <[email protected]> wrote:
>
>
> This function name doesn’t really say what the function does, and more
> describes its type.
>

I will change it to vhost_user_fill_msg_reg_from_tree since it is now doing
the filling directly rather than calling another function to do so.

> > +{
> > +    struct iova_tree_traversal_args *args = data;
> > +    struct vhost_memory_region msg_region;
> > +    VhostUserMemoryRegion region_buffer;
> > +    DMAMap *map = key;
> > +    args->fds[*args->fd_num] = args->u->iso_memory.iso_fd;
>
> Generally: I find it quite hard to read code where general statements
> come after a list of declarations without an empty line, especially if
> there is an empty line later. Visually, this last line to me looks like
> another variable declaration.
>

Got it!  The declarations and statements are now getting separated for clarity.

> > +
> > +    msg_region.guest_phys_addr = map->iova;
> > +    msg_region.memory_size = map->size + 1;
> > +    msg_region.userspace_addr = map->iova;
> > +    vhost_user_fill_msg_region_iso(&region_buffer, args->u, &msg_region);
> > +    args->msg->payload.memory.regions[*args->fd_num] = region_buffer;
>
> Why does `vhost_user_fill_msg_region_iso()` exist when this is the only
> caller, and we could just fill `regions[i]` here directly?
>
> > +    (*args->fd_num)++;
>

vhost_user_fill_msg_region_iso existed before the iova_tree was integrated in
the code base.  I agree that the traversal function can absorb it for
added clarity.

> As noted by Akihiko, there needs to be some bounds checking. At least
> against `VHOST_MEMORY_BASELINE_NREGIONS`, but even better against the
> array size of `args->msg->payload.memory.regions` and the caller should
> pass the size of `fds[]` via `args`.
>
> (And I’m also with Stefan, something with `index` would be better. Maybe
> `region_idx`.)
>

With the current state of the refactor, the size of fds should be the same as
dev->mem->nregions, so I will pass that value in via args and do the check.

The fd_num field is now called idx.

> > +
> > +    return false;
> > +}
> > +
> >   static int init_isolation_regions(struct vhost_dev *dev,
> >                                     VhostUserMsg *msg,
> >                                     int *fds, size_t *fd_num)
> > @@ -1234,6 +1275,24 @@ static int init_isolation_regions(struct vhost_dev *dev,
> >                                         map->translated_addr);
> >       }
> >
> > +    struct iova_tree_traversal_args args = {
> > +        .fd_num = fd_num,
> > +        .fds = fds,
> > +        .msg = msg,
> > +        .u = u
> > +    };
>
> Personal preference: I would like `*fd_num = 0` somewhere here, or
> `assert(*fd_num == 0)` before the iteration. Yes, the caller initializes
> it to zero, but it is just not obvious here, and it seems necessary for
> correct semantics.
>
> (And the open question whether `fd_num` should be renamed here, too, or
> can keep that name. Not sure.)
>

I will set *fd_num = 0 here.   I can see how it makes it easier to see
what is going on.

It may make sense to keep it named fd_num here.  Whereas its main use in the
traversal function is as an index, here and in vhost_user_set_mem_table it is
primarily providing a count of the number of elements in
msg->payload.memory.regions
that have been filled.

> > +
> > +    vhost_iova_tree_foreach(u->iso_iova_tree,
> > +                            vhost_user_iova_tree_traverse_funct, &args);
> > +
> > +    msg->payload.memory.nregions = *fd_num;
> > +
> > +    assert(*fd_num != 0);
>
> Am I wrong or should `*fd_num == nregions`? Is it possible that
> `*fd_num` is less because regions end up joined? Can you add a comment
> on when they would differ?
>
> Hanna

I updated this to check against nregions.  In the current revision, I
do not think
this assertion would be reached if there was an issue with allocating elements
on the iova-tree or filling out the message payloads, but the final sanity check
doesn't hurt.

Best,
Connor