Re: [PATCH] kvm: kvmclock: let KVM_GET_CLOCK return whether the master clock is in use

From: Marcelo Tosatti
Date: Wed Nov 09 2016 - 15:13:22 EST


On Wed, Nov 09, 2016 at 05:48:15PM +0100, Paolo Bonzini wrote:
> Userspace can read the exact value of kvmclock by reading the TSC
> and fetching the timekeeping parameters out of guest memory. This
> however is brittle and not necessary anymore with KVM 4.11.

Hi Paolo,

Can you point to commit or explanation why that is not the case anymore?
Thanks

> Provide
> a mechanism that lets userspace know if the new KVM_GET_CLOCK
> semantics are in effect, and---since we are at it---if the clock
> is stable across all VCPUs.
>
> Cc: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> Cc: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> ---
> Documentation/virtual/kvm/api.txt | 11 +++++++++++
> arch/x86/kvm/x86.c | 10 +++++++---
> include/uapi/linux/kvm.h | 7 +++++++
> 3 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 739db9ab16b2..6bbceb9a3a19 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -777,6 +777,17 @@ Gets the current timestamp of kvmclock as seen by the current guest. In
> conjunction with KVM_SET_CLOCK, it is used to ensure monotonicity on scenarios
> such as migration.
>
> +When KVM_CAP_ADJUST_CLOCK is passed to KVM_CHECK_EXTENSION, it returns the
> +set of bits that KVM can return in struct kvm_clock_data's flag member.
> +
> +The only flag defined now is KVM_CLOCK_TSC_STABLE. If set, the returned
> +value is the exact kvmclock value seen by all VCPUs at the instant
> +when KVM_GET_CLOCK was called. If clear, the returned value is simply
> +CLOCK_MONOTONIC plus a constant offset; the offset can be modified
> +with KVM_SET_CLOCK. KVM will try to make all VCPUs follow this clock,
> +but the exact value read by each VCPU could differ, because the host
> +TSC is not stable.
> +
> struct kvm_clock_data {
> __u64 clock; /* kvmclock current value */
> __u32 flags;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 3017de0431bd..1ba08278a9a9 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2596,7 +2596,6 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> case KVM_CAP_PIT_STATE2:
> case KVM_CAP_SET_IDENTITY_MAP_ADDR:
> case KVM_CAP_XEN_HVM:
> - case KVM_CAP_ADJUST_CLOCK:
> case KVM_CAP_VCPU_EVENTS:
> case KVM_CAP_HYPERV:
> case KVM_CAP_HYPERV_VAPIC:
> @@ -2623,6 +2622,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> #endif
> r = 1;
> break;
> + case KVM_CAP_ADJUST_CLOCK:
> + r = KVM_CLOCK_TSC_STABLE;
> + break;
> case KVM_CAP_X86_SMM:
> /* SMBASE is usually relocated above 1M on modern chipsets,
> * and SMM handlers might indeed rely on 4G segment limits,
> @@ -4103,9 +4105,11 @@ long kvm_arch_vm_ioctl(struct file *filp,
> struct kvm_clock_data user_ns;
> u64 now_ns;
>
> - now_ns = get_kvmclock_ns(kvm);
> + local_irq_disable();
> + now_ns = __get_kvmclock_ns(kvm);
> user_ns.clock = now_ns;
> - user_ns.flags = 0;
> + user_ns.flags = kvm->arch.use_master_clock ? KVM_CLOCK_TSC_STABLE : 0;
> + local_irq_enable();
> memset(&user_ns.pad, 0, sizeof(user_ns.pad));
>
> r = -EFAULT;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 300ef255d1e0..4ee67cb99143 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -972,12 +972,19 @@ struct kvm_irqfd {
> __u8 pad[16];
> };
>
> +/* For KVM_CAP_ADJUST_CLOCK */
> +
> +/* Do not use 1, KVM_CHECK_EXTENSION returned it before we had flags. */
> +#define KVM_CLOCK_TSC_STABLE 2
> +
> struct kvm_clock_data {
> __u64 clock;
> __u32 flags;
> __u32 pad[9];
> };
>
> +/* For KVM_CAP_SW_TLB */
> +
> #define KVM_MMU_FSL_BOOKE_NOHV 0
> #define KVM_MMU_FSL_BOOKE_HV 1
>
> --
> 1.8.3.1