| Message ID | 20241205232437.85235-3-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | target/xtensa: Remove tswap() calls in semihosting | expand |
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),
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), >
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
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),
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(-)