| Message ID | 1445854558-16253-1-git-send-email-amittomer25@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 26, 2015 at 03:45:58PM +0530, Amit wrote: > From: Amit Singh Tomar <amittomer25@gmail.com> > > Define new arm/arm64 specific trace clock using CNTPCT/CNTPCT_EL0 register, > similar to x86-tsc. It can be used to correlate trace events across > hosts and guest that can be useful for debugging purpose. > > Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com> > --- > Changes since RFC: > * Replaced define with static inline > * Revert few changes done in ftrace.txt > * Made it common between ARM and ARM64. > --- > Documentation/trace/ftrace.txt | 12 ++++++++++++ > arch/arm/include/asm/Kbuild | 1 - > arch/arm/include/asm/trace_clock.h | 21 +++++++++++++++++++++ > arch/arm/kernel/Makefile | 2 ++ > arch/arm/kernel/trace_clock.c | 33 +++++++++++++++++++++++++++++++++ > arch/arm64/include/asm/Kbuild | 1 - > arch/arm64/include/asm/trace_clock.h | 21 +++++++++++++++++++++ > arch/arm64/kernel/Makefile | 2 ++ > 8 files changed, 91 insertions(+), 2 deletions(-) > create mode 100644 arch/arm/include/asm/trace_clock.h > create mode 100644 arch/arm/kernel/trace_clock.c > create mode 100644 arch/arm64/include/asm/trace_clock.h > > diff --git a/Documentation/trace/ftrace.txt b/Documentation/trace/ftrace.txt > index ef621d3..13f3737 100644 > --- a/Documentation/trace/ftrace.txt > +++ b/Documentation/trace/ftrace.txt > @@ -351,6 +351,18 @@ of ftrace. Here is a list of some of the key files: > to correlate events across hypervisor/guest if > tb_offset is known. > > + arm-pct: This uses ARM Physical Count register value.This > + is different from default "local" clock which > + usese Virtual Count register.This is consistent > + across processors and can be used to correlate > + events across host/guest. > + > + arm64-pct: This uses ARM64 Physical Timer Count register > + value. This is different from default "local" > + clock which usese Virtual Timer Count register. > + This is consistent across processors and can be > + used to correlate events across host/guest. The above is wrong. Both the physical and virtual counters have the same guarantees w.r.t. consistency across CPUs for the same VM image. Neither is more local than the other. In the host you will have access to CNTVOFF, so you can use that to correlate the virtual counter. Additionally, it's not safe to read the physical counter register unless you know that access has not been denied by the hypervisor; so any Linux image not booted at hyp cannot use this. If the hypervisor emulates the access, it will also be horrifically slow. I don't think this makes sense. > diff --git a/arch/arm/kernel/trace_clock.c b/arch/arm/kernel/trace_clock.c > new file mode 100644 > index 0000000..3bf040d > --- /dev/null > +++ b/arch/arm/kernel/trace_clock.c > @@ -0,0 +1,33 @@ > +/* > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License, version 2, as > + * published by the Free Software Foundation. > + * > + * Copyright (C) 2015 Amit Singh Tomar, amittomer25@gmail.com > + * > + * Thanks to Steven Rostedt and Andre Przywara! > +*/ > + > +#include <asm/trace_clock.h> > +#include <asm/barrier.h> > +#include <asm/arch_timer.h> > + > +static inline u64 get_cntpct(void) > +{ > + u64 cycles; > + > + isb(); > + asm volatile("mrs %0, cntpct_el0" : "=r" (cycles)); > + > + return cycles; > +} This is arch/arm. This cannot build for 32-bit. Regardless, this doesn't belong here. Thanks, Mark.
On Wed, Oct 28, 2015 at 6:17 PM, Mark Rutland <mark.rutland@arm.com> wrote: > On Mon, Oct 26, 2015 at 03:45:58PM +0530, Amit wrote: >> From: Amit Singh Tomar <amittomer25@gmail.com> >> >> Define new arm/arm64 specific trace clock using CNTPCT/CNTPCT_EL0 register, >> similar to x86-tsc. It can be used to correlate trace events across >> hosts and guest that can be useful for debugging purpose. >> >> Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com> >> --- >> Changes since RFC: >> * Replaced define with static inline >> * Revert few changes done in ftrace.txt >> * Made it common between ARM and ARM64. >> --- >> Documentation/trace/ftrace.txt | 12 ++++++++++++ >> arch/arm/include/asm/Kbuild | 1 - >> arch/arm/include/asm/trace_clock.h | 21 +++++++++++++++++++++ >> arch/arm/kernel/Makefile | 2 ++ >> arch/arm/kernel/trace_clock.c | 33 +++++++++++++++++++++++++++++++++ >> arch/arm64/include/asm/Kbuild | 1 - >> arch/arm64/include/asm/trace_clock.h | 21 +++++++++++++++++++++ >> arch/arm64/kernel/Makefile | 2 ++ >> 8 files changed, 91 insertions(+), 2 deletions(-) >> create mode 100644 arch/arm/include/asm/trace_clock.h >> create mode 100644 arch/arm/kernel/trace_clock.c >> create mode 100644 arch/arm64/include/asm/trace_clock.h >> >> diff --git a/Documentation/trace/ftrace.txt b/Documentation/trace/ftrace.txt >> index ef621d3..13f3737 100644 >> --- a/Documentation/trace/ftrace.txt >> +++ b/Documentation/trace/ftrace.txt >> @@ -351,6 +351,18 @@ of ftrace. Here is a list of some of the key files: >> to correlate events across hypervisor/guest if >> tb_offset is known. >> >> + arm-pct: This uses ARM Physical Count register value.This >> + is different from default "local" clock which >> + usese Virtual Count register.This is consistent >> + across processors and can be used to correlate >> + events across host/guest. >> + >> + arm64-pct: This uses ARM64 Physical Timer Count register >> + value. This is different from default "local" >> + clock which usese Virtual Timer Count register. >> + This is consistent across processors and can be >> + used to correlate events across host/guest. > > The above is wrong. Both the physical and virtual counters have the same > guarantees w.r.t. consistency across CPUs for the same VM image. Neither > is more local than the other. I never said virtual counters are not consistent across CPU's but pointed out that Values of Virtual counter seen by host and guest are different(but even in Host context I have seen VCT is not exactly equal to PCT). > In the host you will have access to CNTVOFF, so you can use that to > correlate the virtual counter. I don't think CNTVOFF can be accessible from normal el1 host code and do you think reading it from el2 and then feed it to virtual counter is good idea? > Additionally, it's not safe to read the physical counter register unless > you know that access has not been denied by the hypervisor; so any Linux > image not booted at hyp cannot use this. If the hypervisor emulates the > access, it will also be horrifically slow. > Sorry but are you saying if kernel doesn't boot in HYP mode , I have no way to read Physical counter in Guest? > > This is arch/arm. This cannot build for 32-bit. > I did build it on ARM(32 bit) and for the same made a check using "#ifdef CONFIG_ARM64", so would you relook at it again. Thanks Amit.
[adding Marc and Christoffer] On Thu, Oct 29, 2015 at 12:32:51AM +0530, Amit Tomer wrote: > On Wed, Oct 28, 2015 at 6:17 PM, Mark Rutland <mark.rutland@arm.com> wrote: > > On Mon, Oct 26, 2015 at 03:45:58PM +0530, Amit wrote: > >> From: Amit Singh Tomar <amittomer25@gmail.com> > >> > >> Define new arm/arm64 specific trace clock using CNTPCT/CNTPCT_EL0 register, > >> similar to x86-tsc. It can be used to correlate trace events across > >> hosts and guest that can be useful for debugging purpose. > >> > >> Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com> > >> --- > >> Changes since RFC: > >> * Replaced define with static inline > >> * Revert few changes done in ftrace.txt > >> * Made it common between ARM and ARM64. > >> --- > >> Documentation/trace/ftrace.txt | 12 ++++++++++++ > >> arch/arm/include/asm/Kbuild | 1 - > >> arch/arm/include/asm/trace_clock.h | 21 +++++++++++++++++++++ > >> arch/arm/kernel/Makefile | 2 ++ > >> arch/arm/kernel/trace_clock.c | 33 +++++++++++++++++++++++++++++++++ > >> arch/arm64/include/asm/Kbuild | 1 - > >> arch/arm64/include/asm/trace_clock.h | 21 +++++++++++++++++++++ > >> arch/arm64/kernel/Makefile | 2 ++ > >> 8 files changed, 91 insertions(+), 2 deletions(-) > >> create mode 100644 arch/arm/include/asm/trace_clock.h > >> create mode 100644 arch/arm/kernel/trace_clock.c > >> create mode 100644 arch/arm64/include/asm/trace_clock.h > >> > >> diff --git a/Documentation/trace/ftrace.txt b/Documentation/trace/ftrace.txt > >> index ef621d3..13f3737 100644 > >> --- a/Documentation/trace/ftrace.txt > >> +++ b/Documentation/trace/ftrace.txt > >> @@ -351,6 +351,18 @@ of ftrace. Here is a list of some of the key files: > >> to correlate events across hypervisor/guest if > >> tb_offset is known. > >> > >> + arm-pct: This uses ARM Physical Count register value.This > >> + is different from default "local" clock which > >> + usese Virtual Count register.This is consistent > >> + across processors and can be used to correlate > >> + events across host/guest. > >> + > >> + arm64-pct: This uses ARM64 Physical Timer Count register > >> + value. This is different from default "local" > >> + clock which usese Virtual Timer Count register. > >> + This is consistent across processors and can be > >> + used to correlate events across host/guest. > > > > The above is wrong. Both the physical and virtual counters have the same > > guarantees w.r.t. consistency across CPUs for the same VM image. Neither > > is more local than the other. > > I never said virtual counters are not consistent across CPU's but pointed out > that Values of Virtual counter seen by host and guest are > different(but even in Host context Ok. > I have seen VCT is not exactly equal to PCT). That should only be the case when running a guest (or running at hyp). Accross CPUS, CNTVCT should be consistent for the same OS image (i.e. those CPUs in the same VM will have the same value at the same point in time). > > In the host you will have access to CNTVOFF, so you can use that to > > correlate the virtual counter. > > I don't think CNTVOFF can be accessible from normal el1 host code and > do you think reading it from el2 and then feed it to virtual counter > is good idea? There are KVM APIs for acquring and configuring CNTVOFF for the guest. The host kernel knows what the virtual offset is. You could even configure the offset to be zero if you wanted, which would make the guest and host virtual counters uniform. > > Additionally, it's not safe to read the physical counter register unless > > you know that access has not been denied by the hypervisor; so any Linux > > image not booted at hyp cannot use this. If the hypervisor emulates the > > access, it will also be horrifically slow. > > Sorry but are you saying if kernel doesn't boot in HYP mode , I have no way to > read Physical counter in Guest? Regardless of the host, you cannot know whether it is safe to access in a guest. It could bring down the system. The host kernel could safely access the physical counter were it booted at EL2, because it could grant itself access. Thanks, Mark.
> Regardless of the host, you cannot know whether it is safe to access > in a guest. It could bring down the system. > > The host kernel could safely access the physical counter were it booted > at EL2, because it could grant itself access. May be I am totally wrong here but it doesn't look to be good . if Linux image does not boot at hyp mode, there is no way to use arch timers, Not true? But I could see even if Linux image booted in EL1 has the access to Physical timer/counters that is been allowed by firmware it self. ENTRY(armv8_switch_to_el1) switch_el x0, 0f, 1f, 0f 0: ret 1: /* Initialize Generic Timers */ mrs x0, cnthctl_el2 orr x0, x0, #0x3 /* Enable EL1 access to timers */ msr cnthctl_el2, x0 msr cntvoff_el2, x0 mrs x0, cntkctl_el1 orr x0, x0, #0x3 /* Enable EL0 access to timers */ msr cntkctl_el1, x0 Sorry, if it's just all stupid. Thanks, Amit.
On Thu, Oct 29, 2015 at 07:17:31PM +0530, Amit Tomer wrote: > > Regardless of the host, you cannot know whether it is safe to access > > in a guest. It could bring down the system. > > > > The host kernel could safely access the physical counter were it booted > > at EL2, because it could grant itself access. > > May be I am totally wrong here but it doesn't look to be good . if > Linux image does not boot at hyp mode, > there is no way to use arch timers, Not true? The _virtual_ counter and timer registers are _always_ accessible at EL1. The _physical_ counter and timer registers are not. The kernel can safely use the virtual counter & timer alone, but cannot use the physical counter & timer. It has no way of determining if the physical registers are accessible (and whether they will remain so). Even if they were accessible, the physical values can change arbitrarily (think suspend+resuming a VM), so the physical values are generally useless to a VM. > But I could see even if Linux image booted in EL1 has the access to > Physical timer/counters that is > been allowed by firmware it self. The kernel has no way of knowing whether the physical counter & timer are accessible, even if firmware left them accessible. Their accessibility could change at a moment's notice (e.g. the state of the control registers could be lost over idle). > ENTRY(armv8_switch_to_el1) > switch_el x0, 0f, 1f, 0f > 0: ret > 1: > /* Initialize Generic Timers */ > mrs x0, cnthctl_el2 > orr x0, x0, #0x3 /* Enable EL1 access to timers */ > msr cnthctl_el2, x0 > msr cntvoff_el2, x0 > mrs x0, cntkctl_el1 > orr x0, x0, #0x3 /* Enable EL0 access to timers */ > msr cntkctl_el1, x0 I'm not sure how the above code is relevant. Mark.
diff --git a/Documentation/trace/ftrace.txt b/Documentation/trace/ftrace.txt index ef621d3..13f3737 100644 --- a/Documentation/trace/ftrace.txt +++ b/Documentation/trace/ftrace.txt @@ -351,6 +351,18 @@ of ftrace. Here is a list of some of the key files: to correlate events across hypervisor/guest if tb_offset is known. + arm-pct: This uses ARM Physical Count register value.This + is different from default "local" clock which + usese Virtual Count register.This is consistent + across processors and can be used to correlate + events across host/guest. + + arm64-pct: This uses ARM64 Physical Timer Count register + value. This is different from default "local" + clock which usese Virtual Timer Count register. + This is consistent across processors and can be + used to correlate events across host/guest. + To set a clock, simply echo the clock name into this file. echo global > trace_clock diff --git a/arch/arm/include/asm/Kbuild b/arch/arm/include/asm/Kbuild index be648eb..bc3964b 100644 --- a/arch/arm/include/asm/Kbuild +++ b/arch/arm/include/asm/Kbuild @@ -34,5 +34,4 @@ generic-y += sockios.h generic-y += termbits.h generic-y += termios.h generic-y += timex.h -generic-y += trace_clock.h generic-y += unaligned.h diff --git a/arch/arm/include/asm/trace_clock.h b/arch/arm/include/asm/trace_clock.h new file mode 100644 index 0000000..dc41f75 --- /dev/null +++ b/arch/arm/include/asm/trace_clock.h @@ -0,0 +1,21 @@ +/* + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License, version 2, as + * published by the Free Software Foundation. + * + * Copyright (C) 2015 Amit Singh Tomar, amittomer25@gmail.com +*/ + +#ifndef _ASM_ARM_TRACE_CLOCK_H +#define _ASM_ARM_TRACE_CLOCK_H + +#include <linux/compiler.h> +#include <linux/types.h> + +extern u64 notrace trace_clock_arm_pct(void); + +# define ARCH_TRACE_CLOCKS \ + { trace_clock_arm_pct, "arm-pct", 0}, + +#endif /* _ASM_ARM_TRACE_CLOCK_H */ + diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile index af9e59b..c3abf7c 100644 --- a/arch/arm/kernel/Makefile +++ b/arch/arm/kernel/Makefile @@ -92,4 +92,6 @@ obj-y += psci-call.o obj-$(CONFIG_SMP) += psci_smp.o endif +obj-$(CONFIG_TRACING) += trace_clock.o + extra-y := $(head-y) vmlinux.lds diff --git a/arch/arm/kernel/trace_clock.c b/arch/arm/kernel/trace_clock.c new file mode 100644 index 0000000..3bf040d --- /dev/null +++ b/arch/arm/kernel/trace_clock.c @@ -0,0 +1,33 @@ +/* + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License, version 2, as + * published by the Free Software Foundation. + * + * Copyright (C) 2015 Amit Singh Tomar, amittomer25@gmail.com + * + * Thanks to Steven Rostedt and Andre Przywara! +*/ + +#include <asm/trace_clock.h> +#include <asm/barrier.h> +#include <asm/arch_timer.h> + +static inline u64 get_cntpct(void) +{ + u64 cycles; + + isb(); + asm volatile("mrs %0, cntpct_el0" : "=r" (cycles)); + + return cycles; +} + +u64 notrace trace_clock_arm_pct(void) +{ + return arch_counter_get_cntpct(); +} + +#ifdef CONFIG_ARM64 +u64 notrace trace_clock_arm64_pct(void) +{ + return get_cntpct(); +} +#endif diff --git a/arch/arm64/include/asm/Kbuild b/arch/arm64/include/asm/Kbuild index 70fd9ff..8608f9e 100644 --- a/arch/arm64/include/asm/Kbuild +++ b/arch/arm64/include/asm/Kbuild @@ -50,7 +50,6 @@ generic-y += switch_to.h generic-y += termbits.h generic-y += termios.h generic-y += topology.h -generic-y += trace_clock.h generic-y += types.h generic-y += unaligned.h generic-y += user.h diff --git a/arch/arm64/include/asm/trace_clock.h b/arch/arm64/include/asm/trace_clock.h new file mode 100644 index 0000000..aef4ccd --- /dev/null +++ b/arch/arm64/include/asm/trace_clock.h @@ -0,0 +1,21 @@ +/* + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License, version 2, as + * published by the Free Software Foundation. + * + * Copyright (C) 2015 Amit Singh Tomar, amittomer25@gmail.com +*/ + +#ifndef _ASM_ARM_TRACE_CLOCK_H +#define _ASM_ARM_TRACE_CLOCK_H + +#include <linux/compiler.h> +#include <linux/types.h> + +extern u64 notrace trace_clock_arm64_pct(void); + +# define ARCH_TRACE_CLOCKS \ + { trace_clock_arm64_pct, "arm64-pct", 0}, + +#endif /* _ASM_ARM_TRACE_CLOCK_H */ + diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile index 22dc9bc..b23a149 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -10,6 +10,7 @@ CFLAGS_armv8_deprecated.o := -I$(src) CFLAGS_REMOVE_ftrace.o = -pg CFLAGS_REMOVE_insn.o = -pg CFLAGS_REMOVE_return_address.o = -pg +ARM=../../../arch/arm/kernel # Object file lists. arm64-obj-y := debug-monitors.o entry.o irq.o fpsimd.o \ @@ -36,6 +37,7 @@ arm64-obj-$(CONFIG_EFI) += efi.o efi-stub.o efi-entry.o arm64-obj-$(CONFIG_PCI) += pci.o arm64-obj-$(CONFIG_ARMV8_DEPRECATED) += armv8_deprecated.o arm64-obj-$(CONFIG_ACPI) += acpi.o +arm64-obj-$(CONFIG_TRACING) += $(ARM)/trace_clock.o obj-y += $(arm64-obj-y) vdso/ obj-m += $(arm64-obj-m)