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

[Philippe Mathieu-Daudé]

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),