On Mon, Aug 3, 2026 at 6:25 AM Hanna Czenczek <[email protected]> wrote:
>
> > +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 live
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 = 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 = 0;
> > + size_t total_mmap_size;
>
> Please do not mix variable declarations and normal code (style.rst calls
> it “Mixed declarations”).
>
Got it! This will be fixed.
> > +
> > + /* Get space required for all vrings */
> > + for (int j = 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 = 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 >= u->iso_memory.base_addr).)
>
At the time I was concerned about potential overflow prior to application of
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 before
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 = &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
