Patchwork target-ppc: fix RFI by clearing some bits of MSR

login
register
mail settings
Submitter Thomas Monjalon
Date May 28, 2010, 7:07 p.m.
Message ID <1275073652-20834-1-git-send-email-thomas_ml@monjalon.net>
Download mbox | patch
Permalink /patch/53956/
State New
Headers show

Comments

Thomas Monjalon - May 28, 2010, 7:07 p.m.
From: Thomas Monjalon <thomas@monjalon.net>

Since commit 2ada0ed, "Return From Interrupt" is broken for PPC processors
because some interrupt specifics bits of SRR1 are copied to MSR.

SRR1 is a save of MSR during interrupt.
During RFI, MSR must be restored from SRR1.
But some bits of SRR1 are interrupt-specific and are not used for MSR saving.

This is the specification (ISA 2.06) at chapter 6.4.3 (Interrupt Processing):
"2. Bits 33:36 and 42:47 of SRR1 or HSRR1 are loaded with information specific
    to the interrupt type.
 3. Bits 0:32, 37:41, and 48:63 of SRR1 or HSRR1 are loaded with a copy of the
    corresponding bits of the MSR."

Below is a representation of MSR bits which are not saved:
0:15 16:31 32  33:36    37:41      42:47     48:63
——— | ——— | — X X X X — — — — — X X X X X X | ————
0000 0000 |    7   |   8   |   3   |   F    | 0000

History:
In the initial Qemu implementation (e1833e1), the mask 0x783F0000 was used for
saving MSR in SRR1. But all the bits 32:47 were cleared during RFI restoring.
This was wrong. The commit 2ada0ed explains that this breaks Altivec.
Indeed, bit 38 (for Altivec support) must be saved and restored.
The change of 2ada0ed was to restore all the bits of SRR1 to MSR.
But it's also wrong.

Explanation:
As an example, let's see what's happening after a TLB miss.
According to the e300 manual (E300CORERM table 5-6), the TLB miss interrupts
set the bits 44-47 for KEY, I/D, WAY and S/L. These bits are specifics to the
interrupt and must not be copied into MSR at the end of the interrupt.
With the current implementation, a TLB miss overwrite bits POW, TGPR and ILE.

Fix:
It shouldn't be needed to filter-out bits on MSR saving when interrupt occurs.
Specific bits overwrite MSR ones in SRR1.
But at the end of interrupt (RFI), specifics bits must be cleared before
restoring MSR from SRR1. The mask 0x783F0000 apply here.

Discussion:
The bits of the mask 0x783F0000 are cleared after an interrupt.
I cannot find a specification which talks about this
but I assume it is the truth since Linux can run this way.
Maybe it's not perfect but it's better (works for e300).

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Cc: Alexander Graf <agraf@suse.de>
---
 target-ppc/helper.c    |    1 -
 target-ppc/op_helper.c |    6 +++---
 2 files changed, 3 insertions(+), 4 deletions(-)
Alexander Graf - May 31, 2010, 3:06 p.m.
Thomas Monjalon wrote:
> From: Thomas Monjalon <thomas@monjalon.net>
>
> Since commit 2ada0ed, "Return From Interrupt" is broken for PPC processors
> because some interrupt specifics bits of SRR1 are copied to MSR.
>
> SRR1 is a save of MSR during interrupt.
> During RFI, MSR must be restored from SRR1.
> But some bits of SRR1 are interrupt-specific and are not used for MSR saving.
>
> This is the specification (ISA 2.06) at chapter 6.4.3 (Interrupt Processing):
> "2. Bits 33:36 and 42:47 of SRR1 or HSRR1 are loaded with information specific
>     to the interrupt type.
>  3. Bits 0:32, 37:41, and 48:63 of SRR1 or HSRR1 are loaded with a copy of the
>     corresponding bits of the MSR."
>
> Below is a representation of MSR bits which are not saved:
> 0:15 16:31 32  33:36    37:41      42:47     48:63
> ——— | ——— | — X X X X — — — — — X X X X X X | ————
> 0000 0000 |    7   |   8   |   3   |   F    | 0000
>
> History:
> In the initial Qemu implementation (e1833e1), the mask 0x783F0000 was used for
> saving MSR in SRR1. But all the bits 32:47 were cleared during RFI restoring.
> This was wrong. The commit 2ada0ed explains that this breaks Altivec.
> Indeed, bit 38 (for Altivec support) must be saved and restored.
> The change of 2ada0ed was to restore all the bits of SRR1 to MSR.
> But it's also wrong.
>
> Explanation:
> As an example, let's see what's happening after a TLB miss.
> According to the e300 manual (E300CORERM table 5-6), the TLB miss interrupts
> set the bits 44-47 for KEY, I/D, WAY and S/L. These bits are specifics to the
> interrupt and must not be copied into MSR at the end of the interrupt.
> With the current implementation, a TLB miss overwrite bits POW, TGPR and ILE.
>
> Fix:
> It shouldn't be needed to filter-out bits on MSR saving when interrupt occurs.
> Specific bits overwrite MSR ones in SRR1.
> But at the end of interrupt (RFI), specifics bits must be cleared before
> restoring MSR from SRR1. The mask 0x783F0000 apply here.
>
> Discussion:
> The bits of the mask 0x783F0000 are cleared after an interrupt.
> I cannot find a specification which talks about this
> but I assume it is the truth since Linux can run this way.
> Maybe it's not perfect but it's better (works for e300).
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Cc: Alexander Graf <agraf@suse.de>
>   

Acked-by: Alexander Graf <agraf@suse.de>
Aurelien Jarno - May 31, 2010, 6:45 p.m.
On Fri, May 28, 2010 at 09:07:32PM +0200, Thomas Monjalon wrote:
> From: Thomas Monjalon <thomas@monjalon.net>
> 
> Since commit 2ada0ed, "Return From Interrupt" is broken for PPC processors
> because some interrupt specifics bits of SRR1 are copied to MSR.
> 
> SRR1 is a save of MSR during interrupt.
> During RFI, MSR must be restored from SRR1.
> But some bits of SRR1 are interrupt-specific and are not used for MSR saving.
> 
> This is the specification (ISA 2.06) at chapter 6.4.3 (Interrupt Processing):
> "2. Bits 33:36 and 42:47 of SRR1 or HSRR1 are loaded with information specific
>     to the interrupt type.
>  3. Bits 0:32, 37:41, and 48:63 of SRR1 or HSRR1 are loaded with a copy of the
>     corresponding bits of the MSR."
> 
> Below is a representation of MSR bits which are not saved:
> 0:15 16:31 32  33:36    37:41      42:47     48:63
> ——— | ——— | — X X X X — — — — — X X X X X X | ————
> 0000 0000 |    7   |   8   |   3   |   F    | 0000
> 
> History:
> In the initial Qemu implementation (e1833e1), the mask 0x783F0000 was used for
> saving MSR in SRR1. But all the bits 32:47 were cleared during RFI restoring.
> This was wrong. The commit 2ada0ed explains that this breaks Altivec.
> Indeed, bit 38 (for Altivec support) must be saved and restored.
> The change of 2ada0ed was to restore all the bits of SRR1 to MSR.
> But it's also wrong.
> 
> Explanation:
> As an example, let's see what's happening after a TLB miss.
> According to the e300 manual (E300CORERM table 5-6), the TLB miss interrupts
> set the bits 44-47 for KEY, I/D, WAY and S/L. These bits are specifics to the
> interrupt and must not be copied into MSR at the end of the interrupt.
> With the current implementation, a TLB miss overwrite bits POW, TGPR and ILE.
> 
> Fix:
> It shouldn't be needed to filter-out bits on MSR saving when interrupt occurs.
> Specific bits overwrite MSR ones in SRR1.
> But at the end of interrupt (RFI), specifics bits must be cleared before
> restoring MSR from SRR1. The mask 0x783F0000 apply here.
> 
> Discussion:
> The bits of the mask 0x783F0000 are cleared after an interrupt.
> I cannot find a specification which talks about this
> but I assume it is the truth since Linux can run this way.
> Maybe it's not perfect but it's better (works for e300).
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Cc: Alexander Graf <agraf@suse.de>
> ---
>  target-ppc/helper.c    |    1 -
>  target-ppc/op_helper.c |    6 +++---
>  2 files changed, 3 insertions(+), 4 deletions(-)

Thanks, applied.

> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
> index dabf1fd..5035d92 100644
> --- a/target-ppc/helper.c
> +++ b/target-ppc/helper.c
> @@ -2080,7 +2080,6 @@ static inline void powerpc_excp(CPUState *env, int excp_model, int excp)
>      srr1 = SPR_SRR1;
>      asrr0 = -1;
>      asrr1 = -1;
> -    msr &= ~((target_ulong)0x783F0000);
>      switch (excp) {
>      case POWERPC_EXCP_NONE:
>          /* Should never happen */
> diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
> index 8f2ee98..3c3aa60 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)0x783F0000), 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)0x783F0000), 0);
>  }
>  
>  void helper_hrfid (void)
>  {
>      do_rfi(env->spr[SPR_HSRR0], env->spr[SPR_HSRR1],
> -           ~((target_ulong)0x0), 0);
> +           ~((target_ulong)0x783F0000), 0);
>  }
>  #endif
>  #endif
> -- 
> 1.7.1
> 
> 
> 
> 
>

Patch

diff --git a/target-ppc/helper.c b/target-ppc/helper.c
index dabf1fd..5035d92 100644
--- a/target-ppc/helper.c
+++ b/target-ppc/helper.c
@@ -2080,7 +2080,6 @@  static inline void powerpc_excp(CPUState *env, int excp_model, int excp)
     srr1 = SPR_SRR1;
     asrr0 = -1;
     asrr1 = -1;
-    msr &= ~((target_ulong)0x783F0000);
     switch (excp) {
     case POWERPC_EXCP_NONE:
         /* Should never happen */
diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
index 8f2ee98..3c3aa60 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)0x783F0000), 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)0x783F0000), 0);
 }
 
 void helper_hrfid (void)
 {
     do_rfi(env->spr[SPR_HSRR0], env->spr[SPR_HSRR1],
-           ~((target_ulong)0x0), 0);
+           ~((target_ulong)0x783F0000), 0);
 }
 #endif
 #endif