Re: [PATCH RFC v2 07/13] hw/virtio/vhost-user: create isolation region
Connor Kite <[email protected]>
| Newsgroups | dev.linux.lists.virtio-fs,org.nongnu.qemu-devel |
|---|---|
| Message-ID | <CA+spn3qk45YZC6FDMFk9FAr6aM=OziGfbeNwmScoz9DOU1rawA@mail.gmail.com> |
On Wed, Aug 19, 2026 at 12:36 AM Akihiko Odaki <[email protected]> wrote: > > > +/* Memory region shared with back-end when memory-isolation is active */ > > +typedef struct { > > + void *shared_mem_addr; /* mapped shared memory */ > > + VhostIOVATree *tree; /* controls mapping of regions into IOVA space */ > > + void *vring_hva_addr; /* beginning of vring region in shared memory */ > > + size_t vring_region_size; /* amount of shared memory reserved for vrings */ > > + size_t size; /* size of the mapped shared memory */ > > + int fd; /* descriptor of anonymous file backing shared iso region */ > > + Int128 iso_iova_offset; /* translation from IOVA to hva of iso region */ > > This Int128 usage is misleading. The value derived from iso_iova_offset > is always consumed with int128_get64(), which asserts the value fits in > 64-bit. But we actually need to enforce that the address before > translation resides in the shared memory. So making iso_iova_offset > Int128 is not a good way to assert the real invariant. > > Furthermore, "[PATCH RFC v2 10/13] hw/virtio/vhost-user: handle data > movement with shadow vqs" has: > > ptrdiff_t offset = u->iso_mem_ctx.iso_iova_offset; > > If defined(CONFIG_INT128) && !defined(CONFIG_TCG_INTERPRETER), this will > overflow with u->iso_mem_ctx.iso_iova_offset > PTRDIFF_MAX. Otherwise, > Int128 will be a struct, which causes a compilation error. > > uintptr_t is sufficient if the invariant is enforced in some alternative > way. A negative offset would be represented as wraparound, and a > positive one will never result in wraparound. Just always ensure the > address before translation resides in the shared memory. > > Regards, > Akihiko Odaki > Your point is taken on the Int128! I also got similar feedback from Stefan Hajnoczi and Hanna CzenCzek that it was unnecessary complication. Will move it to uintptr_t and double check the bounds-checking on translations in the later patches to confirm that it didn't depend on the Int128 type. Also, I thought I had moved all cases of ptrdiff_t to Int128, but I apparently missed that one. The offset must have been less than PTRDIFF_MAX in testing since this didn't come up. Will fix and change to uintptr_t as well. Thank you once again! Connor