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

[Glauber Costa <glommer@parallels.com>]

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.


> 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?

I believe only the existing MSRs should be used for this. If you write
to it, and the host is capable of exporting userspace pvclock
information, then it does. If it isn't, then it doesn't.

No reason for the extra setup that is only likely to bring more headache.


> Index: vsyscall/arch/x86/kvm/cpuid.c
> ===================================================================
> --- vsyscall.orig/arch/x86/kvm/cpuid.c
> +++ vsyscall/arch/x86/kvm/cpuid.c
> @@ -411,7 +411,9 @@ static int do_cpuid_ent(struct kvm_cpuid
>  			     (1 << KVM_FEATURE_CLOCKSOURCE2) |
>  			     (1 << KVM_FEATURE_ASYNC_PF) |
>  			     (1 << KVM_FEATURE_PV_EOI) |
> -			     (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
> +			     (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT) |
> +			     (1 << KVM_FEATURE_USERSPACE_CLOCKSOURCE);
> +
>  

The feature bit itself, is obviously fine.