| Message ID | 20171019133918.18367-3-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19/10/2017 15:39, Joao Martins wrote: > Right now there is only a pvclock_pvti_cpu0_va() which is defined > on kvmclock since: > > commit dac16fba6fc5 > ("x86/vdso: Get pvclock data from the vvar VMA instead of the fixmap") > > The only user of this interface so far is kvm. This commit adds a > setter function for the pvti page and moves pvclock_pvti_cpu0_va > to pvclock, which is a more generic place to have it; and would > allow other PV clocksources to use it, such as Xen. > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > Acked-by: Andy Lutomirski <luto@kernel.org> Acked-by: Paolo Bonzini <pbonzini@redhat.com> IOW, the Xen folks are free to pick up the whole series. :) Paolo > --- > Changes since v1: > * Rebased: the only conflict was that I had move the export > pvclock_pvti_cpu0_va() symbol as it is used by kvm PTP driver. > * Do not initialize pvti_cpu0_va to NULL (checkpatch error) > ( Comments from Andy Lutomirski ) > * Removed asm/pvclock.h 'pvclock_set_pvti_cpu0_va' definition > for non !PARAVIRT_CLOCK to better track screwed Kconfig stuff. > * Add his Acked-by (provided the previous adjustment was made) > > Changes since RFC: > (Comments from Andy Lutomirski) > * Add __init to pvclock_set_pvti_cpu0_va > * Add WARN_ON(vclock_was_used(VCLOCK_PVCLOCK)) to > pvclock_set_pvti_cpu0_va > --- > arch/x86/include/asm/pvclock.h | 19 ++++++++++--------- > arch/x86/kernel/kvmclock.c | 7 +------ > arch/x86/kernel/pvclock.c | 14 ++++++++++++++ > 3 files changed, 25 insertions(+), 15 deletions(-) > > diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h > index 448cfe1b48cf..6f228f90cdd7 100644 > --- a/arch/x86/include/asm/pvclock.h > +++ b/arch/x86/include/asm/pvclock.h > @@ -4,15 +4,6 @@ > #include <linux/clocksource.h> > #include <asm/pvclock-abi.h> > > -#ifdef CONFIG_KVM_GUEST > -extern struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void); > -#else > -static inline struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void) > -{ > - return NULL; > -} > -#endif > - > /* some helper functions for xen and kvm pv clock sources */ > u64 pvclock_clocksource_read(struct pvclock_vcpu_time_info *src); > u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src); > @@ -101,4 +92,14 @@ struct pvclock_vsyscall_time_info { > > #define PVTI_SIZE sizeof(struct pvclock_vsyscall_time_info) > > +#ifdef CONFIG_PARAVIRT_CLOCK > +void pvclock_set_pvti_cpu0_va(struct pvclock_vsyscall_time_info *pvti); > +struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void); > +#else > +static inline struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void) > +{ > + return NULL; > +} > +#endif > + > #endif /* _ASM_X86_PVCLOCK_H */ > diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c > index d88967659098..538738047ff5 100644 > --- a/arch/x86/kernel/kvmclock.c > +++ b/arch/x86/kernel/kvmclock.c > @@ -47,12 +47,6 @@ early_param("no-kvmclock", parse_no_kvmclock); > static struct pvclock_vsyscall_time_info *hv_clock; > static struct pvclock_wall_clock wall_clock; > > -struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void) > -{ > - return hv_clock; > -} > -EXPORT_SYMBOL_GPL(pvclock_pvti_cpu0_va); > - > /* > * The wallclock is the time of day when we booted. Since then, some time may > * have elapsed since the hypervisor wrote the data. So we try to account for > @@ -334,6 +328,7 @@ int __init kvm_setup_vsyscall_timeinfo(void) > return 1; > } > > + pvclock_set_pvti_cpu0_va(hv_clock); > put_cpu(); > > kvm_clock.archdata.vclock_mode = VCLOCK_PVCLOCK; > diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c > index 5c3f6d6a5078..cb7d6d9c9c2d 100644 > --- a/arch/x86/kernel/pvclock.c > +++ b/arch/x86/kernel/pvclock.c > @@ -25,8 +25,10 @@ > > #include <asm/fixmap.h> > #include <asm/pvclock.h> > +#include <asm/vgtod.h> > > static u8 valid_flags __read_mostly = 0; > +static struct pvclock_vsyscall_time_info *pvti_cpu0_va __read_mostly; > > void pvclock_set_flags(u8 flags) > { > @@ -144,3 +146,15 @@ void pvclock_read_wallclock(struct pvclock_wall_clock *wall_clock, > > set_normalized_timespec(ts, now.tv_sec, now.tv_nsec); > } > + > +void pvclock_set_pvti_cpu0_va(struct pvclock_vsyscall_time_info *pvti) > +{ > + WARN_ON(vclock_was_used(VCLOCK_PVCLOCK)); > + pvti_cpu0_va = pvti; > +} > + > +struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void) > +{ > + return pvti_cpu0_va; > +} > +EXPORT_SYMBOL_GPL(pvclock_pvti_cpu0_va); >
On 11/06/2017 04:09 PM, Paolo Bonzini wrote: > On 19/10/2017 15:39, Joao Martins wrote: >> Right now there is only a pvclock_pvti_cpu0_va() which is defined >> on kvmclock since: >> >> commit dac16fba6fc5 >> ("x86/vdso: Get pvclock data from the vvar VMA instead of the fixmap") >> >> The only user of this interface so far is kvm. This commit adds a >> setter function for the pvti page and moves pvclock_pvti_cpu0_va >> to pvclock, which is a more generic place to have it; and would >> allow other PV clocksources to use it, such as Xen. >> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >> Acked-by: Andy Lutomirski <luto@kernel.org> > > Acked-by: Paolo Bonzini <pbonzini@redhat.com> > > IOW, the Xen folks are free to pick up the whole series. :) > Thank you! I guess only x86 maintainers Ack is left - any comments? Joao > Paolo > >> --- >> Changes since v1: >> * Rebased: the only conflict was that I had move the export >> pvclock_pvti_cpu0_va() symbol as it is used by kvm PTP driver. >> * Do not initialize pvti_cpu0_va to NULL (checkpatch error) >> ( Comments from Andy Lutomirski ) >> * Removed asm/pvclock.h 'pvclock_set_pvti_cpu0_va' definition >> for non !PARAVIRT_CLOCK to better track screwed Kconfig stuff. >> * Add his Acked-by (provided the previous adjustment was made) >> >> Changes since RFC: >> (Comments from Andy Lutomirski) >> * Add __init to pvclock_set_pvti_cpu0_va >> * Add WARN_ON(vclock_was_used(VCLOCK_PVCLOCK)) to >> pvclock_set_pvti_cpu0_va >> --- >> arch/x86/include/asm/pvclock.h | 19 ++++++++++--------- >> arch/x86/kernel/kvmclock.c | 7 +------ >> arch/x86/kernel/pvclock.c | 14 ++++++++++++++ >> 3 files changed, 25 insertions(+), 15 deletions(-) >> >> diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h >> index 448cfe1b48cf..6f228f90cdd7 100644 >> --- a/arch/x86/include/asm/pvclock.h >> +++ b/arch/x86/include/asm/pvclock.h >> @@ -4,15 +4,6 @@ >> #include <linux/clocksource.h> >> #include <asm/pvclock-abi.h> >> >> -#ifdef CONFIG_KVM_GUEST >> -extern struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void); >> -#else >> -static inline struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void) >> -{ >> - return NULL; >> -} >> -#endif >> - >> /* some helper functions for xen and kvm pv clock sources */ >> u64 pvclock_clocksource_read(struct pvclock_vcpu_time_info *src); >> u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src); >> @@ -101,4 +92,14 @@ struct pvclock_vsyscall_time_info { >> >> #define PVTI_SIZE sizeof(struct pvclock_vsyscall_time_info) >> >> +#ifdef CONFIG_PARAVIRT_CLOCK >> +void pvclock_set_pvti_cpu0_va(struct pvclock_vsyscall_time_info *pvti); >> +struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void); >> +#else >> +static inline struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void) >> +{ >> + return NULL; >> +} >> +#endif >> + >> #endif /* _ASM_X86_PVCLOCK_H */ >> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c >> index d88967659098..538738047ff5 100644 >> --- a/arch/x86/kernel/kvmclock.c >> +++ b/arch/x86/kernel/kvmclock.c >> @@ -47,12 +47,6 @@ early_param("no-kvmclock", parse_no_kvmclock); >> static struct pvclock_vsyscall_time_info *hv_clock; >> static struct pvclock_wall_clock wall_clock; >> >> -struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void) >> -{ >> - return hv_clock; >> -} >> -EXPORT_SYMBOL_GPL(pvclock_pvti_cpu0_va); >> - >> /* >> * The wallclock is the time of day when we booted. Since then, some time may >> * have elapsed since the hypervisor wrote the data. So we try to account for >> @@ -334,6 +328,7 @@ int __init kvm_setup_vsyscall_timeinfo(void) >> return 1; >> } >> >> + pvclock_set_pvti_cpu0_va(hv_clock); >> put_cpu(); >> >> kvm_clock.archdata.vclock_mode = VCLOCK_PVCLOCK; >> diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c >> index 5c3f6d6a5078..cb7d6d9c9c2d 100644 >> --- a/arch/x86/kernel/pvclock.c >> +++ b/arch/x86/kernel/pvclock.c >> @@ -25,8 +25,10 @@ >> >> #include <asm/fixmap.h> >> #include <asm/pvclock.h> >> +#include <asm/vgtod.h> >> >> static u8 valid_flags __read_mostly = 0; >> +static struct pvclock_vsyscall_time_info *pvti_cpu0_va __read_mostly; >> >> void pvclock_set_flags(u8 flags) >> { >> @@ -144,3 +146,15 @@ void pvclock_read_wallclock(struct pvclock_wall_clock *wall_clock, >> >> set_normalized_timespec(ts, now.tv_sec, now.tv_nsec); >> } >> + >> +void pvclock_set_pvti_cpu0_va(struct pvclock_vsyscall_time_info *pvti) >> +{ >> + WARN_ON(vclock_was_used(VCLOCK_PVCLOCK)); >> + pvti_cpu0_va = pvti; >> +} >> + >> +struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void) >> +{ >> + return pvti_cpu0_va; >> +} >> +EXPORT_SYMBOL_GPL(pvclock_pvti_cpu0_va); >> >
On Tue, 7 Nov 2017, Joao Martins wrote: > On 11/06/2017 04:09 PM, Paolo Bonzini wrote: > > On 19/10/2017 15:39, Joao Martins wrote: > >> Right now there is only a pvclock_pvti_cpu0_va() which is defined > >> on kvmclock since: > >> > >> commit dac16fba6fc5 > >> ("x86/vdso: Get pvclock data from the vvar VMA instead of the fixmap") > >> > >> The only user of this interface so far is kvm. This commit adds a > >> setter function for the pvti page and moves pvclock_pvti_cpu0_va > >> to pvclock, which is a more generic place to have it; and would > >> allow other PV clocksources to use it, such as Xen. > >> > >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > >> Acked-by: Andy Lutomirski <luto@kernel.org> > > > > Acked-by: Paolo Bonzini <pbonzini@redhat.com> > > > > IOW, the Xen folks are free to pick up the whole series. :) > > > Thank you! > > I guess only x86 maintainers Ack is left - any comments? The only nit-pick I have are the convoluted function names: pvclock_set_pvti_cpu0_va() pvclock_pvti_cpu0_va() What on earth does that mean? Aside of that can you please make it at least symetric, i.e. _set_ and _get_ ? Other than that: Acked-by: Thomas Gleixner <tglx@linutronix.de>
On 11/08/2017 11:06 AM, Thomas Gleixner wrote: > On Tue, 7 Nov 2017, Joao Martins wrote: >> On 11/06/2017 04:09 PM, Paolo Bonzini wrote: >>> On 19/10/2017 15:39, Joao Martins wrote: >>>> Right now there is only a pvclock_pvti_cpu0_va() which is defined >>>> on kvmclock since: >>>> >>>> commit dac16fba6fc5 >>>> ("x86/vdso: Get pvclock data from the vvar VMA instead of the fixmap") >>>> >>>> The only user of this interface so far is kvm. This commit adds a >>>> setter function for the pvti page and moves pvclock_pvti_cpu0_va >>>> to pvclock, which is a more generic place to have it; and would >>>> allow other PV clocksources to use it, such as Xen. >>>> >>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >>>> Acked-by: Andy Lutomirski <luto@kernel.org> >>> >>> Acked-by: Paolo Bonzini <pbonzini@redhat.com> >>> >>> IOW, the Xen folks are free to pick up the whole series. :) >>> >> Thank you! >> >> I guess only x86 maintainers Ack is left - any comments? > > The only nit-pick I have are the convoluted function names: > > pvclock_set_pvti_cpu0_va() pvclock_pvti_cpu0_va() > > What on earth does that mean? > Those two functions respectively set and get in pvclock common code the address of a page for vCPU 0 containing time info (pvti, which is periodically updated by hypervisor). This region is guest memory and registered with hypervisor by guest PV clocksource and set in pvclock if certain conditions are met (i.e. PVCLOCK_TSC_STABLE_BIT is supported by hypervisor), and the getter is afterwards used by vdso and ptp_kvm. FWIW I merely followed the current style/code of the existent function but there could be a better name like "pvclock_set_data() pvclock_get_data()". Albeit the current names are more explicit on what we should expect to set or return from the functions. > Aside of that can you please make it at least symetric, i.e. _set_ and > _get_ ? > OK - Provided this is changing an exported symbol (pvclock_pvti_cpu0_va in use by ptp_kvm) and a non-functional change would you want me to address in a separate patch or it is OK to have in this one? > Other than that: > > Acked-by: Thomas Gleixner <tglx@linutronix.de> > Thanks!
On Wed, 8 Nov 2017, Joao Martins wrote: > On 11/08/2017 11:06 AM, Thomas Gleixner wrote: > > The only nit-pick I have are the convoluted function names: > > > > pvclock_set_pvti_cpu0_va() pvclock_pvti_cpu0_va() > > > > What on earth does that mean? > > > Those two functions respectively set and get in pvclock common code the address > of a page for vCPU 0 containing time info (pvti, which is periodically updated > by hypervisor). This region is guest memory and registered with hypervisor by > guest PV clocksource and set in pvclock if certain conditions are met (i.e. > PVCLOCK_TSC_STABLE_BIT is supported by hypervisor), and the getter is afterwards > used by vdso and ptp_kvm. > > FWIW I merely followed the current style/code of the existent function but there > could be a better name like "pvclock_set_data() pvclock_get_data()". Albeit the > current names are more explicit on what we should expect to set or return from > the functions. Fair enough. > > > Aside of that can you please make it at least symetric, i.e. _set_ and > > _get_ ? > > > OK - Provided this is changing an exported symbol (pvclock_pvti_cpu0_va in use > by ptp_kvm) and a non-functional change would you want me to address in a > separate patch or it is OK to have in this one? Just fixup the ptp_kvm call site in the very same patch. Thanks, tglx
diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h index 448cfe1b48cf..6f228f90cdd7 100644 --- a/arch/x86/include/asm/pvclock.h +++ b/arch/x86/include/asm/pvclock.h @@ -4,15 +4,6 @@ #include <linux/clocksource.h> #include <asm/pvclock-abi.h> -#ifdef CONFIG_KVM_GUEST -extern struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void); -#else -static inline struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void) -{ - return NULL; -} -#endif - /* some helper functions for xen and kvm pv clock sources */ u64 pvclock_clocksource_read(struct pvclock_vcpu_time_info *src); u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src); @@ -101,4 +92,14 @@ struct pvclock_vsyscall_time_info { #define PVTI_SIZE sizeof(struct pvclock_vsyscall_time_info) +#ifdef CONFIG_PARAVIRT_CLOCK +void pvclock_set_pvti_cpu0_va(struct pvclock_vsyscall_time_info *pvti); +struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void); +#else +static inline struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void) +{ + return NULL; +} +#endif + #endif /* _ASM_X86_PVCLOCK_H */ diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c index d88967659098..538738047ff5 100644 --- a/arch/x86/kernel/kvmclock.c +++ b/arch/x86/kernel/kvmclock.c @@ -47,12 +47,6 @@ early_param("no-kvmclock", parse_no_kvmclock); static struct pvclock_vsyscall_time_info *hv_clock; static struct pvclock_wall_clock wall_clock; -struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void) -{ - return hv_clock; -} -EXPORT_SYMBOL_GPL(pvclock_pvti_cpu0_va); - /* * The wallclock is the time of day when we booted. Since then, some time may * have elapsed since the hypervisor wrote the data. So we try to account for @@ -334,6 +328,7 @@ int __init kvm_setup_vsyscall_timeinfo(void) return 1; } + pvclock_set_pvti_cpu0_va(hv_clock); put_cpu(); kvm_clock.archdata.vclock_mode = VCLOCK_PVCLOCK; diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c index 5c3f6d6a5078..cb7d6d9c9c2d 100644 --- a/arch/x86/kernel/pvclock.c +++ b/arch/x86/kernel/pvclock.c @@ -25,8 +25,10 @@ #include <asm/fixmap.h> #include <asm/pvclock.h> +#include <asm/vgtod.h> static u8 valid_flags __read_mostly = 0; +static struct pvclock_vsyscall_time_info *pvti_cpu0_va __read_mostly; void pvclock_set_flags(u8 flags) { @@ -144,3 +146,15 @@ void pvclock_read_wallclock(struct pvclock_wall_clock *wall_clock, set_normalized_timespec(ts, now.tv_sec, now.tv_nsec); } + +void pvclock_set_pvti_cpu0_va(struct pvclock_vsyscall_time_info *pvti) +{ + WARN_ON(vclock_was_used(VCLOCK_PVCLOCK)); + pvti_cpu0_va = pvti; +} + +struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void) +{ + return pvti_cpu0_va; +} +EXPORT_SYMBOL_GPL(pvclock_pvti_cpu0_va);
Right now there is only a pvclock_pvti_cpu0_va() which is defined on kvmclock since: commit dac16fba6fc5 ("x86/vdso: Get pvclock data from the vvar VMA instead of the fixmap") The only user of this interface so far is kvm. This commit adds a setter function for the pvti page and moves pvclock_pvti_cpu0_va to pvclock, which is a more generic place to have it; and would allow other PV clocksources to use it, such as Xen. Signed-off-by: Joao Martins <joao.m.martins@oracle.com> Acked-by: Andy Lutomirski <luto@kernel.org> --- Changes since v1: * Rebased: the only conflict was that I had move the export pvclock_pvti_cpu0_va() symbol as it is used by kvm PTP driver. * Do not initialize pvti_cpu0_va to NULL (checkpatch error) ( Comments from Andy Lutomirski ) * Removed asm/pvclock.h 'pvclock_set_pvti_cpu0_va' definition for non !PARAVIRT_CLOCK to better track screwed Kconfig stuff. * Add his Acked-by (provided the previous adjustment was made) Changes since RFC: (Comments from Andy Lutomirski) * Add __init to pvclock_set_pvti_cpu0_va * Add WARN_ON(vclock_was_used(VCLOCK_PVCLOCK)) to pvclock_set_pvti_cpu0_va --- arch/x86/include/asm/pvclock.h | 19 ++++++++++--------- arch/x86/kernel/kvmclock.c | 7 +------ arch/x86/kernel/pvclock.c | 14 ++++++++++++++ 3 files changed, 25 insertions(+), 15 deletions(-)