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

[Alexander Graf]

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 :).


Alex