From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=49974 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Ou9j2-00031g-Kp for qemu-devel@nongnu.org; Fri, 10 Sep 2010 15:57:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Ou9j0-0001sC-Vl for qemu-devel@nongnu.org; Fri, 10 Sep 2010 15:57:32 -0400 Received: from mail-ey0-f173.google.com ([209.85.215.173]:53166) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Ou9j0-0001s0-OY for qemu-devel@nongnu.org; Fri, 10 Sep 2010 15:57:30 -0400 Received: by eyf18 with SMTP id 18so2244002eyf.4 for ; Fri, 10 Sep 2010 12:57:29 -0700 (PDT) Date: Fri, 10 Sep 2010 21:57:26 +0200 From: "Edgar E. Iglesias" Subject: Re: [Qemu-devel] [PATCH] target-ppc: clear MSR_POW on interrupt Message-ID: <20100910195726.GC3280@laped.lan> References: <20100721085316.10710.28700.malonedeb@soybean.canonical.com> <20100910160835.GA2276@edde.se.axis.com> <201009101858.11195.thomas.monjalon@openwide.fr> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201009101858.11195.thomas.monjalon@openwide.fr> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Monjalon Cc: qemu-devel@nongnu.org, Alexander Graf 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" > : > > > 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 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 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;