[PATCH v3 0/3] vhost-user: warn when guest RAM is not shared

[PATCH v3 0/3] vhost-user: warn when guest RAM is not shared

[Stefan Hajnoczi]

v3:
 * Rebased
v2:
 * Clarify that object-memory-memfd enables share=on by default [Marc-André]
 * Rebased

vhost-user requires -object memory-backend-*,share=on option so that QEMU uses
mmap(MAP_SHARED) on guest RAM that is shared with the vhost-user device backend
process. This is needed so the QEMU process sees changes made by the vhost-user
device backend process, and vice versa.

Today QEMU and the vhost-user device process will start up and then fail with a
confusing error message if the user forgot to specify share=on.

This patch series adds a warning letting the user know that share=on is
required.

Stefan Hajnoczi (3):
  tests/qtest/vhost-user-test: use share=on with memfd
  memory: add memory_region_is_mapped_shared()
  vhost-user: warn when guest RAM is not shared

 include/exec/memory.h         | 11 +++++++++++
 hw/virtio/vhost-user.c        | 20 ++++++++++++++++----
 softmmu/memory.c              |  6 ++++++
 tests/qtest/vhost-user-test.c |  2 +-
 4 files changed, 34 insertions(+), 5 deletions(-)

-- 
2.31.1

[PATCH v3 1/3] tests/qtest/vhost-user-test: use share=on with memfd

[Stefan Hajnoczi]

Add share=on for consistency and to prevent future bugs in the test.

Note that share=on is the default for memory-backend-memfd so existing
tests work.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qtest/vhost-user-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index 3d6337fb5c..6c0891d429 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -40,7 +40,7 @@
 #define QEMU_CMD_MEM    " -m %d -object memory-backend-file,id=mem,size=%dM," \
                         "mem-path=%s,share=on -numa node,memdev=mem"
 #define QEMU_CMD_MEMFD  " -m %d -object memory-backend-memfd,id=mem,size=%dM," \
-                        " -numa node,memdev=mem"
+                        "share=on -numa node,memdev=mem"
 #define QEMU_CMD_CHR    " -chardev socket,id=%s,path=%s%s"
 #define QEMU_CMD_NETDEV " -netdev vhost-user,id=hs0,chardev=%s,vhostforce"
 
-- 
2.31.1

Re: [PATCH v3 1/3] tests/qtest/vhost-user-test: use share=on with memfd

[Pankaj Gupta]

> Add share=on for consistency and to prevent future bugs in the test.
>
> Note that share=on is the default for memory-backend-memfd so existing
> tests work.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Acked-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qtest/vhost-user-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
> index 3d6337fb5c..6c0891d429 100644
> --- a/tests/qtest/vhost-user-test.c
> +++ b/tests/qtest/vhost-user-test.c
> @@ -40,7 +40,7 @@
>  #define QEMU_CMD_MEM    " -m %d -object memory-backend-file,id=mem,size=%dM," \
>                          "mem-path=%s,share=on -numa node,memdev=mem"
>  #define QEMU_CMD_MEMFD  " -m %d -object memory-backend-memfd,id=mem,size=%dM," \
> -                        " -numa node,memdev=mem"
> +                        "share=on -numa node,memdev=mem"
>  #define QEMU_CMD_CHR    " -chardev socket,id=%s,path=%s%s"
>  #define QEMU_CMD_NETDEV " -netdev vhost-user,id=hs0,chardev=%s,vhostforce"
>

Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>

[PATCH v3 2/3] memory: add memory_region_is_mapped_shared()

[Stefan Hajnoczi]

Add a function to query whether a memory region is mmap(MAP_SHARED).
This will be used to check that vhost-user memory regions can be shared
with the device backend process in the next patch.

An inline function in "exec/memory.h" would have been nice but RAMBlock
fields are only accessible from memory.c (see "exec/ramblock.h").

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/exec/memory.h | 11 +++++++++++
 softmmu/memory.c      |  6 ++++++
 2 files changed, 17 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index c3d417d317..5976f05a01 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2763,6 +2763,17 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
     }
 }
 
+/**
+ * memory_region_is_mapped_shared: check whether a memory region is
+ * mmap(MAP_SHARED)
+ *
+ * Returns %true is a memory region is mmap(MAP_SHARED). This is always false
+ * on memory regions that do not support memory_region_get_ram_ptr().
+ *
+ * @mr: the memory region being queried
+ */
+bool memory_region_is_mapped_shared(MemoryRegion *mr);
+
 /**
  * address_space_read: read from an address space.
  *
diff --git a/softmmu/memory.c b/softmmu/memory.c
index bfedaf9c4d..3c4510c809 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1811,6 +1811,12 @@ bool memory_region_is_ram_device(MemoryRegion *mr)
     return mr->ram_device;
 }
 
+bool memory_region_is_mapped_shared(MemoryRegion *mr)
+{
+    return memory_access_is_direct(mr, false) &&
+           (mr->ram_block->flags & RAM_SHARED);
+}
+
 uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr)
 {
     uint8_t mask = mr->dirty_log_mask;
-- 
2.31.1

Re: [PATCH v3 2/3] memory: add memory_region_is_mapped_shared()

[Pankaj Gupta]

> Add a function to query whether a memory region is mmap(MAP_SHARED).
> This will be used to check that vhost-user memory regions can be shared
> with the device backend process in the next patch.
>
> An inline function in "exec/memory.h" would have been nice but RAMBlock
> fields are only accessible from memory.c (see "exec/ramblock.h").
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/exec/memory.h | 11 +++++++++++
>  softmmu/memory.c      |  6 ++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index c3d417d317..5976f05a01 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2763,6 +2763,17 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
>      }
>  }
>
> +/**
> + * memory_region_is_mapped_shared: check whether a memory region is
> + * mmap(MAP_SHARED)
> + *
> + * Returns %true is a memory region is mmap(MAP_SHARED). This is always false
> + * on memory regions that do not support memory_region_get_ram_ptr().
> + *
> + * @mr: the memory region being queried
> + */
> +bool memory_region_is_mapped_shared(MemoryRegion *mr);
> +
>  /**
>   * address_space_read: read from an address space.
>   *
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index bfedaf9c4d..3c4510c809 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1811,6 +1811,12 @@ bool memory_region_is_ram_device(MemoryRegion *mr)
>      return mr->ram_device;
>  }
>
> +bool memory_region_is_mapped_shared(MemoryRegion *mr)
> +{
> +    return memory_access_is_direct(mr, false) &&
> +           (mr->ram_block->flags & RAM_SHARED);
> +}
> +
>  uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr)
>  {
>      uint8_t mask = mr->dirty_log_mask;

Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>

[PATCH v3 3/3] vhost-user: warn when guest RAM is not shared

[Stefan Hajnoczi]

Memory regions must be mmap(MAP_SHARED) so that modifications made by
the vhost device backend process are visible to QEMU and vice versa. Use
the new memory_region_is_mapped_shared() function to check this and
print a warning if guest RAM is not shared:

  qemu-system-x86_64: -device vhost-user-fs-pci,chardev=char0,tag=myfs: warning: Found vhost-user memory region without MAP_SHARED (did you forget -object memory-*,share=on?)
  qemu-system-x86_64: Failed to read from slave.

This warning makes it clear that memory needs to be configured with
share=on. Without this patch the vhost device is initialized and the
device fails with vague error messages caused by inconsistent memory
views. The warning should make it easier to troubleshoot QEMU
command-lines that lack share=on memory.

I couldn't figure out how to make this a vhost_dev_init() error, because
memory regions aren't necessarily final when it is called. Also, hotplug
can add memory regions without MAP_SHARED in the future, so we still
need to warn about that.

Reported-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/vhost-user.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 29ea2b4fce..3978292514 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2295,11 +2295,23 @@ vhost_user_crypto_close_session(struct vhost_dev *dev, uint64_t session_id)
 static bool vhost_user_mem_section_filter(struct vhost_dev *dev,
                                           MemoryRegionSection *section)
 {
-    bool result;
+    /* An fd is required so we can pass it to the device backend process */
+    if (memory_region_get_fd(section->mr) < 0) {
+        return false;
+    }
 
-    result = memory_region_get_fd(section->mr) >= 0;
-
-    return result;
+    /*
+     * It must be mmap(MAP_SHARED) so memory stores from the device backend
+     * process are visible to us and vice versa.
+     */
+    if (!memory_region_is_mapped_shared(section->mr)) {
+        static bool warned;
+        warn_report_once_cond(&warned, "Found vhost-user memory region "
+                "without MAP_SHARED (did you forget -object "
+                "memory-*,share=on?)");
+        return false;
+    }
+    return true;
 }
 
 static int vhost_user_get_inflight_fd(struct vhost_dev *dev,
-- 
2.31.1

Re: [PATCH v3 0/3] vhost-user: warn when guest RAM is not shared

[Kevin Wolf]

Am 14.07.2021 um 11:29 hat Stefan Hajnoczi geschrieben:
> v3:
>  * Rebased
> v2:
>  * Clarify that object-memory-memfd enables share=on by default [Marc-André]
>  * Rebased
> 
> vhost-user requires -object memory-backend-*,share=on option so that QEMU uses
> mmap(MAP_SHARED) on guest RAM that is shared with the vhost-user device backend
> process. This is needed so the QEMU process sees changes made by the vhost-user
> device backend process, and vice versa.
> 
> Today QEMU and the vhost-user device process will start up and then fail with a
> confusing error message if the user forgot to specify share=on.
> 
> This patch series adds a warning letting the user know that share=on is
> required.

Hmm... This seems to work when I use shared=off for the memory backend,
even though the error message is easy to miss because more error
messages follow:

    $ LANG=C build/qemu-system-x86_64 -object memory-backend-memfd,id=mem,size=1G,share=off -m 1G -M memory-backend=mem -chardev socket,path=/tmp/vhost.sock,id=vhost1 -device vhost-user-blk-pci,chardev=vhost1
    qemu-system-x86_64: -device vhost-user-blk-pci,chardev=vhost1: warning: Found vhost-user memory region without MAP_SHARED (did you forget -object memory-*,share=on?)
    qemu-system-x86_64: Unexpected end-of-file before all data were read
    qemu-system-x86_64: Failed to set msg fds.
    qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Resource temporarily unavailable (11)

However, if I don't create the memory backend at all (which is probably
the most common error users will make), then the new message isn't
printed:

    $ LANG=C build/qemu-system-x86_64 -m 1G -chardev socket,path=/tmp/vhost.sock,id=vhost1 -device vhost-user-blk-pci,chardev=vhost1
    qemu-system-x86_64: Unexpected end-of-file before all data were read
    qemu-system-x86_64: Failed to set msg fds.
    qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Resource temporarily unavailable (11)

Wouldn't we want a warning for the second case, too?

Kevin

Re: [PATCH v3 0/3] vhost-user: warn when guest RAM is not shared

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2234 bytes --]

On Tue, Jul 20, 2021 at 01:32:43PM +0200, Kevin Wolf wrote:
> Am 14.07.2021 um 11:29 hat Stefan Hajnoczi geschrieben:
> > v3:
> >  * Rebased
> > v2:
> >  * Clarify that object-memory-memfd enables share=on by default [Marc-André]
> >  * Rebased
> > 
> > vhost-user requires -object memory-backend-*,share=on option so that QEMU uses
> > mmap(MAP_SHARED) on guest RAM that is shared with the vhost-user device backend
> > process. This is needed so the QEMU process sees changes made by the vhost-user
> > device backend process, and vice versa.
> > 
> > Today QEMU and the vhost-user device process will start up and then fail with a
> > confusing error message if the user forgot to specify share=on.
> > 
> > This patch series adds a warning letting the user know that share=on is
> > required.
> 
> Hmm... This seems to work when I use shared=off for the memory backend,
> even though the error message is easy to miss because more error
> messages follow:
> 
>     $ LANG=C build/qemu-system-x86_64 -object memory-backend-memfd,id=mem,size=1G,share=off -m 1G -M memory-backend=mem -chardev socket,path=/tmp/vhost.sock,id=vhost1 -device vhost-user-blk-pci,chardev=vhost1
>     qemu-system-x86_64: -device vhost-user-blk-pci,chardev=vhost1: warning: Found vhost-user memory region without MAP_SHARED (did you forget -object memory-*,share=on?)
>     qemu-system-x86_64: Unexpected end-of-file before all data were read
>     qemu-system-x86_64: Failed to set msg fds.
>     qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Resource temporarily unavailable (11)
> 
> However, if I don't create the memory backend at all (which is probably
> the most common error users will make), then the new message isn't
> printed:
> 
>     $ LANG=C build/qemu-system-x86_64 -m 1G -chardev socket,path=/tmp/vhost.sock,id=vhost1 -device vhost-user-blk-pci,chardev=vhost1
>     qemu-system-x86_64: Unexpected end-of-file before all data were read
>     qemu-system-x86_64: Failed to set msg fds.
>     qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Resource temporarily unavailable (11)
> 
> Wouldn't we want a warning for the second case, too?

Yes. I think so. I'll investigate.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]