[PATCH] target-arm/translate.c: fix movs pc,lr exception return on ARMv7

[PATCH] target-arm/translate.c: fix movs pc,lr exception return on ARMv7

[Alex Bennée]

This was broken by the fix for 9b6a3ea7a699594162ed3d11e4e04b98568dc5c0.
Specifically a movs pc,lr in the kernels ret_fast_syscall returning to
some thumb mode user space code but store_reg unconditionally aligned
the return PC instead of treating the return as an "interworking"
branch.

I suspect we need to audit all calls to store_reg that might involve the
PC to ensure "interworking" branches are correctly handled. Also I'm not
quite sure how the code worked before 9b6a3e as the store_reg path
wouldn't have triggered the store_cpu_field(var, thumb) to set the
processor mode back to thumb.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target-arm/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 5e21b52..373d68a 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -4318,7 +4318,7 @@ static void gen_mrs_banked(DisasContext *s, int r, int sysm, int rn)
 static void gen_exception_return(DisasContext *s, TCGv_i32 pc)
 {
     TCGv_i32 tmp;
-    store_reg(s, 15, pc);
+    store_reg_bx(s, 15, pc);
     tmp = load_cpu_field(spsr);
     gen_helper_cpsr_write_eret(cpu_env, tmp);
     tcg_temp_free_i32(tmp);
-- 
2.9.3

[Qemu-devel] [PATCH] target-arm/translate.c: fix movs pc, lr exception return on ARMv7

[Alex Bennée]

This was broken by the fix for 9b6a3ea7a699594162ed3d11e4e04b98568dc5c0.
Specifically a movs pc,lr in the kernels ret_fast_syscall returning to
some thumb mode user space code but store_reg unconditionally aligned
the return PC instead of treating the return as an "interworking"
branch.

I suspect we need to audit all calls to store_reg that might involve the
PC to ensure "interworking" branches are correctly handled. Also I'm not
quite sure how the code worked before 9b6a3e as the store_reg path
wouldn't have triggered the store_cpu_field(var, thumb) to set the
processor mode back to thumb.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target-arm/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 5e21b52..373d68a 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -4318,7 +4318,7 @@ static void gen_mrs_banked(DisasContext *s, int r, int sysm, int rn)
 static void gen_exception_return(DisasContext *s, TCGv_i32 pc)
 {
     TCGv_i32 tmp;
-    store_reg(s, 15, pc);
+    store_reg_bx(s, 15, pc);
     tmp = load_cpu_field(spsr);
     gen_helper_cpsr_write_eret(cpu_env, tmp);
     tcg_temp_free_i32(tmp);
-- 
2.9.3

Re: [PATCH] target-arm/translate.c: fix movs pc,lr exception return on ARMv7

[Peter Maydell]

On 14 October 2016 at 16:13, Alex Bennée <alex.bennee@linaro.org> wrote:
> This was broken by the fix for 9b6a3ea7a699594162ed3d11e4e04b98568dc5c0.
> Specifically a movs pc,lr in the kernels ret_fast_syscall returning to
> some thumb mode user space code but store_reg unconditionally aligned
> the return PC instead of treating the return as an "interworking"
> branch.
>
> I suspect we need to audit all calls to store_reg that might involve the
> PC to ensure "interworking" branches are correctly handled. Also I'm not
> quite sure how the code worked before 9b6a3e as the store_reg path
> wouldn't have triggered the store_cpu_field(var, thumb) to set the
> processor mode back to thumb.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

I think this is the wrong fix to the problem -- see the
patch I sent a few days back.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm/translate.c: fix movs pc, lr exception return on ARMv7

[Peter Maydell]

On 14 October 2016 at 16:13, Alex Bennée <alex.bennee@linaro.org> wrote:
> This was broken by the fix for 9b6a3ea7a699594162ed3d11e4e04b98568dc5c0.
> Specifically a movs pc,lr in the kernels ret_fast_syscall returning to
> some thumb mode user space code but store_reg unconditionally aligned
> the return PC instead of treating the return as an "interworking"
> branch.
>
> I suspect we need to audit all calls to store_reg that might involve the
> PC to ensure "interworking" branches are correctly handled. Also I'm not
> quite sure how the code worked before 9b6a3e as the store_reg path
> wouldn't have triggered the store_cpu_field(var, thumb) to set the
> processor mode back to thumb.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

I think this is the wrong fix to the problem -- see the
patch I sent a few days back.

thanks
-- PMM

Re: [PATCH] target-arm/translate.c: fix movs pc,lr exception return on ARMv7

[Peter Maydell]

On 14 October 2016 at 16:13, Alex Bennée <alex.bennee@linaro.org> wrote:
> I suspect we need to audit all calls to store_reg that might involve the
> PC to ensure "interworking" branches are correctly handled. Also I'm not
> quite sure how the code worked before 9b6a3e as the store_reg path
> wouldn't have triggered the store_cpu_field(var, thumb) to set the
> processor mode back to thumb.

The answer to this question, incidentally, is that
the thumb bit is in the SPSR we're restoring, not in
the low bit of the PC value, and it gets written by
gen_helper_cpsr_write_eret().

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm/translate.c: fix movs pc, lr exception return on ARMv7

[Peter Maydell]

On 14 October 2016 at 16:13, Alex Bennée <alex.bennee@linaro.org> wrote:
> I suspect we need to audit all calls to store_reg that might involve the
> PC to ensure "interworking" branches are correctly handled. Also I'm not
> quite sure how the code worked before 9b6a3e as the store_reg path
> wouldn't have triggered the store_cpu_field(var, thumb) to set the
> processor mode back to thumb.

The answer to this question, incidentally, is that
the thumb bit is in the SPSR we're restoring, not in
the low bit of the PC value, and it gets written by
gen_helper_cpsr_write_eret().

thanks
-- PMM

Re: [PATCH] target-arm/translate.c: fix movs pc,lr exception return on ARMv7

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 14 October 2016 at 16:13, Alex Bennée <alex.bennee@linaro.org> wrote:
>> This was broken by the fix for 9b6a3ea7a699594162ed3d11e4e04b98568dc5c0.
>> Specifically a movs pc,lr in the kernels ret_fast_syscall returning to
>> some thumb mode user space code but store_reg unconditionally aligned
>> the return PC instead of treating the return as an "interworking"
>> branch.
>>
>> I suspect we need to audit all calls to store_reg that might involve the
>> PC to ensure "interworking" branches are correctly handled. Also I'm not
>> quite sure how the code worked before 9b6a3e as the store_reg path
>> wouldn't have triggered the store_cpu_field(var, thumb) to set the
>> processor mode back to thumb.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> I think this is the wrong fix to the problem -- see the
> patch I sent a few days back.

Well at least my analysis of the problem was correct even if the
solution was too hacky. Your patch is obviously the better solution ;-)

For ref:

  [PATCH] Fix masking of PC lower bits when doing exception returns

>
> thanks
> -- PMM


--
Alex Bennée

Re: [Qemu-devel] [PATCH] target-arm/translate.c: fix movs pc, lr exception return on ARMv7

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 14 October 2016 at 16:13, Alex Bennée <alex.bennee@linaro.org> wrote:
>> This was broken by the fix for 9b6a3ea7a699594162ed3d11e4e04b98568dc5c0.
>> Specifically a movs pc,lr in the kernels ret_fast_syscall returning to
>> some thumb mode user space code but store_reg unconditionally aligned
>> the return PC instead of treating the return as an "interworking"
>> branch.
>>
>> I suspect we need to audit all calls to store_reg that might involve the
>> PC to ensure "interworking" branches are correctly handled. Also I'm not
>> quite sure how the code worked before 9b6a3e as the store_reg path
>> wouldn't have triggered the store_cpu_field(var, thumb) to set the
>> processor mode back to thumb.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> I think this is the wrong fix to the problem -- see the
> patch I sent a few days back.

Well at least my analysis of the problem was correct even if the
solution was too hacky. Your patch is obviously the better solution ;-)

For ref:

  [PATCH] Fix masking of PC lower bits when doing exception returns

>
> thanks
> -- PMM


--
Alex Bennée