1. Patchew
  2. QEMU
  3. View series

[PATCH v2] virtio: Add aligned ld/st accessors for vring

BillXiang posted 1 patch 10 hours ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260821100856.1794011-1-xiangwencheng@lanxincomputing.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>, "Philippe Mathieu-Daudé" <philmd@mailo.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
[PATCH v2] virtio: Add aligned ld/st accessors for vring
Posted by BillXiang 10 hours ago 
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
Re: [PATCH v2] virtio: Add aligned ld/st accessors for vring
Posted by Peter Maydell 10 hours ago 
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
Re: [PATCH v2] virtio: Add aligned ld/st accessors for vring
Posted by Richard Henderson 3 hours ago 
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~

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.