Re: [PATCH v7 1/5] softmmu: Per-AddressSpace bounce buffering

[Jonathan Cameron]

On Mon, 12 Feb 2024 00:06:13 -0800
Mattias Nissler <mnissler@rivosinc.com> wrote:

> Instead of using a single global bounce buffer, give each AddressSpace
> its own bounce buffer. The MapClient callback mechanism moves to
> AddressSpace accordingly.
> 
> This is in preparation for generalizing bounce buffer handling further
> to allow multiple bounce buffers, with a total allocation limit
> configured per AddressSpace.
> 
> Signed-off-by: Mattias Nissler <mnissler@rivosinc.com>

Been using this for CXL testing (due to interleave everything needs
to be bounced) with virtio-blk-pci.
That needs an additional change as it doesn't use the pci address
space and I'm chasing down one other issue that I think is unrelated
to this patch (spurious huge transfer on power down).

On basis this works for me.

Tested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  include/exec/cpu-common.h |   2 -
>  include/exec/memory.h     |  45 ++++++++++++++++-
>  system/dma-helpers.c      |   4 +-
>  system/memory.c           |   7 +++
>  system/physmem.c          | 101 ++++++++++++++++----------------------
>  5 files changed, 93 insertions(+), 66 deletions(-)
> 
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index 9ead1be100..bd6999fa35 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -148,8 +148,6 @@ void *cpu_physical_memory_map(hwaddr addr,
>                                bool is_write);
>  void cpu_physical_memory_unmap(void *buffer, hwaddr len,
>                                 bool is_write, hwaddr access_len);
> -void cpu_register_map_client(QEMUBH *bh);
> -void cpu_unregister_map_client(QEMUBH *bh);
>  
>  bool cpu_physical_memory_is_io(hwaddr phys_addr);
>  
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 177be23db7..6995a443d3 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1106,6 +1106,19 @@ struct MemoryListener {
>      QTAILQ_ENTRY(MemoryListener) link_as;
>  };
>  
> +typedef struct AddressSpaceMapClient {
> +    QEMUBH *bh;
> +    QLIST_ENTRY(AddressSpaceMapClient) link;
> +} AddressSpaceMapClient;
> +
> +typedef struct {
> +    MemoryRegion *mr;
> +    void *buffer;
> +    hwaddr addr;
> +    hwaddr len;
> +    bool in_use;
> +} BounceBuffer;
> +
>  /**
>   * struct AddressSpace: describes a mapping of addresses to #MemoryRegion 
> objects
>   */
> @@ -1123,6 +1136,12 @@ struct AddressSpace {
>      struct MemoryRegionIoeventfd *ioeventfds;
>      QTAILQ_HEAD(, MemoryListener) listeners;
>      QTAILQ_ENTRY(AddressSpace) address_spaces_link;
> +
> +    /* Bounce buffer to use for this address space. */
> +    BounceBuffer bounce;
> +    /* List of callbacks to invoke when buffers free up */
> +    QemuMutex map_client_list_lock;
> +    QLIST_HEAD(, AddressSpaceMapClient) map_client_list;
>  };
>  
>  typedef struct AddressSpaceDispatch AddressSpaceDispatch;
> @@ -2926,8 +2945,8 @@ bool address_space_access_valid(AddressSpace *as, 
> hwaddr addr, hwaddr len,
>   * May return %NULL and set *@plen to zero(0), if resources needed to perform
>   * the mapping are exhausted.
>   * Use only for reads OR writes - not for read-modify-write operations.
> - * Use cpu_register_map_client() to know when retrying the map operation is
> - * likely to succeed.
> + * Use address_space_register_map_client() to know when retrying the map
> + * operation is likely to succeed.
>   *
>   * @as: #AddressSpace to be accessed
>   * @addr: address within that address space
> @@ -2952,6 +2971,28 @@ void *address_space_map(AddressSpace *as, hwaddr addr,
>  void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
>                           bool is_write, hwaddr access_len);
>  
> +/*
> + * address_space_register_map_client: Register a callback to invoke when
> + * resources for address_space_map() are available again.
> + *
> + * address_space_map may fail when there are not enough resources available,
> + * such as when bounce buffer memory would exceed the limit. The callback can
> + * be used to retry the address_space_map operation. Note that the callback
> + * gets automatically removed after firing.
> + *
> + * @as: #AddressSpace to be accessed
> + * @bh: callback to invoke when address_space_map() retry is appropriate
> + */
> +void address_space_register_map_client(AddressSpace *as, QEMUBH *bh);
> +
> +/*
> + * address_space_unregister_map_client: Unregister a callback that has
> + * previously been registered and not fired yet.
> + *
> + * @as: #AddressSpace to be accessed
> + * @bh: callback to unregister
> + */
> +void address_space_unregister_map_client(AddressSpace *as, QEMUBH *bh);
>  
>  /* Internal functions, part of the implementation of address_space_read.  */
>  MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
> diff --git a/system/dma-helpers.c b/system/dma-helpers.c
> index 9b221cf94e..74013308f5 100644
> --- a/system/dma-helpers.c
> +++ b/system/dma-helpers.c
> @@ -169,7 +169,7 @@ static void dma_blk_cb(void *opaque, int ret)
>      if (dbs->iov.size == 0) {
>          trace_dma_map_wait(dbs);
>          dbs->bh = aio_bh_new(ctx, reschedule_dma, dbs);
> -        cpu_register_map_client(dbs->bh);
> +        address_space_register_map_client(dbs->sg->as, dbs->bh);
>          return;
>      }
>  
> @@ -197,7 +197,7 @@ static void dma_aio_cancel(BlockAIOCB *acb)
>      }
>  
>      if (dbs->bh) {
> -        cpu_unregister_map_client(dbs->bh);
> +        address_space_unregister_map_client(dbs->sg->as, dbs->bh);
>          qemu_bh_delete(dbs->bh);
>          dbs->bh = NULL;
>      }
> diff --git a/system/memory.c b/system/memory.c
> index a229a79988..ad0caef1b8 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -3133,6 +3133,9 @@ void address_space_init(AddressSpace *as, MemoryRegion 
> *root, const char *name)
>      as->ioeventfds = NULL;
>      QTAILQ_INIT(&as->listeners);
>      QTAILQ_INSERT_TAIL(&address_spaces, as, address_spaces_link);
> +    as->bounce.in_use = false;
> +    qemu_mutex_init(&as->map_client_list_lock);
> +    QLIST_INIT(&as->map_client_list);
>      as->name = g_strdup(name ? name : "anonymous");
>      address_space_update_topology(as);
>      address_space_update_ioeventfds(as);
> @@ -3140,6 +3143,10 @@ void address_space_init(AddressSpace *as, MemoryRegion 
> *root, const char *name)
>  
>  static void do_address_space_destroy(AddressSpace *as)
>  {
> +    assert(!qatomic_read(&as->bounce.in_use));
> +    assert(QLIST_EMPTY(&as->map_client_list));
> +    qemu_mutex_destroy(&as->map_client_list_lock);
> +
>      assert(QTAILQ_EMPTY(&as->listeners));
>  
>      flatview_unref(as->current_map);
> diff --git a/system/physmem.c b/system/physmem.c
> index 5e66d9ae36..7170e3473f 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -2974,55 +2974,37 @@ void cpu_flush_icache_range(hwaddr start, hwaddr len)
>                                       NULL, len, FLUSH_CACHE);
>  }
>  
> -typedef struct {
> -    MemoryRegion *mr;
> -    void *buffer;
> -    hwaddr addr;
> -    hwaddr len;
> -    bool in_use;
> -} BounceBuffer;
> -
> -static BounceBuffer bounce;
> -
> -typedef struct MapClient {
> -    QEMUBH *bh;
> -    QLIST_ENTRY(MapClient) link;
> -} MapClient;
> -
> -QemuMutex map_client_list_lock;
> -static QLIST_HEAD(, MapClient) map_client_list
> -    = QLIST_HEAD_INITIALIZER(map_client_list);
> -
> -static void cpu_unregister_map_client_do(MapClient *client)
> +static void
> +address_space_unregister_map_client_do(AddressSpaceMapClient *client)
>  {
>      QLIST_REMOVE(client, link);
>      g_free(client);
>  }
>  
> -static void cpu_notify_map_clients_locked(void)
> +static void address_space_notify_map_clients_locked(AddressSpace *as)
>  {
> -    MapClient *client;
> +    AddressSpaceMapClient *client;
>  
> -    while (!QLIST_EMPTY(&map_client_list)) {
> -        client = QLIST_FIRST(&map_client_list);
> +    while (!QLIST_EMPTY(&as->map_client_list)) {
> +        client = QLIST_FIRST(&as->map_client_list);
>          qemu_bh_schedule(client->bh);
> -        cpu_unregister_map_client_do(client);
> +        address_space_unregister_map_client_do(client);
>      }
>  }
>  
> -void cpu_register_map_client(QEMUBH *bh)
> +void address_space_register_map_client(AddressSpace *as, QEMUBH *bh)
>  {
> -    MapClient *client = g_malloc(sizeof(*client));
> +    AddressSpaceMapClient *client = g_malloc(sizeof(*client));
>  
> -    qemu_mutex_lock(&map_client_list_lock);
> +    qemu_mutex_lock(&as->map_client_list_lock);
>      client->bh = bh;
> -    QLIST_INSERT_HEAD(&map_client_list, client, link);
> +    QLIST_INSERT_HEAD(&as->map_client_list, client, link);
>      /* Write map_client_list before reading in_use.  */
>      smp_mb();
> -    if (!qatomic_read(&bounce.in_use)) {
> -        cpu_notify_map_clients_locked();
> +    if (!qatomic_read(&as->bounce.in_use)) {
> +        address_space_notify_map_clients_locked(as);
>      }
> -    qemu_mutex_unlock(&map_client_list_lock);
> +    qemu_mutex_unlock(&as->map_client_list_lock);
>  }
>  
>  void cpu_exec_init_all(void)
> @@ -3038,28 +3020,27 @@ void cpu_exec_init_all(void)
>      finalize_target_page_bits();
>      io_mem_init();
>      memory_map_init();
> -    qemu_mutex_init(&map_client_list_lock);
>  }
>  
> -void cpu_unregister_map_client(QEMUBH *bh)
> +void address_space_unregister_map_client(AddressSpace *as, QEMUBH *bh)
>  {
> -    MapClient *client;
> +    AddressSpaceMapClient *client;
>  
> -    qemu_mutex_lock(&map_client_list_lock);
> -    QLIST_FOREACH(client, &map_client_list, link) {
> +    qemu_mutex_lock(&as->map_client_list_lock);
> +    QLIST_FOREACH(client, &as->map_client_list, link) {
>          if (client->bh == bh) {
> -            cpu_unregister_map_client_do(client);
> +            address_space_unregister_map_client_do(client);
>              break;
>          }
>      }
> -    qemu_mutex_unlock(&map_client_list_lock);
> +    qemu_mutex_unlock(&as->map_client_list_lock);
>  }
>  
> -static void cpu_notify_map_clients(void)
> +static void address_space_notify_map_clients(AddressSpace *as)
>  {
> -    qemu_mutex_lock(&map_client_list_lock);
> -    cpu_notify_map_clients_locked();
> -    qemu_mutex_unlock(&map_client_list_lock);
> +    qemu_mutex_lock(&as->map_client_list_lock);
> +    address_space_notify_map_clients_locked(as);
> +    qemu_mutex_unlock(&as->map_client_list_lock);
>  }
>  
>  static bool flatview_access_valid(FlatView *fv, hwaddr addr, hwaddr len,
> @@ -3126,8 +3107,8 @@ flatview_extend_translation(FlatView *fv, hwaddr addr,
>   * 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.
>   * Use only for reads OR writes - not for read-modify-write operations.
> - * Use cpu_register_map_client() to know when retrying the map operation is
> - * likely to succeed.
> + * Use address_space_register_map_client() to know when retrying the map
> + * operation is likely to succeed.
>   */
>  void *address_space_map(AddressSpace *as,
>                          hwaddr addr,
> @@ -3150,25 +3131,25 @@ void *address_space_map(AddressSpace *as,
>      mr = flatview_translate(fv, addr, &xlat, &l, is_write, attrs);
>  
>      if (!memory_access_is_direct(mr, is_write)) {
> -        if (qatomic_xchg(&bounce.in_use, true)) {
> +        if (qatomic_xchg(&as->bounce.in_use, true)) {
>              *plen = 0;
>              return NULL;
>          }
>          /* Avoid unbounded allocations */
>          l = MIN(l, TARGET_PAGE_SIZE);
> -        bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, l);
> -        bounce.addr = addr;
> -        bounce.len = l;
> +        as->bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, l);
> +        as->bounce.addr = addr;
> +        as->bounce.len = l;
>  
>          memory_region_ref(mr);
> -        bounce.mr = mr;
> +        as->bounce.mr = mr;
>          if (!is_write) {
>              flatview_read(fv, addr, MEMTXATTRS_UNSPECIFIED,
> -                               bounce.buffer, l);
> +                          as->bounce.buffer, l);
>          }
>  
>          *plen = l;
> -        return bounce.buffer;
> +        return as->bounce.buffer;
>      }
>  
>  
> @@ -3186,7 +3167,7 @@ void *address_space_map(AddressSpace *as,
>  void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
>                           bool is_write, hwaddr access_len)
>  {
> -    if (buffer != bounce.buffer) {
> +    if (buffer != as->bounce.buffer) {
>          MemoryRegion *mr;
>          ram_addr_t addr1;
>  
> @@ -3202,15 +3183,15 @@ void address_space_unmap(AddressSpace *as, void 
> *buffer, hwaddr len,
>          return;
>      }
>      if (is_write) {
> -        address_space_write(as, bounce.addr, MEMTXATTRS_UNSPECIFIED,
> -                            bounce.buffer, access_len);
> +        address_space_write(as, as->bounce.addr, MEMTXATTRS_UNSPECIFIED,
> +                            as->bounce.buffer, access_len);
>      }
> -    qemu_vfree(bounce.buffer);
> -    bounce.buffer = NULL;
> -    memory_region_unref(bounce.mr);
> +    qemu_vfree(as->bounce.buffer);
> +    as->bounce.buffer = NULL;
> +    memory_region_unref(as->bounce.mr);
>      /* Clear in_use before reading map_client_list.  */
> -    qatomic_set_mb(&bounce.in_use, false);
> -    cpu_notify_map_clients();
> +    qatomic_set_mb(&as->bounce.in_use, false);
> +    address_space_notify_map_clients(as);
>  }
>  
>  void *cpu_physical_memory_map(hwaddr addr,