On 9/4/2026 9:34 PM, Peter Xu wrote:
> 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?
When using __builtin_assume_aligned, the compiler will generate
unaligned load/store instructions if the data is not aligned, rather
than emitting byte‑by‑byte code, which may cause errors on processors
that do not support unaligned accesses. That's what I mean: callers must
ensure @addr is naturally aligned to the access size.
>
>>
>>>
>>> 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.
I agree with that. Perhaps we should systematically find all relevant
places and switch to explicit atomic load/store in the future.
>
> 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.
I used your latest code [1] to test virtio‑net with “iperf3 -u -l 64” on
my RISC‑V server, and found that the extra conditional check has
negligible impact.
>
> Thanks,
>
>>
>> Thanks,
>>
>> --
>> BillXiang
>>
>> [1] https://lore.kernel.org/qemu-devel/[email protected]/
>>
>
Thanks,
--
BillXiang
[1] https://lore.kernel.org/qemu-devel/[email protected]/
