[PATCH v7 1/5] ptp_kvm: probe for kvm guest availability

[PATCH v7 1/5] ptp_kvm: probe for kvm guest availability

[Joao Martins]

In the event of moving pvclock_pvti_cpu0_va() definition to common
pvclock code, this function could return a value on non KVM guests.
If user tried to load the module (or have it builtin) it would fail
with a GPF on ptp_kvm_init when running on a Xen guest. Therefore,
ptp_kvm_init() should check whether it is running in a KVM guest.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
New in v7;
---
 drivers/ptp/ptp_kvm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/ptp/ptp_kvm.c b/drivers/ptp/ptp_kvm.c
index 2b1b212c219e..e04d7b2ecb3a 100644
--- a/drivers/ptp/ptp_kvm.c
+++ b/drivers/ptp/ptp_kvm.c
@@ -178,6 +178,9 @@ static int __init ptp_kvm_init(void)
 {
 	long ret;
 
+	if (!kvm_para_available())
+		return -ENODEV;
+
 	clock_pair_gpa = slow_virt_to_phys(&clock_pair);
 	hv_clock = pvclock_pvti_cpu0_va();
 
-- 
2.11.0

[PATCH v7 2/5] x86/pvclock: add setter for pvclock_pvti_cpu0_va

[Joao Martins]

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(-)

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);
-- 
2.11.0

[PATCH v7 5/5] MAINTAINERS: xen, kvm: track pvclock-abi.h changes

[Joao Martins]

This file defines an ABI shared between guest and hypervisor(s)
(KVM, Xen) and as such there should be an correspondent entry in
MAINTAINERS file. Notice that there's already a text notice at the
top of the header file, hence this commit simply enforces it more
explicitly and have both peers noticed when such changes happen.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Acked-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
Changes since v4:
 * Add Paolo's Acked-by
 * Add Konrad's Reviewed-by

Changes since v1:
 * Add Juergen's Gross Acked-by.
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index a74227ad082e..09de17b955ea 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7604,6 +7604,7 @@ S:	Supported
 F:	arch/x86/kvm/
 F:	arch/x86/include/uapi/asm/kvm*
 F:	arch/x86/include/asm/kvm*
+F:	arch/x86/include/asm/pvclock-abi.h
 F:	arch/x86/kernel/kvm.c
 F:	arch/x86/kernel/kvmclock.c
 
@@ -14731,6 +14732,7 @@ F:	arch/x86/xen/
 F:	drivers/*/xen-*front.c
 F:	drivers/xen/
 F:	arch/x86/include/asm/xen/
+F:	arch/x86/include/asm/pvclock-abi.h
 F:	include/xen/
 F:	include/uapi/xen/
 F:	Documentation/ABI/stable/sysfs-hypervisor-xen
-- 
2.11.0

Re: [PATCH v7 1/5] ptp_kvm: probe for kvm guest availability

[Radim Krcmar]

2017-10-19 14:39+0100, Joao Martins:
> In the event of moving pvclock_pvti_cpu0_va() definition to common
> pvclock code, this function could return a value on non KVM guests.
> If user tried to load the module (or have it builtin) it would fail
> with a GPF on ptp_kvm_init when running on a Xen guest. Therefore,
> ptp_kvm_init() should check whether it is running in a KVM guest.
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
> New in v7;
> ---

Acked-by: Radim Krčmář <rkrcmar@redhat.com>

Re: [PATCH v7 2/5] x86/pvclock: add setter for pvclock_pvti_cpu0_va

[Paolo Bonzini]

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

Re: [PATCH v7 2/5] x86/pvclock: add setter for pvclock_pvti_cpu0_va

[Joao Martins]

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

Re: [PATCH v7 2/5] x86/pvclock: add setter for pvclock_pvti_cpu0_va

[Thomas Gleixner]

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>

Re: [PATCH v7 2/5] x86/pvclock: add setter for pvclock_pvti_cpu0_va

[Joao Martins]

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!

Re: [PATCH v7 2/5] x86/pvclock: add setter for pvclock_pvti_cpu0_va

[Thomas Gleixner]

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