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 <address@hidden>
> ---
>   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.