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

tiejun.chen
2013-05-07T02:43:37Z

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.

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

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

In host handler, we always use MASKABLE_EXCEPTION() to define-to-handle some
exceptions: Doorbell interrupt/Decrementer Interrupt/External Input Interrupt:

#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*) \

This would call INTS_DISABLE, which is equal to SOFT_DISABLE_INTS(), to disable
soft interrupt before call all associated handlers:
do_IRQ()/timer_interrupt()/doorbell_exception().

But DO_KVM hook always skips INTS_DISABLE.

So I think we also need to do INTS_DISABLE for kvmppc_restart_interrupt() since
actually that restarts interrupts for the host with a similar way as they are
called by host.

Tiejun