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

* [Qemu-devel] target-ppc: SPR_BOOKE_ESR not set on FP exceptions
@ 2016-07-28 23:32 alarson
  2016-07-29  5:40 ` David Gibson
  0 siblings, 1 reply; 4+ messages in thread
From: alarson @ 2016-07-28 23:32 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc; +Cc: David Gibson, agraf

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread