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

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.

-- PMM