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

On Fri, Jul 24, 2026 at 11:42:09AM +1000, Gavin Shan wrote:
> On 7/23/26 11:46 PM, Peter Xu wrote:
> > On Thu, Jul 23, 2026 at 05:05:08AM -0400, Michael S. Tsirkin wrote:
> > > On Thu, Jul 23, 2026 at 10:52:11AM +0200, Philippe Mathieu-Daudé wrote:
> > > > On 23/7/26 08:04, Michael S. Tsirkin wrote:
> > > > > On Wed, Jul 22, 2026 at 12:41:34PM -0400, Peter Xu wrote:
> > > > > > On Wed, Jul 22, 2026 at 01:58:18AM -0400, Michael S. Tsirkin wrote:
> > > > > > > On Wed, Jul 22, 2026 at 10:53:27AM +1000, Gavin Shan wrote:
> > > > > > > > On 7/22/26 2:27 AM, Peter Xu wrote:
> > > > > > > > > On Tue, Jul 21, 2026 at 03:37:53PM +1000, Gavin Shan wrote:
> > > > > > > > > > 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.
> > > > > > > > > 
> > > > > > > > > I have a generic question on the "unaligned access for x86": I think the
> > > > > > > > > question is about the one Michael raised here on unaligned access may break
> > > > > > > > > x86 here:
> > > > > > > > > 
> > > > > > > > >      https://lore.kernel.org/qemu-devel/[email protected]/
> > > > > > > > > 
> > > > > > > > >      3. (theoretical concern) also on x86, unaligned accesses are
> > > > > > > > >      possible on guest and host, so converting an unaligned access to a
> > > > > > > > >      series of aligned ones can in theory break devices.
> > > > > > > > > 
> > > > > > > > > Is that a real problem we need to consider, or can we start with unified
> > > > > > > > > approach and leave it for later?
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > I'm leaving this question to Michael.
> > > > > > > 
> > > > > > > Knowing what I know about hardware designers, it's something someone
> > > > > > > somewhere does)
> > > > > > > It can be made a separate patch, just to show - it should be all of
> > > > > > > ~10LOC.
> > > > > > 
> > > > > > It's only about removal of anything that might be controversial for now,
> > > > > > thanks.  I also wonder if anything would break, then it's more solid proof
> > > > > > that per-arch change is required.
> > > > > 
> > > > > Repeating:
> > > > > 	I think there is exactly 1 kinda reasonable case. A 2 byte read/write at
> > > > > 	offset 0x1 within a dword. This maps nicely to even classical PCI byte
> > > > > 	enable mechanism and so yes it works if your CPU can initiate these
> > > > > 	things, and it's atomic.
> > > > 
> > > > Isn't this out of the CPU arch, dealt with at the bus level?
> > > > 
> > > > It looks we try to be clever with modern PCI code by optimizing this
> > > > access -- not saying we can change that, I know it is too late after
> > > > 20+ years -- relying on hw behavior that was done that way to support
> > > > legacy hw, in particular broken I/O accesses.
> > > 
> > > Not sure what the question is.  We were dicussing how to emulate unaligned
> > > accesses from x86 guests if they happen.
> > > On an x86 host we can do that easily, and it's just a couple of LOC.
> > > Though Peter Maydell dislikes host arch specific code. But I hope
> > > if it's a separate patch on top and it is visible how small it is,
> > > he will reconsider)
> > 
> > Yes, if we still want x86 specific change, it would be better to be put
> > separately.
> > 
> > Said so, I don't think the e1000e LEDCTL test illustrated what might
> > break..  Isn't that only an exmaple showing unaligned access is
> > "supported", however nothing breaks even if we use 1B*2?
> > 
> > My question was more about a real breakage, hence whenever it happened
> > "it's more solid proof that per-arch change is required".
> > 
> > > On other hosts we can't emulate them 100%, we either need to split
> > > to byte accesses or over-access and mask. Byte accesses feel safer.
> > > What qemu currently does with memmove is clearly not safe in the
> > > general case.
> > 
> > We should have another option that is not arch-dependent but keep the
> > unaligned behavior.
> > 
> > For current master, AFAIU we do unaligned access for both ram_device and
> > rest. Say, even with ram_device_mem_ops, it has both .unaligned=true for
> > both .valid & .impl.  I think it means indeed we have unaligned behavior
> > even for ram_device.  It also means what matters in regards to the Realtek
> > bug was only about aligned access (with subpage presence).
> > 
> > I think it means we can always keep unaligned to stick with
> > memcpy()/memmove(), but only use atomic ops for the aligned cases of
> > 1/2/4/8.  With that, I think we can also remove ram_device_mem_ops and fix
> > the bounce buffer issue.
> > 
> > I think it means we'll stick with memcpy()/memmove() for all archs for
> > unaligned, which is again not safe... but that can be an existing but
> > separate problem to solve too.
> > 
> 
> Lets see if Peter Maydell and Michael are happy with this option. At least,
> we will have unified qemu_ram_move() for all architectures with this option.
> Note that qemu_ram_copy() won't be needed.
> 
> I'm putting note on what's to be done in (v4) if this option is to be picked
> up. Let me know if there are missed points. It's basically combing what's done
> by ram_device_mem_ops to upper layer (e.g. in qemu_ram_move()).
> 
>   address_space_write
>     address_space_to_flatview
>     flatview_write
>       flatview_translate
>       flatview_write_continue
>         flatview_write_continue_step
>           memmove                     // (A) to replace it with qemu_ram_move()
> 
> /**
>  * 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. The access is atomic if the source and destination buffer
>  * aren't overlapped for a well aligned and small-sized access. Otherwise,
>  * fall back to the standard memmove().
>  */
> static void qemu_ram_move(void *dst, const void *src, size_t n)
> {
>     uintptr_t test, len;
> 
>     if (src == dst || n == 0) {
>         return;
>     }
> 
>     /* Overlapped buffers */
>     if (src < (dst + n) && dst < (src + n)) {

s/&&/||/?

It's a bit weird to request memmove() for overlapped, e.g. I don't know if
P2P can overlap too when some fuzzer fills in some MMIO address shifted for
src/dst.. but I think I get what you want to simplify and it looks fine.

Otherwise it looks good.

>         memmove(dst, src, n);
>         return;
>     }
> 
>     test = (uintptr_t)src | (uintptr_t)dst | n;
>     len = test & -test;
> 
>     /* Unaligned or oversized access */
>     if (n > 8 || len != n) {
>         memmove(dst, src, n);
>         return;
>     }
> 
>     switch (len) {
>     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();
>     }
> }

[...]

> Yes, I think one preparatory patch can added in (v4) to replace memcpy() with
> memmove() in the following paths, extending commit 4a73aee8814 ("softmmu: Use
> memmove in flatview_write_continue"). With this replacement, qemu_ram_copy()
> won't be needed in (v4).
> 
>     hw/remote/vfio-user-obj.c::vfu_object_mr_rw
>     include/system/memory.h::address_space_read
>     system/physmem.c::flatview_read_continue_step
>     system/physmem.c::address_space_write_rom

Now after a second look, I think it's safe to drop
address_space_write_rom() in the change list because it always directly
manipulates the real RAM (that plays the ROM role).  I recall it was used
to be used in debugging path, but at least now when I look at master branch
it's not.  So we can drop.

I see you already ruled out address_space_read(), I'm not sure if it's
about having that __builtin_constant_p() early check: I think it's still
better to switch that to the new API too, then we get rid of implicit
assumptions of memcpy() over builtin constants.

Thanks,

-- 
Peter Xu