[PATCH v4 0/3] system/memory: Make ram device region directly accessible

[PATCH v4 0/3] system/memory: Make ram device region directly accessible

[Gavin Shan]

All ram device regions was turned to be indirectly accessible by commit
4a2e242bbb ("memory: Don't use memcpy for ram_device regions"). This leads
to a frozen guest where a NVidia GH100 GPU is passed from host. The memory
in its PCI BAR#4 can be allocated as DMA target buffer. qemu has to take
DMA bounce buffer in address_space_map() to cover the DMA request. However,
the bounce buffer size is 4096 bytes only and it's exhaused very quickly
when the guest has significant disk activities on compiling 'cuda-samples'.
The full log and problem description can be found from PATCH[2/3]'s commit
log.

Fix the issue handled in commit 4a2e242bbb by replacing memmove() with newly
added qemu_ram_move() where the aligned and small-sized accesses are handled
by qatomics, and fall back to memmove() otherwise, for the directly accessible
regions. With this, we can revert 4a2e242bbb to make ram device region directly
accessible again and bypass the bounce buffer in address_space_map() where the
guest hang happens.

PATCH[1] replaces memcpy() with memomve() for directly accessible regions
PATCH[2] uses qemu_ram_move() for directly accessible regions
PATCH[3] makes ram device region directly accessible again

Changelog
=========
v3 -> v4:
  * https://lore.kernel.org/qemu-arm/20260616052552.389021-1-gshan@redhat.com/
  * New PATCH[1/3] to replace memcpy() with memmove() (Peterx)
  * Unified qemu_ram_move() for all architectures     (Peterx/Michael)
v2 -> v3:
  * https://lore.kernel.org/qemu-arm/20260615100200.266968-1-gshan@redhat.com/
  * Documentation for qemu_ram_{copy, move}           (Peter/Michael)
  * Support qemu_ram_move() for overlapped src/dest   (Richard)
  * Use {memcpy, memmove} if step is 16-bytes or more (Michael)
  * Code improvements                                 (Richard/Michael)
v1 -> v2:
  * https://lore.kernel.org/qemu-arm/20260612110307.1264798-1-gshan@redhat.com/
  * Rename address_space_{memcpy, memmove}() to qemu_ram_{copy, move}()
    and move them to physmem.c and memory.h   (Philippe)
  * Use memcpy() and memmove() in qemu_ram_{copy, move}() for the variable
    length case                               (Miachel)
  * Handle unaligned access in qemu_ram_{copy, move}() for all archs
    except i386 and x86_64                    (Richard/Michael)
RFCv1 -> v1:
  * https://lists.nongnu.org/archive/html/qemu-arm/2026-06/msg00307.html
  * Reworked solution based on suggestions from Peter Xu, Peter Maydell
    and Michael S. Tsirkin

Gavin Shan (3):
  system/memory: Use memmove() for directly accessible regions
  system/memory: Use qemu_ram_move() for directly accessible regions
  system/memory: Make ram device region directly accessible

 hw/remote/vfio-user-obj.c |  4 ++--
 include/system/memory.h   | 28 ++++++++++++++++----------
 system/memory.c           | 41 +--------------------------------------
 system/physmem.c          | 39 +++++++++++++++++++++++++++++++++++--
 system/trace-events       |  2 --
 5 files changed, 58 insertions(+), 56 deletions(-)

-- 
2.55.0

[PATCH v4 1/3] system/memory: Use memmove() for directly accessible regions

[Gavin Shan]

Similar to what's done in commit 4a73aee88140 ("softmmu: Use memmove in
flatview_write_continue"), there are more sites where the overlapping
source and destination buffer are allowed for the directly accessible
regions. Use memmove() in those sites, listed as below.

  hw/remote/vfio-user-obj.c::vfu_object_mr_rw
  include/system/memory.h::address_space_read
  system/physmem.c::flatview_read_continue_step

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
v4: New patch
---
 hw/remote/vfio-user-obj.c | 4 ++--
 include/system/memory.h   | 2 +-
 system/physmem.c          | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
index 87fa7b6572..ea50270628 100644
--- a/hw/remote/vfio-user-obj.c
+++ b/hw/remote/vfio-user-obj.c
@@ -375,9 +375,9 @@ static int vfu_object_mr_rw(MemoryRegion *mr, uint8_t *buf, hwaddr offset,
         ram_ptr = memory_region_get_ram_ptr(mr);
 
         if (is_write) {
-            memcpy((ram_ptr + offset), buf, size);
+            memmove((ram_ptr + offset), buf, size);
         } else {
-            memcpy(buf, (ram_ptr + offset), size);
+            memmove(buf, (ram_ptr + offset), size);
         }
 
         return 0;
diff --git a/include/system/memory.h b/include/system/memory.h
index 47a0e06fbf..d6ca431b6d 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -2739,7 +2739,7 @@ MemTxResult address_space_read(const AddressSpace *as, hwaddr addr,
             mr = flatview_translate(fv, addr, &addr1, &l, false, attrs);
             if (len == l && memory_access_is_direct(mr, false, attrs)) {
                 ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
-                memcpy(buf, ptr, len);
+                memmove(buf, ptr, len);
             } else {
                 result = flatview_read_continue(fv, addr, attrs, buf, len,
                                                 addr1, l, mr);
diff --git a/system/physmem.c b/system/physmem.c
index c21ea92915..2c42e365cb 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3363,7 +3363,7 @@ static MemTxResult flatview_read_continue_step(MemTxAttrs attrs, uint8_t *buf,
         uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, l,
                                                false, false);
 
-        memcpy(buf, ram_ptr, *l);
+        memmove(buf, ram_ptr, *l);
 
         return MEMTX_OK;
     }
-- 
2.55.0

Re: [PATCH v4 1/3] system/memory: Use memmove() for directly accessible regions

[Peter Maydell]

On Mon, 27 Jul 2026 at 04:27, Gavin Shan <gshan@redhat.com> wrote:
>
> Similar to what's done in commit 4a73aee88140 ("softmmu: Use memmove in
> flatview_write_continue"), there are more sites where the overlapping
> source and destination buffer are allowed for the directly accessible
> regions. Use memmove() in those sites, listed as below.
>
>   hw/remote/vfio-user-obj.c::vfu_object_mr_rw
>   include/system/memory.h::address_space_read
>   system/physmem.c::flatview_read_continue_step
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Even if the overlap isn't possible, it's definitely better
to use memmove() than to have to think hard about whether
there's some situation where it might happen :-)

thanks
-- PMM

Re: [PATCH v4 1/3] system/memory: Use memmove() for directly accessible regions

[Peter Xu]

On Mon, Jul 27, 2026 at 01:26:47PM +1000, Gavin Shan wrote:
> Similar to what's done in commit 4a73aee88140 ("softmmu: Use memmove in
> flatview_write_continue"), there are more sites where the overlapping
> source and destination buffer are allowed for the directly accessible
> regions. Use memmove() in those sites, listed as below.
> 
>   hw/remote/vfio-user-obj.c::vfu_object_mr_rw
>   include/system/memory.h::address_space_read
>   system/physmem.c::flatview_read_continue_step
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH v4 2/3] system/memory: Use qemu_ram_move() for directly accessible regions

[Gavin Shan]

All ram device regions were turned to be indirectly accessible by commit
4a2e242bbb ("memory: Don't use memcpy for ram_device regions"). This leads
to guest hang on attempt to build 'cuda-samples' as reported by Julia. The
guest is started by the following command lines, with GH100 GPU card passed
from the host.

   host$ lspci | grep GH100
   0009:01:00.0 3D controller: NVIDIA Corporation GH100 [GH200 120GB / 480GB] (rev a1)
   host$ /home/sandbox/gavin/qemu.main/build/qemu-system-aarch64            \
         -machine virt,gic-version=host,ras=on,highmem-mmio-size=4T         \
         -accel kvm -cpu host -smp cpus=48 -m size=8G                       \
         -drive file=/home/gavin/sandbox/images/disk.qcow2,if=none,id=d0    \
         -device virtio-blk-pci,id=vb0,bus=pcie.0,drive=d0,num-queues=4     \
         -device vfio-pci-nohotplug,host=0009:01:00.0,bus=pcie.1.0
           :
   guest$ cd cuda-samples/build
   guest$ make -j 20 clean
   guest$ make -j 20
           :
   [ 54%] Linking CUDA executable graphMemoryNodes
   [ 54%] Built target graphMemoryNodes
   <no more output afterwards, guest becomes frozen here>

   guest$ qemu-system-aarch64: virtio: bogus descriptor or out of resources
   [  555.814025] virtio_blk virtio0: [vda] new size: 268435456 512-byte logical blocks (137 GB/128 GiB)

When the GPU's driver (NVidia open driver) is loaded on guest bootup,
the memory blocks residing in the PCI BAR#4 of the GH100 GPU card can
be presented to the guest through memory hot-add. The page cache can
then be allocated from the hot added memory blocks when cuda-samples
is being built. Afterwards, the page cache is sent to QEMU's virtio-blk
device as part of the DMA request, the bounce buffer has to be used to
accomodate the request as the corresponding memory region (MemoryRegion)
is an indirectly accessible ram device region in qemu. However, the max
bounce bufer size is only 4096 bytes by default and that is exhausted
quickly, leading to a reset on the virtio-blk device and frozen guest
eventually.

  QEMU
  ====
  virtio_blk_handle_output
    virtio_blk_handle_vq
      virtio_blk_get_request
        virtqueue_pop
          virtqueue_split_pop
            virtqueue_map_desc
              address_space_map
                memory_access_is_direct         # Return false
                  memory_region_supports_direct_access

  (qemu) info mtree
  memory-region: pci_bridge_pci
    0000000000000000-ffffffffffffffff (prio 0, container): pci_bridge_pci
      0000042000000000-0000043fffffffff (prio 1, i/o): 0009:01:00.0 base BAR 4
        0000042000000000-0000043fffffffff (prio 0, i/o): 0009:01:00.0 BAR 4
          0000042000000000-000004379fffffff (prio 0, ramd): 0009:01:00.0 BAR 4 mmaps[0]

This adds qemu_ram_move() where the aligned and small-sized accesses are
handled by qatomics, and fall back to memmove() otherwise. The memove()
for the directly accessible regions is replaced by qemu_ram_move() so that
the issue covered by commit 4a2e242bbb (MMIO access instructions were
optimized to SSE instructions) is fixed. This makes 'ram_device_mem_ops'
redundant, paving the way to revert that commit to make the ram device
region directly accessible again in the next patch.

Reported-by: Julia Graham <jugraham@redhat.com>
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Suggested-by: Peter Xu <peterx@redhat.com>
Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
v4: Unified qemu_ram_move() for all architectures where the qatomics are
    used for aligned and small-sized accesses, and fall back to memmove()
    otherwise. system/physmem.c::address_space_write_rom is removed from
    the listed sites.
---
 hw/remote/vfio-user-obj.c |  4 ++--
 include/system/memory.h   | 17 ++++++++++++++++-
 system/physmem.c          | 39 +++++++++++++++++++++++++++++++++++++--
 3 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
index ea50270628..a0498d218f 100644
--- a/hw/remote/vfio-user-obj.c
+++ b/hw/remote/vfio-user-obj.c
@@ -375,9 +375,9 @@ static int vfu_object_mr_rw(MemoryRegion *mr, uint8_t *buf, hwaddr offset,
         ram_ptr = memory_region_get_ram_ptr(mr);
 
         if (is_write) {
-            memmove((ram_ptr + offset), buf, size);
+            qemu_ram_move((ram_ptr + offset), buf, size);
         } else {
-            memmove(buf, (ram_ptr + offset), size);
+            qemu_ram_move(buf, (ram_ptr + offset), size);
         }
 
         return 0;
diff --git a/include/system/memory.h b/include/system/memory.h
index d6ca431b6d..1a28f9b873 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -2666,6 +2666,21 @@ void address_space_register_map_client(AddressSpace *as, QEMUBH *bh);
 void address_space_unregister_map_client(AddressSpace *as, QEMUBH *bh);
 
 /* Internal functions, part of the implementation of address_space_read.  */
+
+/**
+ * qemu_ram_move: move data from or to ramblock
+ *
+ * @dst: destination where the data is moved to
+ * @src: source where the data is moved from
+ * @n: length of data to be moved
+ *
+ * Move @n bytes from @src to @dst with the assumption that @src and @dst
+ * can overlap. The access is atomic if the source and destination buffer
+ * aren't overlapped for a well aligned and small-sized access. Otherwise,
+ * fall back to the standard memmove().
+ */
+void qemu_ram_move(void *dst, const void *src, size_t n);
+
 MemTxResult address_space_read_full(const AddressSpace *as, hwaddr addr,
                                     MemTxAttrs attrs, void *buf, hwaddr len);
 MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
@@ -2739,7 +2754,7 @@ MemTxResult address_space_read(const AddressSpace *as, hwaddr addr,
             mr = flatview_translate(fv, addr, &addr1, &l, false, attrs);
             if (len == l && memory_access_is_direct(mr, false, attrs)) {
                 ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
-                memmove(buf, ptr, len);
+                qemu_ram_move(buf, ptr, len);
             } else {
                 result = flatview_read_continue(fv, addr, attrs, buf, len,
                                                 addr1, l, mr);
diff --git a/system/physmem.c b/system/physmem.c
index 2c42e365cb..18485f7b08 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3158,6 +3158,41 @@ void memory_region_flush_rom_device(MemoryRegion *mr, hwaddr addr, hwaddr size)
     invalidate_and_set_dirty(mr, addr, size);
 }
 
+void qemu_ram_move(void *dst, const void *src, size_t n)
+{
+    uintptr_t test, len;
+
+    if (src == dst || n == 0) {
+        return;
+    }
+
+    test = (uintptr_t)src | (uintptr_t)dst | n;
+    len = test & -test;
+
+    /* Overlapping buffers, unaligned or oversized access */
+    if (n > 8 || len != n) {
+        memmove(dst, src, n);
+        return;
+    }
+
+    switch (len) {
+    case 1:
+        qatomic_set((uint8_t *)dst, qatomic_read((uint8_t *)src));
+        break;
+    case 2:
+        qatomic_set((uint16_t *)dst, qatomic_read((uint16_t *)src));
+        break;
+    case 4:
+        qatomic_set((uint32_t *)dst, qatomic_read((uint32_t *)src));
+        break;
+    case 8:
+        qatomic_set((uint64_t *)dst, qatomic_read((uint64_t *)src));
+        break;
+    default:
+        g_assert_not_reached();
+    }
+}
+
 int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
 {
     unsigned access_size_max = mr->ops->valid.max_access_size;
@@ -3270,7 +3305,7 @@ static MemTxResult flatview_write_continue_step(MemTxAttrs attrs,
         uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, l,
                                                false, true);
 
-        memmove(ram_ptr, buf, *l);
+        qemu_ram_move(ram_ptr, buf, *l);
         invalidate_and_set_dirty(mr, mr_addr, *l);
 
         return MEMTX_OK;
@@ -3363,7 +3398,7 @@ static MemTxResult flatview_read_continue_step(MemTxAttrs attrs, uint8_t *buf,
         uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, l,
                                                false, false);
 
-        memmove(buf, ram_ptr, *l);
+        qemu_ram_move(buf, ram_ptr, *l);
 
         return MEMTX_OK;
     }
-- 
2.55.0

Re: [PATCH v4 2/3] system/memory: Use qemu_ram_move() for directly accessible regions

[Peter Maydell]

On Mon, 27 Jul 2026 at 04:27, Gavin Shan <gshan@redhat.com> wrote:

Initial note: I think this is basically the right thing; I have
some suggestions for beefing up the doc comment and some minor
other things below.

> All ram device regions were turned to be indirectly accessible by commit
> 4a2e242bbb ("memory: Don't use memcpy for ram_device regions"). This leads
> to guest hang on attempt to build 'cuda-samples' as reported by Julia. The
> guest is started by the following command lines, with GH100 GPU card passed
> from the host.
>
>    host$ lspci | grep GH100
>    0009:01:00.0 3D controller: NVIDIA Corporation GH100 [GH200 120GB / 480GB] (rev a1)
>    host$ /home/sandbox/gavin/qemu.main/build/qemu-system-aarch64            \
>          -machine virt,gic-version=host,ras=on,highmem-mmio-size=4T         \
>          -accel kvm -cpu host -smp cpus=48 -m size=8G                       \
>          -drive file=/home/gavin/sandbox/images/disk.qcow2,if=none,id=d0    \
>          -device virtio-blk-pci,id=vb0,bus=pcie.0,drive=d0,num-queues=4     \
>          -device vfio-pci-nohotplug,host=0009:01:00.0,bus=pcie.1.0
>            :
>    guest$ cd cuda-samples/build
>    guest$ make -j 20 clean
>    guest$ make -j 20
>            :
>    [ 54%] Linking CUDA executable graphMemoryNodes
>    [ 54%] Built target graphMemoryNodes
>    <no more output afterwards, guest becomes frozen here>
>
>    guest$ qemu-system-aarch64: virtio: bogus descriptor or out of resources
>    [  555.814025] virtio_blk virtio0: [vda] new size: 268435456 512-byte logical blocks (137 GB/128 GiB)
>
> When the GPU's driver (NVidia open driver) is loaded on guest bootup,
> the memory blocks residing in the PCI BAR#4 of the GH100 GPU card can
> be presented to the guest through memory hot-add. The page cache can
> then be allocated from the hot added memory blocks when cuda-samples
> is being built. Afterwards, the page cache is sent to QEMU's virtio-blk
> device as part of the DMA request, the bounce buffer has to be used to
> accomodate the request as the corresponding memory region (MemoryRegion)
> is an indirectly accessible ram device region in qemu. However, the max
> bounce bufer size is only 4096 bytes by default and that is exhausted
> quickly, leading to a reset on the virtio-blk device and frozen guest
> eventually.
>
>   QEMU
>   ====
>   virtio_blk_handle_output
>     virtio_blk_handle_vq
>       virtio_blk_get_request
>         virtqueue_pop
>           virtqueue_split_pop
>             virtqueue_map_desc
>               address_space_map
>                 memory_access_is_direct         # Return false
>                   memory_region_supports_direct_access
>
>   (qemu) info mtree
>   memory-region: pci_bridge_pci
>     0000000000000000-ffffffffffffffff (prio 0, container): pci_bridge_pci
>       0000042000000000-0000043fffffffff (prio 1, i/o): 0009:01:00.0 base BAR 4
>         0000042000000000-0000043fffffffff (prio 0, i/o): 0009:01:00.0 BAR 4
>           0000042000000000-000004379fffffff (prio 0, ramd): 0009:01:00.0 BAR 4 mmaps[0]
>
> This adds qemu_ram_move() where the aligned and small-sized accesses are
> handled by qatomics, and fall back to memmove() otherwise. The memove()
> for the directly accessible regions is replaced by qemu_ram_move() so that
> the issue covered by commit 4a2e242bbb (MMIO access instructions were
> optimized to SSE instructions) is fixed. This makes 'ram_device_mem_ops'
> redundant, paving the way to revert that commit to make the ram device
> region directly accessible again in the next patch.

I think this commit message should also describe the second category
of bug that we intend it to fix: the one where a device does e.g.
address_space_stb() to a data structure in guest memory and requires
it to write exactly that byte exactly once.


> Reported-by: Julia Graham <jugraham@redhat.com>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Suggested-by: Peter Xu <peterx@redhat.com>
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> --- a/include/system/memory.h
> +++ b/include/system/memory.h
> @@ -2666,6 +2666,21 @@ void address_space_register_map_client(AddressSpace *as, QEMUBH *bh);
>  void address_space_unregister_map_client(AddressSpace *as, QEMUBH *bh);
>
>  /* Internal functions, part of the implementation of address_space_read.  */
> +
> +/**
> + * qemu_ram_move: move data from or to ramblock
> + *
> + * @dst: destination where the data is moved to
> + * @src: source where the data is moved from
> + * @n: length of data to be moved
> + *
> + * Move @n bytes from @src to @dst with the assumption that @src and @dst
> + * can overlap. The access is atomic if the source and destination buffer
> + * aren't overlapped for a well aligned and small-sized access. Otherwise,
> + * fall back to the standard memmove().
> + */

I think we could usefully expand this comment, because the reasons
we need it are not immediately obvious. How about:

===begin===
Move @n bytes from @src to @dst; the memory areas may overlap.
This provides the same semantics as memmove(), plus an additional
stronger guarantee: if @n is 1, 2 or 4 or 8 bytes, and @src
and @dst are both naturally aligned for that access size, and
the memory areas do not overlap, then both the load and the store
will be done as a single atomic access (with the semantics of
qatomic_read() and qatomic_set()).

This is the underlying function that we use to implement accesses
by a guest vCPU or a device DMA operation to a ram block. The
atomic guarantee is needed for two major cases:
 - when the ram block is backed by a PCI BAR passed through
   from a host device (and so it might be hardware registers
   that must be accessed exactly once at the right width)
 - when an emulated device updates a data structure shared in
   guest memory with guest software (e.g. a network device's
   set of tx and rx descriptor blocks), if a write to memory
   is accidentally performed multiple times then it can break
   the guest code.

We don't attempt to perform the exact access when it would
be unaligned, because this can't necessarily be done on
all host architectures; although this is strictly speaking
not doing what would happen on real hardware, we don't think
there are going to be situations where that matters in practice.
===endit===

?

(if you take my suggestion below, then delete "and the
memory areas do not overlap", because the only case
where the accesses are naturally aligned and they
overlap is the case of src == dst)


> diff --git a/system/physmem.c b/system/physmem.c
> index 2c42e365cb..18485f7b08 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -3158,6 +3158,41 @@ void memory_region_flush_rom_device(MemoryRegion *mr, hwaddr addr, hwaddr size)
>      invalidate_and_set_dirty(mr, addr, size);
>  }
>
> +void qemu_ram_move(void *dst, const void *src, size_t n)
> +{
> +    uintptr_t test, len;
> +
> +    if (src == dst || n == 0) {

I think we should not bother testing for src == dst. It's
vanishingly unlikely to actually happen, and if it does
happen then the code will work fine, and it's probably better
to actually do the access in that case than to skip it.

> +        return;
> +    }
> +
> +    test = (uintptr_t)src | (uintptr_t)dst | n;
> +    len = test & -test;

What is this doing? I am not a fan of clever bit twiddling
that isn't commented to explain itself. Readers of the code
shouldn't have to go off and search for an explanation of what
is going on.

> +
> +    /* Overlapping buffers, unaligned or oversized access */
> +    if (n > 8 || len != n) {
> +        memmove(dst, src, n);
> +        return;
> +    }
> +
> +    switch (len) {
> +    case 1:
> +        qatomic_set((uint8_t *)dst, qatomic_read((uint8_t *)src));
> +        break;
> +    case 2:
> +        qatomic_set((uint16_t *)dst, qatomic_read((uint16_t *)src));
> +        break;
> +    case 4:
> +        qatomic_set((uint32_t *)dst, qatomic_read((uint32_t *)src));
> +        break;
> +    case 8:
> +        qatomic_set((uint64_t *)dst, qatomic_read((uint64_t *)src));
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}

thanks
-- PMM

Re: [PATCH v4 2/3] system/memory: Use qemu_ram_move() for directly accessible regions

[Philippe Mathieu-Daudé]

On 27/7/26 14:51, Peter Maydell wrote:
> On Mon, 27 Jul 2026 at 04:27, Gavin Shan <gshan@redhat.com> wrote:
> 
> Initial note: I think this is basically the right thing; I have
> some suggestions for beefing up the doc comment and some minor
> other things below.
> 
>> All ram device regions were turned to be indirectly accessible by commit
>> 4a2e242bbb ("memory: Don't use memcpy for ram_device regions"). This leads
>> to guest hang on attempt to build 'cuda-samples' as reported by Julia. The
>> guest is started by the following command lines, with GH100 GPU card passed
>> from the host.
>>
>>     host$ lspci | grep GH100
>>     0009:01:00.0 3D controller: NVIDIA Corporation GH100 [GH200 120GB / 480GB] (rev a1)
>>     host$ /home/sandbox/gavin/qemu.main/build/qemu-system-aarch64            \
>>           -machine virt,gic-version=host,ras=on,highmem-mmio-size=4T         \
>>           -accel kvm -cpu host -smp cpus=48 -m size=8G                       \
>>           -drive file=/home/gavin/sandbox/images/disk.qcow2,if=none,id=d0    \
>>           -device virtio-blk-pci,id=vb0,bus=pcie.0,drive=d0,num-queues=4     \
>>           -device vfio-pci-nohotplug,host=0009:01:00.0,bus=pcie.1.0
>>             :
>>     guest$ cd cuda-samples/build
>>     guest$ make -j 20 clean
>>     guest$ make -j 20
>>             :
>>     [ 54%] Linking CUDA executable graphMemoryNodes
>>     [ 54%] Built target graphMemoryNodes
>>     <no more output afterwards, guest becomes frozen here>
>>
>>     guest$ qemu-system-aarch64: virtio: bogus descriptor or out of resources
>>     [  555.814025] virtio_blk virtio0: [vda] new size: 268435456 512-byte logical blocks (137 GB/128 GiB)
>>
>> When the GPU's driver (NVidia open driver) is loaded on guest bootup,
>> the memory blocks residing in the PCI BAR#4 of the GH100 GPU card can
>> be presented to the guest through memory hot-add. The page cache can
>> then be allocated from the hot added memory blocks when cuda-samples
>> is being built. Afterwards, the page cache is sent to QEMU's virtio-blk
>> device as part of the DMA request, the bounce buffer has to be used to
>> accomodate the request as the corresponding memory region (MemoryRegion)
>> is an indirectly accessible ram device region in qemu. However, the max
>> bounce bufer size is only 4096 bytes by default and that is exhausted
>> quickly, leading to a reset on the virtio-blk device and frozen guest
>> eventually.
>>
>>    QEMU
>>    ====
>>    virtio_blk_handle_output
>>      virtio_blk_handle_vq
>>        virtio_blk_get_request
>>          virtqueue_pop
>>            virtqueue_split_pop
>>              virtqueue_map_desc
>>                address_space_map
>>                  memory_access_is_direct         # Return false
>>                    memory_region_supports_direct_access
>>
>>    (qemu) info mtree
>>    memory-region: pci_bridge_pci
>>      0000000000000000-ffffffffffffffff (prio 0, container): pci_bridge_pci
>>        0000042000000000-0000043fffffffff (prio 1, i/o): 0009:01:00.0 base BAR 4
>>          0000042000000000-0000043fffffffff (prio 0, i/o): 0009:01:00.0 BAR 4
>>            0000042000000000-000004379fffffff (prio 0, ramd): 0009:01:00.0 BAR 4 mmaps[0]
>>
>> This adds qemu_ram_move() where the aligned and small-sized accesses are
>> handled by qatomics, and fall back to memmove() otherwise. The memove()
>> for the directly accessible regions is replaced by qemu_ram_move() so that
>> the issue covered by commit 4a2e242bbb (MMIO access instructions were
>> optimized to SSE instructions) is fixed. This makes 'ram_device_mem_ops'
>> redundant, paving the way to revert that commit to make the ram device
>> region directly accessible again in the next patch.
> 
> I think this commit message should also describe the second category
> of bug that we intend it to fix: the one where a device does e.g.
> address_space_stb() to a data structure in guest memory and requires
> it to write exactly that byte exactly once.
> 
> 
>> Reported-by: Julia Graham <jugraham@redhat.com>
>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>> Suggested-by: Peter Xu <peterx@redhat.com>
>> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
---

>> +void qemu_ram_move(void *dst, const void *src, size_t n)
>> +{
>> +    uintptr_t test, len;
>> +
>> +    if (src == dst || n == 0) {
> 
> I think we should not bother testing for src == dst. It's
> vanishingly unlikely to actually happen, and if it does
> happen then the code will work fine, and it's probably better
> to actually do the access in that case than to skip it.
> 
>> +        return;
>> +    }
>> +
>> +    test = (uintptr_t)src | (uintptr_t)dst | n;
>> +    len = test & -test;
> 
> What is this doing? I am not a fan of clever bit twiddling
> that isn't commented to explain itself. Readers of the code
> shouldn't have to go off and search for an explanation of what
> is going on.

"max alignment of the 3 values"?

> 
>> +
>> +    /* Overlapping buffers, unaligned or oversized access */
>> +    if (n > 8 || len != n) {
>> +        memmove(dst, src, n);
>> +        return;
>> +    }
>> +
>> +    switch (len) {
>> +    case 1:
>> +        qatomic_set((uint8_t *)dst, qatomic_read((uint8_t *)src));
>> +        break;
>> +    case 2:
>> +        qatomic_set((uint16_t *)dst, qatomic_read((uint16_t *)src));
>> +        break;
>> +    case 4:
>> +        qatomic_set((uint32_t *)dst, qatomic_read((uint32_t *)src));
>> +        break;
>> +    case 8:
>> +        qatomic_set((uint64_t *)dst, qatomic_read((uint64_t *)src));
>> +        break;
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +}

Maybe more readable (untested):

-- >8 --
  void qemu_ram_move(void *dst, const void *src, size_t len)
  {
      if (unlikely(n == 0)) {
          return;
      }
      if (len == 1) {
          qatomic_set((uint8_t *)dst, qatomic_read((uint8_t *)src));
          return;
      } else if (QEMU_PTR_IS_ALIGNED(dst, len) && 
QEMU_PTR_IS_ALIGNED(src, len)) {
          switch (len) {
          case 2:
              qatomic_set((uint16_t *)dst, qatomic_read((uint16_t *)src));
              return;
          case 4:
              qatomic_set((uint32_t *)dst, qatomic_read((uint32_t *)src));
            return;
          case 8:
              qatomic_set((uint64_t *)dst, qatomic_read((uint64_t *)src));
              return;
          default:
              break;
        }
     }
     /* Overlapping buffers, unaligned or oversized access */
     memmove(dst, src, len);
  }
---

Re: [PATCH v4 2/3] system/memory: Use qemu_ram_move() for directly accessible regions

[Gavin Shan]

On 7/27/26 11:52 PM, Philippe Mathieu-Daudé wrote:
> On 27/7/26 14:51, Peter Maydell wrote:
>> On Mon, 27 Jul 2026 at 04:27, Gavin Shan <gshan@redhat.com> wrote:
>>
>> Initial note: I think this is basically the right thing; I have
>> some suggestions for beefing up the doc comment and some minor
>> other things below.
>>
>>> All ram device regions were turned to be indirectly accessible by commit
>>> 4a2e242bbb ("memory: Don't use memcpy for ram_device regions"). This leads
>>> to guest hang on attempt to build 'cuda-samples' as reported by Julia. The
>>> guest is started by the following command lines, with GH100 GPU card passed
>>> from the host.
>>>
>>>     host$ lspci | grep GH100
>>>     0009:01:00.0 3D controller: NVIDIA Corporation GH100 [GH200 120GB / 480GB] (rev a1)
>>>     host$ /home/sandbox/gavin/qemu.main/build/qemu-system-aarch64            \
>>>           -machine virt,gic-version=host,ras=on,highmem-mmio-size=4T         \
>>>           -accel kvm -cpu host -smp cpus=48 -m size=8G                       \
>>>           -drive file=/home/gavin/sandbox/images/disk.qcow2,if=none,id=d0    \
>>>           -device virtio-blk-pci,id=vb0,bus=pcie.0,drive=d0,num-queues=4     \
>>>           -device vfio-pci-nohotplug,host=0009:01:00.0,bus=pcie.1.0
>>>             :
>>>     guest$ cd cuda-samples/build
>>>     guest$ make -j 20 clean
>>>     guest$ make -j 20
>>>             :
>>>     [ 54%] Linking CUDA executable graphMemoryNodes
>>>     [ 54%] Built target graphMemoryNodes
>>>     <no more output afterwards, guest becomes frozen here>
>>>
>>>     guest$ qemu-system-aarch64: virtio: bogus descriptor or out of resources
>>>     [  555.814025] virtio_blk virtio0: [vda] new size: 268435456 512-byte logical blocks (137 GB/128 GiB)
>>>
>>> When the GPU's driver (NVidia open driver) is loaded on guest bootup,
>>> the memory blocks residing in the PCI BAR#4 of the GH100 GPU card can
>>> be presented to the guest through memory hot-add. The page cache can
>>> then be allocated from the hot added memory blocks when cuda-samples
>>> is being built. Afterwards, the page cache is sent to QEMU's virtio-blk
>>> device as part of the DMA request, the bounce buffer has to be used to
>>> accomodate the request as the corresponding memory region (MemoryRegion)
>>> is an indirectly accessible ram device region in qemu. However, the max
>>> bounce bufer size is only 4096 bytes by default and that is exhausted
>>> quickly, leading to a reset on the virtio-blk device and frozen guest
>>> eventually.
>>>
>>>    QEMU
>>>    ====
>>>    virtio_blk_handle_output
>>>      virtio_blk_handle_vq
>>>        virtio_blk_get_request
>>>          virtqueue_pop
>>>            virtqueue_split_pop
>>>              virtqueue_map_desc
>>>                address_space_map
>>>                  memory_access_is_direct         # Return false
>>>                    memory_region_supports_direct_access
>>>
>>>    (qemu) info mtree
>>>    memory-region: pci_bridge_pci
>>>      0000000000000000-ffffffffffffffff (prio 0, container): pci_bridge_pci
>>>        0000042000000000-0000043fffffffff (prio 1, i/o): 0009:01:00.0 base BAR 4
>>>          0000042000000000-0000043fffffffff (prio 0, i/o): 0009:01:00.0 BAR 4
>>>            0000042000000000-000004379fffffff (prio 0, ramd): 0009:01:00.0 BAR 4 mmaps[0]
>>>
>>> This adds qemu_ram_move() where the aligned and small-sized accesses are
>>> handled by qatomics, and fall back to memmove() otherwise. The memove()
>>> for the directly accessible regions is replaced by qemu_ram_move() so that
>>> the issue covered by commit 4a2e242bbb (MMIO access instructions were
>>> optimized to SSE instructions) is fixed. This makes 'ram_device_mem_ops'
>>> redundant, paving the way to revert that commit to make the ram device
>>> region directly accessible again in the next patch.
>>
>> I think this commit message should also describe the second category
>> of bug that we intend it to fix: the one where a device does e.g.
>> address_space_stb() to a data structure in guest memory and requires
>> it to write exactly that byte exactly once.
>>
>>
>>> Reported-by: Julia Graham <jugraham@redhat.com>
>>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>>> Suggested-by: Peter Xu <peterx@redhat.com>
>>> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
>>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
> 
>>> +void qemu_ram_move(void *dst, const void *src, size_t n)
>>> +{
>>> +    uintptr_t test, len;
>>> +
>>> +    if (src == dst || n == 0) {
>>
>> I think we should not bother testing for src == dst. It's
>> vanishingly unlikely to actually happen, and if it does
>> happen then the code will work fine, and it's probably better
>> to actually do the access in that case than to skip it.
>>
>>> +        return;
>>> +    }
>>> +
>>> +    test = (uintptr_t)src | (uintptr_t)dst | n;
>>> +    len = test & -test;
>>
>> What is this doing? I am not a fan of clever bit twiddling
>> that isn't commented to explain itself. Readers of the code
>> shouldn't have to go off and search for an explanation of what
>> is going on.
> 
> "max alignment of the 3 values"?
> 

Yes, it's the maximal length of the aligned access that is determined
by @src, @dst and @n. I've put a comments for (v5).

     /*
      * Maximal length of aligned access that are determined by @src,
      * @dst and @n
      */

>>
>>> +
>>> +    /* Overlapping buffers, unaligned or oversized access */
>>> +    if (n > 8 || len != n) {
>>> +        memmove(dst, src, n);
>>> +        return;
>>> +    }
>>> +
>>> +    switch (len) {
>>> +    case 1:
>>> +        qatomic_set((uint8_t *)dst, qatomic_read((uint8_t *)src));
>>> +        break;
>>> +    case 2:
>>> +        qatomic_set((uint16_t *)dst, qatomic_read((uint16_t *)src));
>>> +        break;
>>> +    case 4:
>>> +        qatomic_set((uint32_t *)dst, qatomic_read((uint32_t *)src));
>>> +        break;
>>> +    case 8:
>>> +        qatomic_set((uint64_t *)dst, qatomic_read((uint64_t *)src));
>>> +        break;
>>> +    default:
>>> +        g_assert_not_reached();
>>> +    }
>>> +}
> 
> Maybe more readable (untested):
> 
> -- >8 --
>   void qemu_ram_move(void *dst, const void *src, size_t len)
>   {
>       if (unlikely(n == 0)) {
>           return;
>       }
>       if (len == 1) {
>           qatomic_set((uint8_t *)dst, qatomic_read((uint8_t *)src));
>           return;
>       } else if (QEMU_PTR_IS_ALIGNED(dst, len) && QEMU_PTR_IS_ALIGNED(src, len)) {
>           switch (len) {
>           case 2:
>               qatomic_set((uint16_t *)dst, qatomic_read((uint16_t *)src));
>               return;
>           case 4:
>               qatomic_set((uint32_t *)dst, qatomic_read((uint32_t *)src));
>             return;
>           case 8:
>               qatomic_set((uint64_t *)dst, qatomic_read((uint64_t *)src));
>               return;
>           default:
>               break;
>         }
>      }
>      /* Overlapping buffers, unaligned or oversized access */
>      memmove(dst, src, len);
>   }
> ---
> 

Thanks for the proposed code, but I'd like to keep what we already had. One thing
I try to avoid from the beginning is the unnecessary nested if statements. Besides,
one irrelevant question is that QEMU_PTR_IS_ALIGNED() looks a bit strange as it
uses % operator, meaning it works even the operand isn't power of 2.

#define QEMU_IS_ALIGNED(n, m) (((n) % (m)) == 0)
#define QEMU_PTR_IS_ALIGNED(p, n) QEMU_IS_ALIGNED((uintptr_t)(p), (n))

QEMU_PTR_IS_ALIGNED(3, 3) => true

Thanks,
Gavin

Re: [PATCH v4 2/3] system/memory: Use qemu_ram_move() for directly accessible regions

[Peter Xu]

On Mon, Jul 27, 2026 at 01:51:45PM +0100, Peter Maydell wrote:
> I think we could usefully expand this comment, because the reasons
> we need it are not immediately obvious. How about:
> 
> ===begin===
> Move @n bytes from @src to @dst; the memory areas may overlap.
> This provides the same semantics as memmove(), plus an additional
> stronger guarantee: if @n is 1, 2 or 4 or 8 bytes, and @src
> and @dst are both naturally aligned for that access size, and
> the memory areas do not overlap, then both the load and the store
> will be done as a single atomic access (with the semantics of
> qatomic_read() and qatomic_set()).
> 
> This is the underlying function that we use to implement accesses
> by a guest vCPU or a device DMA operation to a ram block. The
> atomic guarantee is needed for two major cases:
>  - when the ram block is backed by a PCI BAR passed through
>    from a host device (and so it might be hardware registers
>    that must be accessed exactly once at the right width)
>  - when an emulated device updates a data structure shared in
>    guest memory with guest software (e.g. a network device's
>    set of tx and rx descriptor blocks), if a write to memory
>    is accidentally performed multiple times then it can break
>    the guest code.

Maybe also append it with "the guest code when it busy polls the guest
memory"; I just found that the polling model isn't something obvious too
when reading it first.

> 
> We don't attempt to perform the exact access when it would
> be unaligned, because this can't necessarily be done on
> all host architectures; although this is strictly speaking
> not doing what would happen on real hardware, we don't think
> there are going to be situations where that matters in practice.

Oh yes, mentioning the unaligned part of discussion would also be nice, I
forgot it when replying.

Thanks,

> ===endit===

-- 
Peter Xu

Re: [PATCH v4 2/3] system/memory: Use qemu_ram_move() for directly accessible regions

[Gavin Shan]

On 7/28/26 12:02 AM, Peter Xu wrote:
> On Mon, Jul 27, 2026 at 01:51:45PM +0100, Peter Maydell wrote:
>> I think we could usefully expand this comment, because the reasons
>> we need it are not immediately obvious. How about:
>>
>> ===begin===
>> Move @n bytes from @src to @dst; the memory areas may overlap.
>> This provides the same semantics as memmove(), plus an additional
>> stronger guarantee: if @n is 1, 2 or 4 or 8 bytes, and @src
>> and @dst are both naturally aligned for that access size, and
>> the memory areas do not overlap, then both the load and the store
>> will be done as a single atomic access (with the semantics of
>> qatomic_read() and qatomic_set()).
>>
>> This is the underlying function that we use to implement accesses
>> by a guest vCPU or a device DMA operation to a ram block. The
>> atomic guarantee is needed for two major cases:
>>   - when the ram block is backed by a PCI BAR passed through
>>     from a host device (and so it might be hardware registers
>>     that must be accessed exactly once at the right width)
>>   - when an emulated device updates a data structure shared in
>>     guest memory with guest software (e.g. a network device's
>>     set of tx and rx descriptor blocks), if a write to memory
>>     is accidentally performed multiple times then it can break
>>     the guest code.
> 
> Maybe also append it with "the guest code when it busy polls the guest
> memory"; I just found that the polling model isn't something obvious too
> when reading it first.
> 

The extended context has been included for (v5).

>>
>> We don't attempt to perform the exact access when it would
>> be unaligned, because this can't necessarily be done on
>> all host architectures; although this is strictly speaking
>> not doing what would happen on real hardware, we don't think
>> there are going to be situations where that matters in practice.
> 
> Oh yes, mentioning the unaligned part of discussion would also be nice, I
> forgot it when replying.
> 

+1. The context has been included for (v5) either.

Thanks,
Gavin

Re: [PATCH v4 2/3] system/memory: Use qemu_ram_move() for directly accessible regions

[Gavin Shan]

On 7/27/26 10:51 PM, Peter Maydell wrote:
> On Mon, 27 Jul 2026 at 04:27, Gavin Shan <gshan@redhat.com> wrote:
> 
> Initial note: I think this is basically the right thing; I have
> some suggestions for beefing up the doc comment and some minor
> other things below.
> 

Thanks for your review and comments.

>> All ram device regions were turned to be indirectly accessible by commit
>> 4a2e242bbb ("memory: Don't use memcpy for ram_device regions"). This leads
>> to guest hang on attempt to build 'cuda-samples' as reported by Julia. The
>> guest is started by the following command lines, with GH100 GPU card passed
>> from the host.
>>
>>     host$ lspci | grep GH100
>>     0009:01:00.0 3D controller: NVIDIA Corporation GH100 [GH200 120GB / 480GB] (rev a1)
>>     host$ /home/sandbox/gavin/qemu.main/build/qemu-system-aarch64            \
>>           -machine virt,gic-version=host,ras=on,highmem-mmio-size=4T         \
>>           -accel kvm -cpu host -smp cpus=48 -m size=8G                       \
>>           -drive file=/home/gavin/sandbox/images/disk.qcow2,if=none,id=d0    \
>>           -device virtio-blk-pci,id=vb0,bus=pcie.0,drive=d0,num-queues=4     \
>>           -device vfio-pci-nohotplug,host=0009:01:00.0,bus=pcie.1.0
>>             :
>>     guest$ cd cuda-samples/build
>>     guest$ make -j 20 clean
>>     guest$ make -j 20
>>             :
>>     [ 54%] Linking CUDA executable graphMemoryNodes
>>     [ 54%] Built target graphMemoryNodes
>>     <no more output afterwards, guest becomes frozen here>
>>
>>     guest$ qemu-system-aarch64: virtio: bogus descriptor or out of resources
>>     [  555.814025] virtio_blk virtio0: [vda] new size: 268435456 512-byte logical blocks (137 GB/128 GiB)
>>
>> When the GPU's driver (NVidia open driver) is loaded on guest bootup,
>> the memory blocks residing in the PCI BAR#4 of the GH100 GPU card can
>> be presented to the guest through memory hot-add. The page cache can
>> then be allocated from the hot added memory blocks when cuda-samples
>> is being built. Afterwards, the page cache is sent to QEMU's virtio-blk
>> device as part of the DMA request, the bounce buffer has to be used to
>> accomodate the request as the corresponding memory region (MemoryRegion)
>> is an indirectly accessible ram device region in qemu. However, the max
>> bounce bufer size is only 4096 bytes by default and that is exhausted
>> quickly, leading to a reset on the virtio-blk device and frozen guest
>> eventually.
>>
>>    QEMU
>>    ====
>>    virtio_blk_handle_output
>>      virtio_blk_handle_vq
>>        virtio_blk_get_request
>>          virtqueue_pop
>>            virtqueue_split_pop
>>              virtqueue_map_desc
>>                address_space_map
>>                  memory_access_is_direct         # Return false
>>                    memory_region_supports_direct_access
>>
>>    (qemu) info mtree
>>    memory-region: pci_bridge_pci
>>      0000000000000000-ffffffffffffffff (prio 0, container): pci_bridge_pci
>>        0000042000000000-0000043fffffffff (prio 1, i/o): 0009:01:00.0 base BAR 4
>>          0000042000000000-0000043fffffffff (prio 0, i/o): 0009:01:00.0 BAR 4
>>            0000042000000000-000004379fffffff (prio 0, ramd): 0009:01:00.0 BAR 4 mmaps[0]
>>
>> This adds qemu_ram_move() where the aligned and small-sized accesses are
>> handled by qatomics, and fall back to memmove() otherwise. The memove()
>> for the directly accessible regions is replaced by qemu_ram_move() so that
>> the issue covered by commit 4a2e242bbb (MMIO access instructions were
>> optimized to SSE instructions) is fixed. This makes 'ram_device_mem_ops'
>> redundant, paving the way to revert that commit to make the ram device
>> region directly accessible again in the next patch.
> 
> I think this commit message should also describe the second category
> of bug that we intend it to fix: the one where a device does e.g.
> address_space_stb() to a data structure in guest memory and requires
> it to write exactly that byte exactly once.
> 

Yes, making sense. I've added below context for (v5), which will be
posted shortly.

---

Besides, this also fixes the issue of the unexpected frozen reception on
e1000 NIC in the scenario of DPDK due to the wrong Rx queue full indication
caused by the following memcpy(), which is turned to 3 consective 'strb'
instructions to the same location by glibc-2.24+ for aarch64. With this
applied, the syntax of one-byte store is strictly ensured by a one-byte
qatomic set.
     
   QEMU
   ====
   e1000_receive_iov
     pci_dma_write
       pci_dma_rw
         dma_memory_rw
           dma_memory_rw_relaxed
             address_space_rw
               address_space_write
                 flatview_write
                   flatview_write_continue
                     flatview_write_continue_step
                       memcpy    # 3 consective 'strb' instructions


>> Reported-by: Julia Graham <jugraham@redhat.com>
>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>> Suggested-by: Peter Xu <peterx@redhat.com>
>> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> --- a/include/system/memory.h
>> +++ b/include/system/memory.h
>> @@ -2666,6 +2666,21 @@ void address_space_register_map_client(AddressSpace *as, QEMUBH *bh);
>>   void address_space_unregister_map_client(AddressSpace *as, QEMUBH *bh);
>>
>>   /* Internal functions, part of the implementation of address_space_read.  */
>> +
>> +/**
>> + * qemu_ram_move: move data from or to ramblock
>> + *
>> + * @dst: destination where the data is moved to
>> + * @src: source where the data is moved from
>> + * @n: length of data to be moved
>> + *
>> + * Move @n bytes from @src to @dst with the assumption that @src and @dst
>> + * can overlap. The access is atomic if the source and destination buffer
>> + * aren't overlapped for a well aligned and small-sized access. Otherwise,
>> + * fall back to the standard memmove().
>> + */
> 
> I think we could usefully expand this comment, because the reasons
> we need it are not immediately obvious. How about:
> 
> ===begin===
> Move @n bytes from @src to @dst; the memory areas may overlap.
> This provides the same semantics as memmove(), plus an additional
> stronger guarantee: if @n is 1, 2 or 4 or 8 bytes, and @src
> and @dst are both naturally aligned for that access size, and
> the memory areas do not overlap, then both the load and the store
> will be done as a single atomic access (with the semantics of
> qatomic_read() and qatomic_set()).
> 
> This is the underlying function that we use to implement accesses
> by a guest vCPU or a device DMA operation to a ram block. The
> atomic guarantee is needed for two major cases:
>   - when the ram block is backed by a PCI BAR passed through
>     from a host device (and so it might be hardware registers
>     that must be accessed exactly once at the right width)
>   - when an emulated device updates a data structure shared in
>     guest memory with guest software (e.g. a network device's
>     set of tx and rx descriptor blocks), if a write to memory
>     is accidentally performed multiple times then it can break
>     the guest code.
> 
> We don't attempt to perform the exact access when it would
> be unaligned, because this can't necessarily be done on
> all host architectures; although this is strictly speaking
> not doing what would happen on real hardware, we don't think
> there are going to be situations where that matters in practice.
> ===endit===
> 
> ?
> 

Thanks for the suggested comments to qemu_ram_move(), which looks much
clearer than what we had. I've integrated this for (v5) with minor format
adjustment.

> (if you take my suggestion below, then delete "and the
> memory areas do not overlap", because the only case
> where the accesses are naturally aligned and they
> overlap is the case of src == dst)
> 

I would keep the check (src == dst), see the explanation below.

> 
>> diff --git a/system/physmem.c b/system/physmem.c
>> index 2c42e365cb..18485f7b08 100644
>> --- a/system/physmem.c
>> +++ b/system/physmem.c
>> @@ -3158,6 +3158,41 @@ void memory_region_flush_rom_device(MemoryRegion *mr, hwaddr addr, hwaddr size)
>>       invalidate_and_set_dirty(mr, addr, size);
>>   }
>>
>> +void qemu_ram_move(void *dst, const void *src, size_t n)
>> +{
>> +    uintptr_t test, len;
>> +
>> +    if (src == dst || n == 0) {
> 
> I think we should not bother testing for src == dst. It's
> vanishingly unlikely to actually happen, and if it does
> happen then the code will work fine, and it's probably better
> to actually do the access in that case than to skip it.
> 

We can drop the check of (src == dst), but it will introduce inconsistent
behaviors. For example, qemu_ram_move(0x1, 0x1, 0x2) is finally turned to
memmove(0x1, 0x1,0x2) where no memory movement happens in glibc::memmove(),
but qemu_ram_move(0x0, 0x0, 0x2) is turned to qatomic_set((uint16_t *)dst,
qatomic_read((uint16_t *)src)) where we do have memory movement happening.
So I would like to keep the check of (src == dst) in order for the consistent
behaviors.

>> +        return;
>> +    }
>> +
>> +    test = (uintptr_t)src | (uintptr_t)dst | n;
>> +    len = test & -test;
> 
> What is this doing? I am not a fan of clever bit twiddling
> that isn't commented to explain itself. Readers of the code
> shouldn't have to go off and search for an explanation of what
> is going on.
> 

The following comments will be added for (v5).

     /*
      * Maximal length of aligned access that are determined by @src,
      * @dst and @n
      */

>> +
>> +    /* Overlapping buffers, unaligned or oversized access */
>> +    if (n > 8 || len != n) {
>> +        memmove(dst, src, n);
>> +        return;
>> +    }
>> +
>> +    switch (len) {
>> +    case 1:
>> +        qatomic_set((uint8_t *)dst, qatomic_read((uint8_t *)src));
>> +        break;
>> +    case 2:
>> +        qatomic_set((uint16_t *)dst, qatomic_read((uint16_t *)src));
>> +        break;
>> +    case 4:
>> +        qatomic_set((uint32_t *)dst, qatomic_read((uint32_t *)src));
>> +        break;
>> +    case 8:
>> +        qatomic_set((uint64_t *)dst, qatomic_read((uint64_t *)src));
>> +        break;
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +}

Thanks,
Gavin

[PATCH v4 3/3] system/memory: Make ram device region directly accessible

[Gavin Shan]

This basically reverts 4a2e242bbb30 ("memory: Don't use memcpy for
ram_device regions") to make ram device region directly accessible
again. With this, the bounce buffer is bypassed in address_space_map()
when a ram device region is involved, potentially avoid to overrun
the (small) bounce buffer.

Reported-by: Julia Graham <jugraham@redhat.com>
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Suggested-by: Peter Xu <peterx@redhat.com>
Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 include/system/memory.h | 11 ++---------
 system/memory.c         | 41 +----------------------------------------
 system/trace-events     |  2 --
 3 files changed, 3 insertions(+), 51 deletions(-)

diff --git a/include/system/memory.h b/include/system/memory.h
index 1a28f9b873..8cfba10691 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -2698,15 +2698,8 @@ static inline bool memory_region_supports_direct_access(const MemoryRegion *mr)
     if (memory_region_is_romd(mr)) {
         return true;
     }
-    if (!memory_region_is_ram(mr)) {
-        return false;
-    }
-    /*
-     * RAM DEVICE regions can be accessed directly using memcpy, but it might
-     * be MMIO and access using mempy can be wrong (e.g., using instructions not
-     * intended for MMIO access). So we treat this as IO.
-     */
-    return !memory_region_is_ram_device(mr);
+
+    return memory_region_is_ram(mr);
 }
 
 static inline bool memory_access_is_direct(const MemoryRegion *mr,
diff --git a/system/memory.c b/system/memory.c
index 5fc36708ec..da710bbade 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1364,43 +1364,6 @@ const MemoryRegionOps unassigned_mem_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static uint64_t memory_region_ram_device_read(void *opaque,
-                                              hwaddr addr, unsigned size)
-{
-    MemoryRegion *mr = opaque;
-    uint64_t data = ldn_he_p(mr->ram_block->host + addr, size);
-
-    trace_memory_region_ram_device_read(get_cpu_index(), mr, addr, data, size);
-
-    return data;
-}
-
-static void memory_region_ram_device_write(void *opaque, hwaddr addr,
-                                           uint64_t data, unsigned size)
-{
-    MemoryRegion *mr = opaque;
-
-    trace_memory_region_ram_device_write(get_cpu_index(), mr, addr, data, size);
-
-    stn_he_p(mr->ram_block->host + addr, size, data);
-}
-
-static const MemoryRegionOps ram_device_mem_ops = {
-    .read = memory_region_ram_device_read,
-    .write = memory_region_ram_device_write,
-    .endianness = HOST_BIG_ENDIAN ? DEVICE_BIG_ENDIAN : DEVICE_LITTLE_ENDIAN,
-    .valid = {
-        .min_access_size = 1,
-        .max_access_size = 8,
-        .unaligned = true,
-    },
-    .impl = {
-        .min_access_size = 1,
-        .max_access_size = 8,
-        .unaligned = true,
-    },
-};
-
 bool memory_region_access_valid(MemoryRegion *mr,
                                 hwaddr addr,
                                 unsigned size,
@@ -1692,10 +1655,8 @@ void memory_region_init_ram_device_ptr(MemoryRegion *mr, Object *owner,
                                        const char *name, uint64_t size,
                                        void *ptr)
 {
-    memory_region_init_io(mr, owner, &ram_device_mem_ops, mr, name, size);
-    mr->ram = true;
+    memory_region_init_ram_ptr(mr, owner, name, size, ptr);
     mr->ram_device = true;
-    memory_region_set_ram_ptr(mr, size, ptr);
 }
 
 void memory_region_init_alias(MemoryRegion *mr, Object *owner,
diff --git a/system/trace-events b/system/trace-events
index 51b4a4679a..d483b31419 100644
--- a/system/trace-events
+++ b/system/trace-events
@@ -20,8 +20,6 @@ memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, u
 memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u name '%s'"
 memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u"
-memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
-memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_sync_dirty(const char *mr, const char *listener, int global) "mr '%s' listener '%s' synced (global=%d)"
 flatview_new(void *view, void *root) "%p (root %p)"
 flatview_destroy(void *view, void *root) "%p (root %p)"
-- 
2.55.0

Re: [PATCH v4 3/3] system/memory: Make ram device region directly accessible

[Peter Maydell]

On Mon, 27 Jul 2026 at 04:27, Gavin Shan <gshan@redhat.com> wrote:
>
> This basically reverts 4a2e242bbb30 ("memory: Don't use memcpy for
> ram_device regions") to make ram device region directly accessible
> again. With this, the bounce buffer is bypassed in address_space_map()
> when a ram device region is involved, potentially avoid to overrun
> the (small) bounce buffer.
>
> Reported-by: Julia Graham <jugraham@redhat.com>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Suggested-by: Peter Xu <peterx@redhat.com>
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM