diff mbox

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

Message ID cad555c9cebf52e20386e8fdf45da05b019d5866.1272382040.git.thomas@monjalon.net
State New
Headers show

Commit Message

Thomas Monjalon April 27, 2010, 3:31 p.m. UTC 
From: Thomas Monjalon <thomas@monjalon.net>

Since commit 2ada0ed, "Return From Interrupt" is broken for PPC processors
because the upper bits (POW, TGPR, ILE) of MSR were not cleared.

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

Regarding RFID, since the 32 lower bits of MSR are the same in 64-bit,
the same mask as RFI should apply to RFID.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Cc: Blue Swirl <blauwirbel@gmail.com>
---
 target-ppc/op_helper.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

Comments

Alexander Graf May 2, 2010, 8:12 a.m. UTC | #1 
On 27.04.2010, at 17:31, Thomas Monjalon wrote:

> From: Thomas Monjalon <thomas@monjalon.net>
> 
> 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.

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

Have you tried making the SRR1 setting more clever?

> 
> Regarding RFID, since the 32 lower bits of MSR are the same in 64-bit,
> the same mask as RFI should apply to RFID.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> ---
> target-ppc/op_helper.c |    6 +++---
> 1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
> index 8f2ee98..2bf2ce1 100644
> --- a/target-ppc/op_helper.c
> +++ b/target-ppc/op_helper.c
> @@ -1646,20 +1646,20 @@ static inline void do_rfi(target_ulong nip, target_ulong msr,
> void helper_rfi (void)
> {
>     do_rfi(env->spr[SPR_SRR0], env->spr[SPR_SRR1],
> -           ~((target_ulong)0x0), 1);
> +           ~((target_ulong)0x00070000), 1);

Please use constant defines here. We have MSR_XX.

Alex
diff mbox

Patch

diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
index 8f2ee98..2bf2ce1 100644
--- a/target-ppc/op_helper.c
+++ b/target-ppc/op_helper.c
@@ -1646,20 +1646,20 @@  static inline void do_rfi(target_ulong nip, target_ulong msr,
 void helper_rfi (void)
 {
     do_rfi(env->spr[SPR_SRR0], env->spr[SPR_SRR1],
-           ~((target_ulong)0x0), 1);
+           ~((target_ulong)0x00070000), 1);
 }
 
 #if defined(TARGET_PPC64)
 void helper_rfid (void)
 {
     do_rfi(env->spr[SPR_SRR0], env->spr[SPR_SRR1],
-           ~((target_ulong)0x0), 0);
+           ~((target_ulong)0x00070000), 0);
 }
 
 void helper_hrfid (void)
 {
     do_rfi(env->spr[SPR_HSRR0], env->spr[SPR_HSRR1],
-           ~((target_ulong)0x0), 0);
+           ~((target_ulong)0x00070000), 0);
 }
 #endif
 #endif