> -----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); + 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. Stuart -- 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
