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
