| Message ID | 20210112013140.35979-1-chanho61.park@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clocksource/drivers/arm_arch_timer: export arch_timer_get_rate | expand |
On Tue, Jan 12, 2021 at 10:31:40AM +0900, Chanho Park wrote: > This patch adds to export arch_timer_get_rate function for calculating > absolute timestamp which is based on arch timer like below. > arch_timer_read_counter was already exported but arch_timer_get_rate > wasn't. Thus, this patch tries to export this to use this function from > loadable kernel module. Can you please explain /where/ this would be used? i.e. which module? Generally we try to avoid drivers depending on the specific clocksource, so I think there needs to be stronger rationale for exposing this. Thanks, Mark. > > u32 rate = arch_timer_get_rate() / (1000 * 1000); > u64 abs_ns = arch_timer_read_counter() * 1000 / rate; > > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Chanho Park <chanho61.park@samsung.com> > --- > drivers/clocksource/arm_arch_timer.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > index d0177824c518..f3f49d96dbe9 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -961,6 +961,7 @@ u32 arch_timer_get_rate(void) > { > return arch_timer_rate; > } > +EXPORT_SYMBOL_GPL(arch_timer_get_rate); > > bool arch_timer_evtstrm_available(void) > { > -- > 2.23.0 >
Hi, > On Tue, Jan 12, 2021 at 10:31:40AM +0900, Chanho Park wrote: > > This patch adds to export arch_timer_get_rate function for calculating > > absolute timestamp which is based on arch timer like below. > > arch_timer_read_counter was already exported but arch_timer_get_rate > > wasn't. Thus, this patch tries to export this to use this function from > > loadable kernel module. > > Can you please explain /where/ this would be used? i.e. which module? > > Generally we try to avoid drivers depending on the specific clocksource, > so I think there needs to be stronger rationale for exposing this. I need a system-wide timestamp which can be available from bootloader and kernel stages including virtual machines. Actually, it's necessary to record a timestamp of each log message for system-wide debugging on type-1 hypervisor. RTC can be used for this purpose but we should make it to hypervisor awareness. |---------------|-------------------------| Bootloader VM1 (Guest) |-------------------------| VM2 (Guest) So, the easiest way is using the arm architect timer's timestamp because it's already supported on each VM by the hypervisor. Best Regards, Chanho Park
On 2021-01-12 13:39, Chanho Park wrote: > Hi, > >> On Tue, Jan 12, 2021 at 10:31:40AM +0900, Chanho Park wrote: >> > This patch adds to export arch_timer_get_rate function for calculating >> > absolute timestamp which is based on arch timer like below. >> > arch_timer_read_counter was already exported but arch_timer_get_rate >> > wasn't. Thus, this patch tries to export this to use this function from >> > loadable kernel module. >> >> Can you please explain /where/ this would be used? i.e. which module? >> >> Generally we try to avoid drivers depending on the specific >> clocksource, >> so I think there needs to be stronger rationale for exposing this. > > I need a system-wide timestamp which can be available from bootloader > and kernel stages including virtual machines. > Actually, it's necessary to record a timestamp of each log message for > system-wide debugging on type-1 hypervisor. > RTC can be used for this purpose but we should make it to hypervisor > awareness. > |---------------|-------------------------| > Bootloader VM1 (Guest) > |-------------------------| > VM2 (Guest) > > So, the easiest way is using the arm architect timer's timestamp > because it's already supported on each VM by the hypervisor. This doesn't make much sense. The hypervisor and the VMs are independent software entities, and they don't use symbols from each other. So this symbol is probably used by a module *inside* the VMs, and Mark's question still stands. Thanks, M.
Hi Marc, On Tue, Jan 12, 2021 at 11:45 PM Marc Zyngier <maz@kernel.org> wrote: > > On 2021-01-12 13:39, Chanho Park wrote: > > Hi, > > > >> On Tue, Jan 12, 2021 at 10:31:40AM +0900, Chanho Park wrote: > >> > This patch adds to export arch_timer_get_rate function for calculating > >> > absolute timestamp which is based on arch timer like below. > >> > arch_timer_read_counter was already exported but arch_timer_get_rate > >> > wasn't. Thus, this patch tries to export this to use this function from > >> > loadable kernel module. > >> > >> Can you please explain /where/ this would be used? i.e. which module? > >> > >> Generally we try to avoid drivers depending on the specific > >> clocksource, > >> so I think there needs to be stronger rationale for exposing this. > > > > I need a system-wide timestamp which can be available from bootloader > > and kernel stages including virtual machines. > > Actually, it's necessary to record a timestamp of each log message for > > system-wide debugging on type-1 hypervisor. > > RTC can be used for this purpose but we should make it to hypervisor > > awareness. > > |---------------|-------------------------| > > Bootloader VM1 (Guest) > > |-------------------------| > > VM2 (Guest) > > > > So, the easiest way is using the arm architect timer's timestamp > > because it's already supported on each VM by the hypervisor. > > This doesn't make much sense. The hypervisor and the VMs are > independent software entities, and they don't use symbols from > each other. I meant that each VM needs to be synchronized by the ARM arch timer's timestamp not the symbol itself. The symbol is necessary to calculate the system-wide time by the timestamp(counter) value. The counter of the arm architect timer will be increasing from the bootloader. The hypervisor will not reset the counter and each VMs as well. So, we can use this timestamp for comparing _real_ time :) > > So this symbol is probably used by a module *inside* the VMs, > and Mark's question still stands. > Yes. Actually, we use this timestamp for our internal module which is not yet upstreamed. The module is necessary to record the logs with the system-wide timestamp from bootloader to guest OS.
Chanho, On 2021-01-12 15:14, Chanho Park wrote: > Hi Marc, > > On Tue, Jan 12, 2021 at 11:45 PM Marc Zyngier <maz@kernel.org> wrote: >> >> On 2021-01-12 13:39, Chanho Park wrote: >> > Hi, >> > >> >> On Tue, Jan 12, 2021 at 10:31:40AM +0900, Chanho Park wrote: >> >> > This patch adds to export arch_timer_get_rate function for calculating >> >> > absolute timestamp which is based on arch timer like below. >> >> > arch_timer_read_counter was already exported but arch_timer_get_rate >> >> > wasn't. Thus, this patch tries to export this to use this function from >> >> > loadable kernel module. >> >> >> >> Can you please explain /where/ this would be used? i.e. which module? >> >> >> >> Generally we try to avoid drivers depending on the specific >> >> clocksource, >> >> so I think there needs to be stronger rationale for exposing this. >> > >> > I need a system-wide timestamp which can be available from bootloader >> > and kernel stages including virtual machines. >> > Actually, it's necessary to record a timestamp of each log message for >> > system-wide debugging on type-1 hypervisor. >> > RTC can be used for this purpose but we should make it to hypervisor >> > awareness. >> > |---------------|-------------------------| >> > Bootloader VM1 (Guest) >> > |-------------------------| >> > VM2 (Guest) >> > >> > So, the easiest way is using the arm architect timer's timestamp >> > because it's already supported on each VM by the hypervisor. >> >> This doesn't make much sense. The hypervisor and the VMs are >> independent software entities, and they don't use symbols from >> each other. > > I meant that each VM needs to be synchronized by the ARM arch timer's > timestamp not the symbol itself. > The symbol is necessary to calculate the system-wide time by the > timestamp(counter) value. > > The counter of the arm architect timer will be increasing from the > bootloader. > The hypervisor will not reset the counter and each VMs as well. So, we > can use this timestamp for comparing _real_ time :) Well, you can compare the raw counter values, and do the conversion in a single place. Also, if your system is correctly configured, you have access to CNTFRQ_EL0, which contains the same thing as arch_timer_get_rate(). >> So this symbol is probably used by a module *inside* the VMs, >> and Mark's question still stands. >> > > Yes. Actually, we use this timestamp for our internal module which is > not yet upstreamed. If the module code is not upstream, I don't see the point in exporting this symbol. I suggest you post both as a series so that we can decide whether that is a good idea or not. Thanks, M.
Hi Marc, > -----Original Message----- > From: Marc Zyngier <maz@kernel.org> > Sent: Wednesday, January 13, 2021 12:31 AM > To: Chanho Park <parkch98@gmail.com> > Cc: Mark Rutland <mark.rutland@arm.com>; linux-arm- > kernel@lists.infradead.org; Chanho Park <chanho61.park@samsung.com>; > Daniel Lezcano <daniel.lezcano@linaro.org>; Thomas Gleixner > <tglx@linutronix.de> > Subject: Re: [PATCH] clocksource/drivers/arm_arch_timer: export > arch_timer_get_rate > > Chanho, > > On 2021-01-12 15:14, Chanho Park wrote: > > Hi Marc, > > > > On Tue, Jan 12, 2021 at 11:45 PM Marc Zyngier <maz@kernel.org> wrote: > >> > >> On 2021-01-12 13:39, Chanho Park wrote: > >> > Hi, > >> > > >> >> On Tue, Jan 12, 2021 at 10:31:40AM +0900, Chanho Park wrote: > >> >> > This patch adds to export arch_timer_get_rate function for > >> >> > calculating absolute timestamp which is based on arch timer like > below. > >> >> > arch_timer_read_counter was already exported but > >> >> > arch_timer_get_rate wasn't. Thus, this patch tries to export > >> >> > this to use this function from loadable kernel module. > >> >> > >> >> Can you please explain /where/ this would be used? i.e. which module? > >> >> > >> >> Generally we try to avoid drivers depending on the specific > >> >> clocksource, so I think there needs to be stronger rationale for > >> >> exposing this. > >> > > >> > I need a system-wide timestamp which can be available from > >> > bootloader and kernel stages including virtual machines. > >> > Actually, it's necessary to record a timestamp of each log message > >> > for system-wide debugging on type-1 hypervisor. > >> > RTC can be used for this purpose but we should make it to > >> > hypervisor awareness. > >> > |---------------|-------------------------| > >> > Bootloader VM1 (Guest) > >> > |-------------------------| > >> > VM2 (Guest) > >> > > >> > So, the easiest way is using the arm architect timer's timestamp > >> > because it's already supported on each VM by the hypervisor. > >> > >> This doesn't make much sense. The hypervisor and the VMs are > >> independent software entities, and they don't use symbols from each > >> other. > > > > I meant that each VM needs to be synchronized by the ARM arch timer's > > timestamp not the symbol itself. > > The symbol is necessary to calculate the system-wide time by the > > timestamp(counter) value. > > > > The counter of the arm architect timer will be increasing from the > > bootloader. > > The hypervisor will not reset the counter and each VMs as well. So, we > > can use this timestamp for comparing _real_ time :) > > Well, you can compare the raw counter values, and do the conversion in a > single place. Also, if your system is correctly configured, you have > access to CNTFRQ_EL0, which contains the same thing as > arch_timer_get_rate(). To convert the value in the single place, we may need to use inter-VM communication so that it makes quite complex design/implementation for me. Regarding CNTFRQ_EL0, I already tried to read it by arch_timer_get_cntfrq but I got '0'. It looks like different according to the system. > > >> So this symbol is probably used by a module *inside* the VMs, and > >> Mark's question still stands. > >> > > > > Yes. Actually, we use this timestamp for our internal module which is > > not yet upstreamed. > > If the module code is not upstream, I don't see the point in exporting > this symbol. I suggest you post both as a series so that we can decide > whether that is a good idea or not. Well, I'm not sure the module can be upstream because it's definitely necessary only to analyze a panic in my hypervisor system. Let me find a better way to expose this. Best Regards, Chanho Park
On 2021-01-13 09:56, Chanho Park wrote: > Hi Marc, [...] >> Well, you can compare the raw counter values, and do the conversion in >> a >> single place. Also, if your system is correctly configured, you have >> access to CNTFRQ_EL0, which contains the same thing as >> arch_timer_get_rate(). > > To convert the value in the single place, we may need to use inter-VM > communication so that it makes quite complex design/implementation for > me. > Regarding CNTFRQ_EL0, I already tried to read it by > arch_timer_get_cntfrq > but I got '0'. It looks like different according to the system. That's a bug in your firmware that you should consider fixing. The architecture and the arm64 boot protocol are pretty explicit that CNTFRQ_EL0 must contain the frequency of the timer, and be initialised on all CPUs. You are probably using some DT property to advertise the frequency, which is very much discouraged. >> >> >> So this symbol is probably used by a module *inside* the VMs, and >> >> Mark's question still stands. >> >> >> > >> > Yes. Actually, we use this timestamp for our internal module which is >> > not yet upstreamed. >> >> If the module code is not upstream, I don't see the point in exporting >> this symbol. I suggest you post both as a series so that we can decide >> whether that is a good idea or not. > > Well, I'm not sure the module can be upstream because it's definitely > necessary only to analyze a panic in my hypervisor system. Let me find > a > better way to expose this. If your module doesn't make sense in the upstream kernel, then exporting the symbol doesn't make much sense either. M.
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index d0177824c518..f3f49d96dbe9 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -961,6 +961,7 @@ u32 arch_timer_get_rate(void) { return arch_timer_rate; } +EXPORT_SYMBOL_GPL(arch_timer_get_rate); bool arch_timer_evtstrm_available(void) {
This patch adds to export arch_timer_get_rate function for calculating absolute timestamp which is based on arch timer like below. arch_timer_read_counter was already exported but arch_timer_get_rate wasn't. Thus, this patch tries to export this to use this function from loadable kernel module. u32 rate = arch_timer_get_rate() / (1000 * 1000); u64 abs_ns = arch_timer_read_counter() * 1000 / rate; Cc: Mark Rutland <mark.rutland@arm.com> Cc: Marc Zyngier <maz@kernel.org> Cc: Daniel Lezcano <daniel.lezcano@linaro.org> Cc: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Chanho Park <chanho61.park@samsung.com> --- drivers/clocksource/arm_arch_timer.c | 1 + 1 file changed, 1 insertion(+)