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

On Fri, 21 Aug 2026 at 11:10, BillXiang
<[email protected]> 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 <[email protected]>

> 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