[Qemu-devel] [PATCH 0/3] vhost-user fixes

[Qemu-devel] [PATCH 0/3] vhost-user fixes

[Nikolay Nikolaev]

The latest vhost-user changes changed the VHOST_SET_MEM_TABLE handling.
Now the memory regions are mapped from dev->mem. The BIOS is registered
at address 0xfffc0000 which is out of memory boundaries for guests with
less than 4G RAM. Calling qemu_get_ram_fd with this address causes abort()
in qemu_get_ram_block with "Bad ram offset".

To prevent this situation we introduce a new function to check if the address
maps to any RAMBlock - qemu_is_ram_block. This is used in VHOST_SET_MEM_TABLE
handling to revent the aborting call to qemu_get_ram_fd.

The related vhost-user qtest is also updated to reflect the changes in 
vhost-user message structures.

---

Nikolay Nikolaev (3):
      Add qemu_is_ram_block
      vhost-user: Fix VHOST_SET_MEM_TABLE processing
      qtest: Adapt vhost-user-test to latehs vhost-user changes


 exec.c                  |   15 +++++++++++++++
 hw/virtio/vhost-user.c  |    4 ++++
 include/exec/ram_addr.h |    1 +
 tests/vhost-user-test.c |   11 +++++++++--
 4 files changed, 29 insertions(+), 2 deletions(-)

--
Signature

[Qemu-devel] [PATCH 1/3] Add qemu_is_ram_block

[Nikolay Nikolaev]

This function will check if given address maps into a RAMBlock.

Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 exec.c                  |   15 +++++++++++++++
 include/exec/ram_addr.h |    1 +
 2 files changed, 16 insertions(+)

diff --git a/exec.c b/exec.c
index 5a2a25e..0b1457b 100644
--- a/exec.c
+++ b/exec.c
@@ -743,6 +743,21 @@ void cpu_abort(CPUState *cpu, const char *fmt, ...)
 }
 
 #if !defined(CONFIG_USER_ONLY)
+bool qemu_is_ram_block(ram_addr_t addr)
+{
+    RAMBlock *block;
+    bool found = false;
+
+    QTAILQ_FOREACH(block, &ram_list.blocks, next) {
+        if (addr - block->offset < block->length) {
+            found = true;
+            break;
+        }
+    }
+
+    return found;
+}
+
 static RAMBlock *qemu_get_ram_block(ram_addr_t addr)
 {
     RAMBlock *block;
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index e9eb831..a4b2c3e 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -28,6 +28,7 @@ ram_addr_t qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
 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);
+bool qemu_is_ram_block(ram_addr_t addr);
 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);

[Qemu-devel] [PATCH 2/3] vhost-user: Fix VHOST_SET_MEM_TABLE processing

[Nikolay Nikolaev]

For each memory region we use qemu_get_ram_fd to get the RAMBlock
associated file descriptor. It uses qemu_get_ram_block to find the proper structure.
The latter aborts with "Bad ram offset" when the address is not found.

We'll use the new qemu_is_ram_block to indentify non-RAM regions and avoid qemu_get_ram_fd
call on them.

Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 hw/virtio/vhost-user.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 38e5806..876b080 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -216,6 +216,10 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
     case VHOST_SET_MEM_TABLE:
         for (i = 0; i < dev->mem->nregions; ++i) {
             struct vhost_memory_region *reg = dev->mem->regions + i;
+            if (!qemu_is_ram_block(reg->guest_phys_addr)) {
+                /* this is non-RAM region - skip it */
+                continue;
+            }
             fd = qemu_get_ram_fd(reg->guest_phys_addr);
             if (fd > 0) {
                 msg.memory.regions[fd_num].userspace_addr = reg->userspace_addr;

[Qemu-devel] [PATCH 3/3] qtest: Adapt vhost-user-test to latehs vhost-user changes

[Nikolay Nikolaev]

A new field mmap_offset was added in the vhost-user message, we need to reflect
this change in the test too.

Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 tests/vhost-user-test.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 2af2381..b9dcec1 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -72,6 +72,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 {
@@ -201,6 +202,7 @@ static void read_guest_mem(void)
     uint32_t *guest_mem;
     gint64 end_time;
     int i, j;
+    size_t size;
 
     g_mutex_lock(data_mutex);
 
@@ -227,8 +229,13 @@ static void read_guest_mem(void)
 
         g_assert_cmpint(memory.regions[i].memory_size, >, 1024);
 
-        guest_mem = mmap(0, memory.regions[i].memory_size,
-        PROT_READ | PROT_WRITE, MAP_SHARED, fds[i], 0);
+        size =  memory.regions[i].memory_size + memory.regions[i].mmap_offset;
+
+        guest_mem = mmap(0, size, PROT_READ | PROT_WRITE,
+                         MAP_SHARED, fds[i], 0);
+
+        g_assert(guest_mem != MAP_FAILED);
+        guest_mem += (memory.regions[i].mmap_offset / sizeof(*guest_mem));
 
         for (j = 0; j < 256; j++) {
             uint32_t a = readl(memory.regions[i].guest_phys_addr + j*4);

Re: [Qemu-devel] [PATCH 0/3] vhost-user fixes

[Michael S. Tsirkin]

On Tue, Jul 08, 2014 at 05:05:29PM +0300, Nikolay Nikolaev wrote:
> The latest vhost-user changes changed the VHOST_SET_MEM_TABLE handling.
> Now the memory regions are mapped from dev->mem. The BIOS is registered
> at address 0xfffc0000 which is out of memory boundaries for guests with
> less than 4G RAM. Calling qemu_get_ram_fd with this address causes abort()
> in qemu_get_ram_block with "Bad ram offset".
> 
> To prevent this situation we introduce a new function to check if the address
> maps to any RAMBlock - qemu_is_ram_block. This is used in VHOST_SET_MEM_TABLE
> handling to revent the aborting call to qemu_get_ram_fd.
> 
> The related vhost-user qtest is also updated to reflect the changes in 
> vhost-user message structures.

Ugh this is not good :(. The above change was merged very late,
and now we are seeing fall-out. I guess we'll have to do something like
this, bt mkes one wonder what will this change break, in turn.

Besides, you are not tagging patches either RFC or 2.1, and
you really must. Pls do it next time.

And please find more people to Cc using scripts/get_maintainer.pl.

Paolo, maybe you could look at patch 1?

Thanks!

> ---
> 
> Nikolay Nikolaev (3):
>       Add qemu_is_ram_block
>       vhost-user: Fix VHOST_SET_MEM_TABLE processing
>       qtest: Adapt vhost-user-test to latehs vhost-user changes
> 
> 
>  exec.c                  |   15 +++++++++++++++
>  hw/virtio/vhost-user.c  |    4 ++++
>  include/exec/ram_addr.h |    1 +
>  tests/vhost-user-test.c |   11 +++++++++--
>  4 files changed, 29 insertions(+), 2 deletions(-)
> 
> --
> Signature

Re: [Qemu-devel] [PATCH 2/3] vhost-user: Fix VHOST_SET_MEM_TABLE processing

[Paolo Bonzini]

Il 08/07/2014 16:06, Nikolay Nikolaev ha scritto:
> @@ -216,6 +216,10 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
>      case VHOST_SET_MEM_TABLE:
>          for (i = 0; i < dev->mem->nregions; ++i) {
>              struct vhost_memory_region *reg = dev->mem->regions + i;
> +            if (!qemu_is_ram_block(reg->guest_phys_addr)) {
> +                /* this is non-RAM region - skip it */
> +                continue;
> +            }
>              fd = qemu_get_ram_fd(reg->guest_phys_addr);
>              if (fd > 0) {
>                  msg.memory.regions[fd_num].userspace_addr = reg->userspace_addr;

This is wrong.  qemu_get_ram_fd doesn't accept a guest physical address. 
  ram_addr_t are opaque values that are assigned in qemu_ram_alloc.

In fact, RAM regions are filtered by

static bool vhost_section(MemoryRegionSection *section)
{
     return memory_region_is_ram(section->mr);
}


You can find the ram_addr_t corresponding to the userspace_addr using 
qemu_ram_addr_from_host, and then call qemu_get_ram_fd on it.

Paolo

Re: [Qemu-devel] [PATCH 0/3] vhost-user fixes

[Paolo Bonzini]

Il 08/07/2014 16:05, Nikolay Nikolaev ha scritto:
> The latest vhost-user changes changed the VHOST_SET_MEM_TABLE handling.
> Now the memory regions are mapped from dev->mem. The BIOS is registered
> at address 0xfffc0000 which is out of memory boundaries for guests with
> less than 4G RAM. Calling qemu_get_ram_fd with this address causes abort()
> in qemu_get_ram_block with "Bad ram offset".
>
> To prevent this situation we introduce a new function to check if the address
> maps to any RAMBlock - qemu_is_ram_block. This is used in VHOST_SET_MEM_TABLE
> handling to revent the aborting call to qemu_get_ram_fd.
>
> The related vhost-user qtest is also updated to reflect the changes in
> vhost-user message structures.

Patch 3 seems okay.  Patches 1 and 2 are not but the actual fix is 
similarly simple.

Paolo