Patchwork [PATCHv3] PPC: Fix interrupt MSR value for classic exception models.

login
register
mail settings
Submitter Mark Cave-Ayland
Date April 6, 2012, 7:06 p.m.
Message ID <1333739187-5936-1-git-send-email-mark.cave-ayland@ilande.co.uk>
Download mbox | patch
Permalink /patch/151264/
State New
Headers show

Comments

Mark Cave-Ayland - April 6, 2012, 7:06 p.m.
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 for classic exception
models 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/cpu.h    |    2 ++
 target-ppc/helper.c |   31 ++++++++++++++++++++++++++++---
 2 files changed, 30 insertions(+), 3 deletions(-)
David Gibson - April 11, 2012, 1:08 a.m.
On Fri, Apr 06, 2012 at 08:06:27PM +0100, 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 for classic exception
> models 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/cpu.h    |    2 ++
>  target-ppc/helper.c |   31 ++++++++++++++++++++++++++++---
>  2 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index ca6f1cb..9a1c493 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -428,6 +428,8 @@ struct ppc_slb_t {
>  
>  /*****************************************************************************/
>  /* Machine state register bits definition                                    */
> +#define MSR_BIT(x) ((target_ulong)1 << MSR_##x)
> +
>  #define MSR_SF   63 /* Sixty-four-bit mode                            hflags */
>  #define MSR_TAG  62 /* Tag-active mode (POWERx ?)                            */
>  #define MSR_ISF  61 /* Sixty-four-bit interrupt mode on 630                  */
> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
> index 63a0dec..99beace 100644
> --- a/target-ppc/helper.c
> +++ b/target-ppc/helper.c
> @@ -2478,11 +2478,36 @@ 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;

Sorry, I really should have picked this up in an earlier review round,
but I think the *whole* msr calculation should be made per exception
model, not just the second part.  The common statement above
is.. cryptic, and I'd be mildly surprised if it was correct for all
architected variants.

> -    /* new interrupt handler msr */
> -    new_msr = env->msr & ((target_ulong)1 << MSR_ME);
> +    switch (excp_model) {
> +    case POWERPC_EXCP_STD:
> +    case POWERPC_EXCP_601:
> +    case POWERPC_EXCP_602:
> +    case POWERPC_EXCP_603:
> +    case POWERPC_EXCP_603E:
> +    case POWERPC_EXCP_604:
> +    case POWERPC_EXCP_7x0:
> +    case POWERPC_EXCP_7x5:
> +    case POWERPC_EXCP_74xx:
> +    case POWERPC_EXCP_G2: 
> +        /* new classic 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)
> +              POW, RI */
> +        new_msr = env->msr & ~(MSR_BIT(IR) | MSR_BIT(DR) | MSR_BIT(FE0)
> +                      | MSR_BIT(FE1) | MSR_BIT(EE) | MSR_BIT(BE) | MSR_BIT(FP)
> +                      | MSR_BIT(PMM) | MSR_BIT(PR) | MSR_BIT(SE) | MSR_BIT(POW)
> +                      | MSR_BIT(RI));
> +        break;
> +    default:
> +        /* new interrupt handler msr */
> +        new_msr = env->msr & ((target_ulong)1 << MSR_ME);
> +        break;
> +   }
>  
>      /* target registers */
>      srr0 = SPR_SRR0;
Mark Cave-Ayland - April 13, 2012, 4:53 p.m.
On 11/04/12 02:08, David Gibson wrote:

Hi David,

>> 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 for classic exception
>> models 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/cpu.h    |    2 ++
>>   target-ppc/helper.c |   31 ++++++++++++++++++++++++++++---
>>   2 files changed, 30 insertions(+), 3 deletions(-)
>>
>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>> index ca6f1cb..9a1c493 100644
>> --- a/target-ppc/cpu.h
>> +++ b/target-ppc/cpu.h
>> @@ -428,6 +428,8 @@ struct ppc_slb_t {
>>
>>   /*****************************************************************************/
>>   /* Machine state register bits definition                                    */
>> +#define MSR_BIT(x) ((target_ulong)1<<  MSR_##x)
>> +
>>   #define MSR_SF   63 /* Sixty-four-bit mode                            hflags */
>>   #define MSR_TAG  62 /* Tag-active mode (POWERx ?)                            */
>>   #define MSR_ISF  61 /* Sixty-four-bit interrupt mode on 630                  */
>> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
>> index 63a0dec..99beace 100644
>> --- a/target-ppc/helper.c
>> +++ b/target-ppc/helper.c
>> @@ -2478,11 +2478,36 @@ 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;
>
> Sorry, I really should have picked this up in an earlier review round,
> but I think the *whole* msr calculation should be made per exception
> model, not just the second part.  The common statement above
> is.. cryptic, and I'd be mildly surprised if it was correct for all
> architected variants.

Well the original value above hasn't changed from Alex's original commit 
(I simply augmented the comment), and it agrees with my reading of the 
specification pages as directed by Alex. Given that we also agreed to 
minimise the impact of the patch by making the least amount of changes 
(and it works for my HelenOS tests while preserving the existing 
behaviour), do you still think it makes sense to change the whole MSR 
calculation in this way?


ATB,

Mark.
Alexander Graf - April 18, 2012, 3:30 p.m.
On 04/13/2012 06:53 PM, Mark Cave-Ayland wrote:
> On 11/04/12 02:08, David Gibson wrote:
>
> Hi David,
>
>>> 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 for classic 
>>> exception
>>> models 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/cpu.h    |    2 ++
>>>   target-ppc/helper.c |   31 ++++++++++++++++++++++++++++---
>>>   2 files changed, 30 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>>> index ca6f1cb..9a1c493 100644
>>> --- a/target-ppc/cpu.h
>>> +++ b/target-ppc/cpu.h
>>> @@ -428,6 +428,8 @@ struct ppc_slb_t {
>>>
>>>   
>>> /*****************************************************************************/
>>>   /* Machine state register bits 
>>> definition                                    */
>>> +#define MSR_BIT(x) ((target_ulong)1<<  MSR_##x)
>>> +
>>>   #define MSR_SF   63 /* Sixty-four-bit 
>>> mode                            hflags */
>>>   #define MSR_TAG  62 /* Tag-active mode (POWERx 
>>> ?)                            */
>>>   #define MSR_ISF  61 /* Sixty-four-bit interrupt mode on 
>>> 630                  */
>>> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
>>> index 63a0dec..99beace 100644
>>> --- a/target-ppc/helper.c
>>> +++ b/target-ppc/helper.c
>>> @@ -2478,11 +2478,36 @@ 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;
>>
>> Sorry, I really should have picked this up in an earlier review round,
>> but I think the *whole* msr calculation should be made per exception
>> model, not just the second part.  The common statement above
>> is.. cryptic, and I'd be mildly surprised if it was correct for all
>> architected variants.
>
> Well the original value above hasn't changed from Alex's original 
> commit (I simply augmented the comment), and it agrees with my reading 
> of the specification pages as directed by Alex. Given that we also 
> agreed to minimise the impact of the patch by making the least amount 
> of changes (and it works for my HelenOS tests while preserving the 
> existing behaviour), do you still think it makes sense to change the 
> whole MSR calculation in this way?

Does HelenOS break without the patch? It worked fine for me.


Alex
Jakub Jermar - April 18, 2012, 8:31 p.m.
On 04/18/2012 05:30 PM, Alexander Graf wrote:
> On 04/13/2012 06:53 PM, Mark Cave-Ayland wrote:
>> On 11/04/12 02:08, David Gibson wrote:
>>
>> Hi David,
>>
>>>> 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 for classic
>>>> exception
>>>> models 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/cpu.h    |    2 ++
>>>>   target-ppc/helper.c |   31 ++++++++++++++++++++++++++++---
>>>>   2 files changed, 30 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>>>> index ca6f1cb..9a1c493 100644
>>>> --- a/target-ppc/cpu.h
>>>> +++ b/target-ppc/cpu.h
>>>> @@ -428,6 +428,8 @@ struct ppc_slb_t {
>>>>
>>>>  
>>>> /*****************************************************************************/
>>>>
>>>>   /* Machine state register bits
>>>> definition                                    */
>>>> +#define MSR_BIT(x) ((target_ulong)1<<  MSR_##x)
>>>> +
>>>>   #define MSR_SF   63 /* Sixty-four-bit
>>>> mode                            hflags */
>>>>   #define MSR_TAG  62 /* Tag-active mode (POWERx
>>>> ?)                            */
>>>>   #define MSR_ISF  61 /* Sixty-four-bit interrupt mode on
>>>> 630                  */
>>>> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
>>>> index 63a0dec..99beace 100644
>>>> --- a/target-ppc/helper.c
>>>> +++ b/target-ppc/helper.c
>>>> @@ -2478,11 +2478,36 @@ 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;
>>>
>>> Sorry, I really should have picked this up in an earlier review round,
>>> but I think the *whole* msr calculation should be made per exception
>>> model, not just the second part.  The common statement above
>>> is.. cryptic, and I'd be mildly surprised if it was correct for all
>>> architected variants.
>>
>> Well the original value above hasn't changed from Alex's original
>> commit (I simply augmented the comment), and it agrees with my reading
>> of the specification pages as directed by Alex. Given that we also
>> agreed to minimise the impact of the patch by making the least amount
>> of changes (and it works for my HelenOS tests while preserving the
>> existing behaviour), do you still think it makes sense to change the
>> whole MSR calculation in this way?
> 
> Does HelenOS break without the patch? It worked fine for me.

Hi Alex,

I've just tested QEMU git (which includes the TLB invalidation fix) and
it seems to work with HelenOS mainline quite nice. Not sure if we can
conclude the other fix is not needed though.

Jakub
Jakub Jermar - April 18, 2012, 8:32 p.m.
On 04/18/2012 05:30 PM, Alexander Graf wrote:
> On 04/13/2012 06:53 PM, Mark Cave-Ayland wrote:
>> On 11/04/12 02:08, David Gibson wrote:
>>
>> Hi David,
>>
>>>> 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 for classic
>>>> exception
>>>> models 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/cpu.h    |    2 ++
>>>>   target-ppc/helper.c |   31 ++++++++++++++++++++++++++++---
>>>>   2 files changed, 30 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>>>> index ca6f1cb..9a1c493 100644
>>>> --- a/target-ppc/cpu.h
>>>> +++ b/target-ppc/cpu.h
>>>> @@ -428,6 +428,8 @@ struct ppc_slb_t {
>>>>
>>>>  
>>>> /*****************************************************************************/
>>>>
>>>>   /* Machine state register bits
>>>> definition                                    */
>>>> +#define MSR_BIT(x) ((target_ulong)1<<  MSR_##x)
>>>> +
>>>>   #define MSR_SF   63 /* Sixty-four-bit
>>>> mode                            hflags */
>>>>   #define MSR_TAG  62 /* Tag-active mode (POWERx
>>>> ?)                            */
>>>>   #define MSR_ISF  61 /* Sixty-four-bit interrupt mode on
>>>> 630                  */
>>>> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
>>>> index 63a0dec..99beace 100644
>>>> --- a/target-ppc/helper.c
>>>> +++ b/target-ppc/helper.c
>>>> @@ -2478,11 +2478,36 @@ 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;
>>>
>>> Sorry, I really should have picked this up in an earlier review round,
>>> but I think the *whole* msr calculation should be made per exception
>>> model, not just the second part.  The common statement above
>>> is.. cryptic, and I'd be mildly surprised if it was correct for all
>>> architected variants.
>>
>> Well the original value above hasn't changed from Alex's original
>> commit (I simply augmented the comment), and it agrees with my reading
>> of the specification pages as directed by Alex. Given that we also
>> agreed to minimise the impact of the patch by making the least amount
>> of changes (and it works for my HelenOS tests while preserving the
>> existing behaviour), do you still think it makes sense to change the
>> whole MSR calculation in this way?
> 
> Does HelenOS break without the patch? It worked fine for me.

Hi Alex,

I've just tested QEMU git (which includes the TLB invalidation fix) and
it seems to work with HelenOS mainline quite nice. Not sure if we can
conclude the other fix is not needed though.

Jakub
Alexander Graf - April 18, 2012, 9:34 p.m.
On 18.04.2012, at 22:31, Jakub Jermar wrote:

> On 04/18/2012 05:30 PM, Alexander Graf wrote:
>> On 04/13/2012 06:53 PM, Mark Cave-Ayland wrote:
>>> On 11/04/12 02:08, David Gibson wrote:
>>> 
>>> Hi David,
>>> 
>>>>> 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 for classic
>>>>> exception
>>>>> models 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/cpu.h    |    2 ++
>>>>>  target-ppc/helper.c |   31 ++++++++++++++++++++++++++++---
>>>>>  2 files changed, 30 insertions(+), 3 deletions(-)
>>>>> 
>>>>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>>>>> index ca6f1cb..9a1c493 100644
>>>>> --- a/target-ppc/cpu.h
>>>>> +++ b/target-ppc/cpu.h
>>>>> @@ -428,6 +428,8 @@ struct ppc_slb_t {
>>>>> 
>>>>> 
>>>>> /*****************************************************************************/
>>>>> 
>>>>>  /* Machine state register bits
>>>>> definition                                    */
>>>>> +#define MSR_BIT(x) ((target_ulong)1<<  MSR_##x)
>>>>> +
>>>>>  #define MSR_SF   63 /* Sixty-four-bit
>>>>> mode                            hflags */
>>>>>  #define MSR_TAG  62 /* Tag-active mode (POWERx
>>>>> ?)                            */
>>>>>  #define MSR_ISF  61 /* Sixty-four-bit interrupt mode on
>>>>> 630                  */
>>>>> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
>>>>> index 63a0dec..99beace 100644
>>>>> --- a/target-ppc/helper.c
>>>>> +++ b/target-ppc/helper.c
>>>>> @@ -2478,11 +2478,36 @@ 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;
>>>> 
>>>> Sorry, I really should have picked this up in an earlier review round,
>>>> but I think the *whole* msr calculation should be made per exception
>>>> model, not just the second part.  The common statement above
>>>> is.. cryptic, and I'd be mildly surprised if it was correct for all
>>>> architected variants.
>>> 
>>> Well the original value above hasn't changed from Alex's original
>>> commit (I simply augmented the comment), and it agrees with my reading
>>> of the specification pages as directed by Alex. Given that we also
>>> agreed to minimise the impact of the patch by making the least amount
>>> of changes (and it works for my HelenOS tests while preserving the
>>> existing behaviour), do you still think it makes sense to change the
>>> whole MSR calculation in this way?
>> 
>> Does HelenOS break without the patch? It worked fine for me.
> 
> Hi Alex,
> 
> I've just tested QEMU git (which includes the TLB invalidation fix) and
> it seems to work with HelenOS mainline quite nice. Not sure if we can
> conclude the other fix is not needed though.

Well, the way I read the spec the patch is mostly wrong, hence I was wondering if it actually does fix a real world use case :).

If it does, we'd need to figure out which part of it does fix something. If it doesn't, I don't think we should take it - at least in its current form.

However, it would be great if someone who wants to spend a few hours reading exactly through the different BookE and Book3S variant books could check if what we're doing there is correct for all platforms we emulate. But for normal classic and POWER based PowerPCs, I'm pretty sure the code as we have it is fairly reasonable and does the right thing (unless you run in HV mode). Though I'd love to be proven wrong :).


Alex

PS: Mark, sorry for only pulling you back on this patch so late in the development cycle. I would've done so earlier but I didn't spot the "reserved bits are 0" part myself at first and only did when I got back from vacation - which was only last weekend :).
Mark Cave-Ayland - April 22, 2012, 7:49 p.m.
On 18/04/12 21:31, Jakub Jermar wrote:

>> Does HelenOS break without the patch? It worked fine for me.
>
> Hi Alex,
>
> I've just tested QEMU git (which includes the TLB invalidation fix) and
> it seems to work with HelenOS mainline quite nice. Not sure if we can
> conclude the other fix is not needed though.
>
> Jakub

Hi Alex/Jakub,

I've been away for most of the week so haven't had a chance to look at 
this until now.

I can confirm that HelenOS PPC boots for me once again with git master, 
so if Jakub is happy then that's good enough for me. Alex - if you are 
both happy then I don't see why you can't close 
https://bugs.launchpad.net/qemu/+bug/942299 before the next release.


ATB,

Mark.

Patch

diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index ca6f1cb..9a1c493 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -428,6 +428,8 @@  struct ppc_slb_t {
 
 /*****************************************************************************/
 /* Machine state register bits definition                                    */
+#define MSR_BIT(x) ((target_ulong)1 << MSR_##x)
+
 #define MSR_SF   63 /* Sixty-four-bit mode                            hflags */
 #define MSR_TAG  62 /* Tag-active mode (POWERx ?)                            */
 #define MSR_ISF  61 /* Sixty-four-bit interrupt mode on 630                  */
diff --git a/target-ppc/helper.c b/target-ppc/helper.c
index 63a0dec..99beace 100644
--- a/target-ppc/helper.c
+++ b/target-ppc/helper.c
@@ -2478,11 +2478,36 @@  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_STD:
+    case POWERPC_EXCP_601:
+    case POWERPC_EXCP_602:
+    case POWERPC_EXCP_603:
+    case POWERPC_EXCP_603E:
+    case POWERPC_EXCP_604:
+    case POWERPC_EXCP_7x0:
+    case POWERPC_EXCP_7x5:
+    case POWERPC_EXCP_74xx:
+    case POWERPC_EXCP_G2: 
+        /* new classic 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)
+              POW, RI */
+        new_msr = env->msr & ~(MSR_BIT(IR) | MSR_BIT(DR) | MSR_BIT(FE0)
+                      | MSR_BIT(FE1) | MSR_BIT(EE) | MSR_BIT(BE) | MSR_BIT(FP)
+                      | MSR_BIT(PMM) | MSR_BIT(PR) | MSR_BIT(SE) | MSR_BIT(POW)
+                      | MSR_BIT(RI));
+        break;
+    default:
+        /* new interrupt handler msr */
+        new_msr = env->msr & ((target_ulong)1 << MSR_ME);
+        break;
+   }
 
     /* target registers */
     srr0 = SPR_SRR0;