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

Stefan Hajnoczi posted 3 patches 4 years, 11 months ago
There is a newer version of this series
[PATCH 2/3] memory: add memory_region_is_mapped_shared()
Posted by Stefan Hajnoczi 4 years, 11 months ago
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()
Posted by Marc-André Lureau 4 years, 11 months ago
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
>
>
Re: [PATCH 2/3] memory: add memory_region_is_mapped_shared()
Posted by Stefan Hajnoczi 4 years, 11 months ago
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