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

[Scott Wood]

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?

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

And I'd rather see any fix for this problem stay out of the asm code.

> b. bl      kvmppc_handle_exit
> 
> c. kvmppc_handle_exit()
> {
>         int r = RESUME_HOST;
>         int s;
> 
>         /* update before a new last_exit_type is rewritten */
>         kvmppc_update_timing_stats(vcpu);
> 
>         /* restart interrupts if they were meant for the host */
>         kvmppc_restart_interrupt(vcpu, exit_nr);
> 
>         local_irq_enable();	==> Enable again.
> ....
> 
> And shouldn't we handle kvmppc_restart_interrupt() like the original  
> HOST flow?
> 
> #define MASKABLE_EXCEPTION(trapnum, intnum, label, hdlr,  
> ack)           \
>          
> START_EXCEPTION(label);                                         \
>         NORMAL_EXCEPTION_PROLOG(trapnum, intnum,  
> PROLOG_ADDITION_MASKABLE)\
>         EXCEPTION_COMMON(trapnum, PACA_EXGEN,  
> *INTS_DISABLE*)             \
> 	...

Could you elaborate on what you mean?

-Scott