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

[Alexander Graf]

Am 10.09.2010 um 18:08 schrieb "Edgar E. Iglesias" <address@hidden>:

> 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 <address@hidden>
>>>>>>> 
>>>>>>> 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

>