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

On Mon, Aug 3, 2026 at 6:25=E2=80=AFAM Hanna Czenczek <[email protected]> w=
rote:
>
> > +typedef struct IsolationRegion {
> > +    uint64_t base_addr;
> > +    uint64_t vring_base_addr;
> > +    uint64_t size;
> > +    int iso_fd;
> > +} IsolationRegion;
> > +
> Going beyond Stefan, I would say please document each field with a
> proper doc comment, and ideally the whole struct, too.
>

Roger.  I will comment all fields, except for perhaps size, which seems
self-explanatory.

...

> In general: Please put spaces after the leading /* and before the
> trailing */.
>

Roger!  I believe I have this in a few places and will correct.

...

>
> Patch 13 adds more variables to this, so I think it may make sense to
> have a dedicated struct for these fields.
>

I agree that some of these make sense to be consolidated. However,
I think I will keep the svq array outside of the struct as the svqs don't l=
ive
inside of the isolation region.

I guess the alternative would be to have an `isolation_mode_ctx`
type of struct to hold all of the above.

>
> As Akihiko said, the memset() should come after this, and also...
>
> > +        u->iso_memory.base_addr =3D 0;
>
> ...this is just a subset of the memset().
>

Yes, the cleanup was not fully baked ahead of the RFC post.
This is currently fixed on my end and will make it into the next
rev.

...

> > +    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 =3D 0;
> > +    size_t total_mmap_size;
>
> Please do not mix variable declarations and normal code (style.rst calls
> it =E2=80=9CMixed declarations=E2=80=9D).
>

Got it!  This will be fixed.

> > +
> > +    /* Get space required for all vrings */
> > +    for (int j =3D 0; j < dev->nvqs; j++) {
>
> Why j here and i above and below?
>

Most likely this was inside another loop on the initial implementation, and=
 the
iteration variable name was not changed when the outer loop was removed.
I will change this for next rev.

...

> > +
> > +    uint64_t last_addr =3D int128_get64(int128_add(u->iso_memory.base_=
addr,
> > +                                                 total_mmap_size - 1))=
;
>
> I would like a comment why you went for a 128-bit operation here. I
> assume it is because it checks for overflow, turning overflow into an
> assertion failure, and it can be assumed `qemu_memfd_alloc()` must
> naturally return a pointer such that adding the length of the allocated
> area to it (minus one) will never overflow?
>
> The question is, do we even need to check for overflow then. Not that I
> mind it, in principle, I just find it non-obvious. (The more obvious
> check (imho) would be to just do a 64-bit operation and then
> assert(last_addr >=3D u->iso_memory.base_addr).)
>

At the time I was concerned about potential overflow prior to application o=
f
the -1 operand in case the last memory location was at 0xFFFFFFFFFFFFFFFF.
However, in that case, the -1 would cause the value to wrap back around bef=
ore
any use of the value, so my concern was not needed.

There is a similar 128-bit op in the existing code in
vhost-shadow-virtqueue.c at
line 133.  To my eye, that might also be a good candidate for
simplification unless
I'm missing some other aspect of the use case.

...

> > +        map =3D &g_array_index(buffer_regions, DMAMap, i);
>
> God, I *really*, *really* hate this, and find it really disgusting that
> the documentation actually recommends doing this (`&g_array_index()`)
> instead of just offering a separate macro to get a reference.
>
> And existing qemu code does it all over the place, too.
>
> So I cannot really fault you for it.
>
> Still. Too ugly for me to keep completely silent about it.
>
> </rant>
>
> Hanna
>

Would you typically avoid using g_array / g_array_index?

Best,
Connor