On 7/24/26 11:56 PM, 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 */
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.
The condition is correct to check if src/dst are overlapped. As Michael
pointed out the overlapping case can be well handled by "if (n > 8 || len !=
n)",
so this check has been dropped in v4.
https://lore.kernel.org/qemu-arm/[email protected]/
I don't think src/dst can be overlapped since p2p happens on two separate
devices. My idea to ignore the overlapping case is that would happen very
rarely, maybe only valid in the fuzzer case.
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.
Ack, address_space_write_rom() is dropped in v4.
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.
Yes, qemu_ram_move() is also used for __builtin_constant_p() case in v4.
Thanks,
Gavin
