Patchwork [16/38] KVM: PPC: BookE: Add check_requests helper function

login
register
mail settings
Submitter Alexander Graf
Date Aug. 14, 2012, 11:04 p.m.
Message ID <1344985483-7440-17-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/177500/
State New
Headers show

Comments

Alexander Graf - Aug. 14, 2012, 11:04 p.m.
We need a central place to check for pending requests in. Add one that
only does the timer check we already do in a different place.

Later, this central function can be extended by more checks.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/kvm/booke.c |   24 +++++++++++++++++-------
 1 files changed, 17 insertions(+), 7 deletions(-)
Scott Wood - Aug. 15, 2012, 12:10 a.m.
On 08/14/2012 06:04 PM, Alexander Graf wrote:
> We need a central place to check for pending requests in. Add one that
> only does the timer check we already do in a different place.
> 
> Later, this central function can be extended by more checks.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/powerpc/kvm/booke.c |   24 +++++++++++++++++-------
>  1 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 1d4ce9a..bcf87fe 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -419,13 +419,6 @@ static void kvmppc_core_check_exceptions(struct kvm_vcpu *vcpu)
>  	unsigned long *pending = &vcpu->arch.pending_exceptions;
>  	unsigned int priority;
>  
> -	if (vcpu->requests) {
> -		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
> -			smp_mb();
> -			update_timer_ints(vcpu);
> -		}
> -	}
> -
>  	priority = __ffs(*pending);
>  	while (priority < BOOKE_IRQPRIO_MAX) {
>  		if (kvmppc_booke_irqprio_deliver(vcpu, priority))
> @@ -461,6 +454,14 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
>  	return r;
>  }
>  
> +static void kvmppc_check_requests(struct kvm_vcpu *vcpu)
> +{
> +	if (vcpu->requests) {
> +		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu))
> +			update_timer_ints(vcpu);
> +	}
> +}
> +
>  /*
>   * Common checks before entering the guest world.  Call with interrupts
>   * disabled.
> @@ -485,6 +486,15 @@ static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>  			break;
>  		}
>  
> +		smp_mb();
> +		if (vcpu->requests) {
> +			/* Make sure we process requests preemptable */
> +			local_irq_enable();
> +			kvmppc_check_requests(vcpu);
> +			local_irq_disable();
> +			continue;
> +		}

What previous memory access is the smp_mb() ordering against?

-Scott


--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - Aug. 15, 2012, 12:13 a.m.
On 15.08.2012, at 02:10, Scott Wood wrote:

> On 08/14/2012 06:04 PM, Alexander Graf wrote:
>> We need a central place to check for pending requests in. Add one that
>> only does the timer check we already do in a different place.
>> 
>> Later, this central function can be extended by more checks.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> arch/powerpc/kvm/booke.c |   24 +++++++++++++++++-------
>> 1 files changed, 17 insertions(+), 7 deletions(-)
>> 
>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>> index 1d4ce9a..bcf87fe 100644
>> --- a/arch/powerpc/kvm/booke.c
>> +++ b/arch/powerpc/kvm/booke.c
>> @@ -419,13 +419,6 @@ static void kvmppc_core_check_exceptions(struct kvm_vcpu *vcpu)
>> 	unsigned long *pending = &vcpu->arch.pending_exceptions;
>> 	unsigned int priority;
>> 
>> -	if (vcpu->requests) {
>> -		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
>> -			smp_mb();
>> -			update_timer_ints(vcpu);
>> -		}
>> -	}
>> -
>> 	priority = __ffs(*pending);
>> 	while (priority < BOOKE_IRQPRIO_MAX) {
>> 		if (kvmppc_booke_irqprio_deliver(vcpu, priority))
>> @@ -461,6 +454,14 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
>> 	return r;
>> }
>> 
>> +static void kvmppc_check_requests(struct kvm_vcpu *vcpu)
>> +{
>> +	if (vcpu->requests) {
>> +		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu))
>> +			update_timer_ints(vcpu);
>> +	}
>> +}
>> +
>> /*
>>  * Common checks before entering the guest world.  Call with interrupts
>>  * disabled.
>> @@ -485,6 +486,15 @@ static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>> 			break;
>> 		}
>> 
>> +		smp_mb();
>> +		if (vcpu->requests) {
>> +			/* Make sure we process requests preemptable */
>> +			local_irq_enable();
>> +			kvmppc_check_requests(vcpu);
>> +			local_irq_disable();
>> +			continue;
>> +		}
> 
> What previous memory access is the smp_mb() ordering against?

Against itself really (see the continue?). I might have just gotten this wrong though :).


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood - Aug. 15, 2012, 12:20 a.m.
On 08/14/2012 07:13 PM, Alexander Graf wrote:
> 
> On 15.08.2012, at 02:10, Scott Wood wrote:
> 
>> On 08/14/2012 06:04 PM, Alexander Graf wrote:
>>> We need a central place to check for pending requests in. Add one that
>>> only does the timer check we already do in a different place.
>>>
>>> Later, this central function can be extended by more checks.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>> arch/powerpc/kvm/booke.c |   24 +++++++++++++++++-------
>>> 1 files changed, 17 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>>> index 1d4ce9a..bcf87fe 100644
>>> --- a/arch/powerpc/kvm/booke.c
>>> +++ b/arch/powerpc/kvm/booke.c
>>> @@ -419,13 +419,6 @@ static void kvmppc_core_check_exceptions(struct kvm_vcpu *vcpu)
>>> 	unsigned long *pending = &vcpu->arch.pending_exceptions;
>>> 	unsigned int priority;
>>>
>>> -	if (vcpu->requests) {
>>> -		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
>>> -			smp_mb();
>>> -			update_timer_ints(vcpu);
>>> -		}
>>> -	}
>>> -
>>> 	priority = __ffs(*pending);
>>> 	while (priority < BOOKE_IRQPRIO_MAX) {
>>> 		if (kvmppc_booke_irqprio_deliver(vcpu, priority))
>>> @@ -461,6 +454,14 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
>>> 	return r;
>>> }
>>>
>>> +static void kvmppc_check_requests(struct kvm_vcpu *vcpu)
>>> +{
>>> +	if (vcpu->requests) {
>>> +		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu))
>>> +			update_timer_ints(vcpu);
>>> +	}
>>> +}
>>> +
>>> /*
>>>  * Common checks before entering the guest world.  Call with interrupts
>>>  * disabled.
>>> @@ -485,6 +486,15 @@ static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>>> 			break;
>>> 		}
>>>
>>> +		smp_mb();
>>> +		if (vcpu->requests) {
>>> +			/* Make sure we process requests preemptable */
>>> +			local_irq_enable();
>>> +			kvmppc_check_requests(vcpu);
>>> +			local_irq_disable();
>>> +			continue;
>>> +		}
>>
>> What previous memory access is the smp_mb() ordering against?
> 
> Against itself really (see the continue?). I might have just gotten this wrong though :).

Ordering is already ensured by the hardware in that case since it's the
same address -- and the stuff inside the if statement is more than
enough to ensure the compiler doesn't cache an old value of vcpu->requests.

That said, I think the problem is not that the smp_mb() shouldn't be
there, but that there should be a vcpu->mode setting before it.

-Scott


--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marcelo Tosatti - Aug. 15, 2012, 6:28 p.m.
On Tue, Aug 14, 2012 at 07:10:43PM -0500, Scott Wood wrote:
> On 08/14/2012 06:04 PM, Alexander Graf wrote:
> > We need a central place to check for pending requests in. Add one that
> > only does the timer check we already do in a different place.
> > 
> > Later, this central function can be extended by more checks.
> > 
> > Signed-off-by: Alexander Graf <agraf@suse.de>
> > ---
> >  arch/powerpc/kvm/booke.c |   24 +++++++++++++++++-------
> >  1 files changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> > index 1d4ce9a..bcf87fe 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -419,13 +419,6 @@ static void kvmppc_core_check_exceptions(struct kvm_vcpu *vcpu)
> >  	unsigned long *pending = &vcpu->arch.pending_exceptions;
> >  	unsigned int priority;
> >  
> > -	if (vcpu->requests) {
> > -		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
> > -			smp_mb();
> > -			update_timer_ints(vcpu);
> > -		}
> > -	}
> > -
> >  	priority = __ffs(*pending);
> >  	while (priority < BOOKE_IRQPRIO_MAX) {
> >  		if (kvmppc_booke_irqprio_deliver(vcpu, priority))
> > @@ -461,6 +454,14 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
> >  	return r;
> >  }
> >  
> > +static void kvmppc_check_requests(struct kvm_vcpu *vcpu)
> > +{
> > +	if (vcpu->requests) {
> > +		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu))
> > +			update_timer_ints(vcpu);
> > +	}
> > +}
> > +
> >  /*
> >   * Common checks before entering the guest world.  Call with interrupts
> >   * disabled.
> > @@ -485,6 +486,15 @@ static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
> >  			break;
> >  		}
> >  
> > +		smp_mb();
> > +		if (vcpu->requests) {
> > +			/* Make sure we process requests preemptable */
> > +			local_irq_enable();
> > +			kvmppc_check_requests(vcpu);
> > +			local_irq_disable();
> > +			continue;
> > +		}
> 
> What previous memory access is the smp_mb() ordering against?
> 
> -Scott

It is good practice to document each memory barrier.

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 1d4ce9a..bcf87fe 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -419,13 +419,6 @@  static void kvmppc_core_check_exceptions(struct kvm_vcpu *vcpu)
 	unsigned long *pending = &vcpu->arch.pending_exceptions;
 	unsigned int priority;
 
-	if (vcpu->requests) {
-		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
-			smp_mb();
-			update_timer_ints(vcpu);
-		}
-	}
-
 	priority = __ffs(*pending);
 	while (priority < BOOKE_IRQPRIO_MAX) {
 		if (kvmppc_booke_irqprio_deliver(vcpu, priority))
@@ -461,6 +454,14 @@  int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
 	return r;
 }
 
+static void kvmppc_check_requests(struct kvm_vcpu *vcpu)
+{
+	if (vcpu->requests) {
+		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu))
+			update_timer_ints(vcpu);
+	}
+}
+
 /*
  * Common checks before entering the guest world.  Call with interrupts
  * disabled.
@@ -485,6 +486,15 @@  static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
 			break;
 		}
 
+		smp_mb();
+		if (vcpu->requests) {
+			/* Make sure we process requests preemptable */
+			local_irq_enable();
+			kvmppc_check_requests(vcpu);
+			local_irq_disable();
+			continue;
+		}
+
 		if (kvmppc_core_prepare_to_enter(vcpu)) {
 			/* interrupts got enabled in between, so we
 			   are back at square 1 */