On 10.09.2010, at 18:58, Thomas Monjalon wrote: > Alexander Graf wrote: >> 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 > > 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.
I'm really torn on this one. While you're right, I'd really like to see both issues fixed. To be honest, I don't think the whole function as is makes too much sense. Why would the MSR of an exception handler be based off the MSR we had before? Alex
