| Message ID | 20191017132351.4762-3-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Convert sparc devices to new ptimer API | expand |
On 10/17/19 6:23 AM, Peter Maydell wrote: > Switch the slavio_timer code away from bottom-half based ptimers to > the new transaction-based ptimer API. This just requires adding > begin/commit calls around the various places that modify the ptimer > state, and using the new ptimer_init() function to create the timer. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/timer/slavio_timer.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Hi Peter, On 10/17/19 3:23 PM, Peter Maydell wrote: > Switch the slavio_timer code away from bottom-half based ptimers to > the new transaction-based ptimer API. This just requires adding > begin/commit calls around the various places that modify the ptimer > state, and using the new ptimer_init() function to create the timer. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/timer/slavio_timer.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/hw/timer/slavio_timer.c b/hw/timer/slavio_timer.c > index 692d213897d..0e2efe6fe89 100644 > --- a/hw/timer/slavio_timer.c > +++ b/hw/timer/slavio_timer.c > @@ -30,7 +30,6 @@ > #include "hw/sysbus.h" > #include "migration/vmstate.h" > #include "trace.h" > -#include "qemu/main-loop.h" > #include "qemu/module.h" > > /* > @@ -213,6 +212,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, > saddr = addr >> 2; > switch (saddr) { > case TIMER_LIMIT: > + ptimer_transaction_begin(t->timer); > if (slavio_timer_is_user(tc)) { > uint64_t count; This part is odd since there is a check on t->timer != NULL later, and ptimer_transaction_commit() can't take NULL. This won't happen if you restrict to ptimer_* calls: -- >8 -- @@ -222,18 +222,22 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, t->reached = 0; count = ((uint64_t)t->counthigh << 32) | t->count; trace_slavio_timer_mem_writel_limit(timer_index, count); + ptimer_transaction_begin(t->timer); ptimer_set_count(t->timer, LIMIT_TO_PERIODS(t->limit - count)); + ptimer_transaction_commit(t->timer); } else { // set limit, reset counter qemu_irq_lower(t->irq); t->limit = val & TIMER_MAX_COUNT32; if (t->timer) { + ptimer_transaction_begin(t->timer); if (t->limit == 0) { /* free-run */ ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(TIMER_MAX_COUNT32), 1); } else { ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(t->limit), 1); } + ptimer_transaction_commit(t->timer); } } break; --- > > @@ -236,6 +236,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, > } > } > } > + ptimer_transaction_commit(t->timer); > break; > case TIMER_COUNTER: > if (slavio_timer_is_user(tc)) { > @@ -247,7 +248,9 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, > t->reached = 0; > count = ((uint64_t)t->counthigh) << 32 | t->count; > trace_slavio_timer_mem_writel_limit(timer_index, count); > + ptimer_transaction_begin(t->timer); > ptimer_set_count(t->timer, LIMIT_TO_PERIODS(t->limit - count)); > + ptimer_transaction_commit(t->timer); > } else { > trace_slavio_timer_mem_writel_counter_invalid(); > } > @@ -255,13 +258,16 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, > case TIMER_COUNTER_NORST: > // set limit without resetting counter > t->limit = val & TIMER_MAX_COUNT32; > + ptimer_transaction_begin(t->timer); > if (t->limit == 0) { /* free-run */ > ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(TIMER_MAX_COUNT32), 0); > } else { > ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(t->limit), 0); > } > + ptimer_transaction_commit(t->timer); > break; > case TIMER_STATUS: > + ptimer_transaction_begin(t->timer); > if (slavio_timer_is_user(tc)) { I'd move the begin() here. > // start/stop user counter > if (val & 1) { > @@ -273,6 +279,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, > } > } > t->run = val & 1; > + ptimer_transaction_commit(t->timer); > break; > case TIMER_MODE: > if (timer_index == 0) { > @@ -282,6 +289,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, > unsigned int processor = 1 << i; > CPUTimerState *curr_timer = &s->cputimer[i + 1]; > > + ptimer_transaction_begin(curr_timer->timer); > // check for a change in timer mode for this processor > if ((val & processor) != (s->cputimer_mode & processor)) { Ditto, begin() here. > if (val & processor) { // counter -> user timer > @@ -308,6 +316,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, > trace_slavio_timer_mem_writel_mode_counter(timer_index); > } > } > + ptimer_transaction_commit(curr_timer->timer); > } > } else { > trace_slavio_timer_mem_writel_mode_invalid(); > @@ -367,10 +376,12 @@ static void slavio_timer_reset(DeviceState *d) > curr_timer->count = 0; > curr_timer->reached = 0; > if (i <= s->num_cpus) { > + ptimer_transaction_begin(curr_timer->timer); > ptimer_set_limit(curr_timer->timer, > LIMIT_TO_PERIODS(TIMER_MAX_COUNT32), 1); > ptimer_run(curr_timer->timer, 0); > curr_timer->run = 1; > + ptimer_transaction_commit(curr_timer->timer); > } > } > s->cputimer_mode = 0; > @@ -380,7 +391,6 @@ static void slavio_timer_init(Object *obj) > { > SLAVIO_TIMERState *s = SLAVIO_TIMER(obj); > SysBusDevice *dev = SYS_BUS_DEVICE(obj); > - QEMUBH *bh; > unsigned int i; > TimerContext *tc; > > @@ -392,9 +402,11 @@ static void slavio_timer_init(Object *obj) > tc->s = s; > tc->timer_index = i; > > - bh = qemu_bh_new(slavio_timer_irq, tc); > - s->cputimer[i].timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT); > + s->cputimer[i].timer = ptimer_init(slavio_timer_irq, tc, > + PTIMER_POLICY_DEFAULT); > + ptimer_transaction_begin(s->cputimer[i].timer); > ptimer_set_period(s->cputimer[i].timer, TIMER_PERIOD); > + ptimer_transaction_commit(s->cputimer[i].timer); > > size = i == 0 ? SYS_TIMER_SIZE : CPU_TIMER_SIZE; > snprintf(timer_name, sizeof(timer_name), "timer-%i", i); >
On Thu, 17 Oct 2019 at 15:54, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > Hi Peter, > > On 10/17/19 3:23 PM, Peter Maydell wrote: > > Switch the slavio_timer code away from bottom-half based ptimers to > > the new transaction-based ptimer API. This just requires adding > > begin/commit calls around the various places that modify the ptimer > > state, and using the new ptimer_init() function to create the timer. > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > hw/timer/slavio_timer.c | 20 ++++++++++++++++---- > > 1 file changed, 16 insertions(+), 4 deletions(-) > > > > diff --git a/hw/timer/slavio_timer.c b/hw/timer/slavio_timer.c > > index 692d213897d..0e2efe6fe89 100644 > > --- a/hw/timer/slavio_timer.c > > +++ b/hw/timer/slavio_timer.c > > @@ -30,7 +30,6 @@ > > #include "hw/sysbus.h" > > #include "migration/vmstate.h" > > #include "trace.h" > > -#include "qemu/main-loop.h" > > #include "qemu/module.h" > > > > /* > > @@ -213,6 +212,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, > > saddr = addr >> 2; > > switch (saddr) { > > case TIMER_LIMIT: > > + ptimer_transaction_begin(t->timer); > > if (slavio_timer_is_user(tc)) { > > uint64_t count; > > > This part is odd since there is a check on t->timer != NULL later, and > ptimer_transaction_commit() can't take NULL. Hmm, I hadn't noticed that. I think the bug is the check for NULL, though, beacuse the slavio_timer_init() function always initializes all the timer pointers, so there's no situation where the pointer can be non-NULL as far as I can see. So I think I'd rather fix this by removing the NULL pointer check... > > @@ -255,13 +258,16 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, > > case TIMER_COUNTER_NORST: > > // set limit without resetting counter > > t->limit = val & TIMER_MAX_COUNT32; > > + ptimer_transaction_begin(t->timer); > > if (t->limit == 0) { /* free-run */ > > ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(TIMER_MAX_COUNT32), 0); > > } else { > > ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(t->limit), 0); > > } > > + ptimer_transaction_commit(t->timer); > > break; > > case TIMER_STATUS: > > + ptimer_transaction_begin(t->timer); > > if (slavio_timer_is_user(tc)) { > > I'd move the begin() here. This would be awkward because then it won't neatly nest with the commit call unless you add an extra if() for the commit or otherwise rearrange/duplicate code... > > // start/stop user counter > > if (val & 1) { > > @@ -273,6 +279,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, > > } > > } > > t->run = val & 1; > > + ptimer_transaction_commit(t->timer); ...because the commit should come after we have finished updating the timer state (t->run in this case), because the trigger callback slavio_timer_irq() otherwise sees inconsistent half-updated state if commit() calls it. thanks -- PMM
On 10/17/19 5:00 PM, Peter Maydell wrote: > On Thu, 17 Oct 2019 at 15:54, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> Hi Peter, >> >> On 10/17/19 3:23 PM, Peter Maydell wrote: >>> Switch the slavio_timer code away from bottom-half based ptimers to >>> the new transaction-based ptimer API. This just requires adding >>> begin/commit calls around the various places that modify the ptimer >>> state, and using the new ptimer_init() function to create the timer. >>> >>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >>> --- >>> hw/timer/slavio_timer.c | 20 ++++++++++++++++---- >>> 1 file changed, 16 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/timer/slavio_timer.c b/hw/timer/slavio_timer.c >>> index 692d213897d..0e2efe6fe89 100644 >>> --- a/hw/timer/slavio_timer.c >>> +++ b/hw/timer/slavio_timer.c >>> @@ -30,7 +30,6 @@ >>> #include "hw/sysbus.h" >>> #include "migration/vmstate.h" >>> #include "trace.h" >>> -#include "qemu/main-loop.h" >>> #include "qemu/module.h" >>> >>> /* >>> @@ -213,6 +212,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, >>> saddr = addr >> 2; >>> switch (saddr) { >>> case TIMER_LIMIT: >>> + ptimer_transaction_begin(t->timer); >>> if (slavio_timer_is_user(tc)) { >>> uint64_t count; >> >> >> This part is odd since there is a check on t->timer != NULL later, and >> ptimer_transaction_commit() can't take NULL. > > Hmm, I hadn't noticed that. I think the bug is the check > for NULL, though, beacuse the slavio_timer_init() function > always initializes all the timer pointers, so there's > no situation where the pointer can be non-NULL as far > as I can see. So I think I'd rather fix this by removing > the NULL pointer check... Yes, you are correct. >>> @@ -255,13 +258,16 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, >>> case TIMER_COUNTER_NORST: >>> // set limit without resetting counter >>> t->limit = val & TIMER_MAX_COUNT32; >>> + ptimer_transaction_begin(t->timer); >>> if (t->limit == 0) { /* free-run */ >>> ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(TIMER_MAX_COUNT32), 0); >>> } else { >>> ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(t->limit), 0); >>> } >>> + ptimer_transaction_commit(t->timer); >>> break; >>> case TIMER_STATUS: >>> + ptimer_transaction_begin(t->timer); >>> if (slavio_timer_is_user(tc)) { >> >> I'd move the begin() here. > > This would be awkward because then it won't neatly nest with > the commit call unless you add an extra if() for the > commit or otherwise rearrange/duplicate code... > >>> // start/stop user counter >>> if (val & 1) { >>> @@ -273,6 +279,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, >>> } >>> } >>> t->run = val & 1; >>> + ptimer_transaction_commit(t->timer); > > ...because the commit should come after we have finished > updating the timer state (t->run in this case), because > the trigger callback slavio_timer_irq() otherwise sees > inconsistent half-updated state if commit() calls it. Oh, slavio_timer_irq() calls slavio_timer_get_out() which accesses the ptimer... OK I missed that. Whew we need to be extra cautious with this API... If possible I'd rather see the patch removing the NULL check previous to this one, anyway: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Thanks, Phil.
On Thu, 17 Oct 2019 at 16:22, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 10/17/19 5:00 PM, Peter Maydell wrote: > > ...because the commit should come after we have finished > > updating the timer state (t->run in this case), because > > the trigger callback slavio_timer_irq() otherwise sees > > inconsistent half-updated state if commit() calls it. > > Oh, slavio_timer_irq() calls slavio_timer_get_out() which accesses the > ptimer... OK I missed that. > > Whew we need to be extra cautious with this API... Yes. If the callback function is a trivial "just update the interrupt register bit and set an irq line" one, like about half the ptimer users, then it doesn't matter too much where the commit call goes, but for those users who do more complicated work in the timer callback it gets a little trickier (and I didn't realise this wrinkle until about halfway through doing the API conversion work). It doesn't much matter where the begin call goes (an odd asymmetry), but the commit call is effectively a "voluntarily yield control to the callback function" and so its placement can be important. > If possible I'd rather see the patch removing the NULL check previous to > this one, anyway: > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Thanks; I'll add the NULL-check cleanup in v2. Coverity will probably complain otherwise. thanks -- PMM
diff --git a/hw/timer/slavio_timer.c b/hw/timer/slavio_timer.c index 692d213897d..0e2efe6fe89 100644 --- a/hw/timer/slavio_timer.c +++ b/hw/timer/slavio_timer.c @@ -30,7 +30,6 @@ #include "hw/sysbus.h" #include "migration/vmstate.h" #include "trace.h" -#include "qemu/main-loop.h" #include "qemu/module.h" /* @@ -213,6 +212,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, saddr = addr >> 2; switch (saddr) { case TIMER_LIMIT: + ptimer_transaction_begin(t->timer); if (slavio_timer_is_user(tc)) { uint64_t count; @@ -236,6 +236,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, } } } + ptimer_transaction_commit(t->timer); break; case TIMER_COUNTER: if (slavio_timer_is_user(tc)) { @@ -247,7 +248,9 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, t->reached = 0; count = ((uint64_t)t->counthigh) << 32 | t->count; trace_slavio_timer_mem_writel_limit(timer_index, count); + ptimer_transaction_begin(t->timer); ptimer_set_count(t->timer, LIMIT_TO_PERIODS(t->limit - count)); + ptimer_transaction_commit(t->timer); } else { trace_slavio_timer_mem_writel_counter_invalid(); } @@ -255,13 +258,16 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, case TIMER_COUNTER_NORST: // set limit without resetting counter t->limit = val & TIMER_MAX_COUNT32; + ptimer_transaction_begin(t->timer); if (t->limit == 0) { /* free-run */ ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(TIMER_MAX_COUNT32), 0); } else { ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(t->limit), 0); } + ptimer_transaction_commit(t->timer); break; case TIMER_STATUS: + ptimer_transaction_begin(t->timer); if (slavio_timer_is_user(tc)) { // start/stop user counter if (val & 1) { @@ -273,6 +279,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, } } t->run = val & 1; + ptimer_transaction_commit(t->timer); break; case TIMER_MODE: if (timer_index == 0) { @@ -282,6 +289,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, unsigned int processor = 1 << i; CPUTimerState *curr_timer = &s->cputimer[i + 1]; + ptimer_transaction_begin(curr_timer->timer); // check for a change in timer mode for this processor if ((val & processor) != (s->cputimer_mode & processor)) { if (val & processor) { // counter -> user timer @@ -308,6 +316,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr, trace_slavio_timer_mem_writel_mode_counter(timer_index); } } + ptimer_transaction_commit(curr_timer->timer); } } else { trace_slavio_timer_mem_writel_mode_invalid(); @@ -367,10 +376,12 @@ static void slavio_timer_reset(DeviceState *d) curr_timer->count = 0; curr_timer->reached = 0; if (i <= s->num_cpus) { + ptimer_transaction_begin(curr_timer->timer); ptimer_set_limit(curr_timer->timer, LIMIT_TO_PERIODS(TIMER_MAX_COUNT32), 1); ptimer_run(curr_timer->timer, 0); curr_timer->run = 1; + ptimer_transaction_commit(curr_timer->timer); } } s->cputimer_mode = 0; @@ -380,7 +391,6 @@ static void slavio_timer_init(Object *obj) { SLAVIO_TIMERState *s = SLAVIO_TIMER(obj); SysBusDevice *dev = SYS_BUS_DEVICE(obj); - QEMUBH *bh; unsigned int i; TimerContext *tc; @@ -392,9 +402,11 @@ static void slavio_timer_init(Object *obj) tc->s = s; tc->timer_index = i; - bh = qemu_bh_new(slavio_timer_irq, tc); - s->cputimer[i].timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT); + s->cputimer[i].timer = ptimer_init(slavio_timer_irq, tc, + PTIMER_POLICY_DEFAULT); + ptimer_transaction_begin(s->cputimer[i].timer); ptimer_set_period(s->cputimer[i].timer, TIMER_PERIOD); + ptimer_transaction_commit(s->cputimer[i].timer); size = i == 0 ? SYS_TIMER_SIZE : CPU_TIMER_SIZE; snprintf(timer_name, sizeof(timer_name), "timer-%i", i);
Switch the slavio_timer code away from bottom-half based ptimers to the new transaction-based ptimer API. This just requires adding begin/commit calls around the various places that modify the ptimer state, and using the new ptimer_init() function to create the timer. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/timer/slavio_timer.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) -- 2.20.1