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

[David Gibson]

On Thu, Mar 22, 2012 at 06:57:44PM +0000, 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 <address@hidden>
> Signed-off-by: Martin Sucha <address@hidden>
> ---
>  target-ppc/helper.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
> index 39dcc27..4a21d8d 100644
> --- a/target-ppc/helper.c
> +++ b/target-ppc/helper.c
> @@ -2478,11 +2478,15 @@ 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);
> +    /* 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, HVB (note HVB is a different bit between PPC32/64) */
> +    new_msr = env->msr & ~0xed36ULL & ~((target_ulong) MSR_HVB);

Both here and above, using the #defines rather than a hex constant
would be rather nicer.  As Scott points out this probably need to be a
different mask for BookE, too.

Also, setting HVB to 0 doesn't make sense.  Taking an interrupt can
turn hypervisor state *on* for certain interrupts, but it will never
turn it off.

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