Re: [PATCH v1 01/15] libvhost-user: Fix msg_region->userspace_addr computation
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
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
> Signed-off-by: David Hildenbrand
> ---
> 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
On 04.02.24 23:01, Raphael Norwitz wrote: On Sun, Feb 4, 2024 at 9:36 AM David Hildenbrand 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
Re: [PATCH v1 01/15] libvhost-user: Fix msg_region->userspace_addr computation
On Sun, Feb 4, 2024 at 9:36 AM David Hildenbrand 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 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
> >> Signed-off-by: David Hildenbrand
> >
> > Reviewed-by: Raphael Norwitz
>
>
> --
> Cheers,
>
> David / dhildenb
>
Re: [PATCH v1 01/15] libvhost-user: Fix msg_region->userspace_addr computation
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 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
Signed-off-by: David Hildenbrand
Reviewed-by: Raphael Norwitz
--
Cheers,
David / dhildenb
Re: [PATCH v1 01/15] libvhost-user: Fix msg_region->userspace_addr computation
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 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
> Signed-off-by: David Hildenbrand
Reviewed-by: Raphael Norwitz
> ---
> 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
>
>
