| Message ID | 1275073652-20834-1-git-send-email-thomas_ml@monjalon.net |
|---|---|
| State | New |
| Headers | show |
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>
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 > > > > >
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