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

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

arch/x86/include/asm/mshyperv.h +0 -2

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

drivers/hv/vmbus_drv.c +1 -0

@@ -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 {

include/clocksource/hyperv_timer.h +3 -1

@@ -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)
 {

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.

Quoted 3 lines

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

Quoted 6 lines · Michael Kelley

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:

Quoted 3 lines

Quoted 3 lines

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

Subject: clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h
From: Thomas Gleixner <[email protected]>
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 <[email protected]>

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

arch/x86/include/asm/hyperv_timer.h new file +9 -0

@@ -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

arch/x86/include/asm/mshyperv.h +0 -2

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

drivers/hv/vmbus_drv.c +1 -0

@@ -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 {

include/clocksource/hyperv_timer.h +3 -1

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

Michael!

Quoted 1 line · Thomas Gleixner

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

asm/mshyperv.h

Quoted 8 lines

From: Thomas Gleixner <[email protected]>
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.

Quoted 5 lines

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?

Quoted 61 lines

Signed-off-by Thomas Gleixner <[email protected]>

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

arch/x86/include/asm/hyperv_timer.h new file +9 -0

@@ -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

arch/x86/include/asm/mshyperv.h +0 -2

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

drivers/hv/vmbus_drv.c +1 -0

@@ -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 {

include/clocksource/hyperv_timer.h +3 -1

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

From: Thomas Gleixner <[email protected]> Sent: Wednesday, November 16, 2022 12:52 PM

Quoted 14 lines · Thomas Gleixner

Quoted 1 line

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

asm/mshyperv.h

Quoted 11 lines

From: Thomas Gleixner <[email protected]>
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.

Quoted 4 lines

Quoted 2 lines

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,

Quoted 6 lines

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:

Quoted 3 lines

Quoted 3 lines

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

From: Thomas Gleixner <[email protected]> Sent: Sunday, November 13, 2022 1:50 AM

Quoted 14 lines · Michael Kelley

Quoted 6 lines

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:

Quoted 3 lines

Quoted 3 lines

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

Quoted 2 lines

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