Skip to contentSkip to conversation

vhost-user fixes

PATCH Superseded Nikolay Nikolaev 3 participants 7 messages Jul 8, 2014 – Jul 11, 2014

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Nikolay Nikolaev

author
#0 lore raw

cover vhost-user fixes

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

author
#1 lore raw

PATCH 1/3 Add qemu_is_ram_block

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

Signed-off-by Nikolay Nikolaev <[email protected]>
---
 exec.c                  |   15 +++++++++++++++
 include/exec/ram_addr.h |    1 +
 2 files changed, 16 insertions(+)
exec.c +15 -0
index 5a2a25e..0b1457b 100644
@@ -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;
include/exec/ram_addr.h +1 -0
index e9eb831..a4b2c3e 100644
@@ -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);
author
#2 lore raw

PATCH 2/3 vhost-user: Fix VHOST_SET_MEM_TABLE processing

In reply to 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 <[email protected]>
---
 hw/virtio/vhost-user.c |    4 ++++
 1 file changed, 4 insertions(+)
hw/virtio/vhost-user.c +4 -0
index 38e5806..876b080 100644
@@ -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;
#3 lore raw

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

Il 08/07/2014 16:06, Nikolay Nikolaev ha scritto:

hw/virtio/vhost-user.c static int vhost_user_call(struct vhost_dev *dev, unsigned long int request, · line 225

Quoted 11 lines
hw/virtio/vhost-user.c +4 -0
@@ -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

author
#4 lore raw

PATCH 3/3 qtest: Adapt vhost-user-test to latehs vhost-user changes

In reply to 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 <[email protected]>
---
 tests/vhost-user-test.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
tests/vhost-user-test.c +9 -2
index 2af2381..b9dcec1 100644
@@ -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);
#5 lore raw
In reply to Nikolay Nikolaev
Quoted 6 lines · 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".

Quoted 6 lines

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!

Quoted 16 lines

---

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

#6 lore raw
In reply to Nikolay Nikolaev

Il 08/07/2014 16:05, Nikolay Nikolaev ha scritto:

Quoted 6 lines

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".

Quoted 6 lines

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