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

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

[Stefan Hajnoczi]

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

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

[Stefan Hajnoczi]

For some reason memfd never used share=on. vhost-user relies on
mmap(MAP_SHARED) so this seems like a problem, but the tests still run
without it.

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

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 1a5f5313ff..2db98c4920 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.29.2

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

[Marc-André Lureau]

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

Hi

On Mon, Feb 22, 2021 at 8:11 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> For some reason memfd never used share=on. vhost-user relies on
> mmap(MAP_SHARED) so this seems like a problem, but the tests still run
> without it.
>
>
Simply because it's on by default with memory-backend-memfd (it wouldn't
make much sense to use memfd in the first place without share)

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

But it doesn't hurt to be explicit though.


> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@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 1a5f5313ff..2db98c4920 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.29.2
>
>

[-- Attachment #2: Type: text/html, Size: 2598 bytes --]

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

[Stefan Hajnoczi]

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

On Wed, Feb 24, 2021 at 02:22:31PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Feb 22, 2021 at 8:11 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > For some reason memfd never used share=on. vhost-user relies on
> > mmap(MAP_SHARED) so this seems like a problem, but the tests still run
> > without it.
> >
> >
> Simply because it's on by default with memory-backend-memfd (it wouldn't
> make much sense to use memfd in the first place without share)

Thanks for solving this mystery!

Please update the commit description with this information when merging
(or I'll update it when respinning).

Stefan

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

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

[Thomas Huth]

On 22/02/2021 17.10, Stefan Hajnoczi wrote:
> For some reason memfd never used share=on. vhost-user relies on
> mmap(MAP_SHARED) so this seems like a problem, but the tests still run
> without it.
> 
> Add share=on for consistency and to prevent future bugs in the test.
> 
> 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 1a5f5313ff..2db98c4920 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"

Even if it's not required, it seems to be a good clean up, also with regards 
to the lonely comma at the end of the previous line.

Acked-by: Thomas Huth <thuth@redhat.com>

I assume this will go through the vhost tree, or do you want me to take this 
single patch through my qtest tree?

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

[Stefan Hajnoczi]

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

On Mon, Mar 08, 2021 at 07:31:25AM +0100, Thomas Huth wrote:
> On 22/02/2021 17.10, Stefan Hajnoczi wrote:
> > For some reason memfd never used share=on. vhost-user relies on
> > mmap(MAP_SHARED) so this seems like a problem, but the tests still run
> > without it.
> > 
> > Add share=on for consistency and to prevent future bugs in the test.
> > 
> > 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 1a5f5313ff..2db98c4920 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"
> 
> Even if it's not required, it seems to be a good clean up, also with regards
> to the lonely comma at the end of the previous line.
> 
> Acked-by: Thomas Huth <thuth@redhat.com>
> 
> I assume this will go through the vhost tree, or do you want me to take this
> single patch through my qtest tree?

I think this entire series will go through the vhost tree.

Thanks,
Stefan

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

[PATCH 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 c6fb714e49..7b7dbe9fd0 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2457,6 +2457,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 874a8fccde..e6631e5d4c 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1809,6 +1809,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.29.2

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

[Marc-André Lureau]

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

On Mon, Feb 22, 2021 at 8:11 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> 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 c6fb714e49..7b7dbe9fd0 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2457,6 +2457,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 874a8fccde..e6631e5d4c 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1809,6 +1809,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) &&
>

memory_access_is_direct is a bit special, it treats ram-device differently
from rom-device since commit 4a2e242bbb306 ("memory: Don't use memcpy for
ram_device regions").

I don't think it's an issue for now, but I wonder if in the future we will
share ram-device as well.

Where did you get the idea to use that function? I suppose that's also the
one we use somewhere else in the code path sharing the VM memory.

+           (mr->ram_block->flags & RAM_SHARED);
> +}
> +
>  uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr)
>  {
>      uint8_t mask = mr->dirty_log_mask;
> --
> 2.29.2
>
>

[-- Attachment #2: Type: text/html, Size: 3157 bytes --]

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

[Stefan Hajnoczi]

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

On Wed, Feb 24, 2021 at 02:33:32PM +0400, Marc-André Lureau wrote:
> On Mon, Feb 22, 2021 at 8:11 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > 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 c6fb714e49..7b7dbe9fd0 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -2457,6 +2457,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 874a8fccde..e6631e5d4c 100644
> > --- a/softmmu/memory.c
> > +++ b/softmmu/memory.c
> > @@ -1809,6 +1809,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) &&
> >
> 
> memory_access_is_direct is a bit special, it treats ram-device differently
> from rom-device since commit 4a2e242bbb306 ("memory: Don't use memcpy for
> ram_device regions").
> 
> I don't think it's an issue for now, but I wonder if in the future we will
> share ram-device as well.
> 
> Where did you get the idea to use that function? I suppose that's also the
> one we use somewhere else in the code path sharing the VM memory.

It's used right before qemu_map_ram_ptr() in address_space_read() and
seems like the most comprehensive way to check if a MemoryRegion is
accessed via memory loads/stores.

Do you prefer just memory_region_is_ram()? Or something more complicated
to also allow romd?

Stefan

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

[PATCH 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>
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 2fdd5daf74..17c2fb81be 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2223,11 +2223,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.29.2

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

[Marc-André Lureau]

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

On Mon, Feb 22, 2021 at 8:11 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> 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>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@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 2fdd5daf74..17c2fb81be 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -2223,11 +2223,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.29.2
>
>

[-- Attachment #2: Type: text/html, Size: 3669 bytes --]

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

[Stefan Hajnoczi]

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

On Mon, Feb 22, 2021 at 04:10:14PM +0000, Stefan Hajnoczi wrote:
> 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(-)

Ping for QEMU 6.1

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