| Message ID | 20240202215332.118728-2-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | libvhost-user: support more memslots and cleanup memslot handling code | expand |
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 > >
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>
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 >
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.
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
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!
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]); }
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(-)