On 20 January 2017 at 16:32, Julian Brown <jul...@codesourcery.com> wrote: > This patch introduces an ARM-specific version of the memory read/write, > etc. functions used for semihosting, in order to byte-swap (big-endian) > target memory that is to be interpreted by the (little-endian) host. > The target_memory_rw_debug function is used that knows about the > byte-reversal used for BE32 system mode. > > Signed-off-by: Julian Brown <jul...@codesourcery.com> > --- > include/exec/softmmu-arm-semi.h | 131 > ++++++++++++++++++++++++++++++++++++++++ > target/arm/arm-semi.c | 4 +- > target/arm/cpu.c | 24 ++++++++ > target/arm/cpu.h | 6 ++ > 4 files changed, 163 insertions(+), 2 deletions(-) > create mode 100644 include/exec/softmmu-arm-semi.h > > diff --git a/include/exec/softmmu-arm-semi.h b/include/exec/softmmu-arm-semi.h > new file mode 100644 > index 0000000..bba9ca6 > --- /dev/null > +++ b/include/exec/softmmu-arm-semi.h > @@ -0,0 +1,131 @@ > +/* > + * Helper routines to provide target memory access for ARM semihosting > + * syscalls in system emulation mode. > + * > + * Copyright (c) 2007 CodeSourcery, (c) 2016 Mentor Graphics > + * > + * This code is licensed under the GPL > + */ > + > +#ifndef SOFTMMU_ARM_SEMI_H > +#define SOFTMMU_ARM_SEMI_H 1 > + > +/* In big-endian mode (either BE8 or BE32), values larger than a byte will be > + * transferred to/from memory in big-endian format. Assuming we're on a > + * little-endian host machine, such values will need to be byteswapped before > + * and after the host processes them. > + * > + * This means that byteswapping will occur *twice* in BE32 mode for > + * halfword/word reads/writes. > + */ > + > +static inline bool arm_bswap_needed(CPUARMState *env) > +{ > +#ifdef HOST_WORDS_BIGENDIAN > +#error HOST_WORDS_BIGENDIAN is not supported for ARM semihosting at the > moment. > +#else
This breaks compilation on big-endian systems, right? This needs to be actually implemented... maybe return #ifdef BSWAP_NEEDED 1 ^ #endif (arm_sctlr_b(env) || arm_cpsr_e(env)); (untested, and there may be a less ugly way to phrase that). > + return arm_sctlr_b(env) || arm_cpsr_e(env); > +#endif > +} Also, what about AArch64? Should we just be calling arm_cpu_data_is_big_endian() here? > + > +static inline uint64_t softmmu_tget64(CPUArchState *env, target_ulong addr) > +{ > + uint64_t val; > + > + target_memory_rw_debug(ENV_GET_CPU(env), addr, (uint8_t *)&val, 8, 0); > + if (arm_bswap_needed(env)) { > + return bswap64(val); > + } else { > + return val; > + } > +} > + > +static inline uint32_t softmmu_tget32(CPUArchState *env, target_ulong addr) > +{ > + uint32_t val; > + > + target_memory_rw_debug(ENV_GET_CPU(env), addr, (uint8_t *)&val, 4, 0); > + if (arm_bswap_needed(env)) { > + return bswap32(val); > + } else { > + return val; > + } > +} > + > +static inline uint32_t softmmu_tget8(CPUArchState *env, target_ulong addr) > +{ > + uint8_t val; > + target_memory_rw_debug(ENV_GET_CPU(env), addr, &val, 1, 0); > + return val; > +} > + > +#define get_user_u64(arg, p) ({ arg = softmmu_tget64(env, p); 0; }) > +#define get_user_u32(arg, p) ({ arg = softmmu_tget32(env, p) ; 0; }) > +#define get_user_u8(arg, p) ({ arg = softmmu_tget8(env, p) ; 0; }) > +#define get_user_ual(arg, p) get_user_u32(arg, p) > + > +static inline void softmmu_tput64(CPUArchState *env, > + target_ulong addr, uint64_t val) > +{ > + if (arm_bswap_needed(env)) { > + val = bswap64(val); > + } > + cpu_memory_rw_debug(ENV_GET_CPU(env), addr, (uint8_t *)&val, 8, 1); > +} > + > +static inline void softmmu_tput32(CPUArchState *env, > + target_ulong addr, uint32_t val) > +{ > + if (arm_bswap_needed(env)) { > + val = bswap32(val); > + } > + target_memory_rw_debug(ENV_GET_CPU(env), addr, (uint8_t *)&val, 4, 1); > +} > +#define put_user_u64(arg, p) ({ softmmu_tput64(env, p, arg) ; 0; }) > +#define put_user_u32(arg, p) ({ softmmu_tput32(env, p, arg) ; 0; }) > +#define put_user_ual(arg, p) put_user_u32(arg, p) > + > +static inline void *softmmu_lock_user(CPUArchState *env, > + target_ulong addr, target_ulong len, > + int copy) > +{ > + uint8_t *p; > + /* TODO: Make this something that isn't fixed size. */ > + p = malloc(len); > + if (p && copy) { > + target_memory_rw_debug(ENV_GET_CPU(env), addr, p, len, 0); > + } > + return p; > +} > +#define lock_user(type, p, len, copy) softmmu_lock_user(env, p, len, copy) > +static inline char *softmmu_lock_user_string(CPUArchState *env, > + target_ulong addr) > +{ > + char *p; > + char *s; > + uint8_t c; > + /* TODO: Make this something that isn't fixed size. */ > + s = p = malloc(1024); > + if (!s) { > + return NULL; > + } > + do { > + target_memory_rw_debug(ENV_GET_CPU(env), addr, &c, 1, 0); > + addr++; > + *(p++) = c; > + } while (c); > + return s; > +} > +#define lock_user_string(p) softmmu_lock_user_string(env, p) > +static inline void softmmu_unlock_user(CPUArchState *env, void *p, > + target_ulong addr, target_ulong len) > +{ > + if (len) { > + target_memory_rw_debug(ENV_GET_CPU(env), addr, p, len, 1); > + } > + free(p); > +} > + > +#define unlock_user(s, args, len) softmmu_unlock_user(env, s, args, len) > + > +#endif So this is basically duplicating exec/softmmu-semi.h and then making some minor tweaks to it, in order to use a custom function instead of tswap64 and tswap32. I think we can do a bit better than that without too much extra effort: have exec/softmmu-semi.h start with /* Some targets need to provide a custom function for byteswapping data * that the semihosting operations access; most can use the default * tswap32 and tswap64, though. */ #ifndef TSWAP64_FN #define TSWAP64_FN tswap64 #endif #ifndef TSWAP32_FN #define TSWAP32_FN tswap32 #endif and then have the ARM header define suitable functions and set the TSWAP32_FN TSWAP64_FN before including softmmu-semi.h. Not the most elegant abstraction in the world but it saves on the copy-n-paste. (make the "s/cpu_memory_rw_debug/target_memory_rw_debug/ in softmmu-semi.h" change its own patch since that's purely mechanical). > diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c > index 7cac873..6b235ad 100644 > --- a/target/arm/arm-semi.c > +++ b/target/arm/arm-semi.c > @@ -114,7 +114,7 @@ static inline uint32_t set_swi_errno(CPUARMState *env, > uint32_t code) > return code; > } > > -#include "exec/softmmu-semi.h" > +#include "exec/softmmu-arm-semi.h" > #endif > > static target_ulong arm_semi_syscall_len; > @@ -187,7 +187,7 @@ static void arm_semi_flen_cb(CPUState *cs, target_ulong > ret, target_ulong err) > /* The size is always stored in big-endian order, extract > the value. We assume the size always fit in 32 bits. */ > uint32_t size; > - cpu_memory_rw_debug(cs, arm_flen_buf(cpu) + 32, (uint8_t *)&size, 4, 0); > + target_memory_rw_debug(cs, arm_flen_buf(cpu) + 32, (uint8_t *)&size, 4, > 0); > size = be32_to_cpu(size); > if (is_a64(env)) { > env->xregs[0] = size; > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index bdf86de..74a2fa1 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -1571,6 +1571,27 @@ static gchar *arm_gdb_arch_name(CPUState *cs) > return g_strdup("arm"); > } > > +#ifndef CONFIG_USER_ONLY > +static int arm_cpu_memory_rw_debug(CPUState *cpu, vaddr address, > + uint8_t *buf, int len, bool is_write) > +{ > + target_ulong addr = address; > + ARMCPU *armcpu = ARM_CPU(cpu); > + CPUARMState *env = &armcpu->env; > + > + if (arm_sctlr_b(env)) { > + target_ulong i; > + for (i = 0; i < len; i++) { > + cpu_memory_rw_debug(cpu, (addr + i) ^ 3, &buf[i], 1, is_write); > + } > + } else { > + cpu_memory_rw_debug(cpu, addr, buf, len, is_write); > + } > + > + return 0; > +} > +#endif > + > static void arm_cpu_class_init(ObjectClass *oc, void *data) > { > ARMCPUClass *acc = ARM_CPU_CLASS(oc); > @@ -1588,6 +1609,9 @@ static void arm_cpu_class_init(ObjectClass *oc, void > *data) > cc->has_work = arm_cpu_has_work; > cc->cpu_exec_interrupt = arm_cpu_exec_interrupt; > cc->dump_state = arm_cpu_dump_state; > +#if !defined(CONFIG_USER_ONLY) > + cc->memory_rw_debug = arm_cpu_memory_rw_debug; > +#endif > cc->set_pc = arm_cpu_set_pc; > cc->gdb_read_register = arm_cpu_gdb_read_register; > cc->gdb_write_register = arm_cpu_gdb_write_register; > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index ac8d625..efd0de5 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -2128,6 +2128,12 @@ static inline bool arm_sctlr_b(CPUARMState *env) > (env->cp15.sctlr_el[1] & SCTLR_B) != 0; > } > > +static inline bool arm_cpsr_e(CPUARMState *env) > +{ > + return arm_feature(env, ARM_FEATURE_V7) && > + (env->uncached_cpsr & CPSR_E) != 0; > +} You don't need to gate this on ARM_FEATURE_V7 -- if the CPU isn't v7 then the E bit should never get set. (Compare the check in arm_cpu_data_is_big_endian()). > + > /* Return true if the processor is in big-endian mode. */ > static inline bool arm_cpu_data_is_big_endian(CPUARMState *env) thanks -- PMM