Re: [Qemu-devel] [PATCH v2 4/5] target-ppc: fix RFI by clearing upper bytes of MSR

[Thomas Monjalon <thomas_ml@monjalon.net>]

Alexander Graf wrote:
> On 27.04.2010, at 17:31, Thomas Monjalon wrote:
> > Since commit 2ada0ed, "Return From Interrupt" is broken for PPC
> > processors because the upper bits (POW, TGPR, ILE) of MSR were not
> > cleared.
>
> May I ask for your test case or how you stumbled over this? I haven't seen
> any OS rely on this yet.

I'm running Linux for SBC834x in Qemu. The interrupt controller and board 
definition are not yet published.

The boot process hang with the actual implementation of RFI.

> > Below is a representation of MSR bits:
> > 0 .. 12  13  14  15  16         ..         23  24        ..      31
> > —————  POW TGPR ILE EE PR FP ME FE0 SE BE FE1 CE IP IR DR —— RI LE
> >
> > Only the 2 lower bytes (16-31) of MSR are saved to SRR1 before an
> > interrupt. So only these bytes should be restored and the upper ones
> > (0-15) cleared. But, referring to commit 2ada0ed, clearing all the upper
> > bytes breaks Altivec. The compromise is to clear the well known bits
> > (13-15).
>
> IIRC this is vastly implementation dependent. Book3 lists the bits saved
> when setting SRR1 explicitly and I haven't found a good rule of thumb yet.
> RFI on the other hand is described as MSR <- SRR1. So I'm fairly sure RFI
> is implemented correctly.

From the programming manual (MPCFPE32B):
"The save/restore register 1 (SRR1) is used to save machine status (selected 
bits from the MSR and other implementation-specific status bits as well) on 
interrupts and to restore those values when rfi is executed.
[..]
When an interrupt occurs, SRR1 bits 1–4 and 10–15 are loaded with 
interrupt-specific information and MSR bits 16–23, 25–27, and 30–31 are 
placed into the corresponding bit positions of SRR1. Depending on the 
implementation, additional MSR bits may be copied to SRR1."

From the e300 reference manual (e300CORERM):
"The TGPR bit is cleared by an rfi instruction."

My first try was to clear only TGPR. But it doesn't work properly if POW and 
ILE are not cleared.

> Have you tried making the SRR1 setting more clever?

Yes. POW and TGPR can be filtered-out when saving to SRR1. But it seems that 
ILE must be cleared in RFI (if not, the Linux PCI scan is an endless loop).

I don't if this way is better:

--- a/target-ppc/helper.c
+++ b/target-ppc/helper.c
@@ -2571,2 +2571,2 @@ static inline void powerpc_excp(CPUState *env, int 
excp_model, int excp)
     /* Save MSR */
-    env->spr[srr1] = msr;
+    env->spr[srr1] = msr & ~(1 << MSR_POW | 1 << MSR_TGPR);

--- a/target-ppc/op_helper.c
+++ b/target-ppc/op_helper.c
@@ -1648,2 +1648,2 @@ void helper_rfi (void)
     do_rfi(env->spr[SPR_SRR0], env->spr[SPR_SRR1],
-           ~((target_ulong)0x0), 1);
+           ~((target_ulong) 1 << MSR_ILE), 1);

-- 
Thomas