[PATCH] clocksource/drivers/arm_arch_timer: export arch_timer_get_rate

[PATCH] clocksource/drivers/arm_arch_timer: export arch_timer_get_rate

[Chanho Park]

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

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] clocksource/drivers/arm_arch_timer: export arch_timer_get_rate

[Mark Rutland]

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] clocksource/drivers/arm_arch_timer: export arch_timer_get_rate

[Chanho Park]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] clocksource/drivers/arm_arch_timer: export arch_timer_get_rate

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] clocksource/drivers/arm_arch_timer: export arch_timer_get_rate

[Chanho Park]

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.


-- 
Best Regards,
Chanho Park

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] clocksource/drivers/arm_arch_timer: export arch_timer_get_rate

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] clocksource/drivers/arm_arch_timer: export arch_timer_get_rate

[Chanho Park]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] clocksource/drivers/arm_arch_timer: export arch_timer_get_rate

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel