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) {
...
