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