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

On 7/28/26 7:35 PM, Peter Maydell wrote:
> On Tue, 28 Jul 2026 at 10:22, Philippe Mathieu-Daudé
> <[email protected]> wrote:
>>
>> On 28/7/26 11:01, Peter Maydell wrote:
>>> On Tue, 28 Jul 2026 at 04:01, Gavin Shan <[email protected]> wrote:
>>>>
>>>> On 7/27/26 10:51 PM, Peter Maydell wrote:
> 
> 
>>>>>> +    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.
>>>>>
>>>>
>>>> The following comments will be added for (v5).
>>>>
>>>>        /*
>>>>         * Maximal length of aligned access that are determined by @src,
>>>>         * @dst and @n
>>>>         */
>>>
>>> What I mean is more that it ought to say why "x & -x"
>>> achieves that goal.
>>
>> Hmm isn't "x & -x" restricted to power of 2? Here we have no garanty
>> @test is, because we have no garanty @n is. Ah, this is catched by
>> the 'len != n' check and returns after calling memmove(). Indeed not
>> very clear notation. Again:
>>
>>     ...
>>
>>     /* Overlapping buffers, unaligned or oversized access */
>>     if (!QEMU_IS_ALIGNED((uintptr_t)src | (uintptr_t)dst, n)) {
>>         memmove(dst, src, n);
>>         return;
>>     }
>>
>>     switch (n) {
>>     ...
> 
> I'm not inherently against the bit twiddling (QEMU_IS_ALIGNED
> on a variable length will do a division, maybe we care?);
> I just want that if we do use a bit-twiddling trick that we
> explain why it does the thing we want it to do.
> 

Ok, PeterM and Philippe, Could you please help to check if below code looks
good to you? Thanks a lot :-)

void qemu_ram_move(void *dst, const void *src, size_t n)
{
     if (n == 0) {
         return;
     }

     /* Overlapping areas, unaligned or oversized access */
     if (!is_power_of_2(n) || n > 8 ||
         !QEMU_IS_ALIGNED((uintptr_t)src | (uintptr_t)dst, n)) {
         memmove(dst, src, n);
         return;
     }

      switch (n) {
      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();
      }
}

QEMU_IS_ALIGNED() doesn't require the arguments are power-of-two values.

#define QEMU_IS_ALIGNED(n, m) (((n) % (m)) == 0)   /* QEMU_IS_ALIGNED(3, 3) => true */

I also need to drop "and the memory areas do not overlap" in (v6) from the
comments for include/system/memory.h::qemu_ram_move() as the check 'src == dst'
check has been dropped. Sorry that I sent (v5) too quick because our downstream
need a stabilized version to integrate.

Thanks,
Gavin