On Mon, 27 Jul 2026 at 04:27, Gavin Shan <[email protected]> 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 <[email protected]>
> Suggested-by: Michael S. Tsirkin <[email protected]>
> Suggested-by: Peter Xu <[email protected]>
> Suggested-by: Richard Henderson <[email protected]>
> Suggested-by: Peter Maydell <[email protected]>
> Signed-off-by: Gavin Shan <[email protected]>
> --- 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
