On Tue, Nov 24, 2015 at 11:44:36AM +1100, Benjamin Herrenschmidt wrote:
> 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.

Ok, that makes sense.

> 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.

Huh, ok.

Some comments referencing the ISA might be useful here.


-- 
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