Patchwork [v9,2/4] KVM: PPC: epapr: Add idle hcall support for host

login
register
mail settings
Submitter Yoder Stuart-B08248
Date March 8, 2012, 4:18 a.m.
Message ID <9F6FE96B71CF29479FF1CDC8046E150331148C@039-SN1MPN1-003.039d.mgd.msft.net>
Download mbox | patch
Permalink /patch/145448/
State New
Headers show

Comments

Yoder Stuart-B08248 - March 8, 2012, 4:18 a.m.
> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Wednesday, March 07, 2012 5:39 PM
> To: Wood Scott-B07421
> Cc: Yoder Stuart-B08248; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH v9 2/4] KVM: PPC: epapr: Add idle hcall support for host
> 
> 
> On 08.03.2012, at 00:37, Scott Wood wrote:
> 
> > On 03/07/2012 05:27 PM, Alexander Graf wrote:
> >> On 08.03.2012, at 00:12, Stuart Yoder wrote:
> >>>
> >>> 	if (vcpu->requests) {
> >>> +		/* kvm_vcpu_block() sets KVM_REQ_UNHALT, but it is
> >>> + 		 * not cleared elsewhere as on x86.  Clear it here
> >>> + 		 * for now, otherwise we never go idle.
> >>> + 		 */
> >>> +		clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
> >>
> >> Shouldn't the same thing hit us on non-booke as well? Also, it sounds unrelated to me and probably
> shouldn't be in this patch.
> >
> > Until recently we didn't check for requests in kvm_arch_vcpu_runnable().
> >
> > And yes, book3s will need this too.

Where should this go?  Something like this?


> >>> 		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
> >>> 			smp_mb();
> >>> 			update_timer_ints(vcpu);
> >>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> >>> index ee489f4..2595916 100644
> >>> --- a/arch/powerpc/kvm/powerpc.c
> >>> +++ b/arch/powerpc/kvm/powerpc.c
> >>> @@ -48,8 +48,7 @@ static unsigned int perfmon_refcount;
> >>>
> >>> int kvm_arch_vcpu_runnable(struct kvm_vcpu *v) {
> >>> -	bool ret = !(v->arch.shared->msr & MSR_WE) ||
> >>> -		   !!(v->arch.pending_exceptions) ||
> >>> +	bool ret = !!(v->arch.pending_exceptions) ||
> >>> 		   v->requests;
> >>
> >> Huh?
> >
> > MSR_WE is not going to get set if the idle hcall is used, so this
> > check was preventing us from blocking.
> >
> > The check isn't needed anyway, as nothing can actually change MSR_WE
> > while we're in kvm_vcpu_block(), which is the only user of
> > kvm_arch_vcpu_runnable(), and the MSR_WE path won't call
> > kvm_vcpu_block() if MSR_WE isn't set.
> 
> Ah, this is only removing the MSR_WE check. Ok.

I'll add an additional comment to the patch description.

Stuart

--
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 - March 8, 2012, 12:20 p.m.
On 08.03.2012, at 05:18, Yoder Stuart-B08248 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Wednesday, March 07, 2012 5:39 PM
>> To: Wood Scott-B07421
>> Cc: Yoder Stuart-B08248; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
>> Subject: Re: [PATCH v9 2/4] KVM: PPC: epapr: Add idle hcall support for host
>> 
>> 
>> On 08.03.2012, at 00:37, Scott Wood wrote:
>> 
>>> On 03/07/2012 05:27 PM, Alexander Graf wrote:
>>>> On 08.03.2012, at 00:12, Stuart Yoder wrote:
>>>>> 
>>>>> 	if (vcpu->requests) {
>>>>> +		/* kvm_vcpu_block() sets KVM_REQ_UNHALT, but it is
>>>>> + 		 * not cleared elsewhere as on x86.  Clear it here
>>>>> + 		 * for now, otherwise we never go idle.
>>>>> + 		 */
>>>>> +		clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
>>>> 
>>>> Shouldn't the same thing hit us on non-booke as well? Also, it sounds unrelated to me and probably
>> shouldn't be in this patch.
>>> 
>>> Until recently we didn't check for requests in kvm_arch_vcpu_runnable().
>>> 
>>> And yes, book3s will need this too.
> 
> Where should this go?  Something like this?
> 
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -283,6 +283,8 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
>        /* Tell the guest about our interrupt status */
>        kvmppc_update_int_pending(vcpu, *pending, old_pending);
> 
> +       clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
> +

That should work, yes. Eventually we want to have in-kernel MPIC emulation and handle this properly, but for now that's probably the right approach :).

>        return 0;
> }
> 
> 
>>>>> 		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
>>>>> 			smp_mb();
>>>>> 			update_timer_ints(vcpu);
>>>>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>>>>> index ee489f4..2595916 100644
>>>>> --- a/arch/powerpc/kvm/powerpc.c
>>>>> +++ b/arch/powerpc/kvm/powerpc.c
>>>>> @@ -48,8 +48,7 @@ static unsigned int perfmon_refcount;
>>>>> 
>>>>> int kvm_arch_vcpu_runnable(struct kvm_vcpu *v) {
>>>>> -	bool ret = !(v->arch.shared->msr & MSR_WE) ||
>>>>> -		   !!(v->arch.pending_exceptions) ||
>>>>> +	bool ret = !!(v->arch.pending_exceptions) ||
>>>>> 		   v->requests;
>>>> 
>>>> Huh?
>>> 
>>> MSR_WE is not going to get set if the idle hcall is used, so this
>>> check was preventing us from blocking.
>>> 
>>> The check isn't needed anyway, as nothing can actually change MSR_WE
>>> while we're in kvm_vcpu_block(), which is the only user of
>>> kvm_arch_vcpu_runnable(), and the MSR_WE path won't call
>>> kvm_vcpu_block() if MSR_WE isn't set.
>> 
>> Ah, this is only removing the MSR_WE check. Ok.
> 
> I'll add an additional comment to the patch description.

I was merely misreading the patch, no worries.


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

Patch

--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -283,6 +283,8 @@  int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
        /* Tell the guest about our interrupt status */
        kvmppc_update_int_pending(vcpu, *pending, old_pending);

+       clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+
        return 0;
 }