Patchwork Undefine SWP instruction unless SCTLR.SW bit is set

login
register
mail settings
Submitter Alexey Starikovskiy
Date April 17, 2012, 11:39 a.m.
Message ID <4F8D5664.7000304@gmail.com>
Download mbox | patch
Permalink /patch/153137/
State New
Headers show

Comments

Alexey Starikovskiy - April 17, 2012, 11:39 a.m.
Signed-off-by: Alexey Starikovskiy <aystarik@gmail.com>
---
  target-arm/translate.c |    6 ++++++
  1 files changed, 6 insertions(+), 0 deletions(-)
Peter Maydell - April 17, 2012, 11:55 a.m.
On 17 April 2012 12:39, Alexey Starikovskiy <aystarik@gmail.com> wrote:

Patches should almost always have more than a single line
commit message in my opinion.

> Signed-off-by: Alexey Starikovskiy <aystarik@gmail.com>
> ---
>  target-arm/translate.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 7a3c7d6..b35c85f 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -7415,6 +7415,12 @@ static void disas_arm_insn(CPUARMState * env,
> DisasContext *s)
>                         }
>                         tcg_temp_free(addr);
>                     } else {
> +                        if (!(env->cp15.c1_sys & (1 << 10))) {

This is wrong -- on CPUs without the Multiprocessing Extensions
SCTLR.SW is always zero but the SWP instructions work.

> +                            /* Check if SCTLR.SW is set. Any change to SCTLR
> +                             * invalidates all translations, so we are safe.
> +                             */

This isn't true -- SCTLR writes currently do a tlb_flush() but
not a tb_flush().

-- PMM
Alexey Starikovskiy - April 17, 2012, 12:03 p.m.
On 04/17/2012 03:55 PM, Peter Maydell wrote:
> On 17 April 2012 12:39, Alexey Starikovskiy<aystarik@gmail.com>  wrote:
>
> Patches should almost always have more than a single line
> commit message in my opinion.
>
Ok
>> Signed-off-by: Alexey Starikovskiy<aystarik@gmail.com>
>> ---
>>   target-arm/translate.c |    6 ++++++
>>   1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>> index 7a3c7d6..b35c85f 100644
>> --- a/target-arm/translate.c
>> +++ b/target-arm/translate.c
>> @@ -7415,6 +7415,12 @@ static void disas_arm_insn(CPUARMState * env,
>> DisasContext *s)
>>                          }
>>                          tcg_temp_free(addr);
>>                      } else {
>> +                        if (!(env->cp15.c1_sys&  (1<<  10))) {
> This is wrong -- on CPUs without the Multiprocessing Extensions
> SCTLR.SW is always zero but the SWP instructions work.
You mean ARM_FEATURE_V7MP or something else?
>> +                            /* Check if SCTLR.SW is set. Any change to SCTLR
>> +                             * invalidates all translations, so we are safe.
>> +                             */
> This isn't true -- SCTLR writes currently do a tlb_flush() but
> not a tb_flush().
Right...
> -- PMM
Peter Maydell - April 17, 2012, 12:08 p.m.
On 17 April 2012 13:03, Alexey Starikovskiy <aystarik@gmail.com> wrote:
> On 04/17/2012 03:55 PM, Peter Maydell wrote:
>>
>> On 17 April 2012 12:39, Alexey Starikovskiy<aystarik@gmail.com>  wrote:
>>
>> Patches should almost always have more than a single line
>> commit message in my opinion.
>>
> Ok
>>>
>>> Signed-off-by: Alexey Starikovskiy<aystarik@gmail.com>
>>> ---
>>>  target-arm/translate.c |    6 ++++++
>>>  1 files changed, 6 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>>> index 7a3c7d6..b35c85f 100644
>>> --- a/target-arm/translate.c
>>> +++ b/target-arm/translate.c
>>> @@ -7415,6 +7415,12 @@ static void disas_arm_insn(CPUARMState * env,
>>> DisasContext *s)
>>>                         }
>>>                         tcg_temp_free(addr);
>>>                     } else {
>>> +                        if (!(env->cp15.c1_sys&  (1<<  10))) {
>>
>> This is wrong -- on CPUs without the Multiprocessing Extensions
>> SCTLR.SW is always zero but the SWP instructions work.
>
> You mean ARM_FEATURE_V7MP or something else?

Yes, ARM_FEATURE_V7MP is the bit that tracks whether the CPU
implements the v7 multiprocessing extensions.

Also you'll need to watch out that you don't break handling
of SWP in linux-user mode. Forcing SCTLR.SW to 1 on reset on
v7MP CPUs when CONFIG_USER_ONLY is defined is probably the best
thing, but you'll find that changes there clash with a patchset
I currently have out on the list.

-- PMM

Patch

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 7a3c7d6..b35c85f 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -7415,6 +7415,12 @@  static void disas_arm_insn(CPUARMState * env, DisasContext *s)
                          }
                          tcg_temp_free(addr);
                      } else {
+                        if (!(env->cp15.c1_sys & (1 << 10))) {
+                            /* Check if SCTLR.SW is set. Any change to SCTLR
+                             * invalidates all translations, so we are safe.
+                             */
+                            goto illegal_op;
+                        }
                          /* SWP instruction */
                          rm = (insn) & 0xf;