Re: [patch 6/7] x86/kvmclock: Move kvmclock vsyscall param and init to kvmclock

[Pavel Tatashin]

+#include 

Can be removed, because as Paolo, noticed X86_HYPER_KVM check is not needed.

> +static int __init kvm_setup_vsyscall_timeinfo(void)
> +{
> +#ifdef CONFIG_X86_64
> +   u8 flags;
> +
> +   if (!hv_clock || !kvmclock_vsyscall)
> +   return 0;
> +
> +   flags = pvclock_read_flags(&hv_clock[0].pvti);
> +   if (!(flags & PVCLOCK_TSC_STABLE_BIT))
> +   return 1;
> +
> +   pvclock_set_pvti_cpu0_va(hv_clock);
> +   kvm_clock.archdata.vclock_mode = VCLOCK_PVCLOCK;
> +#endif
> +   return 0;
> +}
> +early_initcall(kvm_setup_vsyscall_timeinfo);
> +

> -
> -int __init kvm_setup_vsyscall_timeinfo(void)
> -{
> -#ifdef CONFIG_X86_64
> -   u8 flags;
> -
> -   if (!hv_clock)
> -   return 0;
> -
> -   flags = pvclock_read_flags(&hv_clock[0].pvti);
> -   if (!(flags & PVCLOCK_TSC_STABLE_BIT))
> -   return 1;
> -
> -   pvclock_set_pvti_cpu0_va(hv_clock);
> -   kvm_clock.archdata.vclock_mode = VCLOCK_PVCLOCK;
> -#endif
> -   return 0;
> -}

I am not sure what the point of moving this function. The patch would
be much smaller without it.

Reviewed-by: Pavel Tatashin 

Re: [patch 6/7] x86/kvmclock: Move kvmclock vsyscall param and init to kvmclock

[Thomas Gleixner]

On Fri, 6 Jul 2018, Paolo Bonzini wrote:
> On 06/07/2018 18:13, Thomas Gleixner wrote:
> > @@ -241,6 +269,9 @@ void __init kvmclock_init(void)
> > return;
> > }
> >  
> > +   if (!hypervisor_is_type(X86_HYPER_KVM))
> > +   kvmclock_vsyscall = 0;
> > +
> 
> No need for this; by the time you get here, the condition will always be
> true.  And if you don't have kvmclock, hv_clock will be NULL and
> initialization will be skipped anyway in kvm_setup_vsyscall_timeinfo.

Right, stupid me.

Re: [patch 6/7] x86/kvmclock: Move kvmclock vsyscall param and init to kvmclock

[Paolo Bonzini]

On 06/07/2018 18:13, Thomas Gleixner wrote:
> @@ -241,6 +269,9 @@ void __init kvmclock_init(void)
>   return;
>   }
>  
> + if (!hypervisor_is_type(X86_HYPER_KVM))
> + kvmclock_vsyscall = 0;
> +

No need for this; by the time you get here, the condition will always be
true.  And if you don't have kvmclock, hv_clock will be NULL and
initialization will be skipped anyway in kvm_setup_vsyscall_timeinfo.

Thanks,

Paolo

[patch 6/7] x86/kvmclock: Move kvmclock vsyscall param and init to kvmclock

[Thomas Gleixner]

There is no point to have this in the kvm code itself and call it from
there. This can be called from an initcall and the parameter is cleared
when the hypervisor is not KVM.

Signed-off-by: Thomas Gleixner 
Cc: Paolo Bonzini 
Cc: Radim Krcmar 
Cc: Peter Zijlstra 
Cc: Juergen Gross 
Cc: Pavel Tatashin 
Cc: steven.sist...@oracle.com
Cc: daniel.m.jor...@oracle.com
Cc: x...@kernel.org
Cc: k...@vger.kernel.org
---
 arch/x86/include/asm/kvm_guest.h |7 -
 arch/x86/kernel/kvm.c|   13 --
 arch/x86/kernel/kvmclock.c   |   49 ---
 3 files changed, 31 insertions(+), 38 deletions(-)

--- a/arch/x86/include/asm/kvm_guest.h
+++ /dev/null
@@ -1,7 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef _ASM_X86_KVM_GUEST_H
-#define _ASM_X86_KVM_GUEST_H
-
-int kvm_setup_vsyscall_timeinfo(void);
-
-#endif /* _ASM_X86_KVM_GUEST_H */
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -45,7 +45,6 @@
 #include 
 #include 
 #include 
-#include 
 
 static int kvmapf = 1;
 
@@ -66,15 +65,6 @@ static int __init parse_no_stealacc(char
 
 early_param("no-steal-acc", parse_no_stealacc);
 
-static int kvmclock_vsyscall = 1;
-static int __init parse_no_kvmclock_vsyscall(char *arg)
-{
-kvmclock_vsyscall = 0;
-return 0;
-}
-
-early_param("no-kvmclock-vsyscall", parse_no_kvmclock_vsyscall);
-
 static DEFINE_PER_CPU_DECRYPTED(struct kvm_vcpu_pv_apf_data, apf_reason) 
__aligned(64);
 static DEFINE_PER_CPU_DECRYPTED(struct kvm_steal_time, steal_time) 
__aligned(64);
 static int has_steal_clock = 0;
@@ -560,9 +550,6 @@ static void __init kvm_guest_init(void)
if (kvm_para_has_feature(KVM_FEATURE_PV_EOI))
apic_set_eoi_write(kvm_guest_apic_eoi_write);
 
-   if (kvmclock_vsyscall)
-   kvm_setup_vsyscall_timeinfo();
-
 #ifdef CONFIG_SMP
smp_ops.smp_prepare_cpus = kvm_smp_prepare_cpus;
smp_ops.smp_prepare_boot_cpu = kvm_smp_prepare_boot_cpu;
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -27,12 +27,14 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
 #include 
 
 static int __initdata kvmclock = 1;
+static int __initdata kvmclock_vsyscall = 1;
 static int msr_kvm_system_time __ro_after_init = MSR_KVM_SYSTEM_TIME;
 static int msr_kvm_wall_clock __ro_after_init = MSR_KVM_WALL_CLOCK;
 static u64 kvm_sched_clock_offset __ro_after_init;
@@ -44,6 +46,13 @@ static int __init parse_no_kvmclock(char
 }
 early_param("no-kvmclock", parse_no_kvmclock);
 
+static int __init parse_no_kvmclock_vsyscall(char *arg)
+{
+   kvmclock_vsyscall = 0;
+   return 0;
+}
+early_param("no-kvmclock-vsyscall", parse_no_kvmclock_vsyscall);
+
 /* Aligned to page sizes to match whats mapped via vsyscalls to userspace */
 #define HV_CLOCK_SIZE  (sizeof(struct pvclock_vsyscall_time_info) * NR_CPUS)
 
@@ -227,6 +236,25 @@ static void kvm_shutdown(void)
native_machine_shutdown();
 }
 
+static int __init kvm_setup_vsyscall_timeinfo(void)
+{
+#ifdef CONFIG_X86_64
+   u8 flags;
+
+   if (!hv_clock || !kvmclock_vsyscall)
+   return 0;
+
+   flags = pvclock_read_flags(&hv_clock[0].pvti);
+   if (!(flags & PVCLOCK_TSC_STABLE_BIT))
+   return 1;
+
+   pvclock_set_pvti_cpu0_va(hv_clock);
+   kvm_clock.archdata.vclock_mode = VCLOCK_PVCLOCK;
+#endif
+   return 0;
+}
+early_initcall(kvm_setup_vsyscall_timeinfo);
+
 void __init kvmclock_init(void)
 {
u8 flags;
@@ -241,6 +269,9 @@ void __init kvmclock_init(void)
return;
}
 
+   if (!hypervisor_is_type(X86_HYPER_KVM))
+   kvmclock_vsyscall = 0;
+
pr_info("kvm-clock: Using msrs %x and %x",
msr_kvm_system_time, msr_kvm_wall_clock);
 
@@ -270,21 +301,3 @@ void __init kvmclock_init(void)
clocksource_register_hz(&kvm_clock, NSEC_PER_SEC);
pv_info.name = "KVM";
 }
-
-int __init kvm_setup_vsyscall_timeinfo(void)
-{
-#ifdef CONFIG_X86_64
-   u8 flags;
-
-   if (!hv_clock)
-   return 0;
-
-   flags = pvclock_read_flags(&hv_clock[0].pvti);
-   if (!(flags & PVCLOCK_TSC_STABLE_BIT))
-   return 1;
-
-   pvclock_set_pvti_cpu0_va(hv_clock);
-   kvm_clock.archdata.vclock_mode = VCLOCK_PVCLOCK;
-#endif
-   return 0;
-}