1. Patchew
  2. QEMU
  3. [PATCH v1 00/15] libvhost-user: support more memslots and cleanup memslot handling code
  4. View patch

[PATCH v1 01/15] libvhost-user: Fix msg_region->userspace_addr computation

David Hildenbrand posted 15 patches 9 months, 3 weeks ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>
There is a newer version of this series
[PATCH v1 01/15] libvhost-user: Fix msg_region->userspace_addr computation
Posted by David Hildenbrand 9 months, 3 weeks ago 
We barely had mmap_offset set in the past. With virtio-mem and
dynamic-memslots that will change.

In vu_add_mem_reg() and vu_set_mem_table_exec_postcopy(), we are
performing pointer arithmetics, which is wrong. Let's simply
use dev_region->mmap_addr instead of "void *mmap_addr".

Fixes: ec94c8e621de ("Support adding individual regions in libvhost-user")
Fixes: 9bb38019942c ("vhost+postcopy: Send address back to qemu")
Cc: Raphael Norwitz <raphael.norwitz@nutanix.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 subprojects/libvhost-user/libvhost-user.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index a3b158c671..7e515ed15d 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -800,8 +800,8 @@ vu_add_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
          * Return the address to QEMU so that it can translate the ufd
          * fault addresses back.
          */
-        msg_region->userspace_addr = (uintptr_t)(mmap_addr +
-                                                 dev_region->mmap_offset);
+        msg_region->userspace_addr = dev_region->mmap_addr +
+                                     dev_region->mmap_offset;
 
         /* Send the message back to qemu with the addresses filled in. */
         vmsg->fd_num = 0;
@@ -969,8 +969,8 @@ vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg *vmsg)
         /* Return the address to QEMU so that it can translate the ufd
          * fault addresses back.
          */
-        msg_region->userspace_addr = (uintptr_t)(mmap_addr +
-                                                 dev_region->mmap_offset);
+        msg_region->userspace_addr = dev_region->mmap_addr +
+                                     dev_region->mmap_offset;
         close(vmsg->fds[i]);
     }
 
-- 
2.43.0
Re: [PATCH v1 01/15] libvhost-user: Fix msg_region->userspace_addr computation
Posted by Michael S. Tsirkin 9 months, 2 weeks ago 
On Fri, Feb 02, 2024 at 10:53:18PM +0100, David Hildenbrand wrote:
> We barely had mmap_offset set in the past. With virtio-mem and
> dynamic-memslots that will change.
> 
> In vu_add_mem_reg() and vu_set_mem_table_exec_postcopy(), we are
> performing pointer arithmetics, which is wrong.

Wrong how? I suspect you mean arithmetic on void * pointers is not portable?

> Let's simply
> use dev_region->mmap_addr instead of "void *mmap_addr".
> 
> Fixes: ec94c8e621de ("Support adding individual regions in libvhost-user")
> Fixes: 9bb38019942c ("vhost+postcopy: Send address back to qemu")
> Cc: Raphael Norwitz <raphael.norwitz@nutanix.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  subprojects/libvhost-user/libvhost-user.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index a3b158c671..7e515ed15d 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -800,8 +800,8 @@ vu_add_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
>           * Return the address to QEMU so that it can translate the ufd
>           * fault addresses back.
>           */
> -        msg_region->userspace_addr = (uintptr_t)(mmap_addr +
> -                                                 dev_region->mmap_offset);
> +        msg_region->userspace_addr = dev_region->mmap_addr +
> +                                     dev_region->mmap_offset;
>  
>          /* Send the message back to qemu with the addresses filled in. */
>          vmsg->fd_num = 0;
> @@ -969,8 +969,8 @@ vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg *vmsg)
>          /* Return the address to QEMU so that it can translate the ufd
>           * fault addresses back.
>           */
> -        msg_region->userspace_addr = (uintptr_t)(mmap_addr +
> -                                                 dev_region->mmap_offset);
> +        msg_region->userspace_addr = dev_region->mmap_addr +
> +                                     dev_region->mmap_offset;
>          close(vmsg->fds[i]);
>      }
>  
> -- 
> 2.43.0
Re: [PATCH v1 01/15] libvhost-user: Fix msg_region->userspace_addr computation
Posted by David Hildenbrand 9 months, 2 weeks ago 
On 13.02.24 18:32, Michael S. Tsirkin wrote:
> On Fri, Feb 02, 2024 at 10:53:18PM +0100, David Hildenbrand wrote:
>> We barely had mmap_offset set in the past. With virtio-mem and
>> dynamic-memslots that will change.
>>
>> In vu_add_mem_reg() and vu_set_mem_table_exec_postcopy(), we are
>> performing pointer arithmetics, which is wrong.
> 
> Wrong how? I suspect you mean arithmetic on void * pointers is not portable?

You are absolutely right, no idea how I concluded that we might have a 
different pointer size here.

I'll either convert this patch into a cleanup or drop it for v2, thanks!

-- 
Cheers,

David / dhildenb
Re: [PATCH v1 01/15] libvhost-user: Fix msg_region->userspace_addr computation
Posted by Raphael Norwitz 9 months, 3 weeks ago 
As a heads up, I've left Nutanix and updated it in MAINTAINERS. Will
be updating it again shortly so tagging these with my new work email.

On Fri, Feb 2, 2024 at 4:54 PM David Hildenbrand <david@redhat.com> wrote:
>
> We barely had mmap_offset set in the past. With virtio-mem and
> dynamic-memslots that will change.
>
> In vu_add_mem_reg() and vu_set_mem_table_exec_postcopy(), we are
> performing pointer arithmetics, which is wrong. Let's simply
> use dev_region->mmap_addr instead of "void *mmap_addr".
>
> Fixes: ec94c8e621de ("Support adding individual regions in libvhost-user")
> Fixes: 9bb38019942c ("vhost+postcopy: Send address back to qemu")
> Cc: Raphael Norwitz <raphael.norwitz@nutanix.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Raphael Norwitz <raphael@enfabrica.net>

> ---
>  subprojects/libvhost-user/libvhost-user.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index a3b158c671..7e515ed15d 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -800,8 +800,8 @@ vu_add_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
>           * Return the address to QEMU so that it can translate the ufd
>           * fault addresses back.
>           */
> -        msg_region->userspace_addr = (uintptr_t)(mmap_addr +
> -                                                 dev_region->mmap_offset);
> +        msg_region->userspace_addr = dev_region->mmap_addr +
> +                                     dev_region->mmap_offset;
>
>          /* Send the message back to qemu with the addresses filled in. */
>          vmsg->fd_num = 0;
> @@ -969,8 +969,8 @@ vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg *vmsg)
>          /* Return the address to QEMU so that it can translate the ufd
>           * fault addresses back.
>           */
> -        msg_region->userspace_addr = (uintptr_t)(mmap_addr +
> -                                                 dev_region->mmap_offset);
> +        msg_region->userspace_addr = dev_region->mmap_addr +
> +                                     dev_region->mmap_offset;
>          close(vmsg->fds[i]);
>      }
>
> --
> 2.43.0
>
>
Re: [PATCH v1 01/15] libvhost-user: Fix msg_region->userspace_addr computation
Posted by David Hildenbrand 9 months, 3 weeks ago 
On 04.02.24 02:35, Raphael Norwitz wrote:
> As a heads up, I've left Nutanix and updated it in MAINTAINERS. Will
> be updating it again shortly so tagging these with my new work email.
> 

Thanks for the fast review! The mail server already complained to me :)

Maybe consider adding yourself as reviewer for vhost as well? (which 
covers libvhost-user), I took your mail address from git history, not 
get_maintainers.pl.

> On Fri, Feb 2, 2024 at 4:54 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> We barely had mmap_offset set in the past. With virtio-mem and
>> dynamic-memslots that will change.
>>
>> In vu_add_mem_reg() and vu_set_mem_table_exec_postcopy(), we are
>> performing pointer arithmetics, which is wrong. Let's simply
>> use dev_region->mmap_addr instead of "void *mmap_addr".
>>
>> Fixes: ec94c8e621de ("Support adding individual regions in libvhost-user")
>> Fixes: 9bb38019942c ("vhost+postcopy: Send address back to qemu")
>> Cc: Raphael Norwitz <raphael.norwitz@nutanix.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> Reviewed-by: Raphael Norwitz <raphael@enfabrica.net>


-- 
Cheers,

David / dhildenb
Re: [PATCH v1 01/15] libvhost-user: Fix msg_region->userspace_addr computation
Posted by Raphael Norwitz 9 months, 3 weeks ago 
On Sun, Feb 4, 2024 at 9:36 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 04.02.24 02:35, Raphael Norwitz wrote:
> > As a heads up, I've left Nutanix and updated it in MAINTAINERS. Will
> > be updating it again shortly so tagging these with my new work email.
> >
>
> Thanks for the fast review! The mail server already complained to me :)
>
> Maybe consider adding yourself as reviewer for vhost as well? (which
> covers libvhost-user), I took your mail address from git history, not
> get_maintainers.pl.

I don't expect I'll have much time to review code outside of
vhost-user-blk/vhost-user-scsi, but happy to add an entry if it helps
folks tag me on relevant patches.

>
> > On Fri, Feb 2, 2024 at 4:54 PM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> We barely had mmap_offset set in the past. With virtio-mem and
> >> dynamic-memslots that will change.
> >>
> >> In vu_add_mem_reg() and vu_set_mem_table_exec_postcopy(), we are
> >> performing pointer arithmetics, which is wrong. Let's simply
> >> use dev_region->mmap_addr instead of "void *mmap_addr".
> >>
> >> Fixes: ec94c8e621de ("Support adding individual regions in libvhost-user")
> >> Fixes: 9bb38019942c ("vhost+postcopy: Send address back to qemu")
> >> Cc: Raphael Norwitz <raphael.norwitz@nutanix.com>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >
> > Reviewed-by: Raphael Norwitz <raphael@enfabrica.net>
>
>
> --
> Cheers,
>
> David / dhildenb
>
Re: [PATCH v1 01/15] libvhost-user: Fix msg_region->userspace_addr computation
Posted by David Hildenbrand 9 months, 3 weeks ago 
On 04.02.24 23:01, Raphael Norwitz wrote:
> On Sun, Feb 4, 2024 at 9:36 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 04.02.24 02:35, Raphael Norwitz wrote:
>>> As a heads up, I've left Nutanix and updated it in MAINTAINERS. Will
>>> be updating it again shortly so tagging these with my new work email.
>>>
>>
>> Thanks for the fast review! The mail server already complained to me :)
>>
>> Maybe consider adding yourself as reviewer for vhost as well? (which
>> covers libvhost-user), I took your mail address from git history, not
>> get_maintainers.pl.
> 
> I don't expect I'll have much time to review code outside of
> vhost-user-blk/vhost-user-scsi, but happy to add an entry if it helps
> folks tag me on relevant patches.

If it helps, it might make sense to split out libvhost-user into a 
separate MAINTAINERS section.

-- 
Cheers,

David / dhildenb

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.