Re: [PATCH 15/16] system/memory: make compilation unit common

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 3/10/25 10:43, Richard Henderson wrote:
> On 3/9/25 21:58, Pierrick Bouvier wrote:
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    system/memory.c    | 22 +++++++++++++++-------
>>    system/meson.build |  2 +-
>>    2 files changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/system/memory.c b/system/memory.c
>> index 4c829793a0a..b401be8b5f1 100644
>> --- a/system/memory.c
>> +++ b/system/memory.c
>> @@ -355,11 +355,11 @@ static void flatview_simplify(FlatView *view)
>>    
>>    static bool memory_region_big_endian(MemoryRegion *mr)
>>    {
>> -#if TARGET_BIG_ENDIAN
>> -    return mr->ops->endianness != DEVICE_LITTLE_ENDIAN;
>> -#else
>> -    return mr->ops->endianness == DEVICE_BIG_ENDIAN;
>> -#endif
>> +    if (target_words_bigendian()) {
>> +        return mr->ops->endianness != DEVICE_LITTLE_ENDIAN;
>> +    } else {
>> +        return mr->ops->endianness == DEVICE_BIG_ENDIAN;
>> +    }
>>    }
> 
> This should use the same expression as for patch 4:
> 
>       return (end == DEVICE_NATIVE_ENDIAN
>               ? target_words_bigendian()
>               : end == DEVICE_BIG_ENDIAN);
> 
> Which perhaps ought to be split out to it's own inline function?
> 

Good point, I'll add this.

>>    
>>    static void adjust_endianness(MemoryRegion *mr, uint64_t *data, MemOp op)
>> @@ -2584,7 +2584,11 @@ void memory_region_add_eventfd(MemoryRegion *mr,
>>        unsigned i;
>>    
>>        if (size) {
>> -        adjust_endianness(mr, &mrfd.data, size_memop(size) | MO_TE);
>> +        if (target_words_bigendian()) {
>> +            adjust_endianness(mr, &mrfd.data, size_memop(size) | MO_BE);
>> +        } else {
>> +            adjust_endianness(mr, &mrfd.data, size_memop(size) | MO_LE);
>> +        }
> 
> Maybe better as
> 
>       MemOp mop = (target_words_bigendian() ? MO_BE : MO_LE) | size_memop(size);
>       adjust_endianness(mr, &mrfd.data, size_memop(size), mop);
> 

Do you think defining MO_TE as this expression is a good idea?

I'm afraid it's a bit too much implicit though, but it would save us 
from the hassle to change a lot of code using MO_BE, MO_LE (and all 
other variants defined in MemOp enum).

> 
> r~