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

On Fri, Jul 17, 2026 at 09:24:03AM -0400, Michael S. Tsirkin wrote:
> On Thu, Jul 09, 2026 at 07:52:12PM +1000, Gavin Shan wrote:
> > Hi Michael and Peter,
> > 
> > On 6/26/26 10:07 AM, Gavin Shan wrote:
> > > On 6/26/26 4:40 AM, Peter Maydell wrote:
> > > > On Thu, 25 Jun 2026 at 17:47, Michael S. Tsirkin <[email protected]> wrote:
> > > > > 
> > > > > On Thu, Jun 25, 2026 at 04:23:47PM +0100, Peter Maydell wrote:
> > > > > > On Thu, 25 Jun 2026 at 15:52, Michael S. Tsirkin <[email protected]> wrote:
> > > > > > > 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.
> > > > > > > 
> > > > > > > I tried reading LEDCTL on e1000e:
> > > > > > > 
> > > > > > >         byte  @ 0xe00: 0x64
> > > > > > >         byte  @ 0xe01: 0x2a
> > > > > > >         byte  @ 0xe02: 0x00
> > > > > > >         byte  @ 0xe03: 0x00
> > > > > > >         word  @ 0xe00: 0x2a64
> > > > > > >         word  @ 0xe01: 0x002a
> > > > > > > 
> > > > > > > Works fine.
> > > > > > 
> > > > > > The e1000e datasheet actually documents what it does in this
> > > > > > case (slightly surprising, since hardware engineers love to
> > > > > > leave this kind of corner case undocumented):
> > > > > > 
> > > > > > # For registers that should be accessed as 32-bit double words,
> > > > > > # partial writes (less than a 32-bit double word) does not take
> > > > > > # effect (such as, the write is ignored).
> > > > > > # Partial reads
> > > > > > # return all 32 bits of data regardless of the byte enables.
> > > > > > #
> > > > > > # Note: Partial reads to clear-by-read registers (such as, ICR)
> > > > > > # can have unexpected results since all 32 bits are actually read
> > > > > > # regardless of the byte enables. Partial reads should not be done.
> > > > > > 
> > > > > > So for this specific device that access is out-of-spec.
> > > > > 
> > > > > You mean that access to clear by read should not be done, right?
> > > > 
> > > > The datasheet is ambiguous about whether "Partial reads should
> > > > not be done" is meant to apply generally or only to clear-by-read
> > > > registers.
> > > > 
> > > 
> > > I think the document intends to say: the partial reads shouldn't be issued to
> > > the clear-by-read registers. Those partial reads on no-read-side-affect registers
> > > would be fine.
> > > 
> > > > > > I guess what I'm wondering is: can we just have code
> > > > > > that does an aligned exact-width access in the 1/2/4/8
> > > > > > byte aligned case, and the host's best approximation to
> > > > > > an unaligned exact-width access for the 2/4/8 byte
> > > > > > unaligned case?
> > > > > 
> > > > > That's my idea, too.
> > > > > 
> > > 
> > > I assume this the conclusion of our discussions? If so, we just need to have
> > > unified function where __builtin_{memcpy, memmove}() are used for 1/2/4/8 bytes
> > > access no matter if the address is aligned, and fall back to {memcpy, memmove}()
> > > for other cases?
> > > 
> > 
> > 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.
> 
> And memcpy is likely not the right thing, ever.

Or maybe better hobble along on a loop over __atomic_store.
Safer.

And as a 2nd step, extend vfio to tell us how is the
device mapped, and then we can maybe fallback to a loop
when we know it is not safe to access with memcpy.


> 
> > Something like below.
> > 
> > -----> 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. The fixed sized accesses like 1/2/4/8 bytes are handled
> > + * by __builtin_memcpy() to avoid the optimizations applied to memcpy(),
> > + * which can be unsafe to DMA/VCPU IO. Fall back to memcpy() for other
> > + * sized accesses and the safety is ensured by users.
> > + */
> > +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. The fixed sized accesses like 1/2/4/8 bytes are handled
> > + * by __builtin_memmove() to avoid the optimizations applied to memmove(),
> > + * which can be unsafe to DMA/VCPU IO. Fall back to memmove() for other
> > + * sized accesses and the safety is ensured by users.
> > + */
> > +void qemu_ram_move(void *dest, const void *src, size_t n);
> > +
> > 
> > -----> system/physmem.c
> > 
> > +void qemu_ram_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);
> > +    }
> > +}
> > +
> > +void qemu_ram_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);
> > +    }
> > +}
> > +
> > 
> > 
> > > > > > (so on sparc you get multiple smaller
> > > > > > accesses, and on most archs including x86 and arm you
> > > > > > get an unaligned load).
> > > > > 
> > > > > I thought unaligned load from uncacheable on arm is
> > > > > also a fault?
> > > > 
> > > > For Arm the distinction is not cacheable/uncacheable
> > > > but Normal vs Device. (Device is essentially for things
> > > > which are not RAM; Normal is for RAM and RAM-like things,
> > > > and includes all of Normal Non-cacheable, Normal WT-Cacheable
> > > > and Normal WB-Cacheable.) Things mapped as Normal memory
> > > > don't generate unaligned faults (unless the guest turned them
> > > > on deliberately). For Device memory, it is IMPLEMENTATION
> > > > DEFINED whether you get an alignment fault or not if you
> > > > map something as Device that could have handled unaligned
> > > > accesses if you had mapped it as Normal.
> > > > 
> > > > > > That would mean the guest could
> > > > > > potentially provoke a fault on the load/store on an
> > > > > > access to a passthrough device, but if you give the
> > > > > > guest passthrough access it can very likely provoke
> > > > > > a fault anyway, depending on exactly what the device is.
> > > > > > 
> > > > > > I think the most likely reason for an unaligned access
> > > > > > in this codepath is "it's actually RAM, either really
> > > > > > host RAM or else something memory-like in a BAR", and
> > > > > > either way if the guest does a 4-byte unaligned access
> > > > > > then doing a 4-byte unaligned access seems better than
> > > > > > second-guessing it, even on non-x86.
> > > > 
> > > > > Right though remember: whether it's RAM doesn't matter. What matters is
> > > > > how we map it.  qemu might fault because it maps NC but guest maps
> > > > > cacheable and it's ok.
> > > > 
> > > > If QEMU and the guest disagree about the memory attributes
> > > > on Arm then we have already lost, because the architecture
> > > > says that memory attribute mismatches result in a variety of
> > > > undesirable effects including things like loss of cache coherency
> > > > (i.e. read and writes via QEMU's NC mapping disagree with ones
> > > > via the guest's cacheable mapping because the latter are hitting
> > > > in the cache and the former are bypassing it).
> > > > 
> > > > > But, all this in theory. At a high level, I personally think going with
> > > > > what you propose as a 1st approximation is entirely reasonable, except
> > > > > for one thing: we really should not crash qemu, since access can be from
> > > > > guest userspace.
> > > > 
> > > > You can't prevent faults entirely, though -- if the device
> > > > being mapped has e.g. behaviour that says "unaligned accesses
> > > > will fault" and then the x86 guest does an unaligned access,
> > > > then the device will trigger a fault, and the fault is what
> > > > you want because it's what the guest would see on real h/w.
> > > > Unfortunately we don't have a convenient way to feed the
> > > > fault back to the guest. At some level if you pass through
> > > > host hardware you're relying on the guest to not do totally
> > > > stupid things.
> > > > 
> > > > thanks
> > > > -- PMM
> > > > 
> > 
> > Thanks,
> > Gavin