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

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"?

> 
>> +
>> +    /* 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);
  }
---