Re: [Qemu-ppc] [PATCH 21/77] ppc: Rework generation of priv and inval interrupts

[David Gibson]

On Wed, Nov 11, 2015 at 11:27:34AM +1100, Benjamin Herrenschmidt wrote:
> Recent server processors use the Hypervisor Emulation Assistance
> interrupt for illegal instructions and *some* type of SPR accesses.
> 
> Also the code was always generating inval instructions even for priv
> violations due to setting the wrong flags
> 
> Finally, the checking for PR/HV was open coded everywhere.
> 
> This reworks it all, using little helper macros for checking, and
> adding the HV interrupt (which gets converted back to program check
> in the slow path of excp_helper.c on CPUs that don't want it).
> 
> Signed-off-by: Benjamin Herrenschmidt <address@hidden>

[snip]
>  static void spr_noaccess(DisasContext *ctx, int gprn, int sprn)
> @@ -4340,7 +4350,7 @@ static inline void gen_op_mfspr(DisasContext *ctx)
>                  printf("Trying to read privileged spr %d (0x%03x) at "
>                         TARGET_FMT_lx "\n", sprn, sprn, ctx->nip - 4);
>              }
> -            gen_inval_exception(ctx, POWERPC_EXCP_PRIV_REG);
> +            gen_priv_exception(ctx, POWERPC_EXCP_PRIV_REG);
>          }
>      } else {
>          /* Not defined */
> @@ -4348,7 +4358,25 @@ static inline void gen_op_mfspr(DisasContext *ctx)
>                   TARGET_FMT_lx "\n", sprn, sprn, ctx->nip - 4);
>          printf("Trying to read invalid spr %d (0x%03x) at "
>                 TARGET_FMT_lx "\n", sprn, sprn, ctx->nip - 4);

So, I'm not 100% following the logic below, but it looks like the
existing code used SPR_NOACCESS to mark things which generated a
privilege exception compared to NULL for things which generated an
invalid instruction exception.  Using that encoding, can you simplify
the logic here?  Alternatively can you use the logic here to avoid the
SPR_NOACESS encoding?

> -        gen_inval_exception(ctx, POWERPC_EXCP_INVAL_SPR);
> +
> +        /* The behaviour depends on MSR:PR and SPR# bit 0x10,
> +         * it can generate a priv, a hv emu or a no-op
> +         */
> +        if (sprn & 0x10) {
> +            if (ctx->pr) {
> +                gen_priv_exception(ctx, POWERPC_EXCP_INVAL_SPR);
> +            }
> +        } else {
> +            if (ctx->pr || sprn == 0 || sprn == 4 || sprn == 5 || sprn == 6) 
> {
> +                gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR);
> +            }
> +        }
> +#if !defined(CONFIG_USER_ONLY)
> +        /* HV priv */
> +        if (ctx->spr_cb[sprn].hea_read) {
> +            gen_priv_exception(ctx, POWERPC_EXCP_INVAL_SPR);
> +        }

If you're in PR mode, and it's an SPR with an hea_read function and
has the 0x10 bit set, won't this call gen_priv_exception twice?

I also see no path here which will call gen_inval_exception(), is that
right?  If you're in HV mode and it's a truly invalid SPRN, isn't that
what you'd want?

> +#endif
>      }
>  }



>  
> @@ -4395,13 +4423,9 @@ static void gen_mtcrf(DisasContext *ctx)
>  #if defined(TARGET_PPC64)
>  static void gen_mtmsrd(DisasContext *ctx)
>  {
> -#if defined(CONFIG_USER_ONLY)
> -    gen_inval_exception(ctx, POWERPC_EXCP_PRIV_REG);
> -#else
> -    if (unlikely(ctx->pr)) {
> -        gen_inval_exception(ctx, POWERPC_EXCP_PRIV_REG);
> -        return;
> -    }
> +    CHK_SV;
> +
> +#if !defined(CONFIG_USER_ONLY)
>      if (ctx->opcode & 0x00010000) {
>          /* Special form that does not need any synchronisation */
>          TCGv t0 = tcg_temp_new();
> @@ -4420,20 +4444,16 @@ static void gen_mtmsrd(DisasContext *ctx)
>          /* Note that mtmsr is not always defined as context-synchronizing */
>          gen_stop_exception(ctx);
>      }
> -#endif
> +#endif /* !defined(CONFIG_USER_ONLY) */
>  }
> -#endif
> +#endif /* defined(TARGET_PPC64) */
>  
>  static void gen_mtmsr(DisasContext *ctx)
>  {
> -#if defined(CONFIG_USER_ONLY)
> -    gen_inval_exception(ctx, POWERPC_EXCP_PRIV_REG);
> -#else
> -    if (unlikely(ctx->pr)) {
> -        gen_inval_exception(ctx, POWERPC_EXCP_PRIV_REG);
> -        return;
> -    }
> -    if (ctx->opcode & 0x00010000) {
> +    CHK_SV;
> +
> +#if !defined(CONFIG_USER_ONLY)
> +   if (ctx->opcode & 0x00010000) {
>          /* Special form that does not need any synchronisation */
>          TCGv t0 = tcg_temp_new();
>          tcg_gen_andi_tl(t0, cpu_gpr[rS(ctx->opcode)], (1 << MSR_RI) | (1 << 
> MSR_EE));
> @@ -4488,7 +4508,7 @@ static void gen_mtspr(DisasContext *ctx)
>                       TARGET_FMT_lx "\n", sprn, sprn, ctx->nip - 4);
>              printf("Trying to write privileged spr %d (0x%03x) at "
>                     TARGET_FMT_lx "\n", sprn, sprn, ctx->nip - 4);
> -            gen_inval_exception(ctx, POWERPC_EXCP_PRIV_REG);
> +            gen_priv_exception(ctx, POWERPC_EXCP_PRIV_REG);
>          }
>      } else {
>          /* Not defined */
> @@ -4496,7 +4516,25 @@ static void gen_mtspr(DisasContext *ctx)
>                   TARGET_FMT_lx "\n", sprn, sprn, ctx->nip - 4);
>          printf("Trying to write invalid spr %d (0x%03x) at "
>                 TARGET_FMT_lx "\n", sprn, sprn, ctx->nip - 4);
> -        gen_inval_exception(ctx, POWERPC_EXCP_INVAL_SPR);
> +
> +        /* The behaviour depends on MSR:PR and SPR# bit 0x10,
> +         * it can generate a priv, a hv emu or a no-op
> +         */
> +        if (sprn & 0x10) {
> +            if (ctx->pr) {
> +                gen_priv_exception(ctx, POWERPC_EXCP_INVAL_SPR);
> +            }
> +        } else {
> +            if (ctx->pr || sprn == 0) {
> +                gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR);
> +            }
> +        }
> +#if !defined(CONFIG_USER_ONLY)
> +        /* HV priv */
> +        if (ctx->spr_cb[sprn].hea_write) {
> +            gen_priv_exception(ctx, POWERPC_EXCP_INVAL_SPR);
> +        }
> +#endif

Same concerns here as for mfspr.

[snip]
>  /* tlbiel */
>  static void gen_tlbiel(DisasContext *ctx)
>  {
>  #if defined(CONFIG_USER_ONLY)
> -    gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> +    GEN_PRIV;
>  #else
> -    if (unlikely(ctx->pr || !ctx->hv)) {
> -        gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> -        return;
> -    }
> +    CHK_SV;

You have CHK_SV here, but the original code checks for HV, as does
your new code for tlbia and tlbiel, is that right?

[snip]
>  /* tlbsync */
>  static void gen_tlbsync(DisasContext *ctx)
>  {
>  #if defined(CONFIG_USER_ONLY)
> -    gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> -#else
> -    if (unlikely(ctx->pr)) {
> -        gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> -        return;
> -    }
> +    GEN_PRIV;
> +#else    
> +    CHK_HV;
> +

Old code didn't check for HV, mode, but AFAICT it should have, so this
looks correct.

[snip]
> @@ -5941,18 +5921,16 @@ static void gen_mfapidi(DisasContext *ctx)
>  static void gen_tlbiva(DisasContext *ctx)
>  {
>  #if defined(CONFIG_USER_ONLY)
> -    gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> +    GEN_PRIV;
>  #else
>      TCGv t0;
> -    if (unlikely(ctx->pr)) {
> -        gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> -        return;
> -    }
> +
> +    CHK_SV;

Is the same thing as tlbivax, or some ancient instruction?  AFAICT the
ISA says tlbivax is hypervisor privileged.

>      t0 = tcg_temp_new();
>      gen_addr_reg_index(ctx, t0);
>      gen_helper_tlbie(cpu_env, cpu_gpr[rB(ctx->opcode)]);
>      tcg_temp_free(t0);
> -#endif
> +#endif /* defined(CONFIG_USER_ONLY) */
>  }

[snip]
>  static void gen_tlbivax_booke206(DisasContext *ctx)
>  {
>  #if defined(CONFIG_USER_ONLY)
> -    gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> +    GEN_PRIV;
>  #else
>      TCGv t0;
> -    if (unlikely(ctx->pr)) {
> -        gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> -        return;
> -    }
>  
> +    CHK_SV;

ISA says tlbivax is hypervisor privileged when the CPU has a
hypervisor mode, which I guess booke206 probably doesn't?

>      t0 = tcg_temp_new();
>      gen_addr_reg_index(ctx, t0);
> -
>      gen_helper_booke206_tlbivax(cpu_env, t0);
>      tcg_temp_free(t0);
> -#endif
> +#endif /* defined(CONFIG_USER_ONLY) */
>  }
>  
>  static void gen_tlbilx_booke206(DisasContext *ctx)
>  {
>  #if defined(CONFIG_USER_ONLY)
> -    gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> +    GEN_PRIV;
>  #else
>      TCGv t0;
> -    if (unlikely(ctx->pr)) {
> -        gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> -        return;
> -    }
>  
> +    CHK_SV;

And apparently hv vs. sv privilege of tlbilx depends on the EPCR
register.  Again, may not be relevant for 2.06.

>      t0 = tcg_temp_new();
>      gen_addr_reg_index(ctx, t0);
>  
> @@ -6672,7 +6574,7 @@ static void gen_tlbilx_booke206(DisasContext *ctx)
>      }
>  
>      tcg_temp_free(t0);
> -#endif
> +#endif /* defined(CONFIG_USER_ONLY) */
>  }

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature