Re: [PATCH v3 1/2] system/memory: Use qemu_ram_{copy, move}() in ram device region accessors

[Gavin Shan <gshan@redhat.com>]

On 7/25/26 2:31 AM, Michael S. Tsirkin wrote:
> On Fri, Jul 24, 2026 at 09:56:48AM -0400, Peter Xu wrote:
>> On Fri, Jul 24, 2026 at 11:42:09AM +1000, Gavin Shan wrote:
>>> On 7/23/26 11:46 PM, Peter Xu wrote:
>>>> On Thu, Jul 23, 2026 at 05:05:08AM -0400, Michael S. Tsirkin wrote:

[...]

>>>> We should have another option that is not arch-dependent but keep the
>>>> unaligned behavior.
>>>>
>>>> For current master, AFAIU we do unaligned access for both ram_device and
>>>> rest. Say, even with ram_device_mem_ops, it has both .unaligned=true for
>>>> both .valid & .impl.  I think it means indeed we have unaligned behavior
>>>> even for ram_device.  It also means what matters in regards to the Realtek
>>>> bug was only about aligned access (with subpage presence).
>>>>
>>>> I think it means we can always keep unaligned to stick with
>>>> memcpy()/memmove(), but only use atomic ops for the aligned cases of
>>>> 1/2/4/8.  With that, I think we can also remove ram_device_mem_ops and fix
>>>> the bounce buffer issue.
>>>>
>>>> I think it means we'll stick with memcpy()/memmove() for all archs for
>>>> unaligned, which is again not safe... but that can be an existing but
>>>> separate problem to solve too.
>>>>
>>>
>>> Lets see if Peter Maydell and Michael are happy with this option. At least,
>>> we will have unified qemu_ram_move() for all architectures with this option.
>>> Note that qemu_ram_copy() won't be needed.
>>>
>>> I'm putting note on what's to be done in (v4) if this option is to be picked
>>> up. Let me know if there are missed points. It's basically combing what's done
>>> by ram_device_mem_ops to upper layer (e.g. in qemu_ram_move()).
>>>
>>>    address_space_write
>>>      address_space_to_flatview
>>>      flatview_write
>>>        flatview_translate
>>>        flatview_write_continue
>>>          flatview_write_continue_step
>>>            memmove                     // (A) to replace it with qemu_ram_move()
>>>
>>> /**
>>>   * qemu_ram_move: move data 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().
>>>   */
>>> static void qemu_ram_move(void *dst, const void *src, size_t n)
>>> {
>>>      uintptr_t test, len;
>>>
>>>      if (src == dst || n == 0) {
>>>          return;
>>>      }
>>>
>>>      /* Overlapped buffers */
> 
> 
> overlapping
> 

Corrected in v4.

https://lore.kernel.org/qemu-arm/20260727032649.145381-1-gshan@redhat.com/

>>>      if (src < (dst + n) && dst < (src + n)) {
>>
>> s/&&/||/?
>>
>> It's a bit weird to request memmove() for overlapped, e.g. I don't know if
>> P2P can overlap too when some fuzzer fills in some MMIO address shifted for
>> src/dst.. but I think I get what you want to simplify and it looks fine.
>>
>> Otherwise it looks good.
> 
> Why do we bother special casing overlapping buffers though?
> I do not get it, looks like rest of logic works exactly
> the same for overlapping and non.
> 

Indeed. The explicit check if src/dst are overlapping has been dropped in v4.

>>>          memmove(dst, src, n);
>>>          return;
>>>      }
>>>
>>>      test = (uintptr_t)src | (uintptr_t)dst | n;
>>>      len = test & -test;
>>>
>>>      /* 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