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
signature.asc
Description: PGP signature
