2018-07-06 16:26:25

by Thomas Gleixner

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

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 <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Radim Krcmar <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Pavel Tatashin <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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 <asm/apic.h>
#include <asm/apicdef.h>
#include <asm/hypervisor.h>
-#include <asm/kvm_guest.h>

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 <linux/sched/clock.h>
#include <linux/mm.h>

+#include <asm/hypervisor.h>
#include <asm/mem_encrypt.h>
#include <asm/x86_init.h>
#include <asm/reboot.h>
#include <asm/kvmclock.h>

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;
-}




2018-07-06 17:44:20

by Paolo Bonzini

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

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

2018-07-06 19:25:37

by Thomas Gleixner

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

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.

2018-07-12 03:15:30

by Pavel Tatashin

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

+#include <asm/hypervisor.h>

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 <[email protected]>