Re: [Qemu-devel] [PATCH v1 3/3] armv7m_systick: abort instead of locking on a bad rate

[KONRAD Frederic]

On 06/29/2017 03:02 PM, Philippe Mathieu-Daudé wrote:

On 06/29/2017 09:43 AM, Peter Maydell wrote:
On 29 June 2017 at 13:35, Philippe Mathieu-Daudé 
 wrote:

This is true it is better to abort here than risking a deadlock.

However it seems to me they are 3 issues here:
- the deadlock pattern is caused by using a global variable,
- stellaris:ssys_calculate_system_clock() no checking 
RCC.SYSDIV and

RCC2.SYSDIV2 values <= 2 are reserved (GUEST_ERROR)
- stellaris:ssys_write(RCC2) not overrides correctly RCC


Stellaris works fine. It's other ARMv7M boards (which might forget
to set system_clock_scale) which don't work.


Oh I misread ssys_calculate_system_clock(), indeed 
system_clock_scale can not get below 5 so no deadlock on Stellaris.


I'd rather take this opportunity to fix the deadlock pattern 
using a
getter/setter on system_clock_scale, doing the zero check in 
the setter and

eventually aborting in the getter, what do you think?


We should be using a clock property on the CPU instead of 
system_clock_scale.
Unfortunately Konrad's series attempting to add that 
infrastructure

is still in the "trying to sort out the right API in code review"
stage. I don't think it's worth trying to fiddle with the API
for it before we have the right eventual infrastructure in place.


I see. I'd rather move the comment and assert() in systick_scale().


Makes sense, I missed the potential divide-by-zero exception
which might happen in SysTick Current Value:

val = ((s->tick - (t + 1)) / systick_scale(s)) + 1;

I'll do the change and re-post a V2 when there will be some input
on the two first patches.

Thanks,
Fred




(What system_clock_scale is actually doing is setting the
emulated frequency of the CPU, since that affects the
frequency of the timer.)

Re: [Qemu-devel] [PATCH v1 3/3] armv7m_systick: abort instead of locking on a bad rate

[Philippe Mathieu-Daudé]

On 06/29/2017 09:43 AM, Peter Maydell wrote:

On 29 June 2017 at 13:35, Philippe Mathieu-Daudé  wrote:

This is true it is better to abort here than risking a deadlock.

However it seems to me they are 3 issues here:
- the deadlock pattern is caused by using a global variable,
- stellaris:ssys_calculate_system_clock() no checking RCC.SYSDIV and
RCC2.SYSDIV2 values <= 2 are reserved (GUEST_ERROR)
- stellaris:ssys_write(RCC2) not overrides correctly RCC


Stellaris works fine. It's other ARMv7M boards (which might forget
to set system_clock_scale) which don't work.


Oh I misread ssys_calculate_system_clock(), indeed system_clock_scale 
can not get below 5 so no deadlock on Stellaris.



I'd rather take this opportunity to fix the deadlock pattern using a
getter/setter on system_clock_scale, doing the zero check in the setter and
eventually aborting in the getter, what do you think?


We should be using a clock property on the CPU instead of system_clock_scale.
Unfortunately Konrad's series attempting to add that infrastructure
is still in the "trying to sort out the right API in code review"
stage. I don't think it's worth trying to fiddle with the API
for it before we have the right eventual infrastructure in place.


I see. I'd rather move the comment and assert() in systick_scale().


(What system_clock_scale is actually doing is setting the
emulated frequency of the CPU, since that affects the
frequency of the timer.)

Re: [Qemu-devel] [PATCH v1 3/3] armv7m_systick: abort instead of locking on a bad rate

[KONRAD Frederic]

On 06/29/2017 02:43 PM, Peter Maydell wrote:

On 29 June 2017 at 13:35, Philippe Mathieu-Daudé  wrote:

This is true it is better to abort here than risking a deadlock.

However it seems to me they are 3 issues here:
- the deadlock pattern is caused by using a global variable,
- stellaris:ssys_calculate_system_clock() no checking RCC.SYSDIV and
RCC2.SYSDIV2 values <= 2 are reserved (GUEST_ERROR)
- stellaris:ssys_write(RCC2) not overrides correctly RCC


Stellaris works fine. It's other ARMv7M boards (which might forget
to set system_clock_scale) which don't work.


Yes actually the issue is with new boards when you forgot to set
system_clock_scale (as it happened to me :)).

Peter you suggest we fix that later when the clock is in place?

Fred




I'd rather take this opportunity to fix the deadlock pattern using a
getter/setter on system_clock_scale, doing the zero check in the setter and
eventually aborting in the getter, what do you think?


We should be using a clock property on the CPU instead of system_clock_scale.
Unfortunately Konrad's series attempting to add that infrastructure
is still in the "trying to sort out the right API in code review"
stage. I don't think it's worth trying to fiddle with the API
for it before we have the right eventual infrastructure in place.

(What system_clock_scale is actually doing is setting the
emulated frequency of the CPU, since that affects the
frequency of the timer.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 3/3] armv7m_systick: abort instead of locking on a bad rate

[Peter Maydell]

On 29 June 2017 at 13:35, Philippe Mathieu-Daudé  wrote:
> This is true it is better to abort here than risking a deadlock.
>
> However it seems to me they are 3 issues here:
> - the deadlock pattern is caused by using a global variable,
> - stellaris:ssys_calculate_system_clock() no checking RCC.SYSDIV and
> RCC2.SYSDIV2 values <= 2 are reserved (GUEST_ERROR)
> - stellaris:ssys_write(RCC2) not overrides correctly RCC

Stellaris works fine. It's other ARMv7M boards (which might forget
to set system_clock_scale) which don't work.

> I'd rather take this opportunity to fix the deadlock pattern using a
> getter/setter on system_clock_scale, doing the zero check in the setter and
> eventually aborting in the getter, what do you think?

We should be using a clock property on the CPU instead of system_clock_scale.
Unfortunately Konrad's series attempting to add that infrastructure
is still in the "trying to sort out the right API in code review"
stage. I don't think it's worth trying to fiddle with the API
for it before we have the right eventual infrastructure in place.

(What system_clock_scale is actually doing is setting the
emulated frequency of the CPU, since that affects the
frequency of the timer.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 3/3] armv7m_systick: abort instead of locking on a bad rate

[Philippe Mathieu-Daudé]

Hi Frederic,

On 06/29/2017 06:28 AM, KONRAD Frederic wrote:

This helps the board developer by asserting that system_clock_rate is not
null. Using systick with a zero rate will lead to a deadlock so better showing
the error.

Signed-off-by: KONRAD Frederic 
---
  hw/timer/armv7m_systick.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/hw/timer/armv7m_systick.c b/hw/timer/armv7m_systick.c
index df8d280..745efb7 100644
--- a/hw/timer/armv7m_systick.c
+++ b/hw/timer/armv7m_systick.c
@@ -54,6 +54,9 @@ static void systick_reload(SysTickState *s, int reset)
  s->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
  }
  s->tick += (s->reload + 1) * systick_scale(s);
+
+/* system_clock_scale = 0 leads to a nasty deadlock, better aborting */
+assert(systick_scale(s));
  timer_mod(s->timer, s->tick);
  }


This is true it is better to abort here than risking a deadlock.

However it seems to me they are 3 issues here:
- the deadlock pattern is caused by using a global variable,
- stellaris:ssys_calculate_system_clock() no checking RCC.SYSDIV and 
RCC2.SYSDIV2 values <= 2 are reserved (GUEST_ERROR)

- stellaris:ssys_write(RCC2) not overrides correctly RCC

I'd rather take this opportunity to fix the deadlock pattern using a 
getter/setter on system_clock_scale, doing the zero check in the setter 
and eventually aborting in the getter, what do you think?


Regards,

Phil.