Skip to content

Commit

Permalink
ptimer: Rename ptimer_init() to ptimer_init_with_bh()
Browse files Browse the repository at this point in the history
Currently the ptimer design uses a QEMU bottom-half as its
mechanism for calling back into the device model using the
ptimer when the timer has expired. Unfortunately this design
is fatally flawed, because it means that there is a lag
between the ptimer updating its own state and the device
callback function updating device state, and guest accesses
to device registers between the two can return inconsistent
device state.

We want to replace the bottom-half design with one where
the guest device's callback is called either immediately
(when the ptimer triggers by timeout) or when the device
model code closes a transaction-begin/end section (when the
ptimer triggers because the device model changed the
ptimer's count value or other state). As the first step,
rename ptimer_init() to ptimer_init_with_bh(), to free up
the ptimer_init() name for the new API. We can then convert
all the ptimer users away from ptimer_init_with_bh() before
removing it entirely.

(Commit created with
 git grep -l ptimer_init | xargs sed -i -e 's/ptimer_init/ptimer_init_with_bh/'
and three overlong lines folded by hand.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-id: 20191008171740.9679-2-peter.maydell@linaro.org
  • Loading branch information
pm215 committed Oct 15, 2019
1 parent fff9f55 commit b014226
Show file tree
Hide file tree
Showing 31 changed files with 56 additions and 54 deletions.
2 changes: 1 addition & 1 deletion hw/arm/musicpal.c
Expand Up @@ -849,7 +849,7 @@ static void mv88w8618_timer_init(SysBusDevice *dev, mv88w8618_timer_state *s,
s->freq = freq;

bh = qemu_bh_new(mv88w8618_timer_tick, s);
s->ptimer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
s->ptimer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
}

static uint64_t mv88w8618_pit_read(void *opaque, hwaddr offset,
Expand Down
2 changes: 1 addition & 1 deletion hw/core/ptimer.c
Expand Up @@ -358,7 +358,7 @@ const VMStateDescription vmstate_ptimer = {
}
};

ptimer_state *ptimer_init(QEMUBH *bh, uint8_t policy_mask)
ptimer_state *ptimer_init_with_bh(QEMUBH *bh, uint8_t policy_mask)
{
ptimer_state *s;

Expand Down
2 changes: 1 addition & 1 deletion hw/dma/xilinx_axidma.c
Expand Up @@ -552,7 +552,7 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)

st->nr = i;
st->bh = qemu_bh_new(timer_hit, st);
st->ptimer = ptimer_init(st->bh, PTIMER_POLICY_DEFAULT);
st->ptimer = ptimer_init_with_bh(st->bh, PTIMER_POLICY_DEFAULT);
ptimer_set_freq(st->ptimer, s->freqhz);
}
return;
Expand Down
2 changes: 1 addition & 1 deletion hw/m68k/mcf5206.c
Expand Up @@ -141,7 +141,7 @@ static m5206_timer_state *m5206_timer_init(qemu_irq irq)

s = g_new0(m5206_timer_state, 1);
bh = qemu_bh_new(m5206_timer_trigger, s);
s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
s->irq = irq;
m5206_timer_reset(s);
return s;
Expand Down
2 changes: 1 addition & 1 deletion hw/m68k/mcf5208.c
Expand Up @@ -192,7 +192,7 @@ static void mcf5208_sys_init(MemoryRegion *address_space, qemu_irq *pic)
for (i = 0; i < 2; i++) {
s = g_new0(m5208_timer_state, 1);
bh = qemu_bh_new(m5208_timer_trigger, s);
s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
memory_region_init_io(&s->iomem, NULL, &m5208_timer_ops, s,
"m5208-timer", 0x00004000);
memory_region_add_subregion(address_space, 0xfc080000 + 0x4000 * i,
Expand Down
2 changes: 1 addition & 1 deletion hw/net/fsl_etsec/etsec.c
Expand Up @@ -393,7 +393,7 @@ static void etsec_realize(DeviceState *dev, Error **errp)


etsec->bh = qemu_bh_new(etsec_timer_hit, etsec);
etsec->ptimer = ptimer_init(etsec->bh, PTIMER_POLICY_DEFAULT);
etsec->ptimer = ptimer_init_with_bh(etsec->bh, PTIMER_POLICY_DEFAULT);
ptimer_set_freq(etsec->ptimer, 100);
}

Expand Down
2 changes: 1 addition & 1 deletion hw/net/lan9118.c
Expand Up @@ -1350,7 +1350,7 @@ static void lan9118_realize(DeviceState *dev, Error **errp)
s->txp = &s->tx_packet;

bh = qemu_bh_new(lan9118_tick, s);
s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
ptimer_set_freq(s->timer, 10000);
ptimer_set_limit(s->timer, 0xffff, 1);
}
Expand Down
2 changes: 1 addition & 1 deletion hw/timer/allwinner-a10-pit.c
Expand Up @@ -271,7 +271,7 @@ static void a10_pit_init(Object *obj)
tc->container = s;
tc->index = i;
bh[i] = qemu_bh_new(a10_pit_timer_cb, tc);
s->timer[i] = ptimer_init(bh[i], PTIMER_POLICY_DEFAULT);
s->timer[i] = ptimer_init_with_bh(bh[i], PTIMER_POLICY_DEFAULT);
}
}

Expand Down
2 changes: 1 addition & 1 deletion hw/timer/altera_timer.c
Expand Up @@ -184,7 +184,7 @@ static void altera_timer_realize(DeviceState *dev, Error **errp)
}

t->bh = qemu_bh_new(timer_hit, t);
t->ptimer = ptimer_init(t->bh, PTIMER_POLICY_DEFAULT);
t->ptimer = ptimer_init_with_bh(t->bh, PTIMER_POLICY_DEFAULT);
ptimer_set_freq(t->ptimer, t->freq_hz);

memory_region_init_io(&t->mmio, OBJECT(t), &timer_ops, t,
Expand Down
6 changes: 3 additions & 3 deletions hw/timer/arm_mptimer.c
Expand Up @@ -228,7 +228,7 @@ static void arm_mptimer_reset(DeviceState *dev)
}
}

static void arm_mptimer_init(Object *obj)
static void arm_mptimer_init_with_bh(Object *obj)
{
ARMMPTimerState *s = ARM_MPTIMER(obj);

Expand Down Expand Up @@ -261,7 +261,7 @@ static void arm_mptimer_realize(DeviceState *dev, Error **errp)
for (i = 0; i < s->num_cpu; i++) {
TimerBlock *tb = &s->timerblock[i];
QEMUBH *bh = qemu_bh_new(timerblock_tick, tb);
tb->timer = ptimer_init(bh, PTIMER_POLICY);
tb->timer = ptimer_init_with_bh(bh, PTIMER_POLICY);
sysbus_init_irq(sbd, &tb->irq);
memory_region_init_io(&tb->iomem, OBJECT(s), &timerblock_ops, tb,
"arm_mptimer_timerblock", 0x20);
Expand Down Expand Up @@ -311,7 +311,7 @@ static const TypeInfo arm_mptimer_info = {
.name = TYPE_ARM_MPTIMER,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(ARMMPTimerState),
.instance_init = arm_mptimer_init,
.instance_init = arm_mptimer_init_with_bh,
.class_init = arm_mptimer_class_init,
};

Expand Down
2 changes: 1 addition & 1 deletion hw/timer/arm_timer.c
Expand Up @@ -173,7 +173,7 @@ static arm_timer_state *arm_timer_init(uint32_t freq)
s->control = TIMER_CTRL_IE;

bh = qemu_bh_new(arm_timer_tick, s);
s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
vmstate_register(NULL, -1, &vmstate_arm_timer, s);
return s;
}
Expand Down
2 changes: 1 addition & 1 deletion hw/timer/cmsdk-apb-dualtimer.c
Expand Up @@ -453,7 +453,7 @@ static void cmsdk_apb_dualtimer_realize(DeviceState *dev, Error **errp)
QEMUBH *bh = qemu_bh_new(cmsdk_dualtimermod_tick, m);

m->parent = s;
m->timer = ptimer_init(bh,
m->timer = ptimer_init_with_bh(bh,
PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD |
PTIMER_POLICY_TRIGGER_ONLY_ON_DECREMENT |
PTIMER_POLICY_NO_IMMEDIATE_RELOAD |
Expand Down
2 changes: 1 addition & 1 deletion hw/timer/cmsdk-apb-timer.c
Expand Up @@ -218,7 +218,7 @@ static void cmsdk_apb_timer_realize(DeviceState *dev, Error **errp)
}

bh = qemu_bh_new(cmsdk_apb_timer_tick, s);
s->timer = ptimer_init(bh,
s->timer = ptimer_init_with_bh(bh,
PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD |
PTIMER_POLICY_TRIGGER_ONLY_ON_DECREMENT |
PTIMER_POLICY_NO_IMMEDIATE_RELOAD |
Expand Down
2 changes: 1 addition & 1 deletion hw/timer/digic-timer.c
Expand Up @@ -129,7 +129,7 @@ static void digic_timer_init(Object *obj)
{
DigicTimerState *s = DIGIC_TIMER(obj);

s->ptimer = ptimer_init(NULL, PTIMER_POLICY_DEFAULT);
s->ptimer = ptimer_init_with_bh(NULL, PTIMER_POLICY_DEFAULT);

/*
* FIXME: there is no documentation on Digic timer
Expand Down
6 changes: 3 additions & 3 deletions hw/timer/etraxfs_timer.c
Expand Up @@ -328,9 +328,9 @@ static void etraxfs_timer_realize(DeviceState *dev, Error **errp)
t->bh_t0 = qemu_bh_new(timer0_hit, t);
t->bh_t1 = qemu_bh_new(timer1_hit, t);
t->bh_wd = qemu_bh_new(watchdog_hit, t);
t->ptimer_t0 = ptimer_init(t->bh_t0, PTIMER_POLICY_DEFAULT);
t->ptimer_t1 = ptimer_init(t->bh_t1, PTIMER_POLICY_DEFAULT);
t->ptimer_wd = ptimer_init(t->bh_wd, PTIMER_POLICY_DEFAULT);
t->ptimer_t0 = ptimer_init_with_bh(t->bh_t0, PTIMER_POLICY_DEFAULT);
t->ptimer_t1 = ptimer_init_with_bh(t->bh_t1, PTIMER_POLICY_DEFAULT);
t->ptimer_wd = ptimer_init_with_bh(t->bh_wd, PTIMER_POLICY_DEFAULT);

sysbus_init_irq(sbd, &t->irq);
sysbus_init_irq(sbd, &t->nmi);
Expand Down
7 changes: 4 additions & 3 deletions hw/timer/exynos4210_mct.c
Expand Up @@ -1429,16 +1429,17 @@ static void exynos4210_mct_init(Object *obj)

/* Global timer */
bh[0] = qemu_bh_new(exynos4210_gfrc_event, s);
s->g_timer.ptimer_frc = ptimer_init(bh[0], PTIMER_POLICY_DEFAULT);
s->g_timer.ptimer_frc = ptimer_init_with_bh(bh[0], PTIMER_POLICY_DEFAULT);
memset(&s->g_timer.reg, 0, sizeof(struct gregs));

/* Local timers */
for (i = 0; i < 2; i++) {
bh[0] = qemu_bh_new(exynos4210_ltick_event, &s->l_timer[i]);
bh[1] = qemu_bh_new(exynos4210_lfrc_event, &s->l_timer[i]);
s->l_timer[i].tick_timer.ptimer_tick =
ptimer_init(bh[0], PTIMER_POLICY_DEFAULT);
s->l_timer[i].ptimer_frc = ptimer_init(bh[1], PTIMER_POLICY_DEFAULT);
ptimer_init_with_bh(bh[0], PTIMER_POLICY_DEFAULT);
s->l_timer[i].ptimer_frc =
ptimer_init_with_bh(bh[1], PTIMER_POLICY_DEFAULT);
s->l_timer[i].id = i;
}

Expand Down
2 changes: 1 addition & 1 deletion hw/timer/exynos4210_pwm.c
Expand Up @@ -393,7 +393,7 @@ static void exynos4210_pwm_init(Object *obj)
for (i = 0; i < EXYNOS4210_PWM_TIMERS_NUM; i++) {
bh = qemu_bh_new(exynos4210_pwm_tick, &s->timer[i]);
sysbus_init_irq(dev, &s->timer[i].irq);
s->timer[i].ptimer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
s->timer[i].ptimer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
s->timer[i].id = i;
s->timer[i].parent = s;
}
Expand Down
4 changes: 2 additions & 2 deletions hw/timer/exynos4210_rtc.c
Expand Up @@ -558,12 +558,12 @@ static void exynos4210_rtc_init(Object *obj)
QEMUBH *bh;

bh = qemu_bh_new(exynos4210_rtc_tick, s);
s->ptimer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
s->ptimer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
ptimer_set_freq(s->ptimer, RTC_BASE_FREQ);
exynos4210_rtc_update_freq(s, 0);

bh = qemu_bh_new(exynos4210_rtc_1Hz_tick, s);
s->ptimer_1Hz = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
s->ptimer_1Hz = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
ptimer_set_freq(s->ptimer_1Hz, RTC_BASE_FREQ);

sysbus_init_irq(dev, &s->alm_irq);
Expand Down
2 changes: 1 addition & 1 deletion hw/timer/grlib_gptimer.c
Expand Up @@ -366,7 +366,7 @@ static void grlib_gptimer_realize(DeviceState *dev, Error **errp)

timer->unit = unit;
timer->bh = qemu_bh_new(grlib_gptimer_hit, timer);
timer->ptimer = ptimer_init(timer->bh, PTIMER_POLICY_DEFAULT);
timer->ptimer = ptimer_init_with_bh(timer->bh, PTIMER_POLICY_DEFAULT);
timer->id = i;

/* One IRQ line for each timer */
Expand Down
4 changes: 2 additions & 2 deletions hw/timer/imx_epit.c
Expand Up @@ -317,10 +317,10 @@ static void imx_epit_realize(DeviceState *dev, Error **errp)
0x00001000);
sysbus_init_mmio(sbd, &s->iomem);

s->timer_reload = ptimer_init(NULL, PTIMER_POLICY_DEFAULT);
s->timer_reload = ptimer_init_with_bh(NULL, PTIMER_POLICY_DEFAULT);

bh = qemu_bh_new(imx_epit_cmp, s);
s->timer_cmp = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
s->timer_cmp = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
}

static void imx_epit_class_init(ObjectClass *klass, void *data)
Expand Down
2 changes: 1 addition & 1 deletion hw/timer/imx_gpt.c
Expand Up @@ -492,7 +492,7 @@ static void imx_gpt_realize(DeviceState *dev, Error **errp)
sysbus_init_mmio(sbd, &s->iomem);

bh = qemu_bh_new(imx_gpt_timeout, s);
s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
}

static void imx_gpt_class_init(ObjectClass *klass, void *data)
Expand Down
2 changes: 1 addition & 1 deletion hw/timer/lm32_timer.c
Expand Up @@ -196,7 +196,7 @@ static void lm32_timer_realize(DeviceState *dev, Error **errp)
LM32TimerState *s = LM32_TIMER(dev);

s->bh = qemu_bh_new(timer_hit, s);
s->ptimer = ptimer_init(s->bh, PTIMER_POLICY_DEFAULT);
s->ptimer = ptimer_init_with_bh(s->bh, PTIMER_POLICY_DEFAULT);

ptimer_set_freq(s->ptimer, s->freq_hz);
}
Expand Down
4 changes: 2 additions & 2 deletions hw/timer/milkymist-sysctl.c
Expand Up @@ -294,8 +294,8 @@ static void milkymist_sysctl_realize(DeviceState *dev, Error **errp)

s->bh0 = qemu_bh_new(timer0_hit, s);
s->bh1 = qemu_bh_new(timer1_hit, s);
s->ptimer0 = ptimer_init(s->bh0, PTIMER_POLICY_DEFAULT);
s->ptimer1 = ptimer_init(s->bh1, PTIMER_POLICY_DEFAULT);
s->ptimer0 = ptimer_init_with_bh(s->bh0, PTIMER_POLICY_DEFAULT);
s->ptimer1 = ptimer_init_with_bh(s->bh1, PTIMER_POLICY_DEFAULT);

ptimer_set_freq(s->ptimer0, s->freq_hz);
ptimer_set_freq(s->ptimer1, s->freq_hz);
Expand Down
2 changes: 1 addition & 1 deletion hw/timer/mss-timer.c
Expand Up @@ -229,7 +229,7 @@ static void mss_timer_init(Object *obj)
struct Msf2Timer *st = &t->timers[i];

st->bh = qemu_bh_new(timer_hit, st);
st->ptimer = ptimer_init(st->bh, PTIMER_POLICY_DEFAULT);
st->ptimer = ptimer_init_with_bh(st->bh, PTIMER_POLICY_DEFAULT);
ptimer_set_freq(st->ptimer, t->freq_hz);
sysbus_init_irq(SYS_BUS_DEVICE(obj), &st->irq);
}
Expand Down
2 changes: 1 addition & 1 deletion hw/timer/puv3_ost.c
Expand Up @@ -129,7 +129,7 @@ static void puv3_ost_realize(DeviceState *dev, Error **errp)
sysbus_init_irq(sbd, &s->irq);

s->bh = qemu_bh_new(puv3_ost_tick, s);
s->ptimer = ptimer_init(s->bh, PTIMER_POLICY_DEFAULT);
s->ptimer = ptimer_init_with_bh(s->bh, PTIMER_POLICY_DEFAULT);
ptimer_set_freq(s->ptimer, 50 * 1000 * 1000);

memory_region_init_io(&s->iomem, OBJECT(s), &puv3_ost_ops, s, "puv3_ost",
Expand Down
2 changes: 1 addition & 1 deletion hw/timer/sh_timer.c
Expand Up @@ -204,7 +204,7 @@ static void *sh_timer_init(uint32_t freq, int feat, qemu_irq irq)
s->irq = irq;

bh = qemu_bh_new(sh_timer_tick, s);
s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);

sh_timer_write(s, OFFSET_TCOR >> 2, s->tcor);
sh_timer_write(s, OFFSET_TCNT >> 2, s->tcnt);
Expand Down
2 changes: 1 addition & 1 deletion hw/timer/slavio_timer.c
Expand Up @@ -393,7 +393,7 @@ static void slavio_timer_init(Object *obj)
tc->timer_index = i;

bh = qemu_bh_new(slavio_timer_irq, tc);
s->cputimer[i].timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
s->cputimer[i].timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
ptimer_set_period(s->cputimer[i].timer, TIMER_PERIOD);

size = i == 0 ? SYS_TIMER_SIZE : CPU_TIMER_SIZE;
Expand Down
2 changes: 1 addition & 1 deletion hw/timer/xilinx_timer.c
Expand Up @@ -221,7 +221,7 @@ static void xilinx_timer_realize(DeviceState *dev, Error **errp)
xt->parent = t;
xt->nr = i;
xt->bh = qemu_bh_new(timer_hit, xt);
xt->ptimer = ptimer_init(xt->bh, PTIMER_POLICY_DEFAULT);
xt->ptimer = ptimer_init_with_bh(xt->bh, PTIMER_POLICY_DEFAULT);
ptimer_set_freq(xt->ptimer, t->freq_hz);
}

Expand Down
2 changes: 1 addition & 1 deletion hw/watchdog/cmsdk-apb-watchdog.c
Expand Up @@ -329,7 +329,7 @@ static void cmsdk_apb_watchdog_realize(DeviceState *dev, Error **errp)
}

bh = qemu_bh_new(cmsdk_apb_watchdog_tick, s);
s->timer = ptimer_init(bh,
s->timer = ptimer_init_with_bh(bh,
PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD |
PTIMER_POLICY_TRIGGER_ONLY_ON_DECREMENT |
PTIMER_POLICY_NO_IMMEDIATE_RELOAD |
Expand Down
11 changes: 6 additions & 5 deletions include/hw/ptimer.h
Expand Up @@ -72,7 +72,7 @@
* ptimer_set_count() or ptimer_set_limit() will not trigger the timer
* (though it will cause a reload). Only a counter decrement to "0"
* will cause a trigger. Not compatible with NO_IMMEDIATE_TRIGGER;
* ptimer_init() will assert() that you don't set both.
* ptimer_init_with_bh() will assert() that you don't set both.
*/
#define PTIMER_POLICY_TRIGGER_ONLY_ON_DECREMENT (1 << 5)

Expand All @@ -81,21 +81,21 @@ typedef struct ptimer_state ptimer_state;
typedef void (*ptimer_cb)(void *opaque);

/**
* ptimer_init - Allocate and return a new ptimer
* ptimer_init_with_bh - Allocate and return a new ptimer
* @bh: QEMU bottom half which is run on timer expiry
* @policy: PTIMER_POLICY_* bits specifying behaviour
*
* The ptimer returned must be freed using ptimer_free().
* The ptimer takes ownership of @bh and will delete it
* when the ptimer is eventually freed.
*/
ptimer_state *ptimer_init(QEMUBH *bh, uint8_t policy_mask);
ptimer_state *ptimer_init_with_bh(QEMUBH *bh, uint8_t policy_mask);

/**
* ptimer_free - Free a ptimer
* @s: timer to free
*
* Free a ptimer created using ptimer_init() (including
* Free a ptimer created using ptimer_init_with_bh() (including
* deleting the bottom half which it is using).
*/
void ptimer_free(ptimer_state *s);
Expand Down Expand Up @@ -178,7 +178,8 @@ void ptimer_set_count(ptimer_state *s, uint64_t count);
* @oneshot: non-zero if this timer should only count down once
*
* Start a ptimer counting down; when it reaches zero the bottom half
* passed to ptimer_init() will be invoked. If the @oneshot argument is zero,
* passed to ptimer_init_with_bh() will be invoked.
* If the @oneshot argument is zero,
* the counter value will then be reloaded from the limit and it will
* start counting down again. If @oneshot is non-zero, then the counter
* will disable itself when it reaches zero.
Expand Down

0 comments on commit b014226

Please sign in to comment.