[Qemu-devel] target-ppc: SPR_BOOKE_ESR not set on FP exceptions

[Qemu-devel] target-ppc: SPR_BOOKE_ESR not set on FP exceptions

[alarson]

The target-ppc/excp_helper.c:powerpc_excp() case POWERPC_EXCP_FP fails
to set "env->spr[SPR_BOOKE_ESR] = ESR_FP;".  I can submit a patch for
that, or anyone can add it, but I notice that in the other cases where
SPR_BOOKE_ESR is set, the "msr" is ALSO set.  Since the "msr" is used
to initialize SRR1, there is a possibility of inadvertently enabling
BookE MSR bits indirectly.  Given that this code is not performance
sensitive, I think it would be safer to set EITHER msr or the ESR, but
not BOTH.  For example:

            if (excp_model == POWERPC_EXCP_BOOKE)
                env->spr[SPR_BOOKE_ESR] = ESR_FP;
            else
                msr |= 0x00100000;

I did a quick check of the bits set in the POWERPC_EXCP_PROGRAM case.
The classic PPC sets SRR1 bits 11--15 depending on the exception.  In
Book E these correspond to bits 43--47, of which (according to my
EREF) only 45 and 46 are currently defined.  BookE MSR bits 45 (Wait
state enable) and 46 (Critical Enable) correspond to classic SRR1 bits
13 (exception is TRAP) and 14 ("SRR0 is not faulting instruction").
If I understand the current code, given this aliasing then when a TRAP
exception occurs on a book E processor it will effectively enable wait
state, and an FP exception (which sets bit 14/46) will set "Critical
Enable".  I'm not sure that either of these features is currently
implemented so this may not have a downstream effect, but never the
less it seems incorrect.

I can submit a patch for the ESR_FP, and/or a change to have the
"either or but not both" settings of MSR and ESR.  Please let me know
which you'd prefer.

Re: [Qemu-devel] target-ppc: SPR_BOOKE_ESR not set on FP exceptions

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2488 bytes --]

On Thu, Jul 28, 2016 at 06:32:27PM -0500, alarson@ddci.com wrote:
> The target-ppc/excp_helper.c:powerpc_excp() case POWERPC_EXCP_FP fails
> to set "env->spr[SPR_BOOKE_ESR] = ESR_FP;".  I can submit a patch for
> that,

Ok, please do.

> or anyone can add it, but I notice that in the other cases where
> SPR_BOOKE_ESR is set, the "msr" is ALSO set.  Since the "msr" is used
> to initialize SRR1, there is a possibility of inadvertently enabling
> BookE MSR bits indirectly.

> Given that this code is not performance
> sensitive, I think it would be safer to set EITHER msr or the ESR, but
> not BOTH.  For example:
> 
>             if (excp_model == POWERPC_EXCP_BOOKE)
>                 env->spr[SPR_BOOKE_ESR] = ESR_FP;
>             else
>                 msr |= 0x00100000;

That does seem sensible, assuming those srr1 bits are no correct for
BookE, which they're not IIRC (ESR takes their place).

> I did a quick check of the bits set in the POWERPC_EXCP_PROGRAM case.
> The classic PPC sets SRR1 bits 11--15 depending on the exception.  In
> Book E these correspond to bits 43--47,

Um.. what?  I'm not understanding where this bit shift is coming
from.  The exception code is setting MSR directly, so it should be the
same bit numbers, although they might have a different meaning on
BookE to BookS.

> of which (according to my
> EREF) only 45 and 46 are currently defined.  BookE MSR bits 45 (Wait
> state enable) and 46 (Critical Enable) correspond to classic SRR1 bits
> 13 (exception is TRAP) and 14 ("SRR0 is not faulting instruction").
> If I understand the current code, given this aliasing then when a TRAP
> exception occurs on a book E processor it will effectively enable wait
> state, and an FP exception (which sets bit 14/46) will set "Critical
> Enable".  I'm not sure that either of these features is currently
> implemented so this may not have a downstream effect, but never the
> less it seems incorrect.

I'm not sure about your analysis of which bits are affect, but yes
this definitely does seem wrong.

> I can submit a patch for the ESR_FP, and/or a change to have the
> "either or but not both" settings of MSR and ESR.  Please let me know
> which you'd prefer.

Both fixes please, as separate patches.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] target-ppc: SPR_BOOKE_ESR not set on FP exceptions

[alarson]

David Gibson <david@gibson.dropbear.id.au> wrote on 07/29/2016 12:40:15 
AM:

> From: David Gibson <david@gibson.dropbear.id.au>
> To: alarson@ddci.com
> Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, agraf@suse.de
> Date: 07/29/2016 12:38 AM
> Subject: Re: target-ppc: SPR_BOOKE_ESR not set on FP exceptions
> 
> On Thu, Jul 28, 2016 at 06:32:27PM -0500, alarson@ddci.com wrote:
...
> > I did a quick check of the bits set in the POWERPC_EXCP_PROGRAM case.
> > The classic PPC sets SRR1 bits 11--15 depending on the exception.  In
> > Book E these correspond to bits 43--47,
> 
> Um.. what?  I'm not understanding where this bit shift is coming
> from. 

Sorry, I was looking at an old "classic" 32-bit manual for the SRR1 
exception definition and a 64-bit manual for the BookE.  They are
the same bits.  MSB0 bit numbering bytes again :-)

Re: [Qemu-devel] target-ppc: SPR_BOOKE_ESR not set on FP exceptions

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]

On Fri, Jul 29, 2016 at 08:04:04AM -0500, alarson@ddci.com wrote:
> David Gibson <david@gibson.dropbear.id.au> wrote on 07/29/2016 12:40:15 
> AM:
> 
> > From: David Gibson <david@gibson.dropbear.id.au>
> > To: alarson@ddci.com
> > Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, agraf@suse.de
> > Date: 07/29/2016 12:38 AM
> > Subject: Re: target-ppc: SPR_BOOKE_ESR not set on FP exceptions
> > 
> > On Thu, Jul 28, 2016 at 06:32:27PM -0500, alarson@ddci.com wrote:
> ...
> > > I did a quick check of the bits set in the POWERPC_EXCP_PROGRAM case.
> > > The classic PPC sets SRR1 bits 11--15 depending on the exception.  In
> > > Book E these correspond to bits 43--47,
> > 
> > Um.. what?  I'm not understanding where this bit shift is coming
> > from. 
> 
> Sorry, I was looking at an old "classic" 32-bit manual for the SRR1 
> exception definition and a 64-bit manual for the BookE.  They are
> the same bits.  MSB0 bit numbering bytes again :-)

Ok.  Can you re-do your analysis and submit a patch to fix this?

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]