From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59465) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X0X2q-0003xI-JQ for qemu-devel@nongnu.org; Fri, 27 Jun 2014 10:22:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X0X2h-0002AU-8z for qemu-devel@nongnu.org; Fri, 27 Jun 2014 10:22:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29807) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X0X2h-0002AJ-0g for qemu-devel@nongnu.org; Fri, 27 Jun 2014 10:22:19 -0400 Date: Fri, 27 Jun 2014 17:22:41 +0300 From: "Michael S. Tsirkin" Message-ID: <20140627142241.GA3536@redhat.com> References: <1403816492-13401-1-git-send-email-damarion@cisco.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3] vhost-user: fix regions provied with VHOST_USER_SET_MEM_TABLE message List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikolay Nikolaev Cc: VirtualOpenSystems Technical Team , Damjan Marion , qemu-devel On Fri, Jun 27, 2014 at 08:02:48AM +0300, Nikolay Nikolaev wrote: >=20 >=20 >=20 > On Fri, Jun 27, 2014 at 12:01 AM, Damjan Marion wr= ote: >=20 > Old code was affected by memory gaps which resulted in buffer point= ers > pointing to address outside of the mapped regions. >=20 > Here we are introducing following changes: > =A0- new function qemu_get_ram_block_host_ptr() returns host pointe= r > =A0 =A0to the ram block, it is needed to calculate offset of specif= ic > =A0 =A0region in the host memory > =A0- new field mmap_offset is added to the VhostUserMemoryRegion. I= t > =A0 =A0contains offset where specific region starts in the mapped m= emory. > =A0 =A0As there is stil no wider adoption of vhost-user agreement w= as made > =A0 =A0that we will not bump version number due to this change > =A0- other fileds in VhostUserMemoryRegion struct are not changed, = as > =A0 =A0they are all needed for usermode app implementation > =A0- region data is not taken from ram_list.blocks anymore, instead= we > =A0 =A0use region data which is alredy calculated for use in vhost-= net > =A0- Now multiple regions can have same FD and user applicaton can = call > =A0 =A0mmap() multiple times with the same FD but with different of= fset > =A0 =A0(user needs to take care for offset page alignment) >=20 > Signed-off-by: Damjan Marion > --- > =A0docs/specs/vhost-user.txt | =A07 ++++--- > =A0exec.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A07 +++++++ > =A0hw/virtio/vhost-user.c =A0 =A0| 23 ++++++++++++++--------- > =A0include/exec/ram_addr.h =A0 | =A01 + > =A04 files changed, 26 insertions(+), 12 deletions(-) >=20 > diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt > index 2641390..6abb697 100644 > --- a/docs/specs/vhost-user.txt > +++ b/docs/specs/vhost-user.txt > @@ -78,13 +78,14 @@ Depending on the request type, payload can be: > =A0 =A0 Padding: 32-bit >=20 > =A0 =A0 A region is: > - =A0 --------------------------------------- > - =A0 | guest address | size | user address | > - =A0 --------------------------------------- > + =A0 ----------------------------------------------------- > + =A0 | guest address | size | user address | mmap offset | > + =A0 ----------------------------------------------------- >=20 > =A0 =A0 Guest address: a 64-bit guest address of the region > =A0 =A0 Size: a 64-bit size > =A0 =A0 User address: a 64-bit user address > + =A0 mmmap offset: 64-bit offset where region starts in the mapped= memory >=20 >=20 > =A0In QEMU the vhost-user message is implemented with the following= struct: > diff --git a/exec.c b/exec.c > index c849405..a94c583 100644 > --- a/exec.c > +++ b/exec.c > @@ -1456,6 +1456,13 @@ int qemu_get_ram_fd(ram_addr_t addr) > =A0 =A0 =A0return block->fd; > =A0} >=20 > +void *qemu_get_ram_block_host_ptr(ram_addr_t addr) > +{ > + =A0 =A0RAMBlock *block =3D qemu_get_ram_block(addr); > + > + =A0 =A0return block->host; > +} > + > =A0/* Return a host pointer to ram allocated with qemu_ram_alloc. > =A0 =A0 With the exception of the softmmu code in this file, this s= hould > =A0 =A0 only be used for local memory (e.g. video ram) that the dev= ice owns, > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index 0df6a93..38e5806 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -14,6 +14,7 @@ > =A0#include "sysemu/kvm.h" > =A0#include "qemu/error-report.h" > =A0#include "qemu/sockets.h" > +#include "exec/ram_addr.h" >=20 > =A0#include > =A0#include > @@ -47,6 +48,7 @@ typedef struct VhostUserMemoryRegion { > =A0 =A0 =A0uint64_t guest_phys_addr; > =A0 =A0 =A0uint64_t memory_size; > =A0 =A0 =A0uint64_t userspace_addr; > + =A0 =A0uint64_t mmap_offset; > =A0} VhostUserMemoryRegion; >=20 > =A0typedef struct VhostUserMemory { > @@ -183,10 +185,10 @@ static int vhost_user_call(struct vhost_dev *= dev, > unsigned long int request, > =A0{ > =A0 =A0 =A0VhostUserMsg msg; > =A0 =A0 =A0VhostUserRequest msg_request; > - =A0 =A0RAMBlock *block =3D 0; > =A0 =A0 =A0struct vhost_vring_file *file =3D 0; > =A0 =A0 =A0int need_reply =3D 0; > =A0 =A0 =A0int fds[VHOST_MEMORY_MAX_NREGIONS]; > + =A0 =A0int i, fd; > =A0 =A0 =A0size_t fd_num =3D 0; >=20 > =A0 =A0 =A0assert(dev->vhost_ops->backend_type =3D=3D VHOST_BACKEND= _TYPE_USER); > @@ -212,14 +214,17 @@ static int vhost_user_call(struct vhost_dev *= dev, > unsigned long int request, > =A0 =A0 =A0 =A0 =A0break; >=20 > =A0 =A0 =A0case VHOST_SET_MEM_TABLE: > - =A0 =A0 =A0 =A0QTAILQ_FOREACH(block, &ram_list.blocks, next) > - =A0 =A0 =A0 =A0{ > - =A0 =A0 =A0 =A0 =A0 =A0if (block->fd > 0) { > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0msg.memory.regions[fd_num].userspa= ce_addr =3D > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(uintptr_t) block->host; > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0msg.memory.regions[fd_num].memory_= size =3D block->length; > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0msg.memory.regions[fd_num].guest_p= hys_addr =3D block-> > offset; > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fds[fd_num++] =3D block->fd; > + =A0 =A0 =A0 =A0for (i =3D 0; i < dev->mem->nregions; ++i) { > + =A0 =A0 =A0 =A0 =A0 =A0struct vhost_memory_region *reg =3D dev->m= em->regions + i; > + =A0 =A0 =A0 =A0 =A0 =A0fd =3D qemu_get_ram_fd(reg->guest_phys_add= r); > + =A0 =A0 =A0 =A0 =A0 =A0if (fd > 0) { > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0msg.memory.regions[fd_num].userspa= ce_addr =3D reg-> > userspace_addr; > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0msg.memory.regions[fd_num].memory_= size =A0=3D reg-> > memory_size; > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0msg.memory.regions[fd_num].guest_p= hys_addr =3D reg-> > guest_phys_addr; > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0msg.memory.regions[fd_num].mmap_of= fset =3D reg-> > userspace_addr - > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(uintptr_t) qemu_get_ram_b= lock_host_ptr(reg-> > guest_phys_addr); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0assert(fd_num < VHOST_MEMORY_MAX_N= REGIONS); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fds[fd_num++] =3D fd; > =A0 =A0 =A0 =A0 =A0 =A0 =A0} > =A0 =A0 =A0 =A0 =A0} >=20 > diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h > index 55ca676..e9eb831 100644 > --- a/include/exec/ram_addr.h > +++ b/include/exec/ram_addr.h > @@ -29,6 +29,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t siz= e, void > *host, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 MemoryRegion *mr); > =A0ram_addr_t qemu_ram_alloc(ram_addr_t size, MemoryRegion *mr); > =A0int qemu_get_ram_fd(ram_addr_t addr); > +void *qemu_get_ram_block_host_ptr(ram_addr_t addr); > =A0void *qemu_get_ram_ptr(ram_addr_t addr); > =A0void qemu_ram_free(ram_addr_t addr); > =A0void qemu_ram_free_from_ptr(ram_addr_t addr); > -- > 1.9.1 >=20 >=20 >=20 > Looks OK to me. Thanks Damjan. >=20 > Acked-By:=A0Nikolay Nikolaev >=20 Nikolay, please don't send HTML mail to list. Plain text only. Thanks, --=20 MST