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
