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

[David Gibson <david@gibson.dropbear.id.au>]

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