Re: [Qemu-devel] [PATCH v3] Undefine SWP instruction unless SCTLR.SW bit is set

[Peter Maydell]

On 17 April 2012 13:50, Alexey Starikovskiy <address@hidden> 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 <address@hidden>
> ---
>  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