* [PATCH v2] virtio: Add aligned ld/st accessors for vring
@ 2026-08-21 10:08 BillXiang
2026-08-21 10:25 ` Peter Maydell
0 siblings, 1 reply; 9+ messages in thread
From: BillXiang @ 2026-08-21 10:08 UTC (permalink / raw)
To: mst
Cc: pbonzini, peterx, philmd, qemu-devel, qemu-riscv,
richard.henderson, xiangwencheng
The generic ld/st*_p() pointer helpers lower to __builtin_memcpy,
which on RISC-V will be expanded to multiple byte-access instructions
rather than a single aligned access by the compiler because it
cannot prove alignment at the call site.
Each cached 16-bit access of a vring field therefore performs several
distinct byte ld/st, which is a memory-tearing hazard for fields that
the guest may access concurrently — most notably avail->idx, where we
find the guest can write a new value between the individual byte loads
and produce a torn read that never existed in memory, as seen in logs
like:
"Guest moved used index from 49417 to 49919"
Here, 49919 (binary 1100 0010-1111 1111) is incorrectly assembled from
the lower byte of 49663 (1100 0001-1111 1111) and the upper byte of
49664 (1100 0010-0000 0000).
Add a parallel set of _aligned cached accessors so the fast (RAM) path
emits a single aligned load instruction, eliminating the tearing window.
Callers MUST ensure @addr is naturally aligned to the access size before
invoking the _aligned helpers; the virtio vring layout guarantees this
for avail->idx and other naturally-aligned fields.
This patch fixes the memory-tearing hazard while also improves performance.
Signed-off-by: BillXiang <xiangwencheng@lanxincomputing.com>
---
hw/virtio/virtio.c | 8 +-
include/qemu/bswap.h | 20 +++++
include/system/memory_cached.h | 16 ++++
.../system/memory_ldst_cached_aligned.h.inc | 77 +++++++++++++++++++
4 files changed, 117 insertions(+), 4 deletions(-)
create mode 100644 include/system/memory_ldst_cached_aligned.h.inc
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index daa5607..f796bdd 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -223,9 +223,9 @@ static inline uint16_t virtio_lduw_phys_cached(VirtIODevice *vdev,
hwaddr pa)
{
if (virtio_vdev_is_big_endian(vdev)) {
- return lduw_be_phys_cached(cache, pa);
+ return lduw_be_phys_cached_aligned(cache, pa);
}
- return lduw_le_phys_cached(cache, pa);
+ return lduw_le_phys_cached_aligned(cache, pa);
}
static inline void virtio_stw_phys_cached(VirtIODevice *vdev,
@@ -233,9 +233,9 @@ static inline void virtio_stw_phys_cached(VirtIODevice *vdev,
hwaddr pa, uint16_t value)
{
if (virtio_vdev_is_big_endian(vdev)) {
- stw_be_phys_cached(cache, pa, value);
+ stw_be_phys_cached_aligned(cache, pa, value);
} else {
- stw_le_phys_cached(cache, pa, value);
+ stw_le_phys_cached_aligned(cache, pa, value);
}
}
diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 387d65c..be9913c 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -301,6 +301,11 @@ static inline int lduw_le_p(const void *ptr)
return (uint16_t)le_bswap(lduw_he_p(ptr), 16);
}
+static inline int lduw_le_p_aligned(const void *ptr)
+{
+ return le16_to_cpu(*(uint16_t *)ptr);
+}
+
static inline int ldsw_le_p(const void *ptr)
{
return (int16_t)le_bswap(lduw_he_p(ptr), 16);
@@ -321,6 +326,11 @@ static inline void stw_le_p(void *ptr, uint16_t v)
stw_he_p(ptr, le_bswap(v, 16));
}
+static inline void stw_le_p_aligned(void *ptr, uint16_t v)
+{
+ *(uint16_t *)ptr = cpu_to_le16(v);
+}
+
static inline void st24_le_p(void *ptr, uint32_t v)
{
st24_he_p(ptr, le_bswap24(v));
@@ -341,6 +351,11 @@ static inline int lduw_be_p(const void *ptr)
return (uint16_t)be_bswap(lduw_he_p(ptr), 16);
}
+static inline int lduw_be_p_aligned(const void *ptr)
+{
+ return be16_to_cpu(*(uint16_t *)ptr);
+}
+
static inline int ldsw_be_p(const void *ptr)
{
return (int16_t)be_bswap(lduw_he_p(ptr), 16);
@@ -361,6 +376,11 @@ static inline void stw_be_p(void *ptr, uint16_t v)
stw_he_p(ptr, be_bswap(v, 16));
}
+static inline void stw_be_p_aligned(void *ptr, uint16_t v)
+{
+ *(uint16_t *)ptr = cpu_to_be16(v);
+}
+
static inline void st24_be_p(void *ptr, uint32_t v)
{
st24_he_p(ptr, be_bswap24(v));
diff --git a/include/system/memory_cached.h b/include/system/memory_cached.h
index 09d4682..6884f77 100644
--- a/include/system/memory_cached.h
+++ b/include/system/memory_cached.h
@@ -96,6 +96,22 @@ void address_space_stb_cached(const MemoryRegionCache *cache,
#define ARG1_DECL const MemoryRegionCache *cache
#include "system/memory_ldst_phys.h.inc"
+/*
+ * Aligned counterparts of the cached load accessors.
+ *
+ * The fast path (direct RAM access) uses the ld*_p_aligned() pointer helpers,
+ * which assume the host pointer is naturally aligned to the access size and
+ * therefore let the compiler emit an aligned load instruction.
+ *
+ * Callers MUST ensure @addr is aligned to the access size before invoking
+ * these helpers; otherwise the behavior is undefined.
+ */
+#define ENDIANNESS _le
+#include "system/memory_ldst_cached_aligned.h.inc"
+
+#define ENDIANNESS _be
+#include "system/memory_ldst_cached_aligned.h.inc"
+
/**
* address_space_cache_init: prepare for repeated access to a physical
* memory region
diff --git a/include/system/memory_ldst_cached_aligned.h.inc b/include/system/memory_ldst_cached_aligned.h.inc
new file mode 100644
index 0000000..62610d3
--- /dev/null
+++ b/include/system/memory_ldst_cached_aligned.h.inc
@@ -0,0 +1,77 @@
+/*
+ * Aligned Memory access templates for MemoryRegionCache
+ *
+ * Callers MUST ensure @addr is aligned to the access size before invoking
+ * these helpers; otherwise the behavior is undefined.
+ *
+ * Copyright (c) 2018 Red Hat, Inc.
+ * Copyright (c) 2018 LanxinComputing, Ltd.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#define ADDRESS_SPACE_LD_CACHED_ALIGNED(size) \
+ glue(glue(address_space_ld, size), glue(ENDIANNESS, _cached_aligned))
+#define ADDRESS_SPACE_LD_CACHED_SLOW(size) \
+ glue(glue(address_space_ld, size), glue(ENDIANNESS, _cached_slow))
+#define LD_P_ALIGNED(size) \
+ glue(glue(ld, size), glue(ENDIANNESS, _p_aligned))
+#define LD_PHYS_CACHED_ALIGNED(size) \
+ glue(glue(ld, size), glue(ENDIANNESS, glue(_phys, _cached_aligned)))
+
+static inline uint16_t ADDRESS_SPACE_LD_CACHED_ALIGNED(uw)(MemoryRegionCache *cache,
+ hwaddr addr, MemTxAttrs attrs, MemTxResult *result)
+{
+ assert(addr < cache->len && 2 <= cache->len - addr);
+ fuzz_dma_read_cb(cache->xlat + addr, 2, cache->mrs.mr);
+ if (likely(cache->ptr)) {
+ return LD_P_ALIGNED(uw)(cache->ptr + addr);
+ } else {
+ return ADDRESS_SPACE_LD_CACHED_SLOW(uw)(cache, addr, attrs, result);
+ }
+}
+
+static inline uint16_t LD_PHYS_CACHED_ALIGNED(uw)(MemoryRegionCache *cache,
+ hwaddr addr)
+{
+ return ADDRESS_SPACE_LD_CACHED_ALIGNED(uw)(cache, addr,
+ MEMTXATTRS_UNSPECIFIED, NULL);
+}
+
+#undef ADDRESS_SPACE_LD_CACHED_ALIGNED
+#undef ADDRESS_SPACE_LD_CACHED_SLOW
+#undef LD_P_ALIGNED
+#undef LD_PHYS_CACHED_ALIGNED
+
+#define ADDRESS_SPACE_ST_CACHED_ALIGNED(size) \
+ glue(glue(address_space_st, size), glue(ENDIANNESS, _cached_aligned))
+#define ADDRESS_SPACE_ST_CACHED_SLOW(size) \
+ glue(glue(address_space_st, size), glue(ENDIANNESS, _cached_slow))
+#define ST_P_ALIGNED(size) \
+ glue(glue(st, size), glue(ENDIANNESS, _p_aligned))
+#define ST_PHYS_CACHED_ALIGNED(size) \
+ glue(glue(st, size), glue(ENDIANNESS, glue(_phys, _cached_aligned)))
+
+static inline void ADDRESS_SPACE_ST_CACHED_ALIGNED(w)(const MemoryRegionCache *cache,
+ hwaddr addr, uint16_t val, MemTxAttrs attrs, MemTxResult *result)
+{
+ assert(addr < cache->len && 2 <= cache->len - addr);
+ if (likely(cache->ptr)) {
+ ST_P_ALIGNED(w)(cache->ptr + addr, val);
+ } else {
+ ADDRESS_SPACE_ST_CACHED_SLOW(w)(cache, addr, val, attrs, result);
+ }
+}
+
+static inline void ST_PHYS_CACHED_ALIGNED(w)(MemoryRegionCache *cache,
+ hwaddr addr, uint16_t val)
+{
+ ADDRESS_SPACE_ST_CACHED_ALIGNED(w)(cache, addr, val, MEMTXATTRS_UNSPECIFIED, NULL);
+}
+
+#undef ADDRESS_SPACE_ST_CACHED_ALIGNED
+#undef ADDRESS_SPACE_ST_CACHED_SLOW
+#undef ST_P_ALIGNED
+#undef ST_PHYS_CACHED_ALIGNED
+
+#undef ENDIANNESS
--
2.53.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2] virtio: Add aligned ld/st accessors for vring
2026-08-21 10:08 [PATCH v2] virtio: Add aligned ld/st accessors for vring BillXiang
@ 2026-08-21 10:25 ` Peter Maydell
2026-08-21 17:32 ` Richard Henderson
0 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2026-08-21 10:25 UTC (permalink / raw)
To: BillXiang
Cc: mst, pbonzini, peterx, philmd, qemu-devel, qemu-riscv,
richard.henderson
On Fri, 21 Aug 2026 at 11:10, BillXiang
<xiangwencheng@lanxincomputing.com> wrote:
>
> The generic ld/st*_p() pointer helpers lower to __builtin_memcpy,
> which on RISC-V will be expanded to multiple byte-access instructions
> rather than a single aligned access by the compiler because it
> cannot prove alignment at the call site.
It is very unfortunate that your host doesn't have working
unaligned accesses. This puts you into the same bucket as
SPARC (i.e. a rare and not very well tested corner case) and
you're likely to find you have a lot of annoying cases
you need to track down to get things working.
> Each cached 16-bit access of a vring field therefore performs several
> distinct byte ld/st, which is a memory-tearing hazard for fields that
> the guest may access concurrently — most notably avail->idx, where we
> find the guest can write a new value between the individual byte loads
> and produce a torn read that never existed in memory, as seen in logs
> like:
> "Guest moved used index from 49417 to 49919"
> Here, 49919 (binary 1100 0010-1111 1111) is incorrectly assembled from
> the lower byte of 49663 (1100 0001-1111 1111) and the upper byte of
> 49664 (1100 0010-0000 0000).
>
> Add a parallel set of _aligned cached accessors so the fast (RAM) path
> emits a single aligned load instruction, eliminating the tearing window.
>
> Callers MUST ensure @addr is naturally aligned to the access size before
> invoking the _aligned helpers; the virtio vring layout guarantees this
> for avail->idx and other naturally-aligned fields.
>
> This patch fixes the memory-tearing hazard while also improves performance.
>
> Signed-off-by: BillXiang <xiangwencheng@lanxincomputing.com>
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 387d65c..be9913c 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -301,6 +301,11 @@ static inline int lduw_le_p(const void *ptr)
> return (uint16_t)le_bswap(lduw_he_p(ptr), 16);
> }
>
> +static inline int lduw_le_p_aligned(const void *ptr)
> +{
> + return le16_to_cpu(*(uint16_t *)ptr);
If the pointer passed in must be a validly aligned one for a uint16_t,
we can make the argument be 'uint16_t*', not void*. Then the compiler
can give us some assistance about not passing the wrong type.
> +#define ADDRESS_SPACE_LD_CACHED_ALIGNED(size) \
> + glue(glue(address_space_ld, size), glue(ENDIANNESS, _cached_aligned))
> +#define ADDRESS_SPACE_LD_CACHED_SLOW(size) \
> + glue(glue(address_space_ld, size), glue(ENDIANNESS, _cached_slow))
> +#define LD_P_ALIGNED(size) \
> + glue(glue(ld, size), glue(ENDIANNESS, _p_aligned))
> +#define LD_PHYS_CACHED_ALIGNED(size) \
> + glue(glue(ld, size), glue(ENDIANNESS, glue(_phys, _cached_aligned)))
> +
> +static inline uint16_t ADDRESS_SPACE_LD_CACHED_ALIGNED(uw)(MemoryRegionCache *cache,
> + hwaddr addr, MemTxAttrs attrs, MemTxResult *result)
> +{
> + assert(addr < cache->len && 2 <= cache->len - addr);
> + fuzz_dma_read_cb(cache->xlat + addr, 2, cache->mrs.mr);
> + if (likely(cache->ptr)) {
> + return LD_P_ALIGNED(uw)(cache->ptr + addr);
> + } else {
> + return ADDRESS_SPACE_LD_CACHED_SLOW(uw)(cache, addr, attrs, result);
> + }
> +}
I'm tempted to suggest some kind of "if pointer is aligned take
aligned path, otherwise take slow path" either here or actually
in lduw_le_p(), but maybe that's a bad idea. Richard ?
(I have a suspicion that other places than this one will assume
that an aligned ldl_he_p() is not going to tear.)
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] virtio: Add aligned ld/st accessors for vring
2026-08-21 10:25 ` Peter Maydell
@ 2026-08-21 17:32 ` Richard Henderson
2026-08-24 4:19 ` BillXiang
0 siblings, 1 reply; 9+ messages in thread
From: Richard Henderson @ 2026-08-21 17:32 UTC (permalink / raw)
To: Peter Maydell, BillXiang
Cc: mst, pbonzini, peterx, philmd, qemu-devel, qemu-riscv
On 8/21/26 03:25, Peter Maydell wrote:
> I'm tempted to suggest some kind of "if pointer is aligned take
> aligned path, otherwise take slow path" either here or actually
> in lduw_le_p(), but maybe that's a bad idea. Richard ?
>
> (I have a suspicion that other places than this one will assume
> that an aligned ldl_he_p() is not going to tear.)
I agree -- I expect most everything assumes ldl_he_p won't tear for aligned accesses.
This kinda begs the question of what atomicity the caller expects. It's not implausible
that an x86 path expects even unaligned accesses not crossing a cacheline to be atomic,
since that's been a thing since 1995. I expect both IBM architectures similarly expect
atomicity by alignment, since that's been a thing for s390 since yonks and Power has the
same language.
We have a bunch of code in accel/tcg/ldst_atomicity.c.inc that can handle this, we'd just
need to provide it with the correct inputs. And I assume we'd still like to inline the
single access on appropriate hosts.
r~
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] virtio: Add aligned ld/st accessors for vring
2026-08-21 17:32 ` Richard Henderson
@ 2026-08-24 4:19 ` BillXiang
2026-08-24 15:23 ` Peter Xu
0 siblings, 1 reply; 9+ messages in thread
From: BillXiang @ 2026-08-24 4:19 UTC (permalink / raw)
To: Richard Henderson, Peter Maydell
Cc: mst, pbonzini, peterx, philmd, qemu-devel, qemu-riscv
On 8/22/2026 1:32 AM, Richard Henderson wrote:
> On 8/21/26 03:25, Peter Maydell wrote:
>> I'm tempted to suggest some kind of "if pointer is aligned take
>> aligned path, otherwise take slow path" either here or actually
>> in lduw_le_p(), but maybe that's a bad idea. Richard ?
>>
>> (I have a suspicion that other places than this one will assume
>> that an aligned ldl_he_p() is not going to tear.)
> I agree -- I expect most everything assumes ldl_he_p won't tear for
> aligned accesses.
Hi Peter, I noticed that in your commit [1], you have pointed out that
ld*_he_p() and st*_he_p() is not atomic especially for vring_avail_idx.
This suggests it’s time to finally implement the atomic functions. And
I think we should provide explicit atomic operations, similar to those
in CPU instruction sets, rather than a single all‑purpose function
cluttered with conditional branches — and it should be the caller’s
responsibility to decide whether to use them.
>
> This kinda begs the question of what atomicity the caller expects. It's
> not implausible that an x86 path expects even unaligned accesses not
> crossing a cacheline to be atomic, since that's been a thing since
> 1995. I expect both IBM architectures similarly expect atomicity by
> alignment, since that's been a thing for s390 since yonks and Power has
> the same language.
>
> We have a bunch of code in accel/tcg/ldst_atomicity.c.inc that can
> handle this, we'd just need to provide it with the correct inputs. And
> I assume we'd still like to inline the single access on appropriate hosts.
>
>
> r~
Hi Richard, I've read your code in accel/tcg/ldst_atomicity.c.inc. Do
you think it would be better to make the load/store_atomic* public?
--
Bill Xiang
[1]
https://lore.kernel.org/qemu-devel/1554826986-37164-4-git-send-email-pbonzini@redhat.com/#r
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] virtio: Add aligned ld/st accessors for vring
2026-08-24 4:19 ` BillXiang
@ 2026-08-24 15:23 ` Peter Xu
2026-09-01 2:44 ` BillXiang
0 siblings, 1 reply; 9+ messages in thread
From: Peter Xu @ 2026-08-24 15:23 UTC (permalink / raw)
To: BillXiang
Cc: Richard Henderson, Peter Maydell, mst, pbonzini, philmd,
qemu-devel, qemu-riscv
On Mon, Aug 24, 2026 at 12:19:15PM +0800, BillXiang wrote:
> Hi Richard, I've read your code in accel/tcg/ldst_atomicity.c.inc. Do
> you think it would be better to make the load/store_atomic* public?
They do not fit by default, as we need to still process unaligned cases?
I wished we can use qemu_mem_move() directly that just got introduced.. but
it does slightly more than wanted. Maybe something like this? Below diff
dropped ldsw_he_p() alone the way as it's never used.
Thanks,
===8<===
diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 387d65c0b0..04a0f63612 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -1,6 +1,7 @@
#ifndef BSWAP_H
#define BSWAP_H
+#include "qemu/atomic.h"
#include "qemu/target-info.h"
#include "exec/memop.h"
@@ -238,62 +239,52 @@ static inline void stb_p(void *ptr, uint8_t v)
*(uint8_t *)ptr = v;
}
-/*
- * Any compiler worth its salt will turn these memcpy into native unaligned
- * operations. Thus we don't need to play games with packed attributes, or
- * inline byte-by-byte stores.
- * Some compilation environments (eg some fortify-source implementations)
- * may intercept memcpy() in a way that defeats the compiler optimization,
- * though, so we use __builtin_memcpy() to give ourselves the best chance
- * of good performance.
- */
-
-static inline int lduw_he_p(const void *ptr)
-{
- uint16_t r;
- __builtin_memcpy(&r, ptr, sizeof(r));
- return r;
-}
-
-static inline int ldsw_he_p(const void *ptr)
-{
- int16_t r;
- __builtin_memcpy(&r, ptr, sizeof(r));
- return r;
-}
+#define LD_HE_P(type, size) \
+ static inline type \
+ glue(glue(ld, size), _he_p)(const void *ptr) \
+ { \
+ type v; \
+ if (unlikely((uintptr_t)ptr & (sizeof(v) - 1))) { \
+ __builtin_memcpy(&v, ptr, sizeof(v)); \
+ } else { \
+ v = qatomic_read((type *)ptr); \
+ } \
+ return v; \
+ }
-static inline void stw_he_p(void *ptr, uint16_t v)
-{
- __builtin_memcpy(ptr, &v, sizeof(v));
-}
+#define ST_HE_P(type, size) \
+ static inline void \
+ glue(glue(st, size), _he_p)(void *ptr, type v) \
+ { \
+ if (unlikely((uintptr_t)ptr & (sizeof(v) - 1))) { \
+ __builtin_memcpy(ptr, &v, sizeof(v)); \
+ } else { \
+ qatomic_set((type *)ptr, v); \
+ } \
+ }
-static inline void st24_he_p(void *ptr, uint32_t v)
-{
- __builtin_memcpy(ptr, &v, 3);
-}
+LD_HE_P(uint16_t, 16)
+LD_HE_P(uint32_t, 32)
+LD_HE_P(uint64_t, 64)
+ST_HE_P(uint16_t, 16)
+ST_HE_P(uint32_t, 32)
+ST_HE_P(uint64_t, 64)
-static inline int ldl_he_p(const void *ptr)
-{
- int32_t r;
- __builtin_memcpy(&r, ptr, sizeof(r));
- return r;
-}
+#undef LD_HE_P
+#undef ST_HE_P
+#undef ADDR_ALIGNED
-static inline void stl_he_p(void *ptr, uint32_t v)
-{
- __builtin_memcpy(ptr, &v, sizeof(v));
-}
+#define lduw_he_p ld16_he_p
+#define ldl_he_p ld32_he_p
+#define ldq_he_p ld64_he_p
-static inline uint64_t ldq_he_p(const void *ptr)
-{
- uint64_t r;
- __builtin_memcpy(&r, ptr, sizeof(r));
- return r;
-}
+#define stw_he_p st16_he_p
+#define stl_he_p st32_he_p
+#define stq_he_p st64_he_p
-static inline void stq_he_p(void *ptr, uint64_t v)
+static inline void st24_he_p(void *ptr, uint32_t v)
{
- __builtin_memcpy(ptr, &v, sizeof(v));
+ __builtin_memcpy(ptr, &v, 3);
}
static inline int lduw_le_p(const void *ptr)
--
2.54.0
--
Peter Xu
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2] virtio: Add aligned ld/st accessors for vring
2026-08-24 15:23 ` Peter Xu
@ 2026-09-01 2:44 ` BillXiang
2026-09-04 13:34 ` Peter Xu
0 siblings, 1 reply; 9+ messages in thread
From: BillXiang @ 2026-09-01 2:44 UTC (permalink / raw)
To: Peter Xu
Cc: Richard Henderson, Peter Maydell, mst, pbonzini, philmd,
qemu-devel, qemu-riscv, BillXiang
On 8/24/2026 11:23 PM, Peter Xu wrote:
> On Mon, Aug 24, 2026 at 12:19:15PM +0800, BillXiang wrote:
>> Hi Richard, I've read your code in accel/tcg/ldst_atomicity.c.inc. Do
>> you think it would be better to make the load/store_atomic* public?
>
> They do not fit by default, as we need to still process unaligned cases?
You mentioned the use of guest CPU context in [1]. However, what I meant
by load/store_atomic* is code like the following:
static inline uint16_t load_atomic2(void *pv)
{
uint16_t *p = __builtin_assume_aligned(pv, 2);
return qatomic_read(p);
}
static inline void store_atomic2(void *pv, uint16_t val)
{
uint16_t *p = __builtin_assume_aligned(pv, 2);
qatomic_set(p, val);
}
I consider these to be generic atomic load/store primitives.
>
> I wished we can use qemu_mem_move() directly that just got introduced.. but
> it does slightly more than wanted. Maybe something like this? Below diff
> dropped ldsw_he_p() alone the way as it's never used.
>
> Thanks,
>
> ===8<===
>
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 387d65c0b0..04a0f63612 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -1,6 +1,7 @@
> #ifndef BSWAP_H
> #define BSWAP_H
>
> +#include "qemu/atomic.h"
> #include "qemu/target-info.h"
> #include "exec/memop.h"
>
> @@ -238,62 +239,52 @@ static inline void stb_p(void *ptr, uint8_t v)
> *(uint8_t *)ptr = v;
> }
>
> -/*
> - * Any compiler worth its salt will turn these memcpy into native unaligned
> - * operations. Thus we don't need to play games with packed attributes, or
> - * inline byte-by-byte stores.
> - * Some compilation environments (eg some fortify-source implementations)
> - * may intercept memcpy() in a way that defeats the compiler optimization,
> - * though, so we use __builtin_memcpy() to give ourselves the best chance
> - * of good performance.
> - */
> -
> -static inline int lduw_he_p(const void *ptr)
> -{
> - uint16_t r;
> - __builtin_memcpy(&r, ptr, sizeof(r));
> - return r;
> -}
> -
> -static inline int ldsw_he_p(const void *ptr)
> -{
> - int16_t r;
> - __builtin_memcpy(&r, ptr, sizeof(r));
> - return r;
> -}
> +#define LD_HE_P(type, size) \
> + static inline type \
> + glue(glue(ld, size), _he_p)(const void *ptr) \
> + { \
> + type v; \
> + if (unlikely((uintptr_t)ptr & (sizeof(v) - 1))) { \
> + __builtin_memcpy(&v, ptr, sizeof(v)); \
> + } else { \
> + v = qatomic_read((type *)ptr); \
> + } \
> + return v; \
> + }
>
> -static inline void stw_he_p(void *ptr, uint16_t v)
> -{
> - __builtin_memcpy(ptr, &v, sizeof(v));
> -}
> +#define ST_HE_P(type, size) \
> + static inline void \
> + glue(glue(st, size), _he_p)(void *ptr, type v) \
> + { \
> + if (unlikely((uintptr_t)ptr & (sizeof(v) - 1))) { \
> + __builtin_memcpy(ptr, &v, sizeof(v)); \
> + } else { \
> + qatomic_set((type *)ptr, v); \
> + } \
> + }
>
> -static inline void st24_he_p(void *ptr, uint32_t v)
> -{
> - __builtin_memcpy(ptr, &v, 3);
> -}
> +LD_HE_P(uint16_t, 16)
> +LD_HE_P(uint32_t, 32)
> +LD_HE_P(uint64_t, 64)
> +ST_HE_P(uint16_t, 16)
> +ST_HE_P(uint32_t, 32)
> +ST_HE_P(uint64_t, 64)
>
> -static inline int ldl_he_p(const void *ptr)
> -{
> - int32_t r;
> - __builtin_memcpy(&r, ptr, sizeof(r));
> - return r;
> -}
> +#undef LD_HE_P
> +#undef ST_HE_P
> +#undef ADDR_ALIGNED
>
> -static inline void stl_he_p(void *ptr, uint32_t v)
> -{
> - __builtin_memcpy(ptr, &v, sizeof(v));
> -}
> +#define lduw_he_p ld16_he_p
> +#define ldl_he_p ld32_he_p
> +#define ldq_he_p ld64_he_p
>
> -static inline uint64_t ldq_he_p(const void *ptr)
> -{
> - uint64_t r;
> - __builtin_memcpy(&r, ptr, sizeof(r));
> - return r;
> -}
> +#define stw_he_p st16_he_p
> +#define stl_he_p st32_he_p
> +#define stq_he_p st64_he_p
>
> -static inline void stq_he_p(void *ptr, uint64_t v)
> +static inline void st24_he_p(void *ptr, uint32_t v)
> {
> - __builtin_memcpy(ptr, &v, sizeof(v));
> + __builtin_memcpy(ptr, &v, 3);
> }
>
> static inline int lduw_le_p(const void *ptr)
IMO, your change serves as a good optimization for the original
load/store by taking advantage of alignment. However, the real issue I'm
trying to address is the atomicity of these operations—for use cases
like avail_idx in virtio, and this should not limited to virtio alone. I
still believe we should provide explicit atomic load/store interfaces
for callers, rather than relying solely on aligned load/store. And I
admit it was my mistake to use the aligned versions here in the first
place. I'll send another version later and look forward to more feedback.
Thanks,
--
BillXiang
[1] https://lore.kernel.org/qemu-devel/apW0SkNxWPCyvVV6@x1.local/
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] virtio: Add aligned ld/st accessors for vring
2026-09-01 2:44 ` BillXiang
@ 2026-09-04 13:34 ` Peter Xu
2026-09-07 10:11 ` BillXiang
0 siblings, 1 reply; 9+ messages in thread
From: Peter Xu @ 2026-09-04 13:34 UTC (permalink / raw)
To: BillXiang
Cc: Richard Henderson, Peter Maydell, mst, pbonzini, philmd,
qemu-devel, qemu-riscv
On Tue, Sep 01, 2026 at 10:44:13AM +0800, BillXiang wrote:
> On 8/24/2026 11:23 PM, Peter Xu wrote:
> > On Mon, Aug 24, 2026 at 12:19:15PM +0800, BillXiang wrote:
> >> Hi Richard, I've read your code in accel/tcg/ldst_atomicity.c.inc. Do
> >> you think it would be better to make the load/store_atomic* public?
> >
> > They do not fit by default, as we need to still process unaligned cases?
>
> You mentioned the use of guest CPU context in [1]. However, what I meant
> by load/store_atomic* is code like the following:
>
> static inline uint16_t load_atomic2(void *pv)
> {
> uint16_t *p = __builtin_assume_aligned(pv, 2);
> return qatomic_read(p);
> }
>
> static inline void store_atomic2(void *pv, uint16_t val)
> {
> uint16_t *p = __builtin_assume_aligned(pv, 2);
> qatomic_set(p, val);
> }
>
> I consider these to be generic atomic load/store primitives.
__builtin_assume_aligned() tells the compiler the address is aligned. What
if it is not?
>
> >
> > I wished we can use qemu_mem_move() directly that just got introduced.. but
> > it does slightly more than wanted. Maybe something like this? Below diff
> > dropped ldsw_he_p() alone the way as it's never used.
> >
> > Thanks,
> >
> > ===8<===
> >
> > diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> > index 387d65c0b0..04a0f63612 100644
> > --- a/include/qemu/bswap.h
> > +++ b/include/qemu/bswap.h
> > @@ -1,6 +1,7 @@
> > #ifndef BSWAP_H
> > #define BSWAP_H
> >
> > +#include "qemu/atomic.h"
> > #include "qemu/target-info.h"
> > #include "exec/memop.h"
> >
> > @@ -238,62 +239,52 @@ static inline void stb_p(void *ptr, uint8_t v)
> > *(uint8_t *)ptr = v;
> > }
> >
> > -/*
> > - * Any compiler worth its salt will turn these memcpy into native unaligned
> > - * operations. Thus we don't need to play games with packed attributes, or
> > - * inline byte-by-byte stores.
> > - * Some compilation environments (eg some fortify-source implementations)
> > - * may intercept memcpy() in a way that defeats the compiler optimization,
> > - * though, so we use __builtin_memcpy() to give ourselves the best chance
> > - * of good performance.
> > - */
> > -
> > -static inline int lduw_he_p(const void *ptr)
> > -{
> > - uint16_t r;
> > - __builtin_memcpy(&r, ptr, sizeof(r));
> > - return r;
> > -}
> > -
> > -static inline int ldsw_he_p(const void *ptr)
> > -{
> > - int16_t r;
> > - __builtin_memcpy(&r, ptr, sizeof(r));
> > - return r;
> > -}
> > +#define LD_HE_P(type, size) \
> > + static inline type \
> > + glue(glue(ld, size), _he_p)(const void *ptr) \
> > + { \
> > + type v; \
> > + if (unlikely((uintptr_t)ptr & (sizeof(v) - 1))) { \
> > + __builtin_memcpy(&v, ptr, sizeof(v)); \
> > + } else { \
> > + v = qatomic_read((type *)ptr); \
> > + } \
> > + return v; \
> > + }
> >
> > -static inline void stw_he_p(void *ptr, uint16_t v)
> > -{
> > - __builtin_memcpy(ptr, &v, sizeof(v));
> > -}
> > +#define ST_HE_P(type, size) \
> > + static inline void \
> > + glue(glue(st, size), _he_p)(void *ptr, type v) \
> > + { \
> > + if (unlikely((uintptr_t)ptr & (sizeof(v) - 1))) { \
> > + __builtin_memcpy(ptr, &v, sizeof(v)); \
> > + } else { \
> > + qatomic_set((type *)ptr, v); \
> > + } \
> > + }
> >
> > -static inline void st24_he_p(void *ptr, uint32_t v)
> > -{
> > - __builtin_memcpy(ptr, &v, 3);
> > -}
> > +LD_HE_P(uint16_t, 16)
> > +LD_HE_P(uint32_t, 32)
> > +LD_HE_P(uint64_t, 64)
> > +ST_HE_P(uint16_t, 16)
> > +ST_HE_P(uint32_t, 32)
> > +ST_HE_P(uint64_t, 64)
> >
> > -static inline int ldl_he_p(const void *ptr)
> > -{
> > - int32_t r;
> > - __builtin_memcpy(&r, ptr, sizeof(r));
> > - return r;
> > -}
> > +#undef LD_HE_P
> > +#undef ST_HE_P
> > +#undef ADDR_ALIGNED
> >
> > -static inline void stl_he_p(void *ptr, uint32_t v)
> > -{
> > - __builtin_memcpy(ptr, &v, sizeof(v));
> > -}
> > +#define lduw_he_p ld16_he_p
> > +#define ldl_he_p ld32_he_p
> > +#define ldq_he_p ld64_he_p
> >
> > -static inline uint64_t ldq_he_p(const void *ptr)
> > -{
> > - uint64_t r;
> > - __builtin_memcpy(&r, ptr, sizeof(r));
> > - return r;
> > -}
> > +#define stw_he_p st16_he_p
> > +#define stl_he_p st32_he_p
> > +#define stq_he_p st64_he_p
> >
> > -static inline void stq_he_p(void *ptr, uint64_t v)
> > +static inline void st24_he_p(void *ptr, uint32_t v)
> > {
> > - __builtin_memcpy(ptr, &v, sizeof(v));
> > + __builtin_memcpy(ptr, &v, 3);
> > }
> >
> > static inline int lduw_le_p(const void *ptr)
>
> IMO, your change serves as a good optimization for the original
> load/store by taking advantage of alignment. However, the real issue I'm
> trying to address is the atomicity of these operations—for use cases
> like avail_idx in virtio, and this should not limited to virtio alone. I
> still believe we should provide explicit atomic load/store interfaces
> for callers, rather than relying solely on aligned load/store. And I
> admit it was my mistake to use the aligned versions here in the first
> place. I'll send another version later and look forward to more feedback.
Explicit atomic load/store maybe still make sense somewhere as API, but I
think Peter Maydell also raised that this exact failure may not be the only
one.
IMHO we could directly switch to aligned access automatically when it can,
the only concern is we add one more test instruction in this path, but I
expect it always hit the aligned case. Also I expect callers of these host
endian APIs to not loop over a range of addr. So irrelevant of whether
above change would make sense, we want to study more on the impact of the
extra if on the callers. The hope is it is minimum impact and in case
there're corner cases we can switch to other API even if existed.
Thanks,
>
> Thanks,
>
> --
> BillXiang
>
> [1] https://lore.kernel.org/qemu-devel/apW0SkNxWPCyvVV6@x1.local/
>
--
Peter Xu
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] virtio: Add aligned ld/st accessors for vring
2026-09-04 13:34 ` Peter Xu
@ 2026-09-07 10:11 ` BillXiang
2026-09-07 10:29 ` Peter Maydell
0 siblings, 1 reply; 9+ messages in thread
From: BillXiang @ 2026-09-07 10:11 UTC (permalink / raw)
To: Peter Xu
Cc: Richard Henderson, Peter Maydell, mst, pbonzini, philmd,
qemu-devel, qemu-riscv
On 9/4/2026 9:34 PM, Peter Xu wrote:
> On Tue, Sep 01, 2026 at 10:44:13AM +0800, BillXiang wrote:
>> On 8/24/2026 11:23 PM, Peter Xu wrote:
>>> On Mon, Aug 24, 2026 at 12:19:15PM +0800, BillXiang wrote:
>>>> Hi Richard, I've read your code in accel/tcg/ldst_atomicity.c.inc. Do
>>>> you think it would be better to make the load/store_atomic* public?
>>>
>>> They do not fit by default, as we need to still process unaligned cases?
>>
>> You mentioned the use of guest CPU context in [1]. However, what I meant
>> by load/store_atomic* is code like the following:
>>
>> static inline uint16_t load_atomic2(void *pv)
>> {
>> uint16_t *p = __builtin_assume_aligned(pv, 2);
>> return qatomic_read(p);
>> }
>>
>> static inline void store_atomic2(void *pv, uint16_t val)
>> {
>> uint16_t *p = __builtin_assume_aligned(pv, 2);
>> qatomic_set(p, val);
>> }
>>
>> I consider these to be generic atomic load/store primitives.
>
> __builtin_assume_aligned() tells the compiler the address is aligned. What
> if it is not?
When using __builtin_assume_aligned, the compiler will generate
unaligned load/store instructions if the data is not aligned, rather
than emitting byte‑by‑byte code, which may cause errors on processors
that do not support unaligned accesses. That's what I mean: callers must
ensure @addr is naturally aligned to the access size.
>
>>
>>>
>>> I wished we can use qemu_mem_move() directly that just got introduced.. but
>>> it does slightly more than wanted. Maybe something like this? Below diff
>>> dropped ldsw_he_p() alone the way as it's never used.
>>>
>>> Thanks,
>>>
>>> ===8<===
>>>
>>> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
>>> index 387d65c0b0..04a0f63612 100644
>>> --- a/include/qemu/bswap.h
>>> +++ b/include/qemu/bswap.h
>>> @@ -1,6 +1,7 @@
>>> #ifndef BSWAP_H
>>> #define BSWAP_H
>>>
>>> +#include "qemu/atomic.h"
>>> #include "qemu/target-info.h"
>>> #include "exec/memop.h"
>>>
>>> @@ -238,62 +239,52 @@ static inline void stb_p(void *ptr, uint8_t v)
>>> *(uint8_t *)ptr = v;
>>> }
>>>
>>> -/*
>>> - * Any compiler worth its salt will turn these memcpy into native unaligned
>>> - * operations. Thus we don't need to play games with packed attributes, or
>>> - * inline byte-by-byte stores.
>>> - * Some compilation environments (eg some fortify-source implementations)
>>> - * may intercept memcpy() in a way that defeats the compiler optimization,
>>> - * though, so we use __builtin_memcpy() to give ourselves the best chance
>>> - * of good performance.
>>> - */
>>> -
>>> -static inline int lduw_he_p(const void *ptr)
>>> -{
>>> - uint16_t r;
>>> - __builtin_memcpy(&r, ptr, sizeof(r));
>>> - return r;
>>> -}
>>> -
>>> -static inline int ldsw_he_p(const void *ptr)
>>> -{
>>> - int16_t r;
>>> - __builtin_memcpy(&r, ptr, sizeof(r));
>>> - return r;
>>> -}
>>> +#define LD_HE_P(type, size) \
>>> + static inline type \
>>> + glue(glue(ld, size), _he_p)(const void *ptr) \
>>> + { \
>>> + type v; \
>>> + if (unlikely((uintptr_t)ptr & (sizeof(v) - 1))) { \
>>> + __builtin_memcpy(&v, ptr, sizeof(v)); \
>>> + } else { \
>>> + v = qatomic_read((type *)ptr); \
>>> + } \
>>> + return v; \
>>> + }
>>>
>>> -static inline void stw_he_p(void *ptr, uint16_t v)
>>> -{
>>> - __builtin_memcpy(ptr, &v, sizeof(v));
>>> -}
>>> +#define ST_HE_P(type, size) \
>>> + static inline void \
>>> + glue(glue(st, size), _he_p)(void *ptr, type v) \
>>> + { \
>>> + if (unlikely((uintptr_t)ptr & (sizeof(v) - 1))) { \
>>> + __builtin_memcpy(ptr, &v, sizeof(v)); \
>>> + } else { \
>>> + qatomic_set((type *)ptr, v); \
>>> + } \
>>> + }
>>>
>>> -static inline void st24_he_p(void *ptr, uint32_t v)
>>> -{
>>> - __builtin_memcpy(ptr, &v, 3);
>>> -}
>>> +LD_HE_P(uint16_t, 16)
>>> +LD_HE_P(uint32_t, 32)
>>> +LD_HE_P(uint64_t, 64)
>>> +ST_HE_P(uint16_t, 16)
>>> +ST_HE_P(uint32_t, 32)
>>> +ST_HE_P(uint64_t, 64)
>>>
>>> -static inline int ldl_he_p(const void *ptr)
>>> -{
>>> - int32_t r;
>>> - __builtin_memcpy(&r, ptr, sizeof(r));
>>> - return r;
>>> -}
>>> +#undef LD_HE_P
>>> +#undef ST_HE_P
>>> +#undef ADDR_ALIGNED
>>>
>>> -static inline void stl_he_p(void *ptr, uint32_t v)
>>> -{
>>> - __builtin_memcpy(ptr, &v, sizeof(v));
>>> -}
>>> +#define lduw_he_p ld16_he_p
>>> +#define ldl_he_p ld32_he_p
>>> +#define ldq_he_p ld64_he_p
>>>
>>> -static inline uint64_t ldq_he_p(const void *ptr)
>>> -{
>>> - uint64_t r;
>>> - __builtin_memcpy(&r, ptr, sizeof(r));
>>> - return r;
>>> -}
>>> +#define stw_he_p st16_he_p
>>> +#define stl_he_p st32_he_p
>>> +#define stq_he_p st64_he_p
>>>
>>> -static inline void stq_he_p(void *ptr, uint64_t v)
>>> +static inline void st24_he_p(void *ptr, uint32_t v)
>>> {
>>> - __builtin_memcpy(ptr, &v, sizeof(v));
>>> + __builtin_memcpy(ptr, &v, 3);
>>> }
>>>
>>> static inline int lduw_le_p(const void *ptr)
>>
>> IMO, your change serves as a good optimization for the original
>> load/store by taking advantage of alignment. However, the real issue I'm
>> trying to address is the atomicity of these operations—for use cases
>> like avail_idx in virtio, and this should not limited to virtio alone. I
>> still believe we should provide explicit atomic load/store interfaces
>> for callers, rather than relying solely on aligned load/store. And I
>> admit it was my mistake to use the aligned versions here in the first
>> place. I'll send another version later and look forward to more feedback.
>
> Explicit atomic load/store maybe still make sense somewhere as API, but I
> think Peter Maydell also raised that this exact failure may not be the only
> one.
I agree with that. Perhaps we should systematically find all relevant
places and switch to explicit atomic load/store in the future.
>
> IMHO we could directly switch to aligned access automatically when it can,
> the only concern is we add one more test instruction in this path, but I
> expect it always hit the aligned case. Also I expect callers of these host
> endian APIs to not loop over a range of addr. So irrelevant of whether
> above change would make sense, we want to study more on the impact of the
> extra if on the callers. The hope is it is minimum impact and in case
> there're corner cases we can switch to other API even if existed.
I used your latest code [1] to test virtio‑net with “iperf3 -u -l 64” on
my RISC‑V server, and found that the extra conditional check has
negligible impact.
>
> Thanks,
>
>>
>> Thanks,
>>
>> --
>> BillXiang
>>
>> [1] https://lore.kernel.org/qemu-devel/apW0SkNxWPCyvVV6@x1.local/
>>
>
Thanks,
--
BillXiang
[1] https://lore.kernel.org/qemu-devel/aoxiBB2Wn8NiL5s6@x1.local/
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] virtio: Add aligned ld/st accessors for vring
2026-09-07 10:11 ` BillXiang
@ 2026-09-07 10:29 ` Peter Maydell
0 siblings, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2026-09-07 10:29 UTC (permalink / raw)
To: BillXiang
Cc: Peter Xu, Richard Henderson, mst, pbonzini, philmd, qemu-devel,
qemu-riscv
On Mon, 7 Sept 2026 at 11:12, BillXiang
<xiangwencheng@lanxincomputing.com> wrote:
>
> On 9/4/2026 9:34 PM, Peter Xu wrote:
> > On Tue, Sep 01, 2026 at 10:44:13AM +0800, BillXiang wrote:
> >> On 8/24/2026 11:23 PM, Peter Xu wrote:
> >>> On Mon, Aug 24, 2026 at 12:19:15PM +0800, BillXiang wrote:
> >>>> Hi Richard, I've read your code in accel/tcg/ldst_atomicity.c.inc. Do
> >>>> you think it would be better to make the load/store_atomic* public?
> >>>
> >>> They do not fit by default, as we need to still process unaligned cases?
> >>
> >> You mentioned the use of guest CPU context in [1]. However, what I meant
> >> by load/store_atomic* is code like the following:
> >>
> >> static inline uint16_t load_atomic2(void *pv)
> >> {
> >> uint16_t *p = __builtin_assume_aligned(pv, 2);
> >> return qatomic_read(p);
> >> }
> >>
> >> static inline void store_atomic2(void *pv, uint16_t val)
> >> {
> >> uint16_t *p = __builtin_assume_aligned(pv, 2);
> >> qatomic_set(p, val);
> >> }
> >>
> >> I consider these to be generic atomic load/store primitives.
> >
> > __builtin_assume_aligned() tells the compiler the address is aligned. What
> > if it is not?
>
> When using __builtin_assume_aligned, the compiler will generate
> unaligned load/store instructions if the data is not aligned, rather
> than emitting byte‑by‑byte code, which may cause errors on processors
> that do not support unaligned accesses. That's what I mean: callers must
> ensure @addr is naturally aligned to the access size.
Indeed. That's why the ldst_atomicity.c.inc code only calls them
when it can guarantee the alignment, with fallback cases for
when it isn't. Those are the lowest level internal functions
in that code, and the higher level ones are things like
load_atom_2() (which might call load_atomic2() if the address
is 2-aligned, or lduw_he_p() if it's not but the caller said
it doesn't care about getting a 2-byte-atomic load for an
unaligned pointer, or a function to do an atomic 8-byte access
and extract the 2 bytes we care about, or if all else fails
falling back to "stop all QEMU threads and do a non-atomic read".
The difficulty in generalizing that code for device use I suspect
is whether the fallback cases remain the right thing to do.
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-09-07 10:29 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-21 10:08 [PATCH v2] virtio: Add aligned ld/st accessors for vring BillXiang
2026-08-21 10:25 ` Peter Maydell
2026-08-21 17:32 ` Richard Henderson
2026-08-24 4:19 ` BillXiang
2026-08-24 15:23 ` Peter Xu
2026-09-01 2:44 ` BillXiang
2026-09-04 13:34 ` Peter Xu
2026-09-07 10:11 ` BillXiang
2026-09-07 10:29 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.