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

[Peter Maydell <peter.maydell@linaro.org>]

On Fri, 6 Dec 2024 at 14:35, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> 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.

You could punt the issue here by having xtensa include
semihosting/uaccess.h, and then replace the direct
cpu_memory_rw_debug / tswap calls with

    get_user_u32(tv.tv_sec, target_tv);
    get_user_u32(tv.tv_sec, target_tv + 4);

These are still doing cpu_memory_rw_debug + tswap under
the hood, but it makes this code in xtensa-semi.c simpler :-)
and you'll need to figure out the plan for get/put_user_*
at some point for Arm and other common-semihosting targets.

> >
> >>               }
> >>               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!

Similarly here
    put_user_u32(regs[3] + str_offset, regs[3] + i * sizeof(uint32_t));

-- PMM