From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56030) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X0OJe-00081a-Av for qemu-devel@nongnu.org; Fri, 27 Jun 2014 01:03:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X0OJY-0007HV-Tj for qemu-devel@nongnu.org; Fri, 27 Jun 2014 01:03:14 -0400 Received: from mail-qc0-f179.google.com ([209.85.216.179]:51500) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X0OJY-0007Fz-P3 for qemu-devel@nongnu.org; Fri, 27 Jun 2014 01:03:08 -0400 Received: by mail-qc0-f179.google.com with SMTP id x3so4134363qcv.10 for ; Thu, 26 Jun 2014 22:03:08 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1403816492-13401-1-git-send-email-damarion@cisco.com> References: <1403816492-13401-1-git-send-email-damarion@cisco.com> From: Nikolay Nikolaev Date: Fri, 27 Jun 2014 08:02:48 +0300 Message-ID: Content-Type: multipart/alternative; boundary=001a113517cead1e2e04fcca3934 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: Damjan Marion Cc: VirtualOpenSystems Technical Team , qemu-devel , mst --001a113517cead1e2e04fcca3934 Content-Type: text/plain; charset=UTF-8 On Fri, Jun 27, 2014 at 12:01 AM, Damjan Marion wrote: > Old code was affected by memory gaps which resulted in buffer pointers > pointing to address outside of the mapped regions. > > Here we are introducing following changes: > - new function qemu_get_ram_block_host_ptr() returns host pointer > to the ram block, it is needed to calculate offset of specific > region in the host memory > - new field mmap_offset is added to the VhostUserMemoryRegion. It > contains offset where specific region starts in the mapped memory. > As there is stil no wider adoption of vhost-user agreement was made > that we will not bump version number due to this change > - other fileds in VhostUserMemoryRegion struct are not changed, as > they are all needed for usermode app implementation > - region data is not taken from ram_list.blocks anymore, instead we > use region data which is alredy calculated for use in vhost-net > - Now multiple regions can have same FD and user applicaton can call > mmap() multiple times with the same FD but with different offset > (user needs to take care for offset page alignment) > > Signed-off-by: Damjan Marion > --- > docs/specs/vhost-user.txt | 7 ++++--- > exec.c | 7 +++++++ > hw/virtio/vhost-user.c | 23 ++++++++++++++--------- > include/exec/ram_addr.h | 1 + > 4 files changed, 26 insertions(+), 12 deletions(-) > > 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: > Padding: 32-bit > > A region is: > - --------------------------------------- > - | guest address | size | user address | > - --------------------------------------- > + ----------------------------------------------------- > + | guest address | size | user address | mmap offset | > + ----------------------------------------------------- > > Guest address: a 64-bit guest address of the region > Size: a 64-bit size > User address: a 64-bit user address > + mmmap offset: 64-bit offset where region starts in the mapped memory > > > In 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) > return block->fd; > } > > +void *qemu_get_ram_block_host_ptr(ram_addr_t addr) > +{ > + RAMBlock *block = qemu_get_ram_block(addr); > + > + return block->host; > +} > + > /* Return a host pointer to ram allocated with qemu_ram_alloc. > With the exception of the softmmu code in this file, this should > only be used for local memory (e.g. video ram) that the device 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 @@ > #include "sysemu/kvm.h" > #include "qemu/error-report.h" > #include "qemu/sockets.h" > +#include "exec/ram_addr.h" > > #include > #include > @@ -47,6 +48,7 @@ typedef struct VhostUserMemoryRegion { > uint64_t guest_phys_addr; > uint64_t memory_size; > uint64_t userspace_addr; > + uint64_t mmap_offset; > } VhostUserMemoryRegion; > > typedef struct VhostUserMemory { > @@ -183,10 +185,10 @@ static int vhost_user_call(struct vhost_dev *dev, > unsigned long int request, > { > VhostUserMsg msg; > VhostUserRequest msg_request; > - RAMBlock *block = 0; > struct vhost_vring_file *file = 0; > int need_reply = 0; > int fds[VHOST_MEMORY_MAX_NREGIONS]; > + int i, fd; > size_t fd_num = 0; > > assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER); > @@ -212,14 +214,17 @@ static int vhost_user_call(struct vhost_dev *dev, > unsigned long int request, > break; > > case VHOST_SET_MEM_TABLE: > - QTAILQ_FOREACH(block, &ram_list.blocks, next) > - { > - if (block->fd > 0) { > - msg.memory.regions[fd_num].userspace_addr = > - (uintptr_t) block->host; > - msg.memory.regions[fd_num].memory_size = block->length; > - msg.memory.regions[fd_num].guest_phys_addr = > block->offset; > - fds[fd_num++] = block->fd; > + for (i = 0; i < dev->mem->nregions; ++i) { > + struct vhost_memory_region *reg = dev->mem->regions + i; > + fd = qemu_get_ram_fd(reg->guest_phys_addr); > + if (fd > 0) { > + msg.memory.regions[fd_num].userspace_addr = > reg->userspace_addr; > + msg.memory.regions[fd_num].memory_size = > reg->memory_size; > + msg.memory.regions[fd_num].guest_phys_addr = > reg->guest_phys_addr; > + msg.memory.regions[fd_num].mmap_offset = > reg->userspace_addr - > + (uintptr_t) > qemu_get_ram_block_host_ptr(reg->guest_phys_addr); > + assert(fd_num < VHOST_MEMORY_MAX_NREGIONS); > + fds[fd_num++] = fd; > } > } > > 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 size, void > *host, > MemoryRegion *mr); > ram_addr_t qemu_ram_alloc(ram_addr_t size, MemoryRegion *mr); > int qemu_get_ram_fd(ram_addr_t addr); > +void *qemu_get_ram_block_host_ptr(ram_addr_t addr); > void *qemu_get_ram_ptr(ram_addr_t addr); > void qemu_ram_free(ram_addr_t addr); > void qemu_ram_free_from_ptr(ram_addr_t addr); > -- > 1.9.1 > > Looks OK to me. Thanks Damjan. Acked-By: Nikolay Nikolaev --001a113517cead1e2e04fcca3934 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable



On Fri, Jun 27, 2014 at 12:01 AM, Damjan Marion &= lt;damarion@cisco.c= om> wrote:
Old code was affected by memory gaps which resulted in buf= fer pointers
pointing to address outside of the mapped regions.

Here we are introducing following changes:
=C2=A0- new function qemu_get_ram_block_host_ptr() returns host pointer
=C2=A0 =C2=A0to the ram block, it is needed to calculate offset of specific=
=C2=A0 =C2=A0region in the host memory
=C2=A0- new field mmap_offset is added to the VhostUserMemoryRegion. It
=C2=A0 =C2=A0contains offset where specific region starts in the mapped mem= ory.
=C2=A0 =C2=A0As there is stil no wider adoption of vhost-user agreement was= made
=C2=A0 =C2=A0that we will not bump version number due to this change
=C2=A0- other fileds in VhostUserMemoryRegion struct are not changed, as =C2=A0 =C2=A0they are all needed for usermode app implementation
=C2=A0- region data is not taken from ram_list.blocks anymore, instead we =C2=A0 =C2=A0use region data which is alredy calculated for use in vhost-ne= t
=C2=A0- Now multiple regions can have same FD and user applicaton can call<= br> =C2=A0 =C2=A0mmap() multiple times with the same FD but with different offs= et
=C2=A0 =C2=A0(user needs to take care for offset page alignment)

Signed-off-by: Damjan Marion <dama= rion@cisco.com>
---
=C2=A0docs/specs/vhost-user.txt | =C2=A07 ++++---
=C2=A0exec.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0| =C2=A07 +++++++
=C2=A0hw/virtio/vhost-user.c =C2=A0 =C2=A0| 23 ++++++++++++++---------
=C2=A0include/exec/ram_addr.h =C2=A0 | =C2=A01 +
=C2=A04 files changed, 26 insertions(+), 12 deletions(-)

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:
=C2=A0 =C2=A0 Padding: 32-bit

=C2=A0 =C2=A0 A region is:
- =C2=A0 ---------------------------------------
- =C2=A0 | guest address | size | user address |
- =C2=A0 ---------------------------------------
+ =C2=A0 -----------------------------------------------------
+ =C2=A0 | guest address | size | user address | mmap offset |
+ =C2=A0 -----------------------------------------------------

=C2=A0 =C2=A0 Guest address: a 64-bit guest address of the region
=C2=A0 =C2=A0 Size: a 64-bit size
=C2=A0 =C2=A0 User address: a 64-bit user address
+ =C2=A0 mmmap offset: 64-bit offset where region starts in the mapped memo= ry


=C2=A0In QEMU the vhost-user message is implemented with the following stru= ct:
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)
=C2=A0 =C2=A0 =C2=A0return block->fd;
=C2=A0}

+void *qemu_get_ram_block_host_ptr(ram_addr_t addr)
+{
+ =C2=A0 =C2=A0RAMBlock *block =3D qemu_get_ram_block(addr);
+
+ =C2=A0 =C2=A0return block->host;
+}
+
=C2=A0/* Return a host pointer to ram allocated with qemu_ram_alloc.
=C2=A0 =C2=A0 With the exception of the softmmu code in this file, this sho= uld
=C2=A0 =C2=A0 only be used for local memory (e.g. video ram) that the devic= e 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 @@
=C2=A0#include "sysemu/kvm.h"
=C2=A0#include "qemu/error-report.h"
=C2=A0#include "qemu/sockets.h"
+#include "exec/ram_addr.h"

=C2=A0#include <fcntl.h>
=C2=A0#include <unistd.h>
@@ -47,6 +48,7 @@ typedef struct VhostUserMemoryRegion {
=C2=A0 =C2=A0 =C2=A0uint64_t guest_phys_addr;
=C2=A0 =C2=A0 =C2=A0uint64_t memory_size;
=C2=A0 =C2=A0 =C2=A0uint64_t userspace_addr;
+ =C2=A0 =C2=A0uint64_t mmap_offset;
=C2=A0} VhostUserMemoryRegion;

=C2=A0typedef struct VhostUserMemory {
@@ -183,10 +185,10 @@ static int vhost_user_call(struct vhost_dev *dev, uns= igned long int request,
=C2=A0{
=C2=A0 =C2=A0 =C2=A0VhostUserMsg msg;
=C2=A0 =C2=A0 =C2=A0VhostUserRequest msg_request;
- =C2=A0 =C2=A0RAMBlock *block =3D 0;
=C2=A0 =C2=A0 =C2=A0struct vhost_vring_file *file =3D 0;
=C2=A0 =C2=A0 =C2=A0int need_reply =3D 0;
=C2=A0 =C2=A0 =C2=A0int fds[VHOST_MEMORY_MAX_NREGIONS];
+ =C2=A0 =C2=A0int i, fd;
=C2=A0 =C2=A0 =C2=A0size_t fd_num =3D 0;

=C2=A0 =C2=A0 =C2=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, uns= igned long int request,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;

=C2=A0 =C2=A0 =C2=A0case VHOST_SET_MEM_TABLE:
- =C2=A0 =C2=A0 =C2=A0 =C2=A0QTAILQ_FOREACH(block, &ram_list.blocks, ne= xt)
- =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.memory.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(uin= tptr_t) block->host;
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0msg.memory.regions= [fd_num].memory_size =3D block->length;
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0msg.memory.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->nregions;= ++i) {
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct vhost_memory_region *reg = =3D dev->mem->regions + i;
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fd =3D qemu_get_ram_fd(reg->g= uest_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.memory.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.memory.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.memory.regions= [fd_num].guest_phys_addr =3D reg->guest_phys_addr;
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0msg.memory.regions= [fd_num].mmap_offset =3D reg->userspace_addr -
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(uin= tptr_t) qemu_get_ram_block_host_ptr(reg->guest_phys_addr);
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0assert(fd_num <= VHOST_MEMORY_MAX_NREGIONS);
+ =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 =C2=A0 =C2=A0}

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 size, void = *host,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 MemoryRegion *mr);
=C2=A0ram_addr_t qemu_ram_alloc(ram_addr_t size, MemoryRegion *mr);
=C2=A0int qemu_get_ram_fd(ram_addr_t addr);
+void *qemu_get_ram_block_host_ptr(ram_addr_t addr);
=C2=A0void *qemu_get_ram_ptr(ram_addr_t addr);
=C2=A0void qemu_ram_free(ram_addr_t addr);
=C2=A0void qemu_ram_free_from_ptr(ram_addr_t addr);
--
1.9.1


Looks= OK to me. Thanks Damjan.

Acked-By:=C2=A0Nikolay= Nikolaev <n.nikola= ev@virtualopensystems.com>

--001a113517cead1e2e04fcca3934--