From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50428) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wzo00-0004mc-A5 for qemu-devel@nongnu.org; Wed, 25 Jun 2014 10:16:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wznzv-0003MZ-8k for qemu-devel@nongnu.org; Wed, 25 Jun 2014 10:16:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:64790) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wznzv-0003MQ-1M for qemu-devel@nongnu.org; Wed, 25 Jun 2014 10:16:27 -0400 Date: Wed, 25 Jun 2014 17:16:51 +0300 From: "Michael S. Tsirkin" Message-ID: <20140625141651.GB15234@redhat.com> References: <20140625135207.GC14578@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] vhost-user: broken mem regions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikolay Nikolaev Cc: "pbonzini@redhat.com" , "Damjan Marion (damarion)" , "qemu-devel@nongnu.org" On Wed, Jun 25, 2014 at 05:13:21PM +0300, Nikolay Nikolaev wrote: >=20 >=20 >=20 > On Wed, Jun 25, 2014 at 5:06 PM, Damjan Marion (damarion) > wrote: >=20 >=20 > On 25 Jun 2014, at 15:53, Michael S. Tsirkin wrote= : >=20 > > On Wed, Jun 25, 2014 at 01:45:09PM +0000, Damjan Marion (damarion= ) wrote: > >> > >> Michael, > >> > >> there is another issue with commited vhost-user code. > > > > I'm answering just this once, but I have a policy against > > answering off-list mail. > > Pls send follow-ups to the list. >=20 > ok, sorry, adding list... >=20 > > > >> If there is bigger mem allocation (i.e. 4G or 6G of RAM) then > >> we have memory gap and then it happens that buffer pointer point= s to > >> memory which is not mmaped. I already filled bug report: > >> > >> https://bugs.launchpad.net/qemu/+bug/1333688 > > > > FYI I mostly ignore launchpad. > > Because of the unfortunate association with Ubuntu, most bugs > > there are distro-specific. > > > >> Bellow is my proposed change to the code. Two things: > >> - it will require changes on the user side also > > > > why would it? > > format seems unchanged, right? >=20 > yes, but it will happen that multiple regions have same FD so call = to mmap > () > should look different, I=E2=80=99m still playing with this on user = side... >=20 > but then you shoudl somehow accumulate the sizes and send just a single= fd, > something along these lines. Not going to work, these regions might not be contigious (e.g. if a higher priority region hides RAM). Just deal with the fact that same fd can appear multiple times. I don't see why it's a big deal anyway. >=20 > > > >> - not sure if qemu_get_ram_fd() is the best way to get FD > > > > Paolo, what do you think? > > > >> Any comments or better idea how to fix this? > >> > >> Thanks, > >> > >> Damjan > >> > >> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > >> index 0df6a93..89fe5c7 100644 > >> --- a/hw/virtio/vhost-user.c > >> +++ b/hw/virtio/vhost-user.c > >> @@ -14,6 +14,7 @@ > >> #include "sysemu/kvm.h" > >> #include "qemu/error-report.h" > >> #include "qemu/sockets.h" > >> +#include "exec/ram_addr.h" > >> > >> #include > >> #include > >> @@ -183,10 +184,10 @@ static int vhost_user_call(struct vhost_de= v *dev, > unsigned long int request, > >> { > >> =C2=A0 =C2=A0 VhostUserMsg msg; > >> =C2=A0 =C2=A0 VhostUserRequest msg_request; > >> - =C2=A0 =C2=A0RAMBlock *block =3D 0; > >> =C2=A0 =C2=A0 struct vhost_vring_file *file =3D 0; > >> =C2=A0 =C2=A0 int need_reply =3D 0; > >> =C2=A0 =C2=A0 int fds[VHOST_MEMORY_MAX_NREGIONS]; > >> + =C2=A0 =C2=A0int i, fd; > >> =C2=A0 =C2=A0 size_t fd_num =3D 0; > >> > >> =C2=A0 =C2=A0 assert(dev->vhost_ops->backend_type =3D=3D VHOST_B= ACKEND_TYPE_USER); > >> @@ -212,14 +213,14 @@ static int vhost_user_call(struct vhost_de= v *dev, > unsigned long int request, > >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 break; > >> > >> =C2=A0 =C2=A0 case VHOST_SET_MEM_TABLE: > >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0QTAILQ_FOREACH(block, &ram_list.blo= cks, next) > >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0{ > >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (block->fd > 0) { > >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0msg.mem= ory.regions[fd_num].userspace_addr =3D > >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0(uintptr_t) block->host; > >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0msg.mem= ory.regions[fd_num].memory_size =3D block->length; > >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0msg.mem= ory.regions[fd_num].guest_phys_addr =3D block-> > offset; > >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fds[fd_= num++] =3D block->fd; > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < dev->mem->nregion= s; ++i) { > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct vhost_memory_r= egion *reg =3D dev->mem->regions + i; > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fd =3D qemu_get_ram_f= d(reg->guest_phys_addr); > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (fd > 0) { > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0msg.mem= ory.regions[fd_num].userspace_addr =3D reg-> > userspace_addr; > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0msg.mem= ory.regions[fd_num].memory_size =C2=A0=3D reg-> > memory_size; > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0msg.mem= ory.regions[fd_num].guest_phys_addr =3D reg-> > memory_size; > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fds[fd_= num++] =3D fd; > >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } > >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 } > >> >=20 >=20 >=20