Re: [Qemu-devel] [PATCH] target-ppc: clear MSR_POW on interrupt

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Fri, Sep 10, 2010 at 06:58:10PM +0200, Thomas Monjalon wrote:
> Alexander Graf wrote:
> > Am 10.09.2010 um 18:08 schrieb "Edgar E. Iglesias" 
> <edgar.iglesias@gmail.com>:
> > > On Fri, Sep 10, 2010 at 05:50:27PM +0200, Alexander Graf wrote:
> > >> Thomas Monjalon wrote:
> > >>> Alexander Graf wrote:
> > >>>> Thomas Monjalon wrote:
> > >>>>> Alexander Graf wrote:
> > >>>>>> Thomas Monjalon wrote:
> > >>>>>>> From: till <608107@bugs.launchpad.net>
> > >>>>>>> 
> > >>>>>>> According to FreeScale's 'Programming Environments Manual for
> > >>>>>>> 32-bit Implementations of the PowerPC Architecture' [MPCFPE32B,
> > >>>>>>> Rev.3, 9/2005], section 6.5, table 6-7, an interrupt resets
> > >>>>>>> MSR_POW to zero but qemu-0.12.4 fails to do so.
> > >>>>>>> Resetting the bit is necessary in order to bring the processor out
> > >>>>>>> of power management since otherwise it goes to sleep right away in
> > >>>>>>> the exception handler, i.e., it is impossible to leave PM-mode.
> > >>>>>> 
> > >>>>>> This doesn't look right. POW shouldn't even get stored in SRR1.
> > >>>>>> Could you please redo the patch and make sure that mtmsr masks out
> > >>>>>> MSR_POW?
> > >>>>> 
> > >>>>> Could you point sections of the specification for these requirements
> > >>>>> ?
> > >>>>> 
> > >>>>> I think SRR1 can save any bits. Non significant bits can be overriden
> > >>>>> and are masked out when RFI occurs.
> > >>>> 
> > >>>> I'm not saying I'm 100% sure on this, but take a look at the e300
> > >>>> reference manual
> > >>>> (http://cache.freescale.com/files/32bit/doc/ref_manual/e300coreRM.pdf)
> > >>>> section 5.2.3. POW is bit 13 in this notion. I don't see it getting
> > >>>> saved to SRR1.
> > >>> 
> > >>> The current implementation (commit c3d420e) save all the bits and
> > >>> restore only the needed ones (excluding POW). So POW is cleared after
> > >>> an interrupt.
> > >>> 
> > >>> But the subject of this patch wasn't on saved bits. It is to clear POW
> > >>> in the interrupt context. For an unknown reason, it's not done and
> > >>> it's clearly a bug to fix.
> > >> 
> > >> I completely agree. In fact, the mere fact that a bit can slip through
> > >> this loop indicates that something goes wrong.
> > >> 
> > >> As far as SRR1 goes, PowerISA 2.06 draws an even more complicated
> > >> picture: Page 816 says that SRR1 bits 42:44 indicate the power save exit
> > >> reason while 46:47 indicate how much state was transferred. Bit 45 would
> > >> be MSR_POW which is defined as "set to 0".
> > >> 
> > >> Because that means that we'll have to set MSR_POW in to 0 in SRR1 and
> > >> the interrupt switching function, we can just set it to 0 on mtmsr. That
> > >> was my point :).
> > > 
> > > Hi,
> > > 
> > > I'm not very familiar with PPC but the way I interpret it, we should
> > > clear the POW bit in both MSR and SRR1 when the interrupt waking the CPU
> > > is taken. While the CPU is sleeping, the POW bit should remain set in
> > > MSR though.
> > > 
> > > As Alex comments, the submited patch fails to make sure the SRR1 bit
> > > 13 is cleared when taking the interrupt, but I don't think clearing
> > > MSR[POW] in mtmsr is the right thing. In practice it probably doesn't
> > > matter much, possibly a bit annoying if you inspect the sleeping state
> > > from a debugger or similar.
> > > 
> > > I think mtmsr should set MSR[POW] and powerpc_excp() should clear
> > > MSR[POW] prior to copying MSR into SRR1.
> > > 
> > > Not sure, but thats the way I interpret it...
> > 
> > Agreed.
> > 
> > Alex
> 
> I think we should first ack and commit this patch as it is sure it is needed.
> 
> Regarding the SRR1 question, it can be another patch because it is more 
> general than the POW bit.
>
> In the commit c3d420e which was about MSR restoring in RFI, I have removed the 
> clearing of bits in SRR1. But I am OK to redo it after check that some 
> exceptions doesn't rely on the presence of these bits. I mean clearing bits in 
> SRR1 can be architecture-specific and exception-specific. Each exception has its 
> own scheme for SRR1 clearing.

But the MSR[POW] should be cleared prior to beeing copied to
SRR1. I dont see the point in submitting a followup patch
that moves the clearing a couple of lines up the c-file.

What about the following?

commit 7022e89d74a4299dbb431c89a684a428119242ab
Author: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Date:   Fri Sep 10 21:36:09 2010 +0200

    powerpc: Clear MSR[POW] when taking exceptions
    
    When taking exceptions, clear the POW or WE bit to inditcate
    that the CPU is awake. Do it prior to saving the MSR into SRR1.
    
    Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>

diff --git a/target-ppc/helper.c b/target-ppc/helper.c
index d342b09..c25c610 100644
--- a/target-ppc/helper.c
+++ b/target-ppc/helper.c
@@ -2075,6 +2075,8 @@ static inline void powerpc_excp(CPUState *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);
     msr = env->msr;
+    /* Taking an exception wakes up the core, clear the POW/WE bit.  */
+    msr &= ~((target_ulong)1 << MSR_POW);
     new_msr = msr;
     srr0 = SPR_SRR0;
     srr1 = SPR_SRR1;