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

[Peter Xu <peterx@redhat.com>]

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é
> > <philmd@oss.qualcomm.com> wrote:
> > > 
> > > On 28/7/26 11:01, Peter Maydell wrote:
> > > > On Tue, 28 Jul 2026 at 04:01, Gavin Shan <gshan@redhat.com> 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/20260728031731.286666-1-gshan@redhat.com/

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