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.
>
> 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.
Thanks,
--
BillXiang
[1] https://lore.kernel.org/qemu-devel/[email protected]/
