diff mbox

target-arm: always use user mode registers as operands for load/store multiple

Message ID 1424350536-30397-1-git-send-email-ild@inbox.ru
State New
Headers show

Commit Message

ild@inbox.ru Feb. 19, 2015, 12:55 p.m. UTC
Pseudocode fragment for STM instruction in ARMv8 spec:

if registers<i> == '1' then // Store User mode register
    MemA[address,4] = Rmode[i, M32_User];

Signed-off-by: Ildar Isaev <ild@inbox.ru>
---
 target-arm/translate.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

Comments

Peter Maydell Feb. 19, 2015, 1:11 p.m. UTC | #1
On 19 February 2015 at 21:55, Ildar Isaev <ild@inbox.ru> wrote:
> Pseudocode fragment for STM instruction in ARMv8 spec:
>
> if registers<i> == '1' then // Store User mode register
>     MemA[address,4] = Rmode[i, M32_User];

Again, do you have an example of the code that we mishandle
and what we get wrong?

thanks
-- PMM
Peter Maydell March 10, 2015, 6:35 p.m. UTC | #2
On 19 February 2015 at 12:55, Ildar Isaev <ild@inbox.ru> wrote:
> Pseudocode fragment for STM instruction in ARMv8 spec:
>
> if registers<i> == '1' then // Store User mode register
>     MemA[address,4] = Rmode[i, M32_User];

This pseudocode is specifically for the "user mode" variant
of STM. The code you're changing implements all forms of
STM/LDM and your patch breaks the normal forms of the
instructions...

> Signed-off-by: Ildar Isaev <ild@inbox.ru>
> ---
>  target-arm/translate.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 622aa03..ca0ce3f 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -8907,16 +8907,14 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
>                              /* load */
>                              tmp = tcg_temp_new_i32();
>                              gen_aa32_ld32u(tmp, addr, get_mem_index(s));
> -                            if (user) {
> +                            if (!user && (i == rn)) {
> +                                loaded_var = tmp;
> +                                loaded_base = 1;
> +                            } else {
>                                  tmp2 = tcg_const_i32(i);
>                                  gen_helper_set_user_reg(cpu_env, tmp2, tmp);
>                                  tcg_temp_free_i32(tmp2);
>                                  tcg_temp_free_i32(tmp);
> -                            } else if (i == rn) {
> -                                loaded_var = tmp;
> -                                loaded_base = 1;
> -                            } else {
> -                                store_reg_from_load(s, i, tmp);

This change is clearly wrong. It changes the behaviour for the
case of !user, i != rn from "call store_reg_from_load()" to
"call gen_helper_set_user_reg()", which means that:
 (1) using a normal LDM from privileged mode will write to
     the wrong registers for r13 and r14 (and r8..r12 if we
     are in FIQ mode)
 (2) using a normal LDM from user mode will have no effect
     if it tries to load r13 and r14

Again, what incorrect behaviour are you actually seeing?

thanks
-- PMM
diff mbox

Patch

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 622aa03..ca0ce3f 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -8907,16 +8907,14 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
                             /* load */
                             tmp = tcg_temp_new_i32();
                             gen_aa32_ld32u(tmp, addr, get_mem_index(s));
-                            if (user) {
+                            if (!user && (i == rn)) {
+                                loaded_var = tmp;
+                                loaded_base = 1;
+                            } else {
                                 tmp2 = tcg_const_i32(i);
                                 gen_helper_set_user_reg(cpu_env, tmp2, tmp);
                                 tcg_temp_free_i32(tmp2);
                                 tcg_temp_free_i32(tmp);
-                            } else if (i == rn) {
-                                loaded_var = tmp;
-                                loaded_base = 1;
-                            } else {
-                                store_reg_from_load(s, i, tmp);
                             }
                         } else {
                             /* store */
@@ -8925,13 +8923,11 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
                                 val = (long)s->pc + 4;
                                 tmp = tcg_temp_new_i32();
                                 tcg_gen_movi_i32(tmp, val);
-                            } else if (user) {
+                            } else {
                                 tmp = tcg_temp_new_i32();
                                 tmp2 = tcg_const_i32(i);
                                 gen_helper_get_user_reg(tmp, cpu_env, tmp2);
                                 tcg_temp_free_i32(tmp2);
-                            } else {
-                                tmp = load_reg(s, i);
                             }
                             gen_aa32_st32(tmp, addr, get_mem_index(s));
                             tcg_temp_free_i32(tmp);