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

[Glauber Costa <glommer@parallels.com>]

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.