Re: [PATCH v3 1/2] system/memory: Use qemu_ram_{copy, move}() in ram device region accessors

On 7/20/26 7:12 PM, Philippe Mathieu-Daudé wrote:
> On 17/7/26 15:55, Michael S. Tsirkin wrote:
>> On Fri, Jul 17, 2026 at 02:49:53PM +0100, Peter Maydell wrote:
>>> On Fri, 17 Jul 2026 at 14:24, Michael S. Tsirkin <[email protected]> wrote:
>>>> On Thu, Jul 09, 2026 at 07:52:12PM +1000, Gavin Shan wrote:
>>>>> Could you help to confirm if the conclusion is to have unified qemu_ram_{copy, move}()
>>>>> for all architectures? In qemu_ram_{copy, move}(), the fixed sized accesses like
>>>>> 1/2/4/8 bytes are handled by __builtin_{memcpy, memmove}() and fall back to
>>>>> memcpy/memmove() for other sized accesses. It's basically what was proposed by
>>>>> Michael :-)
>>>>
>>>> No. __builtin_ and  memcpy/memmove are same on modern linuxes.
>>>>
>>>> I'd say as a 1st step, on x86 hosts we can do a fixed size
>>>> memmove for 1/2/4/8, on other hosts
>>>> *aligned* 1/2/4/8 bytes can do
>>>>   __atomic_store ... __ATOMIC_RELAXED
>>>> and the rest can hobble along on memmove
>>>> and hope for the best.
>>>
>>> I don't think we should be doing something that's x86-host specific,
>>> as I've mentioned previously.
>>>
>>> -- PMM
>>
>> Shrug. Of course we already have arch specific code in asm.
>> But we can make the unaligned 1/2/4/8 for x86 host a separate
>> optimization on top. They are unlikely to be common.
> 
> Note we have include/qemu/host-pci-mmio.h with s390x specific
> optimizations in util/s390x_pci_mmio.c ...
> 

Philippe, Thanks for pointing it out that s390x already had private PCI
MMIO accessors.

If Peter is fine with two variants for x86 and non-x86 architectures.
I can post (v4) for further review. That will be something like below
and let me know if there are any other improvements are needed.

----- include/system/memory.h

+/**
+ * qemu_ram_copy: copy data to ramblock
+ *
+ * @dst: destination where the data is copied to
+ * @src: source where the data is copied from
+ * @n: length of data to be copied
+ *
+ * Copy @n bytes from @src to @dst with the assumption that @src and @dst
+ * do not overlap. For x86, the fixed sized accesses like 1/2/4/8 bytes are
+ * handled by __builtin_memcpy() no matter whether @src or @dst is aligned
+ * to the access size, and fall back to memcpy() for other access sizes. For
+ * other architectures, data is copied from @src to @dst in units of 1/2/4/8
+ * bytes, ensuring proper alignment in each unit and order among the units.
+ */
+void qemu_ram_copy(void *dest, const void *src, size_t n);
+
+/**
+ * qemu_ram_move: move data to ramblock
+ *
+ * @dst: destination where the data is moved to
+ * @src: source where the data is moved from
+ * @n: length of data to be moved
+ *
+ * Move @n bytes from @src to @dst with the assumption that @src and @dst
+ * can overlap. For x86, the fixed sized accesses like 1/2/4/8 bytes are
+ * handled by __builtin_memmove() no matter whether @src or @dst is aligned
+ * to the access size, and fall back to memmove() for other access sizes.
+ * For other architectures, data is moved from @src to @dst in units of
+ * 1/2/4/8 bytes, ensuring proper alignment in each unit and order among
+ * the units.
+ */
+void qemu_ram_move(void *dest, const void *src, size_t n);
+

----- system/physmem.c

+static void qemu_ram_unsafe_copy(void *dst, const void *src, size_t n)
+{
+    switch (n) {
+    case 1:
+        __builtin_memcpy(dst, src, 1);
+        break;
+    case 2:
+        __builtin_memcpy(dst, src, 2);
+        break;
+    case 4:
+        __builtin_memcpy(dst, src, 4);
+        break;
+    case 8:
+        __builtin_memcpy(dst, src, 8);
+        break;
+    default:
+        memcpy(dst, src, n);
+    }
+}
+
+static void qemu_ram_unsafe_move(void *dst, const void *src, size_t n)
+{
+    switch (n) {
+    case 1:
+        __builtin_memmove(dst, src, 1);
+        break;
+    case 2:
+        __builtin_memmove(dst, src, 2);
+        break;
+    case 4:
+        __builtin_memmove(dst, src, 4);
+        break;
+    case 8:
+        __builtin_memmove(dst, src, 8);
+        break;
+    default:
+        memmove(dst, src, n);
+    }
+}
+
+static void qemu_ram_safe_copy(void *dst, const void *src,
+                               size_t n, size_t max_step)
+{
+    uintptr_t test, step;
+
+    /* Aligned maximal step */
+    max_step = pow2floor(max_step);
+
+    while (n) {
+        test = (uintptr_t)src | (uintptr_t)dst | n | max_step;
+        step = test & -test;
+
+        switch (step) {
+        case 1:
+            qatomic_set((uint8_t *)dst, qatomic_read((uint8_t *)src));
+            src += 1;
+            dst += 1;
+            n -= 1;
+            break;
+        case 2:
+            qatomic_set((uint16_t *)dst, qatomic_read((uint16_t *)src));
+            src += 2;
+            dst += 2;
+            n -= 2;
+            break;
+        case 4:
+            qatomic_set((uint32_t *)dst, qatomic_read((uint32_t *)src));
+            src += 4;
+            dst += 4;
+            n -= 4;
+            break;
+        case 8:
+        default:
+            qatomic_set((uint64_t *)dst, qatomic_read((uint64_t *)src));
+            src += 8;
+            dst += 8;
+            n -= 8;
+        }
+    }
+}
+
+static void qemu_ram_safe_copy_backwards(void *dst, const void *src,
+                                         size_t n, size_t max_step)
+{
+    uintptr_t test, step;
+
+    /* Aligned maximal step */
+    max_step = pow2floor(max_step);
+
+    /* End of the blocks */
+    src += n;
+    dst += n;
+
+    while (n) {
+        test = (uintptr_t)src | (uintptr_t)dst | n | max_step;
+        step = test & -test;
+
+        switch (step) {
+        case 1:
+            src -= 1;
+            dst -= 1;
+            n -= 1;
+            qatomic_set((uint8_t *)dst, qatomic_read((uint8_t *)src));
+            break;
+        case 2:
+            src -= 2;
+            dst -= 2;
+            n -= 2;
+            qatomic_set((uint16_t *)dst, qatomic_read((uint16_t *)src));
+            break;
+        case 4:
+            src -= 4;
+            dst -= 4;
+            n -= 4;
+            qatomic_set((uint32_t *)dst, qatomic_read((uint32_t *)src));
+            break;
+        case 8:
+        default:
+            src -= 8;
+            dst -= 8;
+            n -= 8;
+            qatomic_set((uint64_t *)dst, qatomic_read((uint64_t *)src));
+        }
+    }
+}
+
+/* x86 should work with __builtin_{memcpy, memmove}() for IO access */
+#if defined(__i386__) || defined(__x86_64__)
+#define HOST_UNALIGNED_VECTOR_MMIO_OK 1
+#else
+#define HOST_UNALIGNED_VECTOR_MMIO_OK 0
+#endif
+
+void qemu_ram_copy(void *dst, const void *src, size_t n)
+{
+    if (src == dst || n == 0) {
+        return;
+    }
+
+    if (HOST_UNALIGNED_VECTOR_MMIO_OK) {
+        qemu_ram_unsafe_copy(dst, src, n);
+    } else {
+        qemu_ram_safe_copy(dst, src, n, 8);
+    }
+}
+void qemu_ram_move(void *dst, const void *src, size_t n)
+{
+    if (src == dst || n == 0) {
+        return;
+    }
+
+    if (HOST_UNALIGNED_VECTOR_MMIO_OK) {
+        qemu_ram_unsafe_move(dst, src, n);
+    } else if (dst < src) {
+        qemu_ram_safe_copy(dst, src, n, src - dst);
+    } else {
+        qemu_ram_safe_copy_backwards(dst, src, n, dst - src);
+    }
+}


Thanks,
Gavin