On Tue, Jul 28, 2026 at 08:23:48PM +1000, Gavin Shan wrote: > 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.
I think the bit ops should be more efficient. I plan to queue v5 with a fixup, v5 here: https://lore.kernel.org/qemu-devel/[email protected]/ Fixup: diff --git a/system/physmem.c b/system/physmem.c index fbe7df2391..2f37cbeb07 100644 --- a/system/physmem.c +++ b/system/physmem.c @@ -3162,13 +3162,18 @@ void qemu_ram_move(void *dst, const void *src, size_t n) { uintptr_t test, len; - if (src == dst || n == 0) { + if (n == 0) { return; } /* - * Maximal length of aligned access that are determined by @src, - * @dst and @n + * Calculate "the lowest set bit" over @src, @dst and @n, result put + * into @len (which guarantees a power-of-two). With that and the + * later check (len!=n), it makes sure that we will only do the atomic + * ops when: + * + * (1) @n is a power-of-two + * (2) @src and @dst addresses are both aligned to @n */ test = (uintptr_t)src | (uintptr_t)dst | n; len = test & -test; Does it look ok to all of you? Thanks, -- Peter Xu
