Re: [patch 08/16] KVM: x86: introduce facility to support vsyscall pvclock, via MSR

[Glauber Costa <glommer@parallels.com>]

On 11/02/2012 05:00 PM, Marcelo Tosatti wrote:
> On Fri, Nov 02, 2012 at 02:23:06PM +0400, Glauber Costa wrote:
>> On 11/02/2012 01:39 AM, Marcelo Tosatti wrote:
>>> On Thu, Nov 01, 2012 at 06:28:31PM +0400, Glauber Costa wrote:
>>>> On 11/01/2012 02:47 AM, Marcelo Tosatti wrote:
>>>>> Allow a guest to register a second location for the VCPU time info
>>>>>
>>>>> structure for each vcpu (as described by MSR_KVM_SYSTEM_TIME_NEW).
>>>>> This is intended to allow the guest kernel to map this information
>>>>> into a usermode accessible page, so that usermode can efficiently
>>>>> calculate system time from the TSC without having to make a syscall.
>>>>>
>>>>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>>>>
>>>>
>>>> Changelog doesn't make a lot of sense. (specially from first line to the
>>>> second). Please add in here the reasons why we can't (or decided not to)
>>>> use the same page. The info in the last mail thread is good enough, just
>>>> put it here.
>>>
>>> Fixed.
>>>
>>>>> Index: vsyscall/arch/x86/include/asm/kvm_para.h
>>>>> ===================================================================
>>>>> --- vsyscall.orig/arch/x86/include/asm/kvm_para.h
>>>>> +++ vsyscall/arch/x86/include/asm/kvm_para.h
>>>>> @@ -23,6 +23,7 @@
>>>>>  #define KVM_FEATURE_ASYNC_PF		4
>>>>>  #define KVM_FEATURE_STEAL_TIME		5
>>>>>  #define KVM_FEATURE_PV_EOI		6
>>>>> +#define KVM_FEATURE_USERSPACE_CLOCKSOURCE 7
>>>>>  
>>>>>  /* The last 8 bits are used to indicate how to interpret the flags field
>>>>>   * in pvclock structure. If no bits are set, all flags are ignored.
>>>>> @@ -39,6 +40,7 @@
>>>>>  #define MSR_KVM_ASYNC_PF_EN 0x4b564d02
>>>>>  #define MSR_KVM_STEAL_TIME  0x4b564d03
>>>>>  #define MSR_KVM_PV_EOI_EN      0x4b564d04
>>>>> +#define MSR_KVM_USERSPACE_TIME      0x4b564d05
>>>>>  
>>>>
>>>> I accept that it is possible that we may be better off with the page
>>>> mapped twice.
>>>>
>>>> But why do we need an extra MSR? When, and why, would you enable the
>>>> kernel-based pvclock, but disabled the userspace pvclock?
>>>
>>> Because there is no stable TSC available, for example (which cannot
>>> be used to measure passage of time).
>>>
>>
>> What you say is true, but completely unrelated. I am not talking about
>> situations in which userspace pvclock is available and you end up not
>> using it.
>>
>> I am talking about situations in which it is available, you are capable
>> of using it, but then decides for some reason to permanently disabled -
>> as in not setting it up altogether.
>>
>> It seems to me that if the host has code to deal with userspace pvclock,
>> and you already coded the guest in a way that you may or may not use it
>> (dependent on the value of the stable bit), you could very well only
>> check for the cpuid flag, and do the guest setup if available - skipping
>> this MSR dance altogether.
>>
>> Now, of course, there is the problem of communicating the address in
>> which the guest expects the page to be. Skipping the MSR setup would
>> require it to be more or less at a fixed location. We could in principle
>> lay them down together with the already existing pvclock structure. (But
>> granted, I am not sure it is worth it...)
>>
>> I think in general, this question deserves a bit more of attention. We
>> are about to have just the perfect opportunity for this next week, so
>> let's use it.
> 
> In essence you are proposing a different interface to communicate the
> "userspace vsyscall pvclock area", other than the MSR, right?
> 
> If so:
> 
> 1. What is the problem with the MSR interface.
> 2. What advantage this new interface (which honestly i do not
> understand) provides?
> 
No, I am not proposing a different interface to communicate this.
I am proposing *no* interface to communicate this.

I'll give that if it is absolutely necessary to have it on a random
address, and you need to pass it back and forth, of course MSRs would be
the choice. But I am not yet terribly convinced that all the pain of
syncing this from userspace, migrating those values, bookkeeping what is
on, what is not, is worth the pain.