[PATCH 0/2] target/xtensa: Remove tswap() calls in semihosting

[PATCH 0/2] target/xtensa: Remove tswap() calls in semihosting

[Philippe Mathieu-Daudé]

The first patch modifying Xtensa libisa has already been
posted in https://lore.kernel.org/qemu-devel/20241204202602.58083-1-philmd@linaro.org/

Philippe Mathieu-Daudé (2):
  target/xtensa: Implement xtensa_isa_is_big_endian()
  target/xtensa: Remove tswap() calls in semihosting simcall() helper

 include/hw/xtensa/xtensa-isa.h |  1 +
 target/xtensa/xtensa-isa.c     |  7 +++++++
 target/xtensa/xtensa-semi.c    | 16 ++++++++++++++--
 3 files changed, 22 insertions(+), 2 deletions(-)

-- 
2.45.2

[PATCH 1/2] target/xtensa: Implement xtensa_isa_is_big_endian()

[Philippe Mathieu-Daudé]

Xtensa internal fields are opaque, only accessible by
the Xtensa libisa. Implement xtensa_isa_is_big_endian()
to get vCPU endianness. This should be implemented in
libisa, not QEMU, but I couldn't figure out where to
contribute this.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/xtensa/xtensa-isa.h | 1 +
 target/xtensa/xtensa-isa.c     | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/include/hw/xtensa/xtensa-isa.h b/include/hw/xtensa/xtensa-isa.h
index a289531bdc8..1cb8e6ccb66 100644
--- a/include/hw/xtensa/xtensa-isa.h
+++ b/include/hw/xtensa/xtensa-isa.h
@@ -829,6 +829,7 @@ const char *xtensa_funcUnit_name(xtensa_isa isa, xtensa_funcUnit fun);
 
 int xtensa_funcUnit_num_copies(xtensa_isa isa, xtensa_funcUnit fun);
 
+bool xtensa_isa_is_big_endian(xtensa_isa isa);
 
 #ifdef __cplusplus
 }
diff --git a/target/xtensa/xtensa-isa.c b/target/xtensa/xtensa-isa.c
index 630b4f9da1b..36eb4bcf3d4 100644
--- a/target/xtensa/xtensa-isa.c
+++ b/target/xtensa/xtensa-isa.c
@@ -1741,3 +1741,10 @@ int xtensa_funcUnit_num_copies(xtensa_isa isa, xtensa_funcUnit fun)
     CHECK_FUNCUNIT(intisa, fun, XTENSA_UNDEFINED);
     return intisa->funcUnits[fun].num_copies;
 }
+
+bool xtensa_isa_is_big_endian(xtensa_isa isa)
+{
+    xtensa_isa_internal *intisa = (xtensa_isa_internal *)isa;
+
+    return intisa->is_big_endian;
+}
-- 
2.45.2

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

[Philippe Mathieu-Daudé]

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];
             }
             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);
+                if (swap_needed) {
+                    bswap32s(&argptr);
+                }
 
                 cpu_memory_rw_debug(cs,
                                     regs[3] + i * sizeof(uint32_t),
-- 
2.45.2

Re: [PATCH 1/2] target/xtensa: Implement xtensa_isa_is_big_endian()

[Max Filippov]

On Thu, Dec 5, 2024 at 3:24 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> Xtensa internal fields are opaque, only accessible by
> the Xtensa libisa. Implement xtensa_isa_is_big_endian()
> to get vCPU endianness. This should be implemented in
> libisa, not QEMU, but I couldn't figure out where to
> contribute this.

This is a beautified version of what xtensa has in binutils, but
binutils is also
just another user of that internal Tensilica library. Perhaps I should make
an artificial "upstream" for this library just to track this kind of changes.
Let me look at it.

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/xtensa/xtensa-isa.h | 1 +
>  target/xtensa/xtensa-isa.c     | 7 +++++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/include/hw/xtensa/xtensa-isa.h b/include/hw/xtensa/xtensa-isa.h
> index a289531bdc8..1cb8e6ccb66 100644
> --- a/include/hw/xtensa/xtensa-isa.h
> +++ b/include/hw/xtensa/xtensa-isa.h
> @@ -829,6 +829,7 @@ const char *xtensa_funcUnit_name(xtensa_isa isa, xtensa_funcUnit fun);
>
>  int xtensa_funcUnit_num_copies(xtensa_isa isa, xtensa_funcUnit fun);
>
> +bool xtensa_isa_is_big_endian(xtensa_isa isa);

This file doesn't include stdbool.h and other boolean functions in it
(e.g. xtensa_opcode_is_branch()) return int. I'd suggest sticking with
that. With that change:

Acked-by: Max Filippov <jcmvbkbc@gmail.com>

>  #ifdef __cplusplus
>  }
> diff --git a/target/xtensa/xtensa-isa.c b/target/xtensa/xtensa-isa.c
> index 630b4f9da1b..36eb4bcf3d4 100644
> --- a/target/xtensa/xtensa-isa.c
> +++ b/target/xtensa/xtensa-isa.c
> @@ -1741,3 +1741,10 @@ int xtensa_funcUnit_num_copies(xtensa_isa isa, xtensa_funcUnit fun)
>      CHECK_FUNCUNIT(intisa, fun, XTENSA_UNDEFINED);
>      return intisa->funcUnits[fun].num_copies;
>  }
> +
> +bool xtensa_isa_is_big_endian(xtensa_isa isa)
> +{
> +    xtensa_isa_internal *intisa = (xtensa_isa_internal *)isa;
> +
> +    return intisa->is_big_endian;
> +}

-- 
Thanks.
-- Max

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

[Max Filippov]

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?

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

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

-- 
Thanks.
-- Max

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

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

[Peter Maydell]

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

Re: [PATCH 1/2] target/xtensa: Implement xtensa_isa_is_big_endian()

[Philippe Mathieu-Daudé]

Hi Max,

On 6/12/24 13:35, Max Filippov wrote:
> On Thu, Dec 5, 2024 at 3:24 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>> Xtensa internal fields are opaque, only accessible by
>> the Xtensa libisa. Implement xtensa_isa_is_big_endian()
>> to get vCPU endianness. This should be implemented in
>> libisa, not QEMU, but I couldn't figure out where to
>> contribute this.
> 
> This is a beautified version of what xtensa has in binutils, but
> binutils is also
> just another user of that internal Tensilica library. Perhaps I should make
> an artificial "upstream" for this library just to track this kind of changes.
> Let me look at it.

Could you add this method to the Tensilica library?

> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/xtensa/xtensa-isa.h | 1 +
>>   target/xtensa/xtensa-isa.c     | 7 +++++++
>>   2 files changed, 8 insertions(+)
>>
>> diff --git a/include/hw/xtensa/xtensa-isa.h b/include/hw/xtensa/xtensa-isa.h
>> index a289531bdc8..1cb8e6ccb66 100644
>> --- a/include/hw/xtensa/xtensa-isa.h
>> +++ b/include/hw/xtensa/xtensa-isa.h
>> @@ -829,6 +829,7 @@ const char *xtensa_funcUnit_name(xtensa_isa isa, xtensa_funcUnit fun);
>>
>>   int xtensa_funcUnit_num_copies(xtensa_isa isa, xtensa_funcUnit fun);
>>
>> +bool xtensa_isa_is_big_endian(xtensa_isa isa);
> 
> This file doesn't include stdbool.h and other boolean functions in it
> (e.g. xtensa_opcode_is_branch()) return int. I'd suggest sticking with
> that. With that change:
> 
> Acked-by: Max Filippov <jcmvbkbc@gmail.com>
> 
>>   #ifdef __cplusplus
>>   }
>> diff --git a/target/xtensa/xtensa-isa.c b/target/xtensa/xtensa-isa.c
>> index 630b4f9da1b..36eb4bcf3d4 100644
>> --- a/target/xtensa/xtensa-isa.c
>> +++ b/target/xtensa/xtensa-isa.c
>> @@ -1741,3 +1741,10 @@ int xtensa_funcUnit_num_copies(xtensa_isa isa, xtensa_funcUnit fun)
>>       CHECK_FUNCUNIT(intisa, fun, XTENSA_UNDEFINED);
>>       return intisa->funcUnits[fun].num_copies;
>>   }
>> +
>> +bool xtensa_isa_is_big_endian(xtensa_isa isa)
>> +{
>> +    xtensa_isa_internal *intisa = (xtensa_isa_internal *)isa;
>> +
>> +    return intisa->is_big_endian;
>> +}
>