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

On 03.08.26 21:31, Connor Kite wrote:
> 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.

Yes, that’s what I thought, to have an object that would hold everything 
that is used by the isolation code (and rather specific to the isolation 
code).

>> 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.

But that’s a guest-provided length, no? I believe the 128-bit operation 
there is done as a genuine check of invalid guest requests. Whereas 
here, `qemu_memfd_alloc()` already must not return a pointer such that 
adding the length of the allocated buffer would overflow (because it 
guarantees that the whole range from the pointer through the given 
length is valid, right).

> ...
>
>>> +        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?

I mean, in this case, I would have just used a plain C array because the 
size is pre-determined by `nregions`, right.

In general, *I* have not used GArray yet (no particular reason), which 
is why I had not come in contact with the `&g_array_index()` pattern 
yet, which had me stare in disbelief at first. (My main problem being 
that the name `g_array_index` is written in lowercase and thus indicates 
being a normal function, whereas macros in C are generally written in 
uppercase to allow at-a-glance distinction. So it pretends to be a 
normal function, but glib recommends a pattern of use 
(`&g_array_index()`) that would never work with a normal function, and 
that I find a dreadful decision by glib.)

Hanna