qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Benjamin Herrenschmidt
Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 21/77] ppc: Rework generation of priv and inval interrupts
Date: Tue, 24 Nov 2015 11:51:44 +1100

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.

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]