On 08.03.2012, at 05:18, Yoder Stuart-B08248 wrote:
>
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:[email protected]]
>> Sent: Wednesday, March 07, 2012 5:39 PM
>> To: Wood Scott-B07421
>> Cc: Yoder Stuart-B08248; [email protected]; [email protected]
>> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
