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

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