[v3] Undefine SWP instruction unless SCTLR.SW bit is set

ARM v7MP deprecates use of SWP instruction and only defines it
if OS explicitly requests it via setting SCTLR.SW bit.
Such a request is expected to occur only once during OS init, thus
only static checking for this bit and flush of all translations
is done on SCTLR change.

Signed-off-by: Alexey Starikovskiy <aystarik@gmail.com>
---
 target-arm/helper.c    |    7 +++++--
 target-arm/translate.c |   10 ++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

On 17 April 2012 13:50, Alexey Starikovskiy <aystarik@gmail.com> wrote:
> ARM v7MP deprecates use of SWP instruction and only defines it
> if OS explicitly requests it via setting SCTLR.SW bit.
> Such a request is expected to occur only once during OS init, thus
> only static checking for this bit and flush of all translations
> is done on SCTLR change.
>
> Signed-off-by: Alexey Starikovskiy <aystarik@gmail.com>
> ---
>  target-arm/helper.c    |    7 +++++--
>  target-arm/translate.c |   10 ++++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 28f127b..2451eba 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1486,11 +1486,14 @@ void HELPER(set_cp15)(CPUARMState *env,
> uint32_t insn, uint32_t val)
>             op2 = 0;
>         switch (op2) {
>         case 0:
> -            if (!arm_feature(env, ARM_FEATURE_XSCALE) || crm == 0)
> +            if (!arm_feature(env, ARM_FEATURE_XSCALE) || crm == 0) {
>                 env->cp15.c1_sys = val;

Adding this brace is incorrectly changing behaviour.
(I agree that it's pretty obscure and halfway to being an
outright bug. It all goes away with the cp15 rework anyway.)

>             /* ??? Lots of these bits are not implemented.  */
>             /* This may enable/disable the MMU, so do a TLB flush.  */
> -            tlb_flush(env, 1);
> +                tlb_flush(env, 1);
> +            /* This may enable/disable SWP instruction, so do TB flush too */
> +                tb_flush(env);
> +            }
>             break;
>         case 1: /* Auxiliary control register.  */
>             if (arm_feature(env, ARM_FEATURE_XSCALE)) {
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 7a3c7d6..4f17fd0 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -7415,6 +7415,16 @@ static void disas_arm_insn(CPUARMState * env,
> DisasContext *s)
>                         }
>                         tcg_temp_free(addr);
>                     } else {
> +/* User mode allows superset of all ARM instructions, thus disable check */

This is not the reason. You can perfectly well run user-mode with
a specific -cpu option. The reason for this ifdef is that the
Linux kernel has SWP emulation code, so SWP has to work either
because we force the implementation here to work, or by making
it take an UNDEF exception and then implementing it inside
linux-user/main.c. Doing the former is slightly simpler.

(I notice that our SWP implementation isn't atomic at all so
it will really not work as required in a multi-threaded user
mode guest program anyway. Oh well.)

-- PMM

> Adding this brace is incorrectly changing behaviour.
> (I agree that it's pretty obscure and halfway to being an
> outright bug. It all goes away with the cp15 rework anyway.)
Should I add tb_flush() without braces too or set second if() only to
avoid changing the bogus behavior?
>> +            /* This may enable/disable SWP instruction, so do TB flush too */
>> +                tb_flush(env);
>> +            }
> This is not the reason. You can perfectly well run user-mode with
> a specific -cpu option. The reason for this ifdef is that the
> Linux kernel has SWP emulation code, so SWP has to work either
> because we force the implementation here to work, or by making
> it take an UNDEF exception and then implementing it inside
> linux-user/main.c. Doing the former is slightly simpler.
Linux kernel _might_ have emulation for SWP depending on config.
So, what should I do? Change comment?

On 17 April 2012 14:11, Alexey Starikovskiy <aystarik@gmail.com> wrote:
>> Adding this brace is incorrectly changing behaviour.
>> (I agree that it's pretty obscure and halfway to being an
>> outright bug. It all goes away with the cp15 rework anyway.)
> Should I add tb_flush() without braces too or set second if() only to
> avoid changing the bogus behavior?

No, tb_flush() is safe so it doesn't need to go in an extra if().

>>> +            /* This may enable/disable SWP instruction, so do TB flush too */
>>> +                tb_flush(env);
>>> +            }
>> This is not the reason. You can perfectly well run user-mode with
>> a specific -cpu option. The reason for this ifdef is that the
>> Linux kernel has SWP emulation code, so SWP has to work either
>> because we force the implementation here to work, or by making
>> it take an UNDEF exception and then implementing it inside
>> linux-user/main.c. Doing the former is slightly simpler.
> Linux kernel _might_ have emulation for SWP depending on config.

Yes, but linux-user generally just supports emulating a "standard
all features enabled" kind of kernel. Feel free to submit a patch
which makes SWP UNDEF here and handles it in the linux-user code
if you think that would be cleaner, though.

> So, what should I do? Change comment?

That's the minimum requirement, yes.

-- PMM