Patchwork [v2,4/5] target-ppc: fix RFI by clearing upper bytes of MSR

login
register
mail settings
Submitter Thomas Monjalon
Date May 18, 2010, 2 p.m.
Message ID <201005181600.27913.thomas_ml@monjalon.net>
Download mbox | patch
Permalink /patch/52884/
State New
Headers show

Comments

Thomas Monjalon - May 18, 2010, 2 p.m.
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:
Alexander Graf - May 18, 2010, 2:35 p.m.
Thomas Monjalon wrote:
> 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.
>   

Wow, I didn't know there were still new products based on e300.

> The boot process hang with the actual implementation of RFI.
>   

Hrm.

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

According to the 2.06 ISA, for example the DSI sets SRR1 to:

33:36        Set to 0
42:47        Set to 0
Others      Loaded from the MSR



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

According to the 2.06 ISA again, rfi does the following:

The contents of SRR1 are placed into the MSR. If the new MSR value does
not enable any pending exceptions, then the next instruction is fetched,
under control of the new MSR value, from the address SRR0 0:64 || 0b00.
(Note: VLE behavior may be different; see Book VLE).

Book VLE is the Variable Length Encoded Instructions Architecture, so
that doesn't come into play here.

If rfi would clear ILE, how would it be enabled then?


Alex
Thomas Monjalon - May 18, 2010, 3:17 p.m.
Alexander Graf wrote:
> Thomas Monjalon wrote:
> > I'm running Linux for SBC834x in Qemu. The interrupt controller and board
> > definition are not yet published.
>
> Wow, I didn't know there were still new products based on e300.

Sorry, I was not clear. By "not yet published", I mean that I've written Qemu 
code to emulate e300 but I haven't yet send it to the ML. I would prefer to 
fix this RFI issue first.
SBC834x is not a new product.


> > 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.
>
> According to the 2.06 ISA again, rfi does the following:
>
> The contents of SRR1 are placed into the MSR. If the new MSR value does
> not enable any pending exceptions, then the next instruction is fetched,
> under control of the new MSR value, from the address SRR0 0:64 || 0b00.
>
> If rfi would clear ILE, how would it be enabled then?

You should be right. I have to fix a bug elsewhere.

Patch

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