Re: [PATCH] x86/vdso: Remove direct HPET access through the vDSO

[Thomas Gleixner]

On Thu, 7 Apr 2016, Andy Lutomirski wrote:

> Allowing user code to map the HPET is problematic.  HPET
> implementations are notoriously buggy, and there are probably many
> machines on which even MMIO reads from bogus HPET addresses are
> problematic.
> 
> We have a report that the Dell Precision M2800 with:
> 
> ACPI: HPET 0xC8FE6238 38 (v01 DELL   CBX3  01072009 AMI. 0005)
> 
> is either so slow when accessing the HPET or actually hangs in some
> regard, causing soft lockups to be reported if users do unexpected
> things to the HPET.
> 
> The vclock HPET code has also always been a questionable speedup.
> Accessing an HPET is exceedingly slow (on the order of several
> microseconds), so the added overhead in requiring a syscall to read
> the HPET is a small fraction of the total code of accessing it.
> 
> To avoid future problems, let's just delete the code entirely.
> 
> In the long run, this could actually be a speedup.  Waiman Long as a
> patch to optimize the case where multiple CPUs contend for the HPET,
> but that won't help unless all the accesses are mediated by the
> kernel.
> 
> Cc: Waiman Long 
> Reported-by: Rasmus Villemoes 
> Signed-off-by: Andy Lutomirski 

Reviewed-by: Thomas Gleixner 

Re: [PATCH] x86/vdso: Remove direct HPET access through the vDSO

[Thomas Gleixner]

On Thu, 7 Apr 2016, Andy Lutomirski wrote:

> Allowing user code to map the HPET is problematic.  HPET
> implementations are notoriously buggy, and there are probably many
> machines on which even MMIO reads from bogus HPET addresses are
> problematic.
> 
> We have a report that the Dell Precision M2800 with:
> 
> ACPI: HPET 0xC8FE6238 38 (v01 DELL   CBX3  01072009 AMI. 0005)
> 
> is either so slow when accessing the HPET or actually hangs in some
> regard, causing soft lockups to be reported if users do unexpected
> things to the HPET.
> 
> The vclock HPET code has also always been a questionable speedup.
> Accessing an HPET is exceedingly slow (on the order of several
> microseconds), so the added overhead in requiring a syscall to read
> the HPET is a small fraction of the total code of accessing it.
> 
> To avoid future problems, let's just delete the code entirely.
> 
> In the long run, this could actually be a speedup.  Waiman Long as a
> patch to optimize the case where multiple CPUs contend for the HPET,
> but that won't help unless all the accesses are mediated by the
> kernel.
> 
> Cc: Waiman Long 
> Reported-by: Rasmus Villemoes 
> Signed-off-by: Andy Lutomirski 

Reviewed-by: Thomas Gleixner 

Re: [PATCH] x86/vdso: Remove direct HPET access through the vDSO

[Andy Lutomirski]

On Apr 8, 2016 3:36 AM, "Borislav Petkov"  wrote:
>
> On Thu, Apr 07, 2016 at 05:16:59PM -0700, Andy Lutomirski wrote:
> > Allowing user code to map the HPET is problematic.  HPET
> > implementations are notoriously buggy, and there are probably many
> > machines on which even MMIO reads from bogus HPET addresses are
> > problematic.
> >
> > We have a report that the Dell Precision M2800 with:
> >
> > ACPI: HPET 0xC8FE6238 38 (v01 DELL   CBX3  01072009 AMI. 
> > 0005)
> >
> > is either so slow when accessing the HPET or actually hangs in some
> > regard, causing soft lockups to be reported if users do unexpected
> > things to the HPET.
> >
> > The vclock HPET code has also always been a questionable speedup.
> > Accessing an HPET is exceedingly slow (on the order of several
> > microseconds), so the added overhead in requiring a syscall to read
> > the HPET is a small fraction of the total code of accessing it.
> >
> > To avoid future problems, let's just delete the code entirely.
> >
> > In the long run, this could actually be a speedup.  Waiman Long as a
> > patch to optimize the case where multiple CPUs contend for the HPET,
> > but that won't help unless all the accesses are mediated by the
> > kernel.
> >
> > Cc: Waiman Long 
> > Reported-by: Rasmus Villemoes 
> > Signed-off-by: Andy Lutomirski 
> > ---
> >  arch/x86/entry/vdso/vclock_gettime.c  | 15 ---
> >  arch/x86/entry/vdso/vdso-layout.lds.S |  5 ++---
> >  arch/x86/entry/vdso/vma.c | 11 ---
> >  arch/x86/include/asm/clocksource.h|  9 -
> >  arch/x86/kernel/hpet.c|  1 -
> >  arch/x86/kvm/trace.h  |  3 +--
> >  6 files changed, 7 insertions(+), 37 deletions(-)
>
> I like diffstats which remove more lines than add :)
>
> In any case, I'm not well versed in the vdso game but from my experience
> so far, I'm starting to believe that the 'H' in HPET stands for
> Horrific. So the less people get exposed to it, the better.
>
> A question though: userspace does not rely on the hpet page being always
> present and can do fallback. IOW, we're not breaking any existing
> usages, right?

It shouldn't break anything.  It'll just cause the vdso to issue a
real syscall instead of reading the HPET directly.  I strongly doubt
there's user code that pokes the HPET mapping outside the vdso, since
it would have broken when we moved the HPET mapping out of the fixmap
to a randomized address.

>
> --
> Regards/Gruss,
> Boris.
>
> ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86/vdso: Remove direct HPET access through the vDSO

[Andy Lutomirski]

On Apr 8, 2016 3:36 AM, "Borislav Petkov"  wrote:
>
> On Thu, Apr 07, 2016 at 05:16:59PM -0700, Andy Lutomirski wrote:
> > Allowing user code to map the HPET is problematic.  HPET
> > implementations are notoriously buggy, and there are probably many
> > machines on which even MMIO reads from bogus HPET addresses are
> > problematic.
> >
> > We have a report that the Dell Precision M2800 with:
> >
> > ACPI: HPET 0xC8FE6238 38 (v01 DELL   CBX3  01072009 AMI. 
> > 0005)
> >
> > is either so slow when accessing the HPET or actually hangs in some
> > regard, causing soft lockups to be reported if users do unexpected
> > things to the HPET.
> >
> > The vclock HPET code has also always been a questionable speedup.
> > Accessing an HPET is exceedingly slow (on the order of several
> > microseconds), so the added overhead in requiring a syscall to read
> > the HPET is a small fraction of the total code of accessing it.
> >
> > To avoid future problems, let's just delete the code entirely.
> >
> > In the long run, this could actually be a speedup.  Waiman Long as a
> > patch to optimize the case where multiple CPUs contend for the HPET,
> > but that won't help unless all the accesses are mediated by the
> > kernel.
> >
> > Cc: Waiman Long 
> > Reported-by: Rasmus Villemoes 
> > Signed-off-by: Andy Lutomirski 
> > ---
> >  arch/x86/entry/vdso/vclock_gettime.c  | 15 ---
> >  arch/x86/entry/vdso/vdso-layout.lds.S |  5 ++---
> >  arch/x86/entry/vdso/vma.c | 11 ---
> >  arch/x86/include/asm/clocksource.h|  9 -
> >  arch/x86/kernel/hpet.c|  1 -
> >  arch/x86/kvm/trace.h  |  3 +--
> >  6 files changed, 7 insertions(+), 37 deletions(-)
>
> I like diffstats which remove more lines than add :)
>
> In any case, I'm not well versed in the vdso game but from my experience
> so far, I'm starting to believe that the 'H' in HPET stands for
> Horrific. So the less people get exposed to it, the better.
>
> A question though: userspace does not rely on the hpet page being always
> present and can do fallback. IOW, we're not breaking any existing
> usages, right?

It shouldn't break anything.  It'll just cause the vdso to issue a
real syscall instead of reading the HPET directly.  I strongly doubt
there's user code that pokes the HPET mapping outside the vdso, since
it would have broken when we moved the HPET mapping out of the fixmap
to a randomized address.

>
> --
> Regards/Gruss,
> Boris.
>
> ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86/vdso: Remove direct HPET access through the vDSO

[Borislav Petkov]

On Thu, Apr 07, 2016 at 05:16:59PM -0700, Andy Lutomirski wrote:
> Allowing user code to map the HPET is problematic.  HPET
> implementations are notoriously buggy, and there are probably many
> machines on which even MMIO reads from bogus HPET addresses are
> problematic.
> 
> We have a report that the Dell Precision M2800 with:
> 
> ACPI: HPET 0xC8FE6238 38 (v01 DELL   CBX3  01072009 AMI. 0005)
> 
> is either so slow when accessing the HPET or actually hangs in some
> regard, causing soft lockups to be reported if users do unexpected
> things to the HPET.
> 
> The vclock HPET code has also always been a questionable speedup.
> Accessing an HPET is exceedingly slow (on the order of several
> microseconds), so the added overhead in requiring a syscall to read
> the HPET is a small fraction of the total code of accessing it.
> 
> To avoid future problems, let's just delete the code entirely.
> 
> In the long run, this could actually be a speedup.  Waiman Long as a
> patch to optimize the case where multiple CPUs contend for the HPET,
> but that won't help unless all the accesses are mediated by the
> kernel.
> 
> Cc: Waiman Long 
> Reported-by: Rasmus Villemoes 
> Signed-off-by: Andy Lutomirski 
> ---
>  arch/x86/entry/vdso/vclock_gettime.c  | 15 ---
>  arch/x86/entry/vdso/vdso-layout.lds.S |  5 ++---
>  arch/x86/entry/vdso/vma.c | 11 ---
>  arch/x86/include/asm/clocksource.h|  9 -
>  arch/x86/kernel/hpet.c|  1 -
>  arch/x86/kvm/trace.h  |  3 +--
>  6 files changed, 7 insertions(+), 37 deletions(-)

I like diffstats which remove more lines than add :)

In any case, I'm not well versed in the vdso game but from my experience
so far, I'm starting to believe that the 'H' in HPET stands for
Horrific. So the less people get exposed to it, the better.

A question though: userspace does not rely on the hpet page being always
present and can do fallback. IOW, we're not breaking any existing
usages, right?

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86/vdso: Remove direct HPET access through the vDSO

[Borislav Petkov]

On Thu, Apr 07, 2016 at 05:16:59PM -0700, Andy Lutomirski wrote:
> Allowing user code to map the HPET is problematic.  HPET
> implementations are notoriously buggy, and there are probably many
> machines on which even MMIO reads from bogus HPET addresses are
> problematic.
> 
> We have a report that the Dell Precision M2800 with:
> 
> ACPI: HPET 0xC8FE6238 38 (v01 DELL   CBX3  01072009 AMI. 0005)
> 
> is either so slow when accessing the HPET or actually hangs in some
> regard, causing soft lockups to be reported if users do unexpected
> things to the HPET.
> 
> The vclock HPET code has also always been a questionable speedup.
> Accessing an HPET is exceedingly slow (on the order of several
> microseconds), so the added overhead in requiring a syscall to read
> the HPET is a small fraction of the total code of accessing it.
> 
> To avoid future problems, let's just delete the code entirely.
> 
> In the long run, this could actually be a speedup.  Waiman Long as a
> patch to optimize the case where multiple CPUs contend for the HPET,
> but that won't help unless all the accesses are mediated by the
> kernel.
> 
> Cc: Waiman Long 
> Reported-by: Rasmus Villemoes 
> Signed-off-by: Andy Lutomirski 
> ---
>  arch/x86/entry/vdso/vclock_gettime.c  | 15 ---
>  arch/x86/entry/vdso/vdso-layout.lds.S |  5 ++---
>  arch/x86/entry/vdso/vma.c | 11 ---
>  arch/x86/include/asm/clocksource.h|  9 -
>  arch/x86/kernel/hpet.c|  1 -
>  arch/x86/kvm/trace.h  |  3 +--
>  6 files changed, 7 insertions(+), 37 deletions(-)

I like diffstats which remove more lines than add :)

In any case, I'm not well versed in the vdso game but from my experience
so far, I'm starting to believe that the 'H' in HPET stands for
Horrific. So the less people get exposed to it, the better.

A question though: userspace does not rely on the hpet page being always
present and can do fallback. IOW, we're not breaking any existing
usages, right?

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.

[PATCH] x86/vdso: Remove direct HPET access through the vDSO

[Andy Lutomirski]

Allowing user code to map the HPET is problematic.  HPET
implementations are notoriously buggy, and there are probably many
machines on which even MMIO reads from bogus HPET addresses are
problematic.

We have a report that the Dell Precision M2800 with:

ACPI: HPET 0xC8FE6238 38 (v01 DELL   CBX3  01072009 AMI. 0005)

is either so slow when accessing the HPET or actually hangs in some
regard, causing soft lockups to be reported if users do unexpected
things to the HPET.

The vclock HPET code has also always been a questionable speedup.
Accessing an HPET is exceedingly slow (on the order of several
microseconds), so the added overhead in requiring a syscall to read
the HPET is a small fraction of the total code of accessing it.

To avoid future problems, let's just delete the code entirely.

In the long run, this could actually be a speedup.  Waiman Long as a
patch to optimize the case where multiple CPUs contend for the HPET,
but that won't help unless all the accesses are mediated by the
kernel.

Cc: Waiman Long 
Reported-by: Rasmus Villemoes 
Signed-off-by: Andy Lutomirski 
---
 arch/x86/entry/vdso/vclock_gettime.c  | 15 ---
 arch/x86/entry/vdso/vdso-layout.lds.S |  5 ++---
 arch/x86/entry/vdso/vma.c | 11 ---
 arch/x86/include/asm/clocksource.h|  9 -
 arch/x86/kernel/hpet.c|  1 -
 arch/x86/kvm/trace.h  |  3 +--
 6 files changed, 7 insertions(+), 37 deletions(-)

diff --git a/arch/x86/entry/vdso/vclock_gettime.c 
b/arch/x86/entry/vdso/vclock_gettime.c
index 1a50e09c945b..2900189dcd28 100644
--- a/arch/x86/entry/vdso/vclock_gettime.c
+++ b/arch/x86/entry/vdso/vclock_gettime.c
@@ -13,7 +13,6 @@
 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -28,16 +27,6 @@ extern int __vdso_clock_gettime(clockid_t clock, struct 
timespec *ts);
 extern int __vdso_gettimeofday(struct timeval *tv, struct timezone *tz);
 extern time_t __vdso_time(time_t *t);
 
-#ifdef CONFIG_HPET_TIMER
-extern u8 hpet_page
-   __attribute__((visibility("hidden")));
-
-static notrace cycle_t vread_hpet(void)
-{
-   return *(const volatile u32 *)(_page + HPET_COUNTER);
-}
-#endif
-
 #ifdef CONFIG_PARAVIRT_CLOCK
 extern u8 pvclock_page
__attribute__((visibility("hidden")));
@@ -195,10 +184,6 @@ notrace static inline u64 vgetsns(int *mode)
 
if (gtod->vclock_mode == VCLOCK_TSC)
cycles = vread_tsc();
-#ifdef CONFIG_HPET_TIMER
-   else if (gtod->vclock_mode == VCLOCK_HPET)
-   cycles = vread_hpet();
-#endif
 #ifdef CONFIG_PARAVIRT_CLOCK
else if (gtod->vclock_mode == VCLOCK_PVCLOCK)
cycles = vread_pvclock(mode);
diff --git a/arch/x86/entry/vdso/vdso-layout.lds.S 
b/arch/x86/entry/vdso/vdso-layout.lds.S
index 4158acc17df0..a708aa90b507 100644
--- a/arch/x86/entry/vdso/vdso-layout.lds.S
+++ b/arch/x86/entry/vdso/vdso-layout.lds.S
@@ -25,7 +25,7 @@ SECTIONS
 * segment.
 */
 
-   vvar_start = . - 3 * PAGE_SIZE;
+   vvar_start = . - 2 * PAGE_SIZE;
vvar_page = vvar_start;
 
/* Place all vvars at the offsets in asm/vvar.h. */
@@ -35,8 +35,7 @@ SECTIONS
 #undef __VVAR_KERNEL_LDS
 #undef EMIT_VVAR
 
-   hpet_page = vvar_start + PAGE_SIZE;
-   pvclock_page = vvar_start + 2 * PAGE_SIZE;
+   pvclock_page = vvar_start + PAGE_SIZE;
 
. = SIZEOF_HEADERS;
 
diff --git a/arch/x86/entry/vdso/vma.c b/arch/x86/entry/vdso/vma.c
index 10f704584922..b3cf81333a54 100644
--- a/arch/x86/entry/vdso/vma.c
+++ b/arch/x86/entry/vdso/vma.c
@@ -18,7 +18,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 
@@ -129,16 +128,6 @@ static int vvar_fault(const struct vm_special_mapping *sm,
if (sym_offset == image->sym_vvar_page) {
ret = vm_insert_pfn(vma, (unsigned long)vmf->virtual_address,
__pa_symbol(&__vvar_page) >> PAGE_SHIFT);
-   } else if (sym_offset == image->sym_hpet_page) {
-#ifdef CONFIG_HPET_TIMER
-   if (hpet_address && vclock_was_used(VCLOCK_HPET)) {
-   ret = vm_insert_pfn_prot(
-   vma,
-   (unsigned long)vmf->virtual_address,
-   hpet_address >> PAGE_SHIFT,
-   pgprot_noncached(PAGE_READONLY));
-   }
-#endif
} else if (sym_offset == image->sym_pvclock_page) {
struct pvclock_vsyscall_time_info *pvti =
pvclock_pvti_cpu0_va();
diff --git a/arch/x86/include/asm/clocksource.h 
b/arch/x86/include/asm/clocksource.h
index d194266acb28..eae33c7170c8 100644
--- a/arch/x86/include/asm/clocksource.h
+++ b/arch/x86/include/asm/clocksource.h
@@ -3,11 +3,10 @@
 #ifndef _ASM_X86_CLOCKSOURCE_H
 #define _ASM_X86_CLOCKSOURCE_H
 
-#define VCLOCK_NONE0  /* No vDSO clock available.  */

[PATCH] x86/vdso: Remove direct HPET access through the vDSO

[Andy Lutomirski]

Allowing user code to map the HPET is problematic.  HPET
implementations are notoriously buggy, and there are probably many
machines on which even MMIO reads from bogus HPET addresses are
problematic.

We have a report that the Dell Precision M2800 with:

ACPI: HPET 0xC8FE6238 38 (v01 DELL   CBX3  01072009 AMI. 0005)

is either so slow when accessing the HPET or actually hangs in some
regard, causing soft lockups to be reported if users do unexpected
things to the HPET.

The vclock HPET code has also always been a questionable speedup.
Accessing an HPET is exceedingly slow (on the order of several
microseconds), so the added overhead in requiring a syscall to read
the HPET is a small fraction of the total code of accessing it.

To avoid future problems, let's just delete the code entirely.

In the long run, this could actually be a speedup.  Waiman Long as a
patch to optimize the case where multiple CPUs contend for the HPET,
but that won't help unless all the accesses are mediated by the
kernel.

Cc: Waiman Long 
Reported-by: Rasmus Villemoes 
Signed-off-by: Andy Lutomirski 
---
 arch/x86/entry/vdso/vclock_gettime.c  | 15 ---
 arch/x86/entry/vdso/vdso-layout.lds.S |  5 ++---
 arch/x86/entry/vdso/vma.c | 11 ---
 arch/x86/include/asm/clocksource.h|  9 -
 arch/x86/kernel/hpet.c|  1 -
 arch/x86/kvm/trace.h  |  3 +--
 6 files changed, 7 insertions(+), 37 deletions(-)

diff --git a/arch/x86/entry/vdso/vclock_gettime.c 
b/arch/x86/entry/vdso/vclock_gettime.c
index 1a50e09c945b..2900189dcd28 100644
--- a/arch/x86/entry/vdso/vclock_gettime.c
+++ b/arch/x86/entry/vdso/vclock_gettime.c
@@ -13,7 +13,6 @@
 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -28,16 +27,6 @@ extern int __vdso_clock_gettime(clockid_t clock, struct 
timespec *ts);
 extern int __vdso_gettimeofday(struct timeval *tv, struct timezone *tz);
 extern time_t __vdso_time(time_t *t);
 
-#ifdef CONFIG_HPET_TIMER
-extern u8 hpet_page
-   __attribute__((visibility("hidden")));
-
-static notrace cycle_t vread_hpet(void)
-{
-   return *(const volatile u32 *)(_page + HPET_COUNTER);
-}
-#endif
-
 #ifdef CONFIG_PARAVIRT_CLOCK
 extern u8 pvclock_page
__attribute__((visibility("hidden")));
@@ -195,10 +184,6 @@ notrace static inline u64 vgetsns(int *mode)
 
if (gtod->vclock_mode == VCLOCK_TSC)
cycles = vread_tsc();
-#ifdef CONFIG_HPET_TIMER
-   else if (gtod->vclock_mode == VCLOCK_HPET)
-   cycles = vread_hpet();
-#endif
 #ifdef CONFIG_PARAVIRT_CLOCK
else if (gtod->vclock_mode == VCLOCK_PVCLOCK)
cycles = vread_pvclock(mode);
diff --git a/arch/x86/entry/vdso/vdso-layout.lds.S 
b/arch/x86/entry/vdso/vdso-layout.lds.S
index 4158acc17df0..a708aa90b507 100644
--- a/arch/x86/entry/vdso/vdso-layout.lds.S
+++ b/arch/x86/entry/vdso/vdso-layout.lds.S
@@ -25,7 +25,7 @@ SECTIONS
 * segment.
 */
 
-   vvar_start = . - 3 * PAGE_SIZE;
+   vvar_start = . - 2 * PAGE_SIZE;
vvar_page = vvar_start;
 
/* Place all vvars at the offsets in asm/vvar.h. */
@@ -35,8 +35,7 @@ SECTIONS
 #undef __VVAR_KERNEL_LDS
 #undef EMIT_VVAR
 
-   hpet_page = vvar_start + PAGE_SIZE;
-   pvclock_page = vvar_start + 2 * PAGE_SIZE;
+   pvclock_page = vvar_start + PAGE_SIZE;
 
. = SIZEOF_HEADERS;
 
diff --git a/arch/x86/entry/vdso/vma.c b/arch/x86/entry/vdso/vma.c
index 10f704584922..b3cf81333a54 100644
--- a/arch/x86/entry/vdso/vma.c
+++ b/arch/x86/entry/vdso/vma.c
@@ -18,7 +18,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 
@@ -129,16 +128,6 @@ static int vvar_fault(const struct vm_special_mapping *sm,
if (sym_offset == image->sym_vvar_page) {
ret = vm_insert_pfn(vma, (unsigned long)vmf->virtual_address,
__pa_symbol(&__vvar_page) >> PAGE_SHIFT);
-   } else if (sym_offset == image->sym_hpet_page) {
-#ifdef CONFIG_HPET_TIMER
-   if (hpet_address && vclock_was_used(VCLOCK_HPET)) {
-   ret = vm_insert_pfn_prot(
-   vma,
-   (unsigned long)vmf->virtual_address,
-   hpet_address >> PAGE_SHIFT,
-   pgprot_noncached(PAGE_READONLY));
-   }
-#endif
} else if (sym_offset == image->sym_pvclock_page) {
struct pvclock_vsyscall_time_info *pvti =
pvclock_pvti_cpu0_va();
diff --git a/arch/x86/include/asm/clocksource.h 
b/arch/x86/include/asm/clocksource.h
index d194266acb28..eae33c7170c8 100644
--- a/arch/x86/include/asm/clocksource.h
+++ b/arch/x86/include/asm/clocksource.h
@@ -3,11 +3,10 @@
 #ifndef _ASM_X86_CLOCKSOURCE_H
 #define _ASM_X86_CLOCKSOURCE_H
 
-#define VCLOCK_NONE0  /* No vDSO clock available.  */
-#define VCLOCK_TSC 1  /* vDSO should use vread_tsc.*/