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
