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

[Alexander Graf]

On 10.09.2010, at 18:58, Thomas Monjalon wrote:

> Alexander Graf wrote:
>> 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
> 
> 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