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 <[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]>
---
+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
