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
