[Qemu-devel] [PATCH 0/2] PPC: interrupt handler bugfixes v2

[Qemu-devel] [PATCH 0/2] PPC: interrupt handler bugfixes v2

[Mark Cave-Ayland]

This small patch series resolves https://bugs.launchpad.net/qemu/+bug/942299
and enables HelenOS to boot once again under PPC32.

v2:
Include switch statement based upon exception model so that existing BookE 
behaviour is unmodified.

Replace hard-coded MSR constant with symbolic bit names.


Mark Cave-Ayland (2):
  PPC: Fix interrupt MSR value within the PPC interrupt handler.
  PPC: Fix TLB invalidation bug within the PPC interrupt handler.

 target-ppc/helper.c |   25 +++++++++++++++++++++----
 1 files changed, 21 insertions(+), 4 deletions(-)

-- 
1.7.2.5

[Qemu-devel] [PATCH 1/2] PPC: Fix interrupt MSR value within the PPC interrupt handler.

[Mark Cave-Ayland]

Commit 41557447d30eeb944e42069513df13585f5e6c7f introduced a new method of
calculating the MSR for the interrupt context. However this doesn't quite
agree with the PowerISA 2.06B specification (pp. 811-814) since too many
bits were being cleared.

This patch corrects the calculation of the interrupt MSR whilst including
additional comments to clarify which bits are being changed within both the
MSR and the interrupt MSR.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Martin Sucha <sucha14@uniba.sk>
---
 target-ppc/helper.c |   23 ++++++++++++++++++++---
 1 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/target-ppc/helper.c b/target-ppc/helper.c
index 39dcc27..653f818 100644
--- a/target-ppc/helper.c
+++ b/target-ppc/helper.c
@@ -2459,6 +2459,8 @@ static inline void dump_syscall(CPUPPCState *env)
 /* Note that this function should be greatly optimized
  * when called with a constant excp, from ppc_hw_interrupt
  */
+#define MSR_BIT(x) ((target_ulong)1 << x)
+
 static inline void powerpc_excp(CPUPPCState *env, int excp_model, int excp)
 {
     target_ulong msr, new_msr, vector;
@@ -2478,11 +2480,26 @@ static inline void powerpc_excp(CPUPPCState *env, int excp_model, int excp)
     qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
                   " => %08x (%02x)\n", env->nip, excp, env->error_code);
 
-    /* new srr1 value excluding must-be-zero bits */
+    /* new srr1 value with interrupt-specific bits defaulting to zero */
     msr = env->msr & ~0x783f0000ULL;
 
-    /* new interrupt handler msr */
-    new_msr = env->msr & ((target_ulong)1 << MSR_ME);
+    switch (excp_model) {
+    case POWERPC_EXCP_BOOKE:
+        /* new interrupt handler msr */
+        new_msr = env->msr & ((target_ulong)1 << MSR_ME);
+        break;
+
+    default:
+        /* new interrupt handler msr (as per PowerISA 2.06B p.811 and p.814): 
+           1) force the following bits to zero
+              IR, DR, FE0, FE1, EE, BE, FP, PMM, PR, SE
+           2) default the following bits to zero (can be overidden later on)
+              RI */
+        new_msr = env->msr & ~(MSR_BIT(MSR_IR) | MSR_BIT(MSR_DR) 
+                      | MSR_BIT(MSR_FE0)| MSR_BIT(MSR_FE1) | MSR_BIT(MSR_EE) 
+                      | MSR_BIT(MSR_BE) | MSR_BIT(MSR_FP) | MSR_BIT(MSR_PMM) 
+                      | MSR_BIT(MSR_PR) | MSR_BIT(MSR_SE) | MSR_BIT(MSR_RI));
+    }
 
     /* target registers */
     srr0 = SPR_SRR0;
-- 
1.7.2.5

[Qemu-devel] [PATCH 2/2] PPC: Fix TLB invalidation bug within the PPC interrupt handler.

[Mark Cave-Ayland]

Commit 41557447d30eeb944e42069513df13585f5e6c7f also introduced a subtle TLB
flush bug. By applying a mask to the interrupt MSR which cleared the IR/DR
bits at the start of the interrupt handler, the logic towards the end of the
handler to force a TLB flush if either one of these bits were set would never
be triggered.

This patch simply changes the IR/DR bit check in the TLB flush logic to use
the original MSR value (albeit with some interrupt-specific bits cleared) so
that the IR/DR bits are preserved at the point where the check takes place.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target-ppc/helper.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/target-ppc/helper.c b/target-ppc/helper.c
index 653f818..84e49a1 100644
--- a/target-ppc/helper.c
+++ b/target-ppc/helper.c
@@ -2977,7 +2977,7 @@ static inline void powerpc_excp(CPUPPCState *env, int excp_model, int excp)
     if (asrr1 != -1)
         env->spr[asrr1] = env->spr[srr1];
     /* If we disactivated any translation, flush TLBs */
-    if (new_msr & ((1 << MSR_IR) | (1 << MSR_DR)))
+    if (msr & ((1 << MSR_IR) | (1 << MSR_DR)))
         tlb_flush(env, 1);
 
     if (msr_ile) {
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH 1/2] PPC: Fix interrupt MSR value within the PPC interrupt handler.

[Scott Wood]

On 03/27/2012 10:41 AM, Mark Cave-Ayland wrote:
> Commit 41557447d30eeb944e42069513df13585f5e6c7f introduced a new method of
> calculating the MSR for the interrupt context. However this doesn't quite
> agree with the PowerISA 2.06B specification (pp. 811-814) since too many
> bits were being cleared.
> 
> This patch corrects the calculation of the interrupt MSR whilst including
> additional comments to clarify which bits are being changed within both the
> MSR and the interrupt MSR.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Martin Sucha <sucha14@uniba.sk>
> ---
>  target-ppc/helper.c |   23 ++++++++++++++++++++---
>  1 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
> index 39dcc27..653f818 100644
> --- a/target-ppc/helper.c
> +++ b/target-ppc/helper.c
> @@ -2459,6 +2459,8 @@ static inline void dump_syscall(CPUPPCState *env)
>  /* Note that this function should be greatly optimized
>   * when called with a constant excp, from ppc_hw_interrupt
>   */
> +#define MSR_BIT(x) ((target_ulong)1 << x)

If we're going to make this specific to MSRs, might as well cut down on
the user's verbosity:

#define MSR_BIT(x) ((target_ulong)1 << MSR_##x)

...and move it to a header file.

Or possibly have the header file define a set of MSRBIT_IR, MSRBIT_DR, etc.

>  static inline void powerpc_excp(CPUPPCState *env, int excp_model, int excp)
>  {
>      target_ulong msr, new_msr, vector;
> @@ -2478,11 +2480,26 @@ static inline void powerpc_excp(CPUPPCState *env, int excp_model, int excp)
>      qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
>                    " => %08x (%02x)\n", env->nip, excp, env->error_code);
>  
> -    /* new srr1 value excluding must-be-zero bits */
> +    /* new srr1 value with interrupt-specific bits defaulting to zero */
>      msr = env->msr & ~0x783f0000ULL;
>  
> -    /* new interrupt handler msr */
> -    new_msr = env->msr & ((target_ulong)1 << MSR_ME);
> +    switch (excp_model) {
> +    case POWERPC_EXCP_BOOKE:
> +        /* new interrupt handler msr */
> +        new_msr = env->msr & ((target_ulong)1 << MSR_ME);
> +        break;
> +
> +    default:
> +        /* new interrupt handler msr (as per PowerISA 2.06B p.811 and p.814): 
> +           1) force the following bits to zero
> +              IR, DR, FE0, FE1, EE, BE, FP, PMM, PR, SE
> +           2) default the following bits to zero (can be overidden later on)
> +              RI */
> +        new_msr = env->msr & ~(MSR_BIT(MSR_IR) | MSR_BIT(MSR_DR) 
> +                      | MSR_BIT(MSR_FE0)| MSR_BIT(MSR_FE1) | MSR_BIT(MSR_EE) 
> +                      | MSR_BIT(MSR_BE) | MSR_BIT(MSR_FP) | MSR_BIT(MSR_PMM) 
> +                      | MSR_BIT(MSR_PR) | MSR_BIT(MSR_SE) | MSR_BIT(MSR_RI));
> +    }

What about POWERPC_EXCP_40x?  And are all the classic chips OK with the
2.06B implementation?

BTW, it's unfortunate that QEMU uses the same namespacing for PPC
exceptions as for PPC exception models.  Makes grepping for exception
models a pain.

-Scott

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] PPC: Fix TLB invalidation bug within the PPC interrupt handler.

[David Gibson]

On Tue, Mar 27, 2012 at 04:41:55PM +0100, Mark Cave-Ayland wrote:
> Commit 41557447d30eeb944e42069513df13585f5e6c7f also introduced a subtle TLB
> flush bug. By applying a mask to the interrupt MSR which cleared the IR/DR
> bits at the start of the interrupt handler, the logic towards the end of the
> handler to force a TLB flush if either one of these bits were set would never
> be triggered.
> 
> This patch simply changes the IR/DR bit check in the TLB flush logic to use
> the original MSR value (albeit with some interrupt-specific bits cleared) so
> that the IR/DR bits are preserved at the point where the check takes place.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] PPC: Fix interrupt MSR value within the PPC interrupt handler.

[David Gibson]

On Tue, Mar 27, 2012 at 12:47:32PM -0500, Scott Wood wrote:
> On 03/27/2012 10:41 AM, Mark Cave-Ayland wrote:
> > Commit 41557447d30eeb944e42069513df13585f5e6c7f introduced a new method of
> > calculating the MSR for the interrupt context. However this doesn't quite
> > agree with the PowerISA 2.06B specification (pp. 811-814) since too many
> > bits were being cleared.
> > 
> > This patch corrects the calculation of the interrupt MSR whilst including
> > additional comments to clarify which bits are being changed within both the
> > MSR and the interrupt MSR.
> > 
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > Signed-off-by: Martin Sucha <sucha14@uniba.sk>
> > ---
> >  target-ppc/helper.c |   23 ++++++++++++++++++++---
> >  1 files changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-ppc/helper.c b/target-ppc/helper.c
> > index 39dcc27..653f818 100644
> > --- a/target-ppc/helper.c
> > +++ b/target-ppc/helper.c
> > @@ -2459,6 +2459,8 @@ static inline void dump_syscall(CPUPPCState *env)
> >  /* Note that this function should be greatly optimized
> >   * when called with a constant excp, from ppc_hw_interrupt
> >   */
> > +#define MSR_BIT(x) ((target_ulong)1 << x)
> 
> If we're going to make this specific to MSRs, might as well cut down on
> the user's verbosity:
> 
> #define MSR_BIT(x) ((target_ulong)1 << MSR_##x)
> 
> ...and move it to a header file.
> 
> Or possibly have the header file define a set of MSRBIT_IR, MSRBIT_DR, etc.
> 
> >  static inline void powerpc_excp(CPUPPCState *env, int excp_model, int excp)
> >  {
> >      target_ulong msr, new_msr, vector;
> > @@ -2478,11 +2480,26 @@ static inline void powerpc_excp(CPUPPCState *env, int excp_model, int excp)
> >      qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
> >                    " => %08x (%02x)\n", env->nip, excp, env->error_code);
> >  
> > -    /* new srr1 value excluding must-be-zero bits */
> > +    /* new srr1 value with interrupt-specific bits defaulting to zero */
> >      msr = env->msr & ~0x783f0000ULL;
> >  
> > -    /* new interrupt handler msr */
> > -    new_msr = env->msr & ((target_ulong)1 << MSR_ME);
> > +    switch (excp_model) {
> > +    case POWERPC_EXCP_BOOKE:
> > +        /* new interrupt handler msr */
> > +        new_msr = env->msr & ((target_ulong)1 << MSR_ME);
> > +        break;
> > +
> > +    default:
> > +        /* new interrupt handler msr (as per PowerISA 2.06B p.811 and p.814): 
> > +           1) force the following bits to zero
> > +              IR, DR, FE0, FE1, EE, BE, FP, PMM, PR, SE
> > +           2) default the following bits to zero (can be overidden later on)
> > +              RI */
> > +        new_msr = env->msr & ~(MSR_BIT(MSR_IR) | MSR_BIT(MSR_DR) 
> > +                      | MSR_BIT(MSR_FE0)| MSR_BIT(MSR_FE1) | MSR_BIT(MSR_EE) 
> > +                      | MSR_BIT(MSR_BE) | MSR_BIT(MSR_FP) | MSR_BIT(MSR_PMM) 
> > +                      | MSR_BIT(MSR_PR) | MSR_BIT(MSR_SE) | MSR_BIT(MSR_RI));
> > +    }
> 
> What about POWERPC_EXCP_40x?  And are all the classic chips OK with the
> 2.06B implementation?

Hrm, yeah.  I think what you ought to do is to use the new logic just
for the "classic" exception models.  Have the default branch remain
the one that just masks ME.  That's wrong, but it's the same wrong as
we have already, and we can fix it later once we've verified what the
right thing to do is for 40x and BookE.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] PPC: Fix TLB invalidation bug within the PPC interrupt handler.

[Andreas Färber]

Am 28.03.2012 02:45, schrieb David Gibson:
> On Tue, Mar 27, 2012 at 04:41:55PM +0100, Mark Cave-Ayland wrote:
>> Commit 41557447d30eeb944e42069513df13585f5e6c7f also introduced a subtle TLB
>> flush bug. By applying a mask to the interrupt MSR which cleared the IR/DR
>> bits at the start of the interrupt handler, the logic towards the end of the
>> handler to force a TLB flush if either one of these bits were set would never
>> be triggered.
>>
>> This patch simply changes the IR/DR bit check in the TLB flush logic to use
>> the original MSR value (albeit with some interrupt-specific bits cleared) so
>> that the IR/DR bits are preserved at the point where the check takes place.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Acked-by: David Gibson <david@gibson.dropbear.id.au>

Thanks, compile-tested and applied to ppc-next:
http://repo.or.cz/w/qemu/agraf.git/shortlog/refs/heads/ppc-next

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] PPC: Fix interrupt MSR value within the PPC interrupt handler.

[Mark Cave-Ayland]

On 28/03/12 01:46, David Gibson wrote:

Hi David,

>> If we're going to make this specific to MSRs, might as well cut down on
>> the user's verbosity:
>>
>> #define MSR_BIT(x) ((target_ulong)1<<  MSR_##x)
>>
>> ...and move it to a header file.
>>
>> Or possibly have the header file define a set of MSRBIT_IR, MSRBIT_DR, etc.

I think I prefer your macro above and move it to a relevant part of 
target-ppc/cpu.h with the other MSR defines.

>>>   static inline void powerpc_excp(CPUPPCState *env, int excp_model, int excp)
>>>   {
>>>       target_ulong msr, new_msr, vector;
>>> @@ -2478,11 +2480,26 @@ static inline void powerpc_excp(CPUPPCState *env, int excp_model, int excp)
>>>       qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
>>>                     " =>  %08x (%02x)\n", env->nip, excp, env->error_code);
>>>
>>> -    /* new srr1 value excluding must-be-zero bits */
>>> +    /* new srr1 value with interrupt-specific bits defaulting to zero */
>>>       msr = env->msr&  ~0x783f0000ULL;
>>>
>>> -    /* new interrupt handler msr */
>>> -    new_msr = env->msr&  ((target_ulong)1<<  MSR_ME);
>>> +    switch (excp_model) {
>>> +    case POWERPC_EXCP_BOOKE:
>>> +        /* new interrupt handler msr */
>>> +        new_msr = env->msr&  ((target_ulong)1<<  MSR_ME);
>>> +        break;
>>> +
>>> +    default:
>>> +        /* new interrupt handler msr (as per PowerISA 2.06B p.811 and p.814):
>>> +           1) force the following bits to zero
>>> +              IR, DR, FE0, FE1, EE, BE, FP, PMM, PR, SE
>>> +           2) default the following bits to zero (can be overidden later on)
>>> +              RI */
>>> +        new_msr = env->msr&  ~(MSR_BIT(MSR_IR) | MSR_BIT(MSR_DR)
>>> +                      | MSR_BIT(MSR_FE0)| MSR_BIT(MSR_FE1) | MSR_BIT(MSR_EE)
>>> +                      | MSR_BIT(MSR_BE) | MSR_BIT(MSR_FP) | MSR_BIT(MSR_PMM)
>>> +                      | MSR_BIT(MSR_PR) | MSR_BIT(MSR_SE) | MSR_BIT(MSR_RI));
>>> +    }
>>
>> What about POWERPC_EXCP_40x?  And are all the classic chips OK with the
>> 2.06B implementation?
>
> Hrm, yeah.  I think what you ought to do is to use the new logic just
> for the "classic" exception models.  Have the default branch remain
> the one that just masks ME.  That's wrong, but it's the same wrong as
> we have already, and we can fix it later once we've verified what the
> right thing to do is for 40x and BookE.

I'm actually coming at this from a fixing what was potentially an 
OpenBIOS bug rather than a PPC angle, so I have to admit I have no I 
idea which ones are the "classic" exception models. Would you consider 
this to be just EXCP_STD, EXCP_6* and EXCP_7*?


Many thanks,

Mark.

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] PPC: Fix interrupt MSR value within the PPC interrupt handler.

[Scott Wood]

On 03/29/2012 04:11 AM, Mark Cave-Ayland wrote:
>>> What about POWERPC_EXCP_40x?  And are all the classic chips OK with the
>>> 2.06B implementation?
>>
>> Hrm, yeah.  I think what you ought to do is to use the new logic just
>> for the "classic" exception models.  Have the default branch remain
>> the one that just masks ME.  That's wrong, but it's the same wrong as
>> we have already, and we can fix it later once we've verified what the
>> right thing to do is for 40x and BookE.
> 
> I'm actually coming at this from a fixing what was potentially an
> OpenBIOS bug rather than a PPC angle, so I have to admit I have no I
> idea which ones are the "classic" exception models. Would you consider
> this to be just EXCP_STD, EXCP_6* and EXCP_7*?

Also POWERPC_EXCP_G2, and maybe POWERPC_EXCP_970?  Even on server
there's a question of whether it's a 2.06 chip or previous version of
the architecture.

One thing that sticks out for classic chips that is missing here is
MSR[POW], which should be cleared on exceptions.

-Scott