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/[email protected]/
>
--
Peter Xu
