[24/30] KVM: PPC: booke: call resched after every exit

Instead of checking whether we should reschedule only when we exited
due to an interrupt, let's always check before entering the guest back
again. This gets the target more in line with the other archs.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/kvm/booke.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

On 02/17/2012 11:13 AM, Alexander Graf wrote:
> Instead of checking whether we should reschedule only when we exited
> due to an interrupt, let's always check before entering the guest back
> again. This gets the target more in line with the other archs.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/powerpc/kvm/booke.c |   15 ++++++++++-----
>  1 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index bfb2092..de30b6d 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -572,6 +572,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>                         unsigned int exit_nr)
>  {
>  	int r = RESUME_HOST;
> +	int resched_needed = 1;
>  
>  	/* update before a new last_exit_type is rewritten */
>  	kvmppc_update_timing_stats(vcpu);
> @@ -602,25 +603,21 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  
>  	switch (exit_nr) {
>  	case BOOKE_INTERRUPT_MACHINE_CHECK:
> -		kvm_resched(vcpu);
>  		r = RESUME_GUEST;
>  		break;
>  
>  	case BOOKE_INTERRUPT_EXTERNAL:
>  		kvmppc_account_exit(vcpu, EXT_INTR_EXITS);
> -		kvm_resched(vcpu);
>  		r = RESUME_GUEST;
>  		break;
>  
>  	case BOOKE_INTERRUPT_DECREMENTER:
>  		kvmppc_account_exit(vcpu, DEC_EXITS);
> -		kvm_resched(vcpu);
>  		r = RESUME_GUEST;
>  		break;
>  
>  	case BOOKE_INTERRUPT_DOORBELL:
>  		kvmppc_account_exit(vcpu, DBELL_EXITS);
> -		kvm_resched(vcpu);
>  		r = RESUME_GUEST;
>  		break;
>  
> @@ -869,8 +866,16 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  		BUG();
>  	}
>  
> -	local_irq_disable();
> +	/* make sure we reschedule if we need to */
> +	while (resched_needed) {
> +		local_irq_disable();
>  
> +		resched_needed = need_resched();
> +		if (resched_needed) {
> +			local_irq_enable();
> +			cond_resched();
> +		}
> +	}
>  	kvmppc_core_prepare_to_enter(vcpu);
>  
>  	if (!(r & RESUME_HOST)) {

kvmppc_core_prepare_to_enter can enable interrupts (and block) if guest
MSR_WE is set.  We may take an interrupt that wants a resched after
waking but before interrupts are disabled again.

We also want to check for a resched in kvmppc_vcpu_run.  So, the resched
check belongs in kvmppc_core_prepare_to_enter, something like:

/* Check pending exceptions and deliver one, if possible. */
void kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
{
	WARN_ON_ONCE(!irqs_disabled());

	while (true) {
		if (signal_pending(current))
			break;

		if (need_resched()) {
			local_irq_enable();
			cond_resched();
			local_irq_disable();
			continue;
		}

		kvmppc_core_check_exceptions(vcpu);

		if (vcpu->arch.shared->msr & MSR_WE) {
			local_irq_enable();
			kvm_vcpu_block(vcpu);
			local_irq_disable();
	
			kvmppc_set_exit_type(vcpu,
				EMULATED_MTMSRWE_EXITS);
			continue;
		}

		break;
	}
}

It would be simpler (both here and in the idle hcall) if we could just
drop support for CONFIG_PREEMPT=n. :-P

-Scott

On 18.02.2012, at 00:00, Scott Wood wrote:

> On 02/17/2012 11:13 AM, Alexander Graf wrote:
>> Instead of checking whether we should reschedule only when we exited
>> due to an interrupt, let's always check before entering the guest back
>> again. This gets the target more in line with the other archs.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> arch/powerpc/kvm/booke.c |   15 ++++++++++-----
>> 1 files changed, 10 insertions(+), 5 deletions(-)
>> 
>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>> index bfb2092..de30b6d 100644
>> --- a/arch/powerpc/kvm/booke.c
>> +++ b/arch/powerpc/kvm/booke.c
>> @@ -572,6 +572,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>>                        unsigned int exit_nr)
>> {
>> 	int r = RESUME_HOST;
>> +	int resched_needed = 1;
>> 
>> 	/* update before a new last_exit_type is rewritten */
>> 	kvmppc_update_timing_stats(vcpu);
>> @@ -602,25 +603,21 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> 
>> 	switch (exit_nr) {
>> 	case BOOKE_INTERRUPT_MACHINE_CHECK:
>> -		kvm_resched(vcpu);
>> 		r = RESUME_GUEST;
>> 		break;
>> 
>> 	case BOOKE_INTERRUPT_EXTERNAL:
>> 		kvmppc_account_exit(vcpu, EXT_INTR_EXITS);
>> -		kvm_resched(vcpu);
>> 		r = RESUME_GUEST;
>> 		break;
>> 
>> 	case BOOKE_INTERRUPT_DECREMENTER:
>> 		kvmppc_account_exit(vcpu, DEC_EXITS);
>> -		kvm_resched(vcpu);
>> 		r = RESUME_GUEST;
>> 		break;
>> 
>> 	case BOOKE_INTERRUPT_DOORBELL:
>> 		kvmppc_account_exit(vcpu, DBELL_EXITS);
>> -		kvm_resched(vcpu);
>> 		r = RESUME_GUEST;
>> 		break;
>> 
>> @@ -869,8 +866,16 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> 		BUG();
>> 	}
>> 
>> -	local_irq_disable();
>> +	/* make sure we reschedule if we need to */
>> +	while (resched_needed) {
>> +		local_irq_disable();
>> 
>> +		resched_needed = need_resched();
>> +		if (resched_needed) {
>> +			local_irq_enable();
>> +			cond_resched();
>> +		}
>> +	}
>> 	kvmppc_core_prepare_to_enter(vcpu);
>> 
>> 	if (!(r & RESUME_HOST)) {
> 
> kvmppc_core_prepare_to_enter can enable interrupts (and block) if guest
> MSR_WE is set.  We may take an interrupt that wants a resched after
> waking but before interrupts are disabled again.
> 
> We also want to check for a resched in kvmppc_vcpu_run.  So, the resched
> check belongs in kvmppc_core_prepare_to_enter, something like:
> 
> /* Check pending exceptions and deliver one, if possible. */
> void kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
> {
> 	WARN_ON_ONCE(!irqs_disabled());
> 
> 	while (true) {
> 		if (signal_pending(current))
> 			break;
> 
> 		if (need_resched()) {
> 			local_irq_enable();
> 			cond_resched();
> 			local_irq_disable();
> 			continue;
> 		}
> 
> 		kvmppc_core_check_exceptions(vcpu);
> 
> 		if (vcpu->arch.shared->msr & MSR_WE) {
> 			local_irq_enable();
> 			kvm_vcpu_block(vcpu);
> 			local_irq_disable();
> 	
> 			kvmppc_set_exit_type(vcpu,
> 				EMULATED_MTMSRWE_EXITS);
> 			continue;
> 		}
> 
> 		break;
> 	}
> }
> 
> It would be simpler (both here and in the idle hcall) if we could just
> drop support for CONFIG_PREEMPT=n. :-P

When running with CONFIG_PREEMPT=n we don't have to worry about interrupts being enabled though, since we only preempt on known good checkpoints, right? While for CONFIG_PREEMPT=y we can always get preempted, rendering most of these checks void.

So essentially we could just be lazy and do a "best effort" resched check, but not worry about races wrt guest entry/exit, right? And for real preemption modes we don't have to worry about any of the resched stuff IIUC, correct?


Alex

On 02/20/2012 07:17 AM, Alexander Graf wrote:
> 
> On 18.02.2012, at 00:00, Scott Wood wrote:
> 
>> It would be simpler (both here and in the idle hcall) if we could just
>> drop support for CONFIG_PREEMPT=n. :-P
> 
> When running with CONFIG_PREEMPT=n we don't have to worry about interrupts being enabled though,

That's exactly when we *do* need to worry.  Interrupts can cause the
"need resched" condition to become true.

> since we only preempt on known good checkpoints, right?

Yes, and "entering the guest" is supposed to be one of those known good
checkpoints, similar to returning to userspace.  If we miss a reschedule
here, we're may not have another chance until the next timer interrupt.
 The code for returning to userspace is similarly structured.

-Scott