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

[Benjamin Herrenschmidt]

On Fri, 2015-11-20 at 18:45 +1100, David Gibson wrote:
> 
> 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?

Well, so the SPR_NOACCESS has to do with how you react to a known SPR
who has explicit access permissions. The logic below is described in
the ISA for an unknown SPR number.

I don't know whether the access permission of "known" SPRs always
honor the 0x10 bit trick, and changing that in qemu would be a
fairly large patch. So I'd rather stick to the logic here for
"unknown" SPRs which matches the ISA definition.

I'll update the patch though for arch 2.07 as it defines a few
reserved SPRs as no-ops.

However:

> > -        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);
> > +        }

That latest bit is bogus.

> 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?

Yes, I've removed it. It should be handled by the SPR_NOACCESS.

> 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?

No, the ISA says it's a nop.

Cheers,
Ben.

> > +#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) */
> >  }
>