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

BillXiang posted 1 patch 1 day, 11 hours ago
Failed in applying to current master (apply log)
There is a newer version of this series
hw/virtio/virtio.c                            |  8 +-
include/qemu/bswap.h                          | 41 ++++++++++
include/system/memory_cached.h                | 16 ++++
.../system/memory_ldst_cached_aligned.h.inc   | 76 +++++++++++++++++++
4 files changed, 137 insertions(+), 4 deletions(-)
create mode 100644 include/system/memory_ldst_cached_aligned.h.inc
[PATCH] virtio: Add aligned ld/st accessors for vring
Posted by BillXiang 1 day, 11 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                          | 41 ++++++++++
 include/system/memory_cached.h                | 16 ++++
 .../system/memory_ldst_cached_aligned.h.inc   | 76 +++++++++++++++++++
 4 files changed, 137 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..c23f5a2 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -255,6 +255,20 @@ static inline int lduw_he_p(const void *ptr)
     return r;
 }
 
+static inline int lduw_he_p_aligned(const void *ptr)
+{
+    uint16_t r;
+    __builtin_memcpy(&r, __builtin_assume_aligned(ptr, sizeof(r)), sizeof(r));
+    return r;
+}
+
 static inline int ldsw_he_p(const void *ptr)
 {
     int16_t r;
@@ -267,6 +281,11 @@ static inline void stw_he_p(void *ptr, uint16_t v)
     __builtin_memcpy(ptr, &v, sizeof(v));
 }
 
+static inline void stw_he_p_aligned(void *ptr, uint16_t v)
+{
+    __builtin_memcpy(__builtin_assume_aligned(ptr, sizeof(v)), &v, sizeof(v));
+}
+
 static inline void st24_he_p(void *ptr, uint32_t v)
 {
     __builtin_memcpy(ptr, &v, 3);
@@ -301,6 +320,12 @@ 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 (uint16_t)le_bswap(lduw_he_p_aligned(ptr), 16);
+}
+
 static inline int ldsw_le_p(const void *ptr)
 {
     return (int16_t)le_bswap(lduw_he_p(ptr), 16);
@@ -321,6 +346,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)
+{
+    stw_he_p_aligned(ptr, le_bswap(v, 16));
+}
+
 static inline void st24_le_p(void *ptr, uint32_t v)
 {
     st24_he_p(ptr, le_bswap24(v));
@@ -341,6 +371,12 @@ 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 (uint16_t)be_bswap(lduw_he_p_aligned(ptr), 16);
+}
+
 static inline int ldsw_be_p(const void *ptr)
 {
     return (int16_t)be_bswap(lduw_he_p(ptr), 16);
@@ -361,6 +397,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)
+{
+    stw_he_p_aligned(ptr, be_bswap(v, 16));
+}
+
 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..cfa7a26 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..c21c135
--- /dev/null
+++ b/include/system/memory_ldst_cached_aligned.h.inc
@@ -0,0 +1,76 @@
+/*
+ *  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] virtio: Add aligned ld/st accessors for vring
Posted by Richard Henderson 1 day, 5 hours ago
On 8/20/26 02:46, BillXiang wrote:
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 387d65c..c23f5a2 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -255,6 +255,20 @@ static inline int lduw_he_p(const void *ptr)
>       return r;
>   }
>   
> +static inline int lduw_he_p_aligned(const void *ptr)
> +{
> +    uint16_t r;
> +    __builtin_memcpy(&r, __builtin_assume_aligned(ptr, sizeof(r)), sizeof(r));
> +    return r;
> +}

If you know it's aligned, then you don't need anything special: just a normal C memory 
access.  E.g.

     *(uint16_t *)ptr

and then of course no need for a wrapper function.

> +static inline int lduw_le_p_aligned(const void *ptr)
> +{
> +    return (uint16_t)le_bswap(lduw_he_p_aligned(ptr), 16);
> +}

And this:  le16_to_cpu(*(uint16_t *)ptr)


r~
Re: [PATCH] virtio: Add aligned ld/st accessors for vring
Posted by BillXiang 18 hours ago
On 8/20/2026 10:59 PM, Richard Henderson wrote:
> On 8/20/26 02:46, BillXiang wrote:
>> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
>> index 387d65c..c23f5a2 100644
>> --- a/include/qemu/bswap.h
>> +++ b/include/qemu/bswap.h
>> @@ -255,6 +255,20 @@ static inline int lduw_he_p(const void *ptr)
>>       return r;
>>   }
>> +static inline int lduw_he_p_aligned(const void *ptr)
>> +{
>> +    uint16_t r;
>> +    __builtin_memcpy(&r, __builtin_assume_aligned(ptr, sizeof(r)), 
>> sizeof(r));
>> +    return r;
>> +}
> 
> If you know it's aligned, then you don't need anything special: just a 
> normal C memory access.  E.g.
> 
>      *(uint16_t *)ptr
> 
> and then of course no need for a wrapper function.

Noted, thanks! I’ll resend it with the changes.

> 
>> +static inline int lduw_le_p_aligned(const void *ptr)
>> +{
>> +    return (uint16_t)le_bswap(lduw_he_p_aligned(ptr), 16);
>> +}
> 
> And this:  le16_to_cpu(*(uint16_t *)ptr)
> 
> 
> r~