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

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:
> 
>>>> +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.
>>>
>>
>> We can drop the check of (src == dst), but it will introduce inconsistent
>> behaviors. For example, qemu_ram_move(0x1, 0x1, 0x2) is finally turned to
>> memmove(0x1, 0x1,0x2) where no memory movement happens in glibc::memmove(),
>> but qemu_ram_move(0x0, 0x0, 0x2) is turned to qatomic_set((uint16_t *)dst,
>> qatomic_read((uint16_t *)src)) where we do have memory movement happening.
>> So I would like to keep the check of (src == dst) in order for the consistent
>> behaviors.
> 
> Well, it depends on which kind of consistency you want.
> If we do the src == dst check, then we have the inconsistency
> that a small aligned access always happens exactly once,
> *unless* it happens that src == dst, in which case it doesn't
> happen even though the caller asked for it.
> 
> You can see this in the documentation comment I suggested above:
> if you don't check 'src == dst' then you can document it as
> "memmove, plus for a small access where src and dst are naturally
> aligned we guarantee it to happen exactly once atomically".
> If you do check src == dst then it gets more complicated,
> because you have to say "memmove, plus for a small access
> where src and dest are naturally aligned *and* src != dst then ...".
> 
> I think that because it's the small sized accesses where we
> know that it might be necessary to really actually do the
> access, that we should do that also for src == dst.
> 
>>>> +    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) {
   ...