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(®ion_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
