[RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[Scott Wood]

On 05/06/2013 09:43:37 PM, tiejun.chen wrote:
> On 05/07/2013 10:06 AM, Scott Wood wrote:
>> On 05/06/2013 08:56:25 PM, tiejun.chen wrote:
>>> On 05/07/2013 07:50 AM, Scott Wood wrote:
>>>> On 05/05/2013 10:13:17 PM, tiejun.chen wrote:
>>>>> On 05/06/2013 11:10 AM, Tiejun Chen wrote:
>>>>>> For the external interrupt, the decrementer exception and the  
>>>>>> doorbell
>>>>>> excpetion, we also need to soft-disable interrupts while doing  
>>>>>> as host
>>>>>> interrupt handlers since the DO_KVM hook is always performed to  
>>>>>> skip
>>>>>> EXCEPTION_COMMON then miss this original chance with the 'ints'
>>>>>> (INTS_DISABLE).
>>>> 
>>>> http://patchwork.ozlabs.org/patch/241344/
>>>> http://patchwork.ozlabs.org/patch/241412/
>>>> 
>>>> :-)
>>> 
>>> I'm observing the same behaviour as well:
>>> 
>>>     WARN_ON_ONCE(!irqs_disabled());
>> 
>> So, could you explain the benefits of your approach over what's  
>> being discussed
>> in those threads?
> 
> They're a long thread so I think I need to take time to see :)
> 
>> 
>>>> Why wouldn't we always disable them?  kvmppc_handle_exit() will  
>>>> enable
>>>> interrupts when it's ready.
>>> 
>>> This only disable soft interrupt for kvmppc_restart_interrupt()  
>>> that restarts
>>> interrupts if they were meant for the host:
>>> 
>>> a. SOFT_DISABLE_INTS() only for BOOKE_INTERRUPT_EXTERNAL |
>>> BOOKE_INTERRUPT_DECREMENTER | BOOKE_INTERRUPT_DOORBELL
>> 
>> Those aren't the only exceptions that can end up going to the host.   
>> We could
>> get a TLB miss that results in a heavyweight MMIO exit, etc.
> 
> This is like host handler, so I'm just disabling soft interrupt  
> during kvmppc_restart_interrupt() for Doorbell interrupt/Decrementer  
> Interrupt/External Input Interrupt.
> 
> I don't see anything should be disabled for any TLB exception in host  
> handler.

Every exception type needs consistent lazy EE state once we hit the  
local_irq_enable() (or any other C code that cares).

Plus, if you're going to add code to make something conditional, you  
should have a good reason for making it conditional.  Being more like  
the 64-bit host handler for its own sake isn't good enough, especially  
if you introduce differences between 32 and 64 bit in the process.

>> And I'd rather see any fix for this problem stay out of the asm code.
> 
> We already have an appropriate SOFT_DISABLE_INTS so I think we can  
> take this easily :)

An unconditional call to hard_irq_disable() at the beginning of  
kvmppc_handle_exit() should suffice.  I already have this in the next  
version of my patch that I'll be posting shortly.

Note that we need this on 32-bit as well, so that trace_hardirqs_off()  
gets called.

-Scott