Am 10.09.2010 um 18:08 schrieb "Edgar E. Iglesias" <edgar.igles...@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 <608...@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 >
