| Message ID | 1498728533-23160-4-git-send-email-frederic.konrad@adacore.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <frederic.konrad@adacore.com> > --- > 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.
On 29 June 2017 at 13:35, Philippe Mathieu-Daudé <f4bug@amsat.org> 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
On 06/29/2017 02:43 PM, Peter Maydell wrote: > On 29 June 2017 at 13:35, Philippe Mathieu-Daudé <f4bug@amsat.org> 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 >
On 06/29/2017 09:43 AM, Peter Maydell wrote: > On 29 June 2017 at 13:35, Philippe Mathieu-Daudé <f4bug@amsat.org> 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.)
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é >> <f4bug@amsat.org> 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.) >
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 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 <frederic.konrad@adacore.com> --- hw/timer/armv7m_systick.c | 3 +++ 1 file changed, 3 insertions(+)