[PATCH RFC 1/1] memory: Address space map listener

[PATCH RFC 1/1] memory: Address space map listener

[Antonio Caggiano]

Introduce a MemoryListener callback for address space map events.

This will require a change to the memory listener callbacks: while it
currently uses "self" as first argument for the callbacks, this new
approach is going to use an "opaque" member, effectively following the
model used for MemoryRegion and MemoryRegionOps.

Signed-off-by: Antonio Caggiano <quic_acaggian@quicinc.com>
---
 include/exec/memory.h | 19 +++++++++++++++++++
 softmmu/physmem.c     | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 7ec6df3289..f959d53a12 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1045,6 +1045,18 @@ struct MemoryListener {
      */
     void (*coalesced_io_del)(MemoryListener *listener, MemoryRegionSection *section,
                                hwaddr addr, hwaddr len);
+
+    /**
+     * @map:
+     *
+     * Called during an address space map.
+     *
+     * @opaque: User data opaque object
+     * @addr: address within that address space
+     * @len: length of buffer
+     */
+    void (*map)(void *opaque, hwaddr addr, hwaddr len);
+
     /**
      * @priority:
      *
@@ -1054,6 +1066,13 @@ struct MemoryListener {
      */
     unsigned priority;
 
+    /**
+     * @opaque:
+     *
+     * Opaque pointer to user data
+     */
+    void *opaque;
+
     /**
      * @name:
      *
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 9486a1ebdf..0f8bad6b40 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -3246,6 +3246,38 @@ flatview_extend_translation(FlatView *fv, hwaddr addr,
     }
 }
 
+enum ListenerDirection { Forward, Reverse };
+
+/*
+ * This will require a change to the memory listener callbacks:
+ * while it currently uses "self" as first argument for the callbacks, this new
+ * approach is going to use an "opaque" member, effectively following the model
+ * used for MemoryRegion and MemoryRegionOps.
+ */
+#define MEMORY_LISTENER_CALL(_as, _callback, _direction, _args...) \
+    do {                                                                \
+        MemoryListener *_listener;                                      \
+                                                                        \
+        switch (_direction) {                                           \
+        case Forward:                                                   \
+            QTAILQ_FOREACH(_listener, &(_as)->listeners, link_as) {     \
+                if (_listener->_callback) {                             \
+                    _listener->_callback(_listener->opaque, ##_args);   \
+                }                                                       \
+            }                                                           \
+            break;                                                      \
+        case Reverse:                                                   \
+            QTAILQ_FOREACH_REVERSE(_listener, &(_as)->listeners, link_as) { \
+                if (_listener->_callback) {                             \
+                    _listener->_callback(_listener->opaque, ##_args);   \
+                }                                                       \
+            }                                                           \
+            break;                                                      \
+        default:                                                        \
+            abort();                                                    \
+        }                                                               \
+    } while (0)
+
 /* Map a physical memory region into a host virtual address.
  * May map a subset of the requested range, given by and returned in *plen.
  * May return NULL if resources needed to perform the mapping are exhausted.
@@ -3268,6 +3300,8 @@ void *address_space_map(AddressSpace *as,
         return NULL;
     }
 
+    MEMORY_LISTENER_CALL(as, map, Reverse, addr, len);
+
     l = len;
     RCU_READ_LOCK_GUARD();
     fv = address_space_to_flatview(as);
-- 
2.40.0

[PATCH RFC 0/1] MemoryListener address_space_map callback

[Antonio Caggiano]

Hi! This is the RFC about the memory issue I mentioned in our last KVM call.

In our use case, QEMU is used as a library, where RAM and Alias MemoryRegions
are created by listening to read/write events through MemoryRegionOps callbacks.
In this case, no read/write happened yet, so we did not have a chance to create
those memory regions yet.

The callstack looks like this:
- virtio_blk_get_request
  - virtqueue_pop -> virtqueue_split_pop -> virtqueue_map_desc
    - dma_memory_map -> address_space_map

The address_space_map function calls flatview_translate to get the memory region
for a certain address. If the memory region is not directly accessible, the
bounce buffer is used which only allows one mapping at a time, forcing to unmap
before mapping again.

The virtqueue_map_desc function calls iteratively address_space_map for a region
of 4KB but address_space_map is only mapping 1KB using the bounce buffer.
Then virtqueue_map_desc calls address_space_map again for mapping the missing
3KB, but address_space_map returns NULL as the bounce is in use now.

With this patch a MemoryListener callback is introduced for listening to address
space map events, before calling flatview_translate, so that listeners might
have a chance to create any needed alias or RAM memory region for that address
space. Effectively making flatview_translate return a directly accessible memory
region, and avoiding address_space_map to use the bounce buffer.

This will require a change to the memory listener callbacks: while it
currently uses "self" as first argument for the callbacks, this new
approach is going to use an "opaque" member, effectively following the
model used for MemoryRegion and MemoryRegionOps.

Antonio Caggiano (1):
  memory: Address space map listener

 include/exec/memory.h | 19 +++++++++++++++++++
 softmmu/physmem.c     | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

-- 
2.40.0

[PATCH RFC 1/1] memory: Address space map listener

[Antonio Caggiano]

Introduce a MemoryListener callback for address space map events.

This will require a change to the memory listener callbacks: while it
currently uses "self" as first argument for the callbacks, this new
approach is going to use an "opaque" member, effectively following the
model used for MemoryRegion and MemoryRegionOps.

Signed-off-by: Antonio Caggiano <quic_acaggian@quicinc.com>
---
 include/exec/memory.h | 19 +++++++++++++++++++
 softmmu/physmem.c     | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 7ec6df3289..f959d53a12 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1045,6 +1045,18 @@ struct MemoryListener {
      */
     void (*coalesced_io_del)(MemoryListener *listener, MemoryRegionSection *section,
                                hwaddr addr, hwaddr len);
+
+    /**
+     * @map:
+     *
+     * Called during an address space map.
+     *
+     * @opaque: User data opaque object
+     * @addr: address within that address space
+     * @len: length of buffer
+     */
+    void (*map)(void *opaque, hwaddr addr, hwaddr len);
+
     /**
      * @priority:
      *
@@ -1054,6 +1066,13 @@ struct MemoryListener {
      */
     unsigned priority;
 
+    /**
+     * @opaque:
+     *
+     * Opaque pointer to user data
+     */
+    void *opaque;
+
     /**
      * @name:
      *
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 9486a1ebdf..0f8bad6b40 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -3246,6 +3246,38 @@ flatview_extend_translation(FlatView *fv, hwaddr addr,
     }
 }
 
+enum ListenerDirection { Forward, Reverse };
+
+/*
+ * This will require a change to the memory listener callbacks:
+ * while it currently uses "self" as first argument for the callbacks, this new
+ * approach is going to use an "opaque" member, effectively following the model
+ * used for MemoryRegion and MemoryRegionOps.
+ */
+#define MEMORY_LISTENER_CALL(_as, _callback, _direction, _args...) \
+    do {                                                                \
+        MemoryListener *_listener;                                      \
+                                                                        \
+        switch (_direction) {                                           \
+        case Forward:                                                   \
+            QTAILQ_FOREACH(_listener, &(_as)->listeners, link_as) {     \
+                if (_listener->_callback) {                             \
+                    _listener->_callback(_listener->opaque, ##_args);   \
+                }                                                       \
+            }                                                           \
+            break;                                                      \
+        case Reverse:                                                   \
+            QTAILQ_FOREACH_REVERSE(_listener, &(_as)->listeners, link_as) { \
+                if (_listener->_callback) {                             \
+                    _listener->_callback(_listener->opaque, ##_args);   \
+                }                                                       \
+            }                                                           \
+            break;                                                      \
+        default:                                                        \
+            abort();                                                    \
+        }                                                               \
+    } while (0)
+
 /* Map a physical memory region into a host virtual address.
  * May map a subset of the requested range, given by and returned in *plen.
  * May return NULL if resources needed to perform the mapping are exhausted.
@@ -3268,6 +3300,8 @@ void *address_space_map(AddressSpace *as,
         return NULL;
     }
 
+    MEMORY_LISTENER_CALL(as, map, Reverse, addr, len);
+
     l = len;
     RCU_READ_LOCK_GUARD();
     fv = address_space_to_flatview(as);
-- 
2.40.0

Re: [PATCH RFC 1/1] memory: Address space map listener

[David Hildenbrand]

On 05.04.23 14:57, Antonio Caggiano wrote:
> Introduce a MemoryListener callback for address space map events.
> 

Why?

-- 
Thanks,

David / dhildenb

Re: [PATCH RFC 1/1] memory: Address space map listener

[Antonio Caggiano]

Hi David,

On 05/04/23 15:23, David Hildenbrand wrote:
> On 05.04.23 14:57, Antonio Caggiano wrote:
>> Introduce a MemoryListener callback for address space map events.
>>
> 
> Why?
> 

Please, have a look at the cover letter "[PATCH RFC 0/1] MemoryListener 
address_space_map callback" with a detail explanation of the issue and 
the reason behind this. While I think it solves the issue for my use 
case, it might not be the best or even the right solution for it.

Cheers,
Antonio

Re: [PATCH RFC 1/1] memory: Address space map listener

[David Hildenbrand]

On 05.04.23 16:25, Antonio Caggiano wrote:
> Hi David,
> 
> On 05/04/23 15:23, David Hildenbrand wrote:
>> On 05.04.23 14:57, Antonio Caggiano wrote:
>>> Introduce a MemoryListener callback for address space map events.
>>>
>>
>> Why?
>>
> 
> Please, have a look at the cover letter "[PATCH RFC 0/1] MemoryListener
> address_space_map callback" with a detail explanation of the issue and
> the reason behind this. While I think it solves the issue for my use
> case, it might not be the best or even the right solution for it.

Oh, you did not CC me on the cover letter, so I missed it ...

-- 
Thanks,

David / dhildenb