On Fri, Jul 24, 2026 at 09:56:48AM -0400, Peter Xu wrote:
> 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 */
overlapping
> > 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.
Why do we bother special casing overlapping buffers though?
I do not get it, looks like rest of logic works exactly
the same for overlapping and non.
> > 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
