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
