Re: [Qemu-arm] [PATCH v2 56/68] target/arm: Convert T16, Change processor state

[Peter Maydell]

On Mon, 19 Aug 2019 at 22:39, Richard Henderson
<address@hidden> wrote:
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/translate.c | 85 ++++++++++++++++++++----------------------
>  target/arm/t16.decode  | 12 ++++++
>  2 files changed, 52 insertions(+), 45 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 414c562fb3..368f0ab147 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -7474,6 +7474,11 @@ static int negate(DisasContext *s, int x)
>      return -x;
>  }
>
> +static int plus_2(DisasContext *s, int x)
> +{
> +    return x + 2;
> +}
> +
>  static int times_2(DisasContext *s, int x)
>  {
>      return x * 2;
> @@ -10152,6 +10157,9 @@ static bool trans_CPS(DisasContext *s, arg_CPS *a)
>  {
>      uint32_t mask, val;
>
> +    if (ENABLE_ARCH_6 && arm_dc_feature(s, ARM_FEATURE_M)) {
> +        return false;
> +    }

I don't think this condition is quite right. We want to
do two things:
 (1) this is the A/R-profile CPS, so it shouldn't
be decoded for any ARM_FEATURE_M CPU
 (2) for A/R-profile, all the CPS instructions are v6-or-better

(All M-profile CPUs are at v6-or-better, which is why the
legacy decoder gets away with doing its ARCH(6) check
up front rather than only in the A/R-profile arm of its
if statement.)

>      if (IS_USER(s)) {>          /* Implemented as NOP in user mode.  */
>          return true;
> @@ -10182,6 +10190,36 @@ static bool trans_CPS(DisasContext *s, arg_CPS *a)
>      return true;
>  }
>
> +static bool trans_CPS_v6m(DisasContext *s, arg_CPS_v6m *a)
> +{
> +    TCGv_i32 tmp, addr;
> +
> +    if (!(ENABLE_ARCH_6 && arm_dc_feature(s, ARM_FEATURE_M))) {
> +        return false;
> +    }

Similarly, this one need not check ENABLE_ARCH_6.
That is, this is the generic M-profile CPS, it's
not specific to v6M, and FEATURE_M always implies
ARCH_6 anyway. Usually we name M-profile specific
functions _v7m, not _v6m, for mostly historical
reasons relating to our having implemented v7m first,
so maybe we should follow that here. I have made
a bit of an inconsistent hash of this with the v8M
support, where sometimes I use _v8m because the
function is only in v8M and not v7M, and sometimes
_v7m because it's an M-profile function even if it
happens that it only kicks in or is called for
v8M CPUs. But we do not curretly have any functions
with a _v6m suffix so we should probably go with _v7m here.

> +    if (IS_USER(s)) {
> +        /* Implemented as NOP in user mode.  */
> +        return true;
> +    }
> +
> +    tmp = tcg_const_i32(a->im);
> +    /* FAULTMASK */
> +    if (a->F) {
> +        addr = tcg_const_i32(19);
> +        gen_helper_v7m_msr(cpu_env, addr, tmp);
> +        tcg_temp_free_i32(addr);
> +    }
> +    /* PRIMASK */
> +    if (a->I) {
> +        addr = tcg_const_i32(16);
> +        gen_helper_v7m_msr(cpu_env, addr, tmp);
> +        tcg_temp_free_i32(addr);
> +    }
> +    tcg_temp_free_i32(tmp);
> +    gen_lookup_tb(s);
> +    return true;
> +}

thanks
-- PMM