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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.