[PATCH] clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h

[PATCH] clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h

[Thomas Gleixner]

clocksource/hyperv_timer.h is included into the VDSO build. It includes
asm/mshyperv.h which in turn includes the world and some more.

This worked so far by chance, but any subtle change in the include chain
results in a build breakage because VDSO builds are building user space
libraries.

Include asm/hyperv-tlfs.h instead which contains everything what the VDSO
build needs and move the hv_get_raw_timer() define into the header file.

Fixup drivers/hv/vmbus_drv.c which relies on the indirect include of
asm/mshyperv.h.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/mshyperv.h    |    2 --
 drivers/hv/vmbus_drv.c             |    1 +
 include/clocksource/hyperv_timer.h |    4 +++-
 3 files changed, 4 insertions(+), 3 deletions(-)

--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -19,8 +19,6 @@ typedef int (*hyperv_fill_flush_list_fun
 		struct hv_guest_mapping_flush_list *flush,
 		void *data);
 
-#define hv_get_raw_timer() rdtsc_ordered()
-
 void hyperv_vector_handler(struct pt_regs *regs);
 
 #if IS_ENABLED(CONFIG_HYPERV)
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -37,6 +37,7 @@
 #include <linux/dma-map-ops.h>
 #include <linux/pci.h>
 #include <clocksource/hyperv_timer.h>
+#include <asm/mshyperv.h>
 #include "hyperv_vmbus.h"
 
 struct vmbus_dynid {
--- a/include/clocksource/hyperv_timer.h
+++ b/include/clocksource/hyperv_timer.h
@@ -15,7 +15,7 @@
 
 #include <linux/clocksource.h>
 #include <linux/math64.h>
-#include <asm/mshyperv.h>
+#include <asm/hyperv-tlfs.h>
 
 #define HV_MAX_MAX_DELTA_TICKS 0xffffffff
 #define HV_MIN_DELTA_TICKS 1
@@ -34,6 +34,8 @@ extern void hv_init_clocksource(void);
 
 extern struct ms_hyperv_tsc_page *hv_get_tsc_page(void);
 
+#define hv_get_raw_timer() rdtsc_ordered()
+
 static inline notrace u64
 hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg, u64 *cur_tsc)
 {

RE: [PATCH] clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h

[Michael Kelley (LINUX)]

From: Thomas Gleixner <tglx@linutronix.de> Sent: Saturday, November 12, 2022 11:03 AM
> 
> clocksource/hyperv_timer.h is included into the VDSO build. It includes
> asm/mshyperv.h which in turn includes the world and some more.
> 
> This worked so far by chance, but any subtle change in the include chain
> results in a build breakage because VDSO builds are building user space
> libraries.
> 
> Include asm/hyperv-tlfs.h instead which contains everything what the VDSO
> build needs and move the hv_get_raw_timer() define into the header file.

We've been keeping x86-isms out of hyperv_timer.c and hyperv_timer.h so
that they are architecture independent.  That's why the #define for
hv_get_raw_timer() is in an x86-specific include file.

But I can see the problem with too much getting dragged into the VDSO
builds.  If hv_get_raw_timer() is added to hyperv_timer.h, it should
be under #ifdef CONFIG_X86.  Adding an #ifdef isn't ideal, and a more
more proper solution might be to have a separate hyperv_timer.h include
file under arch/x86/include/asm.  But the latter seems like overkill for just
hv_get_raw_timer(), so I'm OK with the #ifdef.

Or does someone have a better idea?

Michael

> 
> Fixup drivers/hv/vmbus_drv.c which relies on the indirect include of
> asm/mshyperv.h.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/include/asm/mshyperv.h    |    2 --
>  drivers/hv/vmbus_drv.c             |    1 +
>  include/clocksource/hyperv_timer.h |    4 +++-
>  3 files changed, 4 insertions(+), 3 deletions(-)
> 
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -19,8 +19,6 @@ typedef int (*hyperv_fill_flush_list_fun
>  		struct hv_guest_mapping_flush_list *flush,
>  		void *data);
> 
> -#define hv_get_raw_timer() rdtsc_ordered()
> -
>  void hyperv_vector_handler(struct pt_regs *regs);
> 
>  #if IS_ENABLED(CONFIG_HYPERV)
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -37,6 +37,7 @@
>  #include <linux/dma-map-ops.h>
>  #include <linux/pci.h>
>  #include <clocksource/hyperv_timer.h>
> +#include <asm/mshyperv.h>
>  #include "hyperv_vmbus.h"
> 
>  struct vmbus_dynid {
> --- a/include/clocksource/hyperv_timer.h
> +++ b/include/clocksource/hyperv_timer.h
> @@ -15,7 +15,7 @@
> 
>  #include <linux/clocksource.h>
>  #include <linux/math64.h>
> -#include <asm/mshyperv.h>
> +#include <asm/hyperv-tlfs.h>
> 
>  #define HV_MAX_MAX_DELTA_TICKS 0xffffffff
>  #define HV_MIN_DELTA_TICKS 1
> @@ -34,6 +34,8 @@ extern void hv_init_clocksource(void);
> 
>  extern struct ms_hyperv_tsc_page *hv_get_tsc_page(void);
> 
> +#define hv_get_raw_timer() rdtsc_ordered()
> +
>  static inline notrace u64
>  hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg, u64 *cur_tsc)
>  {

RE: [PATCH] clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h

[Thomas Gleixner]

On Sat, Nov 12 2022 at 21:55, Michael Kelley wrote:
> But I can see the problem with too much getting dragged into the VDSO
> builds.  If hv_get_raw_timer() is added to hyperv_timer.h, it should
> be under #ifdef CONFIG_X86.  Adding an #ifdef isn't ideal, and a more
> more proper solution might be to have a separate hyperv_timer.h include
> file under arch/x86/include/asm.  But the latter seems like overkill for just
> hv_get_raw_timer(), so I'm OK with the #ifdef.

We surely can have asm/hyperv_timer.h but TBH:

>>  static inline notrace u64
>>  hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg, u64 *cur_tsc)
>>  {

hv_read_tsc_page_tsc() does not look architecture agnostic either. TSC
is pretty x86 specific :)

Thanks,

        tglx

RE: [PATCH] clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h

[Thomas Gleixner]

On Sun, Nov 13 2022 at 10:50, Thomas Gleixner wrote:
> On Sat, Nov 12 2022 at 21:55, Michael Kelley wrote:
>> But I can see the problem with too much getting dragged into the VDSO
>> builds.  If hv_get_raw_timer() is added to hyperv_timer.h, it should
>> be under #ifdef CONFIG_X86.  Adding an #ifdef isn't ideal, and a more
>> more proper solution might be to have a separate hyperv_timer.h include
>> file under arch/x86/include/asm.  But the latter seems like overkill for just
>> hv_get_raw_timer(), so I'm OK with the #ifdef.
>
> We surely can have asm/hyperv_timer.h but TBH:
>
>>>  static inline notrace u64
>>>  hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg, u64 *cur_tsc)
>>>  {
>
> hv_read_tsc_page_tsc() does not look architecture agnostic either. TSC
> is pretty x86 specific :)

Though the below makes sense on its own because it ensures that msr.h is
included, which is required for making clocksource/hyperv_timer.h self
contained.

Thanks,

        tglx
---
Subject: clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h
From: Thomas Gleixner <tglx@linutronix.de>
Date: Sat, 12 Nov 2022 19:08:15 +0100

clocksource/hyperv_timer.h is included into the VDSO build. It includes
asm/mshyperv.h which in turn includes the world and some more. This worked
so far by chance, but any subtle change in the include chain results in a
build breakage because VDSO builds are building user space libraries.

Include asm/hyperv-tlfs.h instead which contains everything what the
VDSO build needs and move the hv_get_raw_timer() define into a separate
header file which also includes asm/msr.h to resolve rdtsc_ordered().

Fixup drivers/hv/vmbus_drv.c which relies on the indirect include of
asm/mshyperv.h.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/hyperv_timer.h |    9 +++++++++
 arch/x86/include/asm/mshyperv.h     |    2 --
 drivers/hv/vmbus_drv.c              |    1 +
 include/clocksource/hyperv_timer.h  |    2 +-
 4 files changed, 11 insertions(+), 3 deletions(-)

--- /dev/null
+++ b/arch/x86/include/asm/hyperv_timer.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_HYPERV_TIMER_H
+#define _ASM_X86_HYPERV_TIMER_H
+
+#include <asm/msr.h>
+
+#define hv_get_raw_timer() rdtsc_ordered()
+
+#endif
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -19,8 +19,6 @@ typedef int (*hyperv_fill_flush_list_fun
 		struct hv_guest_mapping_flush_list *flush,
 		void *data);
 
-#define hv_get_raw_timer() rdtsc_ordered()
-
 void hyperv_vector_handler(struct pt_regs *regs);
 
 #if IS_ENABLED(CONFIG_HYPERV)
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -37,6 +37,7 @@
 #include <linux/dma-map-ops.h>
 #include <linux/pci.h>
 #include <clocksource/hyperv_timer.h>
+#include <asm/mshyperv.h>
 #include "hyperv_vmbus.h"
 
 struct vmbus_dynid {
--- a/include/clocksource/hyperv_timer.h
+++ b/include/clocksource/hyperv_timer.h
@@ -15,7 +15,7 @@
 
 #include <linux/clocksource.h>
 #include <linux/math64.h>
-#include <asm/mshyperv.h>
+#include <asm/hyperv-tlfs.h>
 
 #define HV_MAX_MAX_DELTA_TICKS 0xffffffff
 #define HV_MIN_DELTA_TICKS 1

RE: [PATCH] clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h

[Thomas Gleixner]

On Sun, Nov 13 2022 at 11:33, Thomas Gleixner wrote:
> --- a/include/clocksource/hyperv_timer.h
> +++ b/include/clocksource/hyperv_timer.h
> @@ -15,7 +15,7 @@
>  
>  #include <linux/clocksource.h>
>  #include <linux/math64.h>
> -#include <asm/mshyperv.h>
> +#include <asm/hyperv-tlfs.h>

Bah, that obviously wants to include the new header...

---
Subject: clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h
From: Thomas Gleixner <tglx@linutronix.de>
Date: Sat, 12 Nov 2022 19:08:15 +0100

clocksource/hyperv_timer.h is included into the VDSO build. It includes
asm/mshyperv.h which in turn includes the world and some more. This worked
so far by chance, but any subtle change in the include chain results in a
build breakage because VDSO builds are building user space libraries.

Include asm/hyperv-tlfs.h instead which contains everything what the VDSO
build needs and move the hv_get_raw_timer() define into the header file.

Fixup drivers/hv/vmbus_drv.c which relies on the indirect include of
asm/mshyperv.h.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/hyperv_timer.h |    9 +++++++++
 arch/x86/include/asm/mshyperv.h     |    2 --
 drivers/hv/vmbus_drv.c              |    1 +
 include/clocksource/hyperv_timer.h  |    3 ++-
 4 files changed, 12 insertions(+), 3 deletions(-)

--- /dev/null
+++ b/arch/x86/include/asm/hyperv_timer.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_HYPERV_TIMER_H
+#define _ASM_X86_HYPERV_TIMER_H
+
+#include <asm/msr.h>
+
+#define hv_get_raw_timer() rdtsc_ordered()
+
+#endif
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -19,8 +19,6 @@ typedef int (*hyperv_fill_flush_list_fun
 		struct hv_guest_mapping_flush_list *flush,
 		void *data);
 
-#define hv_get_raw_timer() rdtsc_ordered()
-
 void hyperv_vector_handler(struct pt_regs *regs);
 
 #if IS_ENABLED(CONFIG_HYPERV)
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -37,6 +37,7 @@
 #include <linux/dma-map-ops.h>
 #include <linux/pci.h>
 #include <clocksource/hyperv_timer.h>
+#include <asm/mshyperv.h>
 #include "hyperv_vmbus.h"
 
 struct vmbus_dynid {
--- a/include/clocksource/hyperv_timer.h
+++ b/include/clocksource/hyperv_timer.h
@@ -15,7 +15,8 @@
 
 #include <linux/clocksource.h>
 #include <linux/math64.h>
-#include <asm/mshyperv.h>
+#include <asm/hyperv_timer.h>
+#include <asm/hyperv-tlfs.h>
 
 #define HV_MAX_MAX_DELTA_TICKS 0xffffffff
 #define HV_MIN_DELTA_TICKS 1

RE: [PATCH] clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h

[Thomas Gleixner]

On Sun, Nov 13 2022 at 11:40, Thomas Gleixner wrote:
> Bah, that obviously wants to include the new header...
>
> --- a/include/clocksource/hyperv_timer.h
> +++ b/include/clocksource/hyperv_timer.h
> @@ -15,7 +15,8 @@
>  
>  #include <linux/clocksource.h>
>  #include <linux/math64.h>
> -#include <asm/mshyperv.h>
> +#include <asm/hyperv_timer.h>

and that breaks when CONFIG_HYPERV_TIMER=n and compiled for ARM64.

Sigh...

---
Subject: clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h
From: Thomas Gleixner <tglx@linutronix.de>
Date: Sat, 12 Nov 2022 19:08:15 +0100

clocksource/hyperv_timer.h is included into the VDSO build. It includes
asm/mshyperv.h which in turn includes the world and some more. This worked
so far by chance, but any subtle change in the include chain results in a
build breakage because VDSO builds are building user space libraries.

Include asm/hyperv-tlfs.h instead which contains everything what the VDSO
build needs and move the hv_get_raw_timer() define into the header file.

Fixup drivers/hv/vmbus_drv.c which relies on the indirect include of
asm/mshyperv.h.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/hyperv_timer.h |    9 +++++++++
 arch/x86/include/asm/mshyperv.h     |    2 --
 drivers/hv/vmbus_drv.c              |    1 +
 include/clocksource/hyperv_timer.h  |    4 +++-
 4 files changed, 13 insertions(+), 3 deletions(-)

--- /dev/null
+++ b/arch/x86/include/asm/hyperv_timer.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_HYPERV_TIMER_H
+#define _ASM_X86_HYPERV_TIMER_H
+
+#include <asm/msr.h>
+
+#define hv_get_raw_timer() rdtsc_ordered()
+
+#endif
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -19,8 +19,6 @@ typedef int (*hyperv_fill_flush_list_fun
 		struct hv_guest_mapping_flush_list *flush,
 		void *data);
 
-#define hv_get_raw_timer() rdtsc_ordered()
-
 void hyperv_vector_handler(struct pt_regs *regs);
 
 #if IS_ENABLED(CONFIG_HYPERV)
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -37,6 +37,7 @@
 #include <linux/dma-map-ops.h>
 #include <linux/pci.h>
 #include <clocksource/hyperv_timer.h>
+#include <asm/mshyperv.h>
 #include "hyperv_vmbus.h"
 
 struct vmbus_dynid {
--- a/include/clocksource/hyperv_timer.h
+++ b/include/clocksource/hyperv_timer.h
@@ -15,13 +15,15 @@
 
 #include <linux/clocksource.h>
 #include <linux/math64.h>
-#include <asm/mshyperv.h>
+#include <asm/hyperv-tlfs.h>
 
 #define HV_MAX_MAX_DELTA_TICKS 0xffffffff
 #define HV_MIN_DELTA_TICKS 1
 
 #ifdef CONFIG_HYPERV_TIMER
 
+#include <asm/hyperv_timer.h>
+
 /* Routines called by the VMbus driver */
 extern int hv_stimer_alloc(bool have_percpu_irqs);
 extern int hv_stimer_cleanup(unsigned int cpu);

RE: [PATCH] clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h

[Thomas Gleixner]

Michael!

On Sun, Nov 13 2022 at 22:21, Thomas Gleixner wrote:
> Subject: clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Sat, 12 Nov 2022 19:08:15 +0100
>
> clocksource/hyperv_timer.h is included into the VDSO build. It includes
> asm/mshyperv.h which in turn includes the world and some more. This worked
> so far by chance, but any subtle change in the include chain results in a
> build breakage because VDSO builds are building user space libraries.
>
> Include asm/hyperv-tlfs.h instead which contains everything what the VDSO
> build needs and move the hv_get_raw_timer() define into the header file.
>
> Fixup drivers/hv/vmbus_drv.c which relies on the indirect include of
> asm/mshyperv.h.

Any comments on this latest version?

> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/include/asm/hyperv_timer.h |    9 +++++++++
>  arch/x86/include/asm/mshyperv.h     |    2 --
>  drivers/hv/vmbus_drv.c              |    1 +
>  include/clocksource/hyperv_timer.h  |    4 +++-
>  4 files changed, 13 insertions(+), 3 deletions(-)
>
> --- /dev/null
> +++ b/arch/x86/include/asm/hyperv_timer.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_HYPERV_TIMER_H
> +#define _ASM_X86_HYPERV_TIMER_H
> +
> +#include <asm/msr.h>
> +
> +#define hv_get_raw_timer() rdtsc_ordered()
> +
> +#endif
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -19,8 +19,6 @@ typedef int (*hyperv_fill_flush_list_fun
>  		struct hv_guest_mapping_flush_list *flush,
>  		void *data);
>  
> -#define hv_get_raw_timer() rdtsc_ordered()
> -
>  void hyperv_vector_handler(struct pt_regs *regs);
>  
>  #if IS_ENABLED(CONFIG_HYPERV)
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -37,6 +37,7 @@
>  #include <linux/dma-map-ops.h>
>  #include <linux/pci.h>
>  #include <clocksource/hyperv_timer.h>
> +#include <asm/mshyperv.h>
>  #include "hyperv_vmbus.h"
>  
>  struct vmbus_dynid {
> --- a/include/clocksource/hyperv_timer.h
> +++ b/include/clocksource/hyperv_timer.h
> @@ -15,13 +15,15 @@
>  
>  #include <linux/clocksource.h>
>  #include <linux/math64.h>
> -#include <asm/mshyperv.h>
> +#include <asm/hyperv-tlfs.h>
>  
>  #define HV_MAX_MAX_DELTA_TICKS 0xffffffff
>  #define HV_MIN_DELTA_TICKS 1
>  
>  #ifdef CONFIG_HYPERV_TIMER
>  
> +#include <asm/hyperv_timer.h>
> +
>  /* Routines called by the VMbus driver */
>  extern int hv_stimer_alloc(bool have_percpu_irqs);
>  extern int hv_stimer_cleanup(unsigned int cpu);

RE: [PATCH] clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h

[Michael Kelley (LINUX)]

From: Thomas Gleixner <tglx@linutronix.de> Sent: Wednesday, November 16, 2022 12:52 PM
>
> Michael!
> 
> On Sun, Nov 13 2022 at 22:21, Thomas Gleixner wrote:
> > Subject: clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not
> asm/mshyperv.h
> > From: Thomas Gleixner <tglx@linutronix.de>
> > Date: Sat, 12 Nov 2022 19:08:15 +0100
> >
> > clocksource/hyperv_timer.h is included into the VDSO build. It includes
> > asm/mshyperv.h which in turn includes the world and some more. This worked
> > so far by chance, but any subtle change in the include chain results in a
> > build breakage because VDSO builds are building user space libraries.
> >
> > Include asm/hyperv-tlfs.h instead which contains everything what the VDSO
> > build needs and move the hv_get_raw_timer() define into the header file.
> >
> > Fixup drivers/hv/vmbus_drv.c which relies on the indirect include of
> > asm/mshyperv.h.
> 
> Any comments on this latest version?

Sorry.  This looks good to me.  Maybe the commit message needs a
bit of tweaking -- it's not clear what "move hv_get_raw_timer()
define into the header file" exactly refers to.  But otherwise,

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

> 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > ---
> >  arch/x86/include/asm/hyperv_timer.h |    9 +++++++++
> >  arch/x86/include/asm/mshyperv.h     |    2 --
> >  drivers/hv/vmbus_drv.c              |    1 +
> >  include/clocksource/hyperv_timer.h  |    4 +++-
> >  4 files changed, 13 insertions(+), 3 deletions(-)
> >
> > --- /dev/null
> > +++ b/arch/x86/include/asm/hyperv_timer.h
> > @@ -0,0 +1,9 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _ASM_X86_HYPERV_TIMER_H
> > +#define _ASM_X86_HYPERV_TIMER_H
> > +
> > +#include <asm/msr.h>
> > +
> > +#define hv_get_raw_timer() rdtsc_ordered()
> > +
> > +#endif
> > --- a/arch/x86/include/asm/mshyperv.h
> > +++ b/arch/x86/include/asm/mshyperv.h
> > @@ -19,8 +19,6 @@ typedef int (*hyperv_fill_flush_list_fun
> >  		struct hv_guest_mapping_flush_list *flush,
> >  		void *data);
> >
> > -#define hv_get_raw_timer() rdtsc_ordered()
> > -
> >  void hyperv_vector_handler(struct pt_regs *regs);
> >
> >  #if IS_ENABLED(CONFIG_HYPERV)
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -37,6 +37,7 @@
> >  #include <linux/dma-map-ops.h>
> >  #include <linux/pci.h>
> >  #include <clocksource/hyperv_timer.h>
> > +#include <asm/mshyperv.h>
> >  #include "hyperv_vmbus.h"
> >
> >  struct vmbus_dynid {
> > --- a/include/clocksource/hyperv_timer.h
> > +++ b/include/clocksource/hyperv_timer.h
> > @@ -15,13 +15,15 @@
> >
> >  #include <linux/clocksource.h>
> >  #include <linux/math64.h>
> > -#include <asm/mshyperv.h>
> > +#include <asm/hyperv-tlfs.h>
> >
> >  #define HV_MAX_MAX_DELTA_TICKS 0xffffffff
> >  #define HV_MIN_DELTA_TICKS 1
> >
> >  #ifdef CONFIG_HYPERV_TIMER
> >
> > +#include <asm/hyperv_timer.h>
> > +
> >  /* Routines called by the VMbus driver */
> >  extern int hv_stimer_alloc(bool have_percpu_irqs);
> >  extern int hv_stimer_cleanup(unsigned int cpu);

RE: [PATCH] clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h

[Thomas Gleixner]

On Wed, Nov 16 2022 at 22:34, Michael Kelley wrote:
> From: Thomas Gleixner <tglx@linutronix.de> Sent: Wednesday, November 16, 2022 12:52 PM
>> On Sun, Nov 13 2022 at 22:21, Thomas Gleixner wrote:
>> > Subject: clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not
>> asm/mshyperv.h
>> > From: Thomas Gleixner <tglx@linutronix.de>
>> > Date: Sat, 12 Nov 2022 19:08:15 +0100
>> >
>> > clocksource/hyperv_timer.h is included into the VDSO build. It includes
>> > asm/mshyperv.h which in turn includes the world and some more. This worked
>> > so far by chance, but any subtle change in the include chain results in a
>> > build breakage because VDSO builds are building user space libraries.
>> >
>> > Include asm/hyperv-tlfs.h instead which contains everything what the VDSO
>> > build needs and move the hv_get_raw_timer() define into the header file.
>> >
>> > Fixup drivers/hv/vmbus_drv.c which relies on the indirect include of
>> > asm/mshyperv.h.
>> 
>> Any comments on this latest version?
>
> Sorry.  This looks good to me.  Maybe the commit message needs a
> bit of tweaking -- it's not clear what "move hv_get_raw_timer()
> define into the header file" exactly refers to.  But otherwise,

That should obviously be 'into a separate header file, which is included
from clocksource/hyperv_timer.h' or something like that.

Thanks,

        tglx

[tip: timers/core] clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     d3fe5c2f206148f86f85f56671109a3118bf0d2b
Gitweb:        https://git.kernel.org/tip/d3fe5c2f206148f86f85f56671109a3118bf0d2b
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Sun, 13 Nov 2022 22:21:15 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 17 Nov 2022 11:41:19 +01:00

clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h

clocksource/hyperv_timer.h is included into the VDSO build. It includes
asm/mshyperv.h which in turn includes the world and some more. This worked
so far by chance, but any subtle change in the include chain results in a
build breakage because VDSO builds are building user space libraries.

Include asm/hyperv-tlfs.h instead which contains everything what the VDSO
build needs except the hv_get_raw_timer() define. Move this define into a
separate header file, which contains the prerequisites (msr.h) and is
included by clocksource/hyperv_timer.h.

Fixup drivers/hv/vmbus_drv.c which relies on the indirect include of
asm/mshyperv.h.

With that the VDSO build only pulls in the minimum requirements.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Link: https://lore.kernel.org/r/87fsemtut0.ffs@tglx

---
 arch/x86/include/asm/hyperv_timer.h |  9 +++++++++
 arch/x86/include/asm/mshyperv.h     |  2 --
 drivers/hv/vmbus_drv.c              |  1 +
 include/clocksource/hyperv_timer.h  |  4 +++-
 4 files changed, 13 insertions(+), 3 deletions(-)
 create mode 100644 arch/x86/include/asm/hyperv_timer.h

diff --git a/arch/x86/include/asm/hyperv_timer.h b/arch/x86/include/asm/hyperv_timer.h
new file mode 100644
index 0000000..388fa81
--- /dev/null
+++ b/arch/x86/include/asm/hyperv_timer.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_HYPERV_TIMER_H
+#define _ASM_X86_HYPERV_TIMER_H
+
+#include <asm/msr.h>
+
+#define hv_get_raw_timer() rdtsc_ordered()
+
+#endif
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index 61f0c20..6d502f3 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -19,8 +19,6 @@ typedef int (*hyperv_fill_flush_list_func)(
 		struct hv_guest_mapping_flush_list *flush,
 		void *data);
 
-#define hv_get_raw_timer() rdtsc_ordered()
-
 void hyperv_vector_handler(struct pt_regs *regs);
 
 #if IS_ENABLED(CONFIG_HYPERV)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 8b2e413..1f5d37a 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -37,6 +37,7 @@
 #include <linux/dma-map-ops.h>
 #include <linux/pci.h>
 #include <clocksource/hyperv_timer.h>
+#include <asm/mshyperv.h>
 #include "hyperv_vmbus.h"
 
 struct vmbus_dynid {
diff --git a/include/clocksource/hyperv_timer.h b/include/clocksource/hyperv_timer.h
index b3f5d73..b4a3935 100644
--- a/include/clocksource/hyperv_timer.h
+++ b/include/clocksource/hyperv_timer.h
@@ -15,13 +15,15 @@
 
 #include <linux/clocksource.h>
 #include <linux/math64.h>
-#include <asm/mshyperv.h>
+#include <asm/hyperv-tlfs.h>
 
 #define HV_MAX_MAX_DELTA_TICKS 0xffffffff
 #define HV_MIN_DELTA_TICKS 1
 
 #ifdef CONFIG_HYPERV_TIMER
 
+#include <asm/hyperv_timer.h>
+
 /* Routines called by the VMbus driver */
 extern int hv_stimer_alloc(bool have_percpu_irqs);
 extern int hv_stimer_cleanup(unsigned int cpu);

[tip: irq/core] clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the irq/core branch of tip:

Commit-ID:     e5dfd093eca01a5d8d967f959a2372d7d82eb59c
Gitweb:        https://git.kernel.org/tip/e5dfd093eca01a5d8d967f959a2372d7d82eb59c
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Sun, 13 Nov 2022 22:21:15 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 17 Nov 2022 13:58:32 +01:00

clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h

clocksource/hyperv_timer.h is included into the VDSO build. It includes
asm/mshyperv.h which in turn includes the world and some more. This worked
so far by chance, but any subtle change in the include chain results in a
build breakage because VDSO builds are building user space libraries.

Include asm/hyperv-tlfs.h instead which contains everything what the VDSO
build needs except the hv_get_raw_timer() define. Move this define into a
separate header file, which contains the prerequisites (msr.h) and is
included by clocksource/hyperv_timer.h.

Fixup drivers/hv/vmbus_drv.c which relies on the indirect include of
asm/mshyperv.h.

With that the VDSO build only pulls in the minimum requirements.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Link: https://lore.kernel.org/r/87fsemtut0.ffs@tglx

---
 arch/x86/include/asm/hyperv_timer.h |  9 +++++++++
 arch/x86/include/asm/mshyperv.h     |  2 --
 drivers/hv/vmbus_drv.c              |  1 +
 include/clocksource/hyperv_timer.h  |  4 +++-
 4 files changed, 13 insertions(+), 3 deletions(-)
 create mode 100644 arch/x86/include/asm/hyperv_timer.h

diff --git a/arch/x86/include/asm/hyperv_timer.h b/arch/x86/include/asm/hyperv_timer.h
new file mode 100644
index 0000000..388fa81
--- /dev/null
+++ b/arch/x86/include/asm/hyperv_timer.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_HYPERV_TIMER_H
+#define _ASM_X86_HYPERV_TIMER_H
+
+#include <asm/msr.h>
+
+#define hv_get_raw_timer() rdtsc_ordered()
+
+#endif
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index 61f0c20..6d502f3 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -19,8 +19,6 @@ typedef int (*hyperv_fill_flush_list_func)(
 		struct hv_guest_mapping_flush_list *flush,
 		void *data);
 
-#define hv_get_raw_timer() rdtsc_ordered()
-
 void hyperv_vector_handler(struct pt_regs *regs);
 
 #if IS_ENABLED(CONFIG_HYPERV)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 8b2e413..1f5d37a 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -37,6 +37,7 @@
 #include <linux/dma-map-ops.h>
 #include <linux/pci.h>
 #include <clocksource/hyperv_timer.h>
+#include <asm/mshyperv.h>
 #include "hyperv_vmbus.h"
 
 struct vmbus_dynid {
diff --git a/include/clocksource/hyperv_timer.h b/include/clocksource/hyperv_timer.h
index b3f5d73..b4a3935 100644
--- a/include/clocksource/hyperv_timer.h
+++ b/include/clocksource/hyperv_timer.h
@@ -15,13 +15,15 @@
 
 #include <linux/clocksource.h>
 #include <linux/math64.h>
-#include <asm/mshyperv.h>
+#include <asm/hyperv-tlfs.h>
 
 #define HV_MAX_MAX_DELTA_TICKS 0xffffffff
 #define HV_MIN_DELTA_TICKS 1
 
 #ifdef CONFIG_HYPERV_TIMER
 
+#include <asm/hyperv_timer.h>
+
 /* Routines called by the VMbus driver */
 extern int hv_stimer_alloc(bool have_percpu_irqs);
 extern int hv_stimer_cleanup(unsigned int cpu);

RE: [PATCH] clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h

[Michael Kelley (LINUX)]

From: Thomas Gleixner <tglx@linutronix.de> Sent: Sunday, November 13, 2022 1:50 AM
> 
> On Sat, Nov 12 2022 at 21:55, Michael Kelley wrote:
> > But I can see the problem with too much getting dragged into the VDSO
> > builds.  If hv_get_raw_timer() is added to hyperv_timer.h, it should
> > be under #ifdef CONFIG_X86.  Adding an #ifdef isn't ideal, and a more
> > more proper solution might be to have a separate hyperv_timer.h include
> > file under arch/x86/include/asm.  But the latter seems like overkill for just
> > hv_get_raw_timer(), so I'm OK with the #ifdef.
> 
> We surely can have asm/hyperv_timer.h but TBH:
> 
> >>  static inline notrace u64
> >>  hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg, u64 *cur_tsc)
> >>  {
> 
> hv_read_tsc_page_tsc() does not look architecture agnostic either. TSC
> is pretty x86 specific :)

Yes, the naming still says "tsc".  But there's nothing in the code that actually
requires the TSC if hv_get_raw_timer() maps to some other hardware counter on
a different architecture.   That's why the hv_get_raw_timer() abstraction is there
in the first place.  If we didn't care about x86-isms, hv_read_tsc_page_tsc() would
just directly invoke rdtsc_ordered().

Michael

> 
> Thanks,
> 
>         tglx
> 

RE: [PATCH] clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h

[Thomas Gleixner]

On Sun, Nov 13 2022 at 13:12, Michael Kelley wrote:
> From: Thomas Gleixner <tglx@linutronix.de> Sent: Sunday, November 13, 2022 1:50 AM
>> On Sat, Nov 12 2022 at 21:55, Michael Kelley wrote:
>> > But I can see the problem with too much getting dragged into the VDSO
>> > builds.  If hv_get_raw_timer() is added to hyperv_timer.h, it should
>> > be under #ifdef CONFIG_X86.  Adding an #ifdef isn't ideal, and a more
>> > more proper solution might be to have a separate hyperv_timer.h include
>> > file under arch/x86/include/asm.  But the latter seems like overkill for just
>> > hv_get_raw_timer(), so I'm OK with the #ifdef.
>> 
>> We surely can have asm/hyperv_timer.h but TBH:
>> 
>> >>  static inline notrace u64
>> >>  hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg, u64 *cur_tsc)
>> >>  {
>> 
>> hv_read_tsc_page_tsc() does not look architecture agnostic either. TSC
>> is pretty x86 specific :)
>
> Yes, the naming still says "tsc".  But there's nothing in the code that actually
> requires the TSC if hv_get_raw_timer() maps to some other hardware counter on
> a different architecture.   That's why the hv_get_raw_timer() abstraction is there
> in the first place.  If we didn't care about x86-isms, hv_read_tsc_page_tsc() would
> just directly invoke rdtsc_ordered().

Not really intuitive, but anyway...