diff mbox

[v7,16/26] tcg/arm: Clarify tcg_out_bx for arm4 host

Message ID 20170526211638.32301-17-rth@twiddle.net (mailing list archive)
State New, archived
Headers show

Commit Message

Richard Henderson May 26, 2017, 9:16 p.m. UTC
In theory this would re-enable usage of QEMU on an armv4 host.
Whether this is worthwhile is debatable -- we've been unconditionally
issuing the armv5t BX instruction in the prologue since 2011 without
complaint.  Possibly we should simply require an armv6 host.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 tcg/arm/tcg-target.inc.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

Comments

Philippe Mathieu-Daudé May 27, 2017, 4:41 p.m. UTC | #1
On 05/26/2017 06:16 PM, Richard Henderson wrote:
> In theory this would re-enable usage of QEMU on an armv4 host.
> Whether this is worthwhile is debatable -- we've been unconditionally
> issuing the armv5t BX instruction in the prologue since 2011 without
> complaint.  Possibly we should simply require an armv6 host.
 >

I remember having some issue 2 years ago trying to run on a ARM926EJ-S 
but the gcc/newlib revisions were good candidates to give problem so I 
didn't go further. Few NAS still run on ARMv4 cpu (some StrongARM).
Anyway on boards running ARMv4 cpu, problems are more likely to come 
from available memory to run an OS and QEMU rather than the TCG codebase.

> Signed-off-by: Richard Henderson <rth@twiddle.net>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tcg/arm/tcg-target.inc.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c
> index e75a6d4..590c57d 100644
> --- a/tcg/arm/tcg-target.inc.c
> +++ b/tcg/arm/tcg-target.inc.c
> @@ -329,11 +329,6 @@ static const uint8_t tcg_cond_to_arm_cond[] = {
>      [TCG_COND_GTU] = COND_HI,
>  };
>
> -static inline void tcg_out_bx(TCGContext *s, int cond, int rn)
> -{
> -    tcg_out32(s, (cond << 28) | 0x012fff10 | rn);
> -}
> -
>  static inline void tcg_out_b(TCGContext *s, int cond, int32_t offset)
>  {
>      tcg_out32(s, (cond << 28) | 0x0a000000 |
> @@ -402,6 +397,18 @@ static inline void tcg_out_mov_reg(TCGContext *s, int cond, int rd, int rm)
>      }
>  }
>
> +static inline void tcg_out_bx(TCGContext *s, int cond, TCGReg rn)
> +{
> +    /* Unless the C portion of QEMU is compiled as thumb, we don't
> +       actually need true BX semantics; merely a branch to an address
> +       held in a register.  */
> +    if (use_armv5t_instructions) {
> +        tcg_out32(s, (cond << 28) | 0x012fff10 | rn);
> +    } else {
> +        tcg_out_mov_reg(s, cond, TCG_REG_PC, rn);
> +    }
> +}
> +
>  static inline void tcg_out_dat_imm(TCGContext *s,
>                  int cond, int opc, int rd, int rn, int im)
>  {
> @@ -977,7 +984,7 @@ static inline void tcg_out_st8(TCGContext *s, int cond,
>   * with the code buffer limited to 16MB we wouldn't need the long case.
>   * But we also use it for the tail-call to the qemu_ld/st helpers, which does.
>   */
> -static inline void tcg_out_goto(TCGContext *s, int cond, tcg_insn_unit *addr)
> +static void tcg_out_goto(TCGContext *s, int cond, tcg_insn_unit *addr)
>  {
>      intptr_t addri = (intptr_t)addr;
>      ptrdiff_t disp = tcg_pcrel_diff(s, addr);
> @@ -987,15 +994,9 @@ static inline void tcg_out_goto(TCGContext *s, int cond, tcg_insn_unit *addr)
>          return;
>      }
>
> +    assert(use_armv5t_instructions || (addri & 1) == 0);
>      tcg_out_movi32(s, cond, TCG_REG_TMP, addri);
> -    if (use_armv5t_instructions) {
> -        tcg_out_bx(s, cond, TCG_REG_TMP);
> -    } else {
> -        if (addri & 1) {
> -            tcg_abort();
> -        }
> -        tcg_out_mov_reg(s, cond, TCG_REG_PC, TCG_REG_TMP);
> -    }
> +    tcg_out_bx(s, cond, TCG_REG_TMP);
>  }
>
>  /* The call case is mostly used for helpers - so it's not unreasonable
>
diff mbox

Patch

diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c
index e75a6d4..590c57d 100644
--- a/tcg/arm/tcg-target.inc.c
+++ b/tcg/arm/tcg-target.inc.c
@@ -329,11 +329,6 @@  static const uint8_t tcg_cond_to_arm_cond[] = {
     [TCG_COND_GTU] = COND_HI,
 };
 
-static inline void tcg_out_bx(TCGContext *s, int cond, int rn)
-{
-    tcg_out32(s, (cond << 28) | 0x012fff10 | rn);
-}
-
 static inline void tcg_out_b(TCGContext *s, int cond, int32_t offset)
 {
     tcg_out32(s, (cond << 28) | 0x0a000000 |
@@ -402,6 +397,18 @@  static inline void tcg_out_mov_reg(TCGContext *s, int cond, int rd, int rm)
     }
 }
 
+static inline void tcg_out_bx(TCGContext *s, int cond, TCGReg rn)
+{
+    /* Unless the C portion of QEMU is compiled as thumb, we don't
+       actually need true BX semantics; merely a branch to an address
+       held in a register.  */
+    if (use_armv5t_instructions) {
+        tcg_out32(s, (cond << 28) | 0x012fff10 | rn);
+    } else {
+        tcg_out_mov_reg(s, cond, TCG_REG_PC, rn);
+    }
+}
+
 static inline void tcg_out_dat_imm(TCGContext *s,
                 int cond, int opc, int rd, int rn, int im)
 {
@@ -977,7 +984,7 @@  static inline void tcg_out_st8(TCGContext *s, int cond,
  * with the code buffer limited to 16MB we wouldn't need the long case.
  * But we also use it for the tail-call to the qemu_ld/st helpers, which does.
  */
-static inline void tcg_out_goto(TCGContext *s, int cond, tcg_insn_unit *addr)
+static void tcg_out_goto(TCGContext *s, int cond, tcg_insn_unit *addr)
 {
     intptr_t addri = (intptr_t)addr;
     ptrdiff_t disp = tcg_pcrel_diff(s, addr);
@@ -987,15 +994,9 @@  static inline void tcg_out_goto(TCGContext *s, int cond, tcg_insn_unit *addr)
         return;
     }
 
+    assert(use_armv5t_instructions || (addri & 1) == 0);
     tcg_out_movi32(s, cond, TCG_REG_TMP, addri);
-    if (use_armv5t_instructions) {
-        tcg_out_bx(s, cond, TCG_REG_TMP);
-    } else {
-        if (addri & 1) {
-            tcg_abort();
-        }
-        tcg_out_mov_reg(s, cond, TCG_REG_PC, TCG_REG_TMP);
-    }
+    tcg_out_bx(s, cond, TCG_REG_TMP);
 }
 
 /* The call case is mostly used for helpers - so it's not unreasonable