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
