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