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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jun 16, 2026 at 07:31:04AM +1000, Gavin Shan wrote:
> On 6/16/26 5:42 AM, Michael S. Tsirkin wrote:
> > On Tue, Jun 16, 2026 at 05:24:15AM +1000, Gavin Shan wrote:
> > > Hi Peter and Michael,
> > > 
> > > On 6/16/26 1:12 AM, Peter Maydell wrote:
> > > > On Mon, 15 Jun 2026 at 15:56, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > 
> > > > > On Mon, Jun 15, 2026 at 11:57:09AM +0100, Peter Maydell wrote:
> > > > > > On Mon, 15 Jun 2026 at 11:03, Gavin Shan <gshan@redhat.com> wrote:
> > > > > > > 
> > > > > > > All ram device regions were turned to be indirectly accessible by commit
> > > > > > > 4a2e242bbb ("memory: Don't use memcpy for ram_device regions"). This leads
> > > > > > > to guest hang on attempt to build 'cuda-samples' as reported by Julia. The
> > > > > > > guest is started by the following command lines, with GH100 GPU card passed
> > > > > > > from the host.
> > > > > > 
> > > > > > > diff --git a/include/system/memory.h b/include/system/memory.h
> > > > > > > index 1417132f6d..5878727d09 100644
> > > > > > > --- a/include/system/memory.h
> > > > > > > +++ b/include/system/memory.h
> > > > > > > @@ -2897,6 +2897,8 @@ void address_space_register_map_client(AddressSpace *as, QEMUBH *bh);
> > > > > > >    void address_space_unregister_map_client(AddressSpace *as, QEMUBH *bh);
> > > > > > > 
> > > > > > >    /* Internal functions, part of the implementation of address_space_read.  */
> > > > > > > +void qemu_ram_copy(void *dest, const void *src, size_t n);
> > > > > > > +void qemu_ram_move(void *dest, const void *src, size_t n);
> > > > > > 
> > > > > > New function prototypes in include headers need documentation comments.
> > > > > > 
> > > > > > In particular for these, it's really important that we clearly say
> > > > > > what semantics we are attempting to provide with them, so that
> > > > > > (a) when we're reviewing or later updating the implementation we
> > > > > > know what we are trying to provide
> > > > > > (b) when we're looking at the callsites we know what the function
> > > > > > is guaranteeing to us and what it is not, and thus whether it's
> > > > > > OK to use it or we need something els.
> > > > > > 
> > > > > > > +#if defined(__i386__) || defined(__x86_64__)
> > > > > > > +#define HOST_UNALIGNED_MMIO_OK 1
> > > > > > > +#else
> > > > > > > +#define HOST_UNALIGNED_MMIO_OK 0
> > > > > > > +#endif
> > > > > > 
> > > > > > We need to do something better than this. We can't
> > > > > > just say "oh, we trust that on x86 this works": it is
> > > > > > neither actually true that the compiler guarantees it
> > > > > 
> > > > > sorry guarantee what?
> > > > 
> > > > Well, that's part of my point about "we need a doc comment":
> > > > what exactly are we trying to guarantee ? But whatever it is,
> > > > "hardcoded ifdef that privileges x86" is definitely the wrong
> > > > answer.
> > > > 
> > > 
> > > Could you please check the following comments (documentation context) for the
> > > added functions look good to you? Please let me know if there are anything
> > > can be improved.
> > > 
> > > +
> > > +/**
> > > + * qemu_ram_copy: copy data to a RAMBlock
> > > + *
> > > + * @dest: destination where the data is copied to. It's the pointer returned
> > > + *        by ramblock_ptr() and its variants.
> > > + * @src: source where the data is copied from. It can be a regular memory block
> > > + *       or a pointer returned by ramblock_ptr() and its variants.
> > > + * @n: length of data to be copied
> > > + *
> > > + * The destination is always a pointer to data area of a RAMBlock which can
> > > + * be for a regular memory block or a MMIO region. The source can be a pointer
> > > + * to a regular memory block or a MMIO region. Atomicity isn't guranteed by
> > > + * memcpy() to copy data between those MMIO regions as we do in this function.
> > > + * Besides, unaligned accesses are well handled on all architectures except
> > > + * i386 and x86_64 where the unaligned accesses are supported by the CPUs.
> > > + *
> > > + * The ensured atomicity and alignment consideration, which aren't needed
> > > + * by data copy between two regular memory blocks. So performance penalty
> > > + * is introduced by this function in such circumstance.
> > > + */
> > > +void qemu_ram_copy(void *dest, const void *src, size_t n);
> > > +
> > > +/**
> > > + * qemu_ram_move: move data to a RAMBlock
> > > + *
> > > + * @dest: destination where the data is moved to. It's the pointer returned
> > > + *        by ramblock_ptr() and its variants.
> > > + * @src: source where the data is moved from. It can be a regular memory block
> > > + *       or a pointer returned by ramblock_ptr() and its variants.
> > > + * @n: length of data to be moved
> > > + *
> > > + * The destination is always a pointer to data area of a RAMBlock which can
> > > + * be for a regular memory block or a MMIO region. The source can be a pointer
> > > + * to a regular memory block or a MMIO region. Atomicity isn't guranteed by
> > > + * memmove() to move data from or to those MMIO regions as we do in this
> > > + * function. Besides, unaligned accesses are well handled on all architectures
> > > + * except i386 and x86_64 where the unaligned accesses are supported by the
> > > + * CPUs.
> > > + *
> > > + * The ensured atomicity and alignment consideration, which aren't needed
> > > + * by data movement between two regular memory blocks. So performance penalty
> > > + * is introduced by this function in such circumstance.
> > > + */
> > > +void qemu_ram_move(void *dest, const void *src, size_t n);
> > > +
> > 
> > 
> > I apologize, I don't understand what these comments are saying, at all.
> > 
> 
> Sorry to hear that, Michael. There are two things done in the newly added
> function qemu_ram_{copy, move}(), comparing to the original memcpy() and
> memmove():
> 
> - Data copy or movement on MMIO region(s) using __bultin_{memcopy, memmove}()
>   on x86, or qatomic_set() on other architecutures, to avoid the unexpected
>   optimization done by the compiler on memcpy/memmove. Unsafe instructions
>   can be produced by the compiler's optimization. This fixes the issue resolved
>   by commit 4a2e242bbb ("memory: Don't use memcpy for ram_device regions")
> 
> - The unaligned access is handled by forcing the access size aligned to (src | dest | n)
>   on non-x86 architectures, comparing to the original memcpy() and memmove().

Well first of all let's start with listing what the problems are.
Below is my understanding of the issues:

1. On x86, memcpy is different from __builtin_memcpy if
one uses old 1.0 force-headers from 2019. Likely no longer relevant.


2. variable length memcpy can translate 2,4,8 byte guest access
into multiple byte accesses. doing this for mmio is
guaranteed to break devices.


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.

4. also on x86, vector instructions for large (>16 byte) writes
into pgprot_noncached memory are safe and faster than multiple 8 byte
ones. 

5. also on x86 it so happens that if you write a fixed-size memcpy this
gets optimized to a single store/load and it works for aligned and
unaligned addresses on that architecture. How to ensure this keeps being
correct is left as an excerise for the reader. But qemu already relies
on this and did for years. 

6. on non-x86 both unaligned accesses and vector instructions
for accessing  UC memory are illegal.

7. standard vfio gives KVM VM_ALLOW_ANY_UNCACHED, so even on non x86
guest can
map the memory as as pgprot_noncached/ioremap or pgprot_writecombine/ioremap_uc.
If it does the second then it can use unaligned or vector for access.
This is why normal passthrough tends to work - it never traps to qemu at
all.

But for qemu, vfio uses  pgprot_noncached unconditionally so qemu
can't use unaligned or vector instructions on non-x86.


8. But for nvgrace RAM, vfio has a driver that uses pgprot_writecombine/ioremap_uc.
so qemu could safely use unaligned/vector instructioons even on non-x86.

9. Except sadly, vfio currently does not tell qemu how it maps
the memory, so qemu can not know what is safe on non-x86.





Now, what is to be done?


A. on x86, we must avoid converting 2,4,8 byte accesses into byte accesses.
At least for aligned, perferably for unaligned accesses too.
Fixed width memcpy seems to work for this. Whether we should bother with
__builtin to work around broken old fortify headers, I donnu.
I do not have any answer how to check that compiler does this correctly.
If anyone is motivated enough, adding a GCC builtin could be possible.
Given qemu did this for years, I think we can leave solving this for
another day.

B. Also on x86, I do not see why we should not use memcpy for large
accesses if we can. Better perf.

C. on non-x86, we currently must not memcpy since we do not know
if it is pgprot_noncached. yes, performance will be
bad for DMA into device RAM.




D.  It goes without saying that casting an unaligned address to unint32_t
(be it for qatomic_set or whatever) is undefined behaviour in C
and so a bad idea on any architecture.


E. also for non-x86, we really should teach vfio to tell qemu whether
it maps device pgprot_noncached or pgprot_writecombine.
we will then be able to use memcpy for >8 accesses.



Anyone, correct me if I'm wrong? Maybe I should start a new thread with
this summary?



> 
> Please let me know if we're on same page. If we do, I can revise the comments
> accordingly.
> 
> > 
> > 
> > > > -- PMM
> > > > 
> 
> Thanks,
> Gavin