On 08.03.2012, at 05:18, Yoder Stuart-B08248 wrote: > > >> -----Original Message----- >> From: Alexander Graf [mailto:agraf@xxxxxxx] >> Sent: Wednesday, March 07, 2012 5:39 PM >> To: Wood Scott-B07421 >> Cc: Yoder Stuart-B08248; kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx >> 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" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
