Re: [PATCH 2/2] target/xtensa: Remove tswap() calls in semihosting simcall() helper

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 6/12/24 13:41, Max Filippov wrote:
> On Thu, Dec 5, 2024 at 3:24 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> In preparation of heterogeneous emulation where cores with
>> different endianness can run concurrently, we need to remove
>> the tswap() calls -- which use a fixed per-binary endianness.
>>
>> Get the endianness of the CPU accessed using the libisa
>> xtensa_isa_is_big_endian() call and replace the tswap() calls
>> by bswap() ones when necessary.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/xtensa/xtensa-semi.c | 16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/xtensa/xtensa-semi.c b/target/xtensa/xtensa-semi.c
>> index fa21b7e11fc..21d23e39de5 100644
>> --- a/target/xtensa/xtensa-semi.c
>> +++ b/target/xtensa/xtensa-semi.c
>> @@ -328,10 +328,17 @@ void HELPER(simcall)(CPUXtensaState *env)
>>               struct timeval tv = {0};
>>
>>               if (target_tv) {
>> +                bool cpu_big_endian = xtensa_isa_is_big_endian(env->config->isa);
>> +                bool swap_needed = HOST_BIG_ENDIAN != cpu_big_endian;
>> +
>>                   cpu_memory_rw_debug(cs, target_tv,
>>                           (uint8_t *)target_tvv, sizeof(target_tvv), 0);
>> -                tv.tv_sec = (int32_t)tswap32(target_tvv[0]);
>> -                tv.tv_usec = (int32_t)tswap32(target_tvv[1]);
>> +                if (swap_needed) {
>> +                    bswap32s(&target_tvv[0]);
>> +                    bswap32s(&target_tvv[1]);
>> +                }
>> +                tv.tv_sec = (int32_t)target_tvv[0];
>> +                tv.tv_usec = (int32_t)target_tvv[1];
> 
> This conversion looks a bit heavy. Maybe it would be better to
> give tswap*() an additional env argument and give the target
> a way to deal with it?

Yeah, something like that. TBH I'm not really happy with these
changes, and am having hard time figuring out a simple and
meaningful API.

> 
>>               }
>>               if (fd < 3 && sim_console) {
>>                   if ((fd == 1 || fd == 2) && rq == SELECT_ONE_WRITE) {
>> @@ -381,6 +388,8 @@ void HELPER(simcall)(CPUXtensaState *env)
>>               int argc = semihosting_get_argc();
>>               int str_offset = (argc + 1) * sizeof(uint32_t);
>>               int i;
>> +            bool cpu_big_endian = xtensa_isa_is_big_endian(env->config->isa);
>> +            bool swap_needed = HOST_BIG_ENDIAN != cpu_big_endian;
>>               uint32_t argptr;
>>
>>               for (i = 0; i < argc; ++i) {
>> @@ -388,6 +397,9 @@ void HELPER(simcall)(CPUXtensaState *env)
>>                   int str_size = strlen(str) + 1;
>>
>>                   argptr = tswap32(regs[3] + str_offset);
> 
> The tswap() is still here.

Oops!

> 
>> +                if (swap_needed) {
>> +                    bswap32s(&argptr);
>> +                }
>>
>>                   cpu_memory_rw_debug(cs,
>>                                       regs[3] + i * sizeof(uint32_t),
>