KVM ARM Archive on lore.kernel.org
 help / color / Atom feed
* [RESEND PATCH] drivers: arm arch timer: Correct fault programming of CNTKCTL_EL1.EVNTI
@ 2020-07-17  9:21 Keqian Zhu
  2020-07-28  7:39 ` zhukeqian
  2020-07-28 10:16 ` Marc Zyngier
  0 siblings, 2 replies; 4+ messages in thread
From: Keqian Zhu @ 2020-07-17  9:21 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, kvmarm, kvm
  Cc: Marc Zyngier, Catalin Marinas, Will Deacon

ARM virtual counter supports event stream. It can only trigger an event
when the trigger bit of CNTVCT_EL0 changes from 0 to 1 (or from 1 to 0),
so the actual period of event stream is 2 ^ (cntkctl_evnti + 1). For
example, when the trigger bit is 0, then it triggers an event for every
two cycles.

Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
---
 drivers/clocksource/arm_arch_timer.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index ecf7b7db2d05..06d99a4b1b9b 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -799,10 +799,20 @@ static void __arch_timer_setup(unsigned type,
 static void arch_timer_evtstrm_enable(int divider)
 {
 	u32 cntkctl = arch_timer_get_cntkctl();
+	int cntkctl_evnti;
+
+	/*
+	 * Note that it can only trigger an event when the trigger bit
+	 * of CNTVCT_EL0 changes from 1 to 0 (or from 0 to 1), so the
+	 * actual period of event stream is 2 ^ (cntkctl_evnti + 1).
+	 */
+	cntkctl_evnti = divider - 1;
+	cntkctl_evnti = min(cntkctl_evnti, 15);
+	cntkctl_evnti = max(cntkctl_evnti, 0);
 
 	cntkctl &= ~ARCH_TIMER_EVT_TRIGGER_MASK;
 	/* Set the divider and enable virtual event stream */
-	cntkctl |= (divider << ARCH_TIMER_EVT_TRIGGER_SHIFT)
+	cntkctl |= (cntkctl_evnti << ARCH_TIMER_EVT_TRIGGER_SHIFT)
 			| ARCH_TIMER_VIRT_EVT_EN;
 	arch_timer_set_cntkctl(cntkctl);
 	arch_timer_set_evtstrm_feature();
@@ -816,10 +826,11 @@ static void arch_timer_configure_evtstream(void)
 	/* Find the closest power of two to the divisor */
 	evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ;
 	pos = fls(evt_stream_div);
-	if (pos > 1 && !(evt_stream_div & (1 << (pos - 2))))
+	if ((pos == 1) || (pos > 1 && !(evt_stream_div & (1 << (pos - 2)))))
 		pos--;
+
 	/* enable event stream */
-	arch_timer_evtstrm_enable(min(pos, 15));
+	arch_timer_evtstrm_enable(pos);
 }
 
 static void arch_counter_set_user_access(void)
-- 
2.19.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RESEND PATCH] drivers: arm arch timer: Correct fault programming of CNTKCTL_EL1.EVNTI
  2020-07-17  9:21 [RESEND PATCH] drivers: arm arch timer: Correct fault programming of CNTKCTL_EL1.EVNTI Keqian Zhu
@ 2020-07-28  7:39 ` zhukeqian
  2020-07-28 10:16 ` Marc Zyngier
  1 sibling, 0 replies; 4+ messages in thread
From: zhukeqian @ 2020-07-28  7:39 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, kvmarm, kvm
  Cc: Marc Zyngier, Catalin Marinas, Will Deacon

Friendly ping.
Is this an effective bugfix?

On 2020/7/17 17:21, Keqian Zhu wrote:
> ARM virtual counter supports event stream. It can only trigger an event
> when the trigger bit of CNTVCT_EL0 changes from 0 to 1 (or from 1 to 0),
> so the actual period of event stream is 2 ^ (cntkctl_evnti + 1). For
> example, when the trigger bit is 0, then it triggers an event for every
> two cycles.
> 
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> ---
>  drivers/clocksource/arm_arch_timer.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index ecf7b7db2d05..06d99a4b1b9b 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -799,10 +799,20 @@ static void __arch_timer_setup(unsigned type,
>  static void arch_timer_evtstrm_enable(int divider)
>  {
>  	u32 cntkctl = arch_timer_get_cntkctl();
> +	int cntkctl_evnti;
> +
> +	/*
> +	 * Note that it can only trigger an event when the trigger bit
> +	 * of CNTVCT_EL0 changes from 1 to 0 (or from 0 to 1), so the
> +	 * actual period of event stream is 2 ^ (cntkctl_evnti + 1).
> +	 */
> +	cntkctl_evnti = divider - 1;
> +	cntkctl_evnti = min(cntkctl_evnti, 15);
> +	cntkctl_evnti = max(cntkctl_evnti, 0);
>  
>  	cntkctl &= ~ARCH_TIMER_EVT_TRIGGER_MASK;
>  	/* Set the divider and enable virtual event stream */
> -	cntkctl |= (divider << ARCH_TIMER_EVT_TRIGGER_SHIFT)
> +	cntkctl |= (cntkctl_evnti << ARCH_TIMER_EVT_TRIGGER_SHIFT)
>  			| ARCH_TIMER_VIRT_EVT_EN;
>  	arch_timer_set_cntkctl(cntkctl);
>  	arch_timer_set_evtstrm_feature();
> @@ -816,10 +826,11 @@ static void arch_timer_configure_evtstream(void)
>  	/* Find the closest power of two to the divisor */
>  	evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ;
>  	pos = fls(evt_stream_div);
> -	if (pos > 1 && !(evt_stream_div & (1 << (pos - 2))))
> +	if ((pos == 1) || (pos > 1 && !(evt_stream_div & (1 << (pos - 2)))))
>  		pos--;
> +
>  	/* enable event stream */
> -	arch_timer_evtstrm_enable(min(pos, 15));
> +	arch_timer_evtstrm_enable(pos);
>  }
>  
>  static void arch_counter_set_user_access(void)
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RESEND PATCH] drivers: arm arch timer: Correct fault programming of CNTKCTL_EL1.EVNTI
  2020-07-17  9:21 [RESEND PATCH] drivers: arm arch timer: Correct fault programming of CNTKCTL_EL1.EVNTI Keqian Zhu
  2020-07-28  7:39 ` zhukeqian
@ 2020-07-28 10:16 ` Marc Zyngier
  2020-07-28 11:59   ` zhukeqian
  1 sibling, 1 reply; 4+ messages in thread
From: Marc Zyngier @ 2020-07-28 10:16 UTC (permalink / raw)
  To: Keqian Zhu
  Cc: kvm, Catalin Marinas, linux-kernel, Will Deacon, kvmarm,
	linux-arm-kernel

On 2020-07-17 10:21, Keqian Zhu wrote:
> ARM virtual counter supports event stream. It can only trigger an event
> when the trigger bit of CNTVCT_EL0 changes from 0 to 1 (or from 1 to 
> 0),
> so the actual period of event stream is 2 ^ (cntkctl_evnti + 1). For
> example, when the trigger bit is 0, then it triggers an event for every
> two cycles.
> 
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> ---
>  drivers/clocksource/arm_arch_timer.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c
> b/drivers/clocksource/arm_arch_timer.c
> index ecf7b7db2d05..06d99a4b1b9b 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -799,10 +799,20 @@ static void __arch_timer_setup(unsigned type,
>  static void arch_timer_evtstrm_enable(int divider)
>  {
>  	u32 cntkctl = arch_timer_get_cntkctl();
> +	int cntkctl_evnti;
> +
> +	/*
> +	 * Note that it can only trigger an event when the trigger bit
> +	 * of CNTVCT_EL0 changes from 1 to 0 (or from 0 to 1), so the
> +	 * actual period of event stream is 2 ^ (cntkctl_evnti + 1).
> +	 */
> +	cntkctl_evnti = divider - 1;
> +	cntkctl_evnti = min(cntkctl_evnti, 15);
> +	cntkctl_evnti = max(cntkctl_evnti, 0);
> 
>  	cntkctl &= ~ARCH_TIMER_EVT_TRIGGER_MASK;
>  	/* Set the divider and enable virtual event stream */
> -	cntkctl |= (divider << ARCH_TIMER_EVT_TRIGGER_SHIFT)
> +	cntkctl |= (cntkctl_evnti << ARCH_TIMER_EVT_TRIGGER_SHIFT)
>  			| ARCH_TIMER_VIRT_EVT_EN;
>  	arch_timer_set_cntkctl(cntkctl);
>  	arch_timer_set_evtstrm_feature();
> @@ -816,10 +826,11 @@ static void arch_timer_configure_evtstream(void)
>  	/* Find the closest power of two to the divisor */
>  	evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ;
>  	pos = fls(evt_stream_div);
> -	if (pos > 1 && !(evt_stream_div & (1 << (pos - 2))))
> +	if ((pos == 1) || (pos > 1 && !(evt_stream_div & (1 << (pos - 2)))))
>  		pos--;
> +
>  	/* enable event stream */
> -	arch_timer_evtstrm_enable(min(pos, 15));
> +	arch_timer_evtstrm_enable(pos);
>  }
> 
>  static void arch_counter_set_user_access(void)

This looks like a very convoluted fix. If the problem you are
trying to fix is that the event frequency is at most half of
that of the counter, why isn't the patch below the most
obvious fix?

Thanks,

         M.

diff --git a/drivers/clocksource/arm_arch_timer.c 
b/drivers/clocksource/arm_arch_timer.c
index 6c3e84180146..0a65414b781f 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -824,8 +824,12 @@ static void arch_timer_configure_evtstream(void)
  {
  	int evt_stream_div, pos;

-	/* Find the closest power of two to the divisor */
-	evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ;
+	/*
+	 * Find the closest power of two to the divisor. As the event
+	 * stream can at most be generated at half the frequency of the
+	 * counter, use half the frequency when computing the divider.
+	 */
+	evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ / 2;
  	pos = fls(evt_stream_div);
  	if (pos > 1 && !(evt_stream_div & (1 << (pos - 2))))
  		pos--;

-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RESEND PATCH] drivers: arm arch timer: Correct fault programming of CNTKCTL_EL1.EVNTI
  2020-07-28 10:16 ` Marc Zyngier
@ 2020-07-28 11:59   ` zhukeqian
  0 siblings, 0 replies; 4+ messages in thread
From: zhukeqian @ 2020-07-28 11:59 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: kvm, Catalin Marinas, linux-kernel, Will Deacon, kvmarm,
	linux-arm-kernel

Hi Marc,

On 2020/7/28 18:16, Marc Zyngier wrote:
> On 2020-07-17 10:21, Keqian Zhu wrote:
>> ARM virtual counter supports event stream. It can only trigger an event
>> when the trigger bit of CNTVCT_EL0 changes from 0 to 1 (or from 1 to 0),
>> so the actual period of event stream is 2 ^ (cntkctl_evnti + 1). For
>> example, when the trigger bit is 0, then it triggers an event for every
>> two cycles.
>>
>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>> ---
>>  drivers/clocksource/arm_arch_timer.c | 17 ++++++++++++++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c
>> b/drivers/clocksource/arm_arch_timer.c
>> index ecf7b7db2d05..06d99a4b1b9b 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -799,10 +799,20 @@ static void __arch_timer_setup(unsigned type,
>>  static void arch_timer_evtstrm_enable(int divider)
>>  {
>>      u32 cntkctl = arch_timer_get_cntkctl();
>> +    int cntkctl_evnti;
>> +
>> +    /*
>> +     * Note that it can only trigger an event when the trigger bit
>> +     * of CNTVCT_EL0 changes from 1 to 0 (or from 0 to 1), so the
>> +     * actual period of event stream is 2 ^ (cntkctl_evnti + 1).
>> +     */
>> +    cntkctl_evnti = divider - 1;
>> +    cntkctl_evnti = min(cntkctl_evnti, 15);
>> +    cntkctl_evnti = max(cntkctl_evnti, 0);
>>
>>      cntkctl &= ~ARCH_TIMER_EVT_TRIGGER_MASK;
>>      /* Set the divider and enable virtual event stream */
>> -    cntkctl |= (divider << ARCH_TIMER_EVT_TRIGGER_SHIFT)
>> +    cntkctl |= (cntkctl_evnti << ARCH_TIMER_EVT_TRIGGER_SHIFT)
>>              | ARCH_TIMER_VIRT_EVT_EN;
>>      arch_timer_set_cntkctl(cntkctl);
>>      arch_timer_set_evtstrm_feature();
>> @@ -816,10 +826,11 @@ static void arch_timer_configure_evtstream(void)
>>      /* Find the closest power of two to the divisor */
>>      evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ;
>>      pos = fls(evt_stream_div);
>> -    if (pos > 1 && !(evt_stream_div & (1 << (pos - 2))))
>> +    if ((pos == 1) || (pos > 1 && !(evt_stream_div & (1 << (pos - 2)))))
>>          pos--;
>> +
>>      /* enable event stream */
>> -    arch_timer_evtstrm_enable(min(pos, 15));
>> +    arch_timer_evtstrm_enable(pos);
>>  }
>>
>>  static void arch_counter_set_user_access(void)
> 
> This looks like a very convoluted fix. If the problem you are
> trying to fix is that the event frequency is at most half of
> that of the counter, why isn't the patch below the most
> obvious fix?
> 
> Thanks,
> 
>         M.
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 6c3e84180146..0a65414b781f 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -824,8 +824,12 @@ static void arch_timer_configure_evtstream(void)
>  {
>      int evt_stream_div, pos;
> 
> -    /* Find the closest power of two to the divisor */
> -    evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ;
> +    /*
> +     * Find the closest power of two to the divisor. As the event
> +     * stream can at most be generated at half the frequency of the
> +     * counter, use half the frequency when computing the divider.
> +     */
> +    evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ / 2;
>      pos = fls(evt_stream_div);
>      if (pos > 1 && !(evt_stream_div & (1 << (pos - 2))))
I think here does not consider the case of pos==1 (though it will not occur...)
>          pos--;
> 

It looks good to me.

Thanks,
Keqian
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-17  9:21 [RESEND PATCH] drivers: arm arch timer: Correct fault programming of CNTKCTL_EL1.EVNTI Keqian Zhu
2020-07-28  7:39 ` zhukeqian
2020-07-28 10:16 ` Marc Zyngier
2020-07-28 11:59   ` zhukeqian

KVM ARM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvmarm/0 kvmarm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvmarm kvmarm/ https://lore.kernel.org/kvmarm \
		kvmarm@lists.cs.columbia.edu
	public-inbox-index kvmarm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/edu.columbia.cs.lists.kvmarm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git