Re: [PATCH v5 4/9] KVM-HV: KVM Steal time implementation

[Glauber Costa <glommer@redhat.com>]

On 07/11/2011 09:58 AM, Avi Kivity wrote:
> On 07/07/2011 08:07 PM, Glauber Costa wrote:
>>>> +static void record_steal_time(struct kvm_vcpu *vcpu)
>>>> +{
>>>> + u64 delta;
>>>> +
>>>> + if (!(vcpu->arch.st.msr_val& KVM_MSR_ENABLED))
>>>> + return;
>>>> +
>>>> + if (unlikely(kvm_read_guest_cached(vcpu->kvm,&vcpu->arch.st.stime,
>>>> + &vcpu->arch.st.steal, sizeof(struct kvm_steal_time))))
>>>> + return;
>>>
>>> The guest memory page is not pinned, sleeping via
>>> __copy_from_user/to_user is not allowed in vcpu_load context. Either pin
>>> it or use atomic acessors.
>>
>>
>> I do recognize the problem.
>> Avi, what's your take here?
>>
>
> The easiest solution is to set a KVM_REQ bit in atomic context, and move
> the sleepy code to vcpu_enter_guest().

Or I can move it all inside vcpu_run, or close enough to it. This will 
account more hypervisor time as steal time, but it seemed to be what 
some people wanted in the first place.

Given the simplification we would win - not needing a REQ set, it might 
be worth it.

>>>> + case MSR_KVM_STEAL_TIME:
>>>> + vcpu->arch.st.msr_val = data;
>>>> +
>>>> + if (!(data& KVM_MSR_ENABLED)) {
>>>> + break;
>>>> + }
>>>
>>> On failure below this point, msr_val should be cleared of
>>> KVM_MSR_ENABLED?
>> No, msr_val has to hold whatever the guest wrote into it.
>> We should probably use an independent variable here to indicate that
>> we failed to activate it.
>
> If we fail, we return a #GP to the guest (and don't write any value into
> the msr).
>