Re: [PATCH 1/2] include/exec: Make ld*_p and st*_p functions available for generic code, too

[Thomas Huth <thuth@redhat.com>]

On 17/05/2023 12.18, Philippe Mathieu-Daudé wrote:
> Hi Thomas,
> 
> On 17/5/23 09:42, Thomas Huth wrote:
>> This will allow to move more code into the target independent source set.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   include/exec/cpu-all.h | 25 ----------------
>>   include/exec/tswap.h   | 66 ++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 66 insertions(+), 25 deletions(-)
>>
>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
>> index ad824fee52..0daa4c06e5 100644
>> --- a/include/exec/cpu-all.h
>> +++ b/include/exec/cpu-all.h
>> @@ -55,31 +55,6 @@
>>   #define bswaptls(s) bswap64s(s)
>>   #endif
>> -/* Target-endianness CPU memory access functions. These fit into the
>> - * {ld,st}{type}{sign}{size}{endian}_p naming scheme described in bswap.h.
>> - */
>> -#if TARGET_BIG_ENDIAN
>> -#define lduw_p(p) lduw_be_p(p)
>> -#define ldsw_p(p) ldsw_be_p(p)
>> -#define ldl_p(p) ldl_be_p(p)
>> -#define ldq_p(p) ldq_be_p(p)
>> -#define stw_p(p, v) stw_be_p(p, v)
>> -#define stl_p(p, v) stl_be_p(p, v)
>> -#define stq_p(p, v) stq_be_p(p, v)
>> -#define ldn_p(p, sz) ldn_be_p(p, sz)
>> -#define stn_p(p, sz, v) stn_be_p(p, sz, v)
>> -#else
>> -#define lduw_p(p) lduw_le_p(p)
>> -#define ldsw_p(p) ldsw_le_p(p)
>> -#define ldl_p(p) ldl_le_p(p)
>> -#define ldq_p(p) ldq_le_p(p)
>> -#define stw_p(p, v) stw_le_p(p, v)
>> -#define stl_p(p, v) stl_le_p(p, v)
>> -#define stq_p(p, v) stq_le_p(p, v)
>> -#define ldn_p(p, sz) ldn_le_p(p, sz)
>> -#define stn_p(p, sz, v) stn_le_p(p, sz, v)
>> -#endif
>> -
>>   /* MMU memory access macros */
>>   #if defined(CONFIG_USER_ONLY)
>> diff --git a/include/exec/tswap.h b/include/exec/tswap.h
>> index 68944a880b..2774820bbe 100644
>> --- a/include/exec/tswap.h
>> +++ b/include/exec/tswap.h
>> @@ -69,4 +69,70 @@ static inline void tswap64s(uint64_t *s)
>>       }
>>   }
>> +/*
>> + * Target-endianness CPU memory access functions. These fit into the
>> + * {ld,st}{type}{sign}{size}{endian}_p naming scheme described in bswap.h.
>> + */
>> +
>> +static inline int lduw_p(const void *ptr)
>> +{
>> +    return (uint16_t)tswap16(lduw_he_p(ptr));
>> +}
>> +
>> +static inline int ldsw_p(const void *ptr)
>> +{
>> +    return (int16_t)tswap16(lduw_he_p(ptr));
>> +}
>> +
>> +static inline int ldl_p(const void *ptr)
>> +{
>> +    return tswap32(ldl_he_p(ptr));
>> +}
>> +
>> +static inline uint64_t ldq_p(const void *ptr)
>> +{
>> +    return tswap64(ldq_he_p(ptr));
>> +}
> 
> Hmm I am a bit confused, I was working on removing the tswapXX API
> from softmmu [*] (restricting it locally to gdbstub). Now I realize
> commit 24be3369ad ("include/exec: Provide the tswap() functions for target 
> independent code, too") exposes it furthermore. I thought the
> ld/st API was clearer and enough for all our uses, but maybe I am
> wrong and we need this API.

I think we need both. In some cases, you want to read/write values in a 
certain endianess, and in some cases you have some generic code that just 
wants to write some values in the endianess of the target (where the generic 
code does not know the endianess of the target by default, e.g. since it is 
one time linked with a big endian target and one time with a little endian 
target). In the latter case, we need these functions here from tswap.h.

  Thomas