[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH 21/77] ppc: Rework generation of priv and inval in
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCH 21/77] ppc: Rework generation of priv and inval interrupts |
Date: |
Tue, 24 Nov 2015 13:22:14 +1100 |
User-agent: |
Mutt/1.5.23 (2015-06-09) |
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
- [Qemu-ppc] [PATCH 17/77] ppc: Add PPC_64H instruction flag to POWER7 and POWER8, (continued)
[Qemu-ppc] [PATCH 28/77] ppc/xics: Rename existing XICS classe to XICS_SPAPR, Benjamin Herrenschmidt, 2015/11/10
[Qemu-ppc] [PATCH 25/77] ppc: Add P7/P8 Power Management instructions, Benjamin Herrenschmidt, 2015/11/10
[Qemu-ppc] [PATCH 30/77] ppc/xics: Implement H_IPOLL using an accessor, Benjamin Herrenschmidt, 2015/11/10
[Qemu-ppc] [PATCH 31/77] ppc/xics: Remove unused xics_set_irq_type(), Benjamin Herrenschmidt, 2015/11/10
[Qemu-ppc] [PATCH 29/77] ppc/xics: Move SPAPR specific code to a separate file, Benjamin Herrenschmidt, 2015/11/10