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

[David Gibson]

On Tue, Nov 24, 2015 at 11:51:44AM +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2015-11-20 at 18:45 +1100, David Gibson wrote:
> > 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?
> 
> Yes. tlbiel is supervisor accessible (for weird reasons).
> 
> > [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.
> 
> Yes, this is a hypervisor instruction.
> 
> > [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.
> 
> "tlbiva" is the 4xx variant, there is no hypervisor mode on these
> things.
> 
> > >      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?
> 
> Right so here, the "problem" is that afaik, TCG doesn't implement
> the BookE hypervisor mode. So with my limited BookE testing
> ability I prefer sticking to a mechanical replacement that matches
> the existing code. It can be fixed later if necessary.

Fair enough.

> > >      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.
> 
> Well, here too, I basically preserve existing BookE TCG behaviour,
> whether it's correct or not. That can be fixed separately if somebody
> cares about BookE HV mode.
> 
> > >      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