* [PATCH v2 1/3] hw/timer/slavio_timer: Remove useless check for NULL t->timer
2019-10-21 13:43 [PATCH v2 0/3] Convert sparc devices to new ptimer API Peter Maydell
@ 2019-10-21 13:43 ` Peter Maydell
2019-10-21 14:03 ` Philippe Mathieu-Daudé
2019-10-21 16:24 ` Richard Henderson
2019-10-21 13:43 ` [PATCH v2 2/3] hw/timer/grlib_gptimer.c: Switch to transaction-based ptimer API Peter Maydell
` (3 subsequent siblings)
4 siblings, 2 replies; 11+ messages in thread
From: Peter Maydell @ 2019-10-21 13:43 UTC (permalink / raw)
To: qemu-devel
Cc: Philippe Mathieu-Daudé, KONRAD Frederic, Richard Henderson,
Mark Cave-Ayland, Fabien Chouteau
In the slavio timer devcie, the ptimer TimerContext::timer is
always created by slavio_timer_init(), so there's no need to
check it for NULL; remove the single unneeded NULL check.
This will be useful to avoid compiler/Coverity errors when
a subsequent change adds a use of t->timer before the location
we currently do the NULL check.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
hw/timer/slavio_timer.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/hw/timer/slavio_timer.c b/hw/timer/slavio_timer.c
index 692d213897d..890dd53f8d8 100644
--- a/hw/timer/slavio_timer.c
+++ b/hw/timer/slavio_timer.c
@@ -227,13 +227,11 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
// set limit, reset counter
qemu_irq_lower(t->irq);
t->limit = val & TIMER_MAX_COUNT32;
- if (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);
- }
+ 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);
}
}
break;
--
2.20.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] hw/timer/slavio_timer: Remove useless check for NULL t->timer
2019-10-21 13:43 ` [PATCH v2 1/3] hw/timer/slavio_timer: Remove useless check for NULL t->timer Peter Maydell
@ 2019-10-21 14:03 ` Philippe Mathieu-Daudé
2019-10-21 16:24 ` Richard Henderson
1 sibling, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-21 14:03 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Richard Henderson, KONRAD Frederic, Mark Cave-Ayland,
Fabien Chouteau
On 10/21/19 3:43 PM, Peter Maydell wrote:
> In the slavio timer devcie, the ptimer TimerContext::timer is
typo "device"
> always created by slavio_timer_init(), so there's no need to
> check it for NULL; remove the single unneeded NULL check.
>
> This will be useful to avoid compiler/Coverity errors when
> a subsequent change adds a use of t->timer before the location
> we currently do the NULL check.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> hw/timer/slavio_timer.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/hw/timer/slavio_timer.c b/hw/timer/slavio_timer.c
> index 692d213897d..890dd53f8d8 100644
> --- a/hw/timer/slavio_timer.c
> +++ b/hw/timer/slavio_timer.c
> @@ -227,13 +227,11 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
> // set limit, reset counter
> qemu_irq_lower(t->irq);
> t->limit = val & TIMER_MAX_COUNT32;
> - if (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);
> - }
> + 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);
> }
> }
> break;
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] hw/timer/slavio_timer: Remove useless check for NULL t->timer
2019-10-21 13:43 ` [PATCH v2 1/3] hw/timer/slavio_timer: Remove useless check for NULL t->timer Peter Maydell
2019-10-21 14:03 ` Philippe Mathieu-Daudé
@ 2019-10-21 16:24 ` Richard Henderson
1 sibling, 0 replies; 11+ messages in thread
From: Richard Henderson @ 2019-10-21 16:24 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Philippe Mathieu-Daudé, KONRAD Frederic, Mark Cave-Ayland,
Fabien Chouteau
On 10/21/19 6:43 AM, Peter Maydell wrote:
> In the slavio timer devcie, the ptimer TimerContext::timer is
> always created by slavio_timer_init(), so there's no need to
> check it for NULL; remove the single unneeded NULL check.
>
> This will be useful to avoid compiler/Coverity errors when
> a subsequent change adds a use of t->timer before the location
> we currently do the NULL check.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> hw/timer/slavio_timer.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] hw/timer/grlib_gptimer.c: Switch to transaction-based ptimer API
2019-10-21 13:43 [PATCH v2 0/3] Convert sparc devices to new ptimer API Peter Maydell
2019-10-21 13:43 ` [PATCH v2 1/3] hw/timer/slavio_timer: Remove useless check for NULL t->timer Peter Maydell
@ 2019-10-21 13:43 ` Peter Maydell
2019-10-21 13:43 ` [PATCH v2 3/3] hw/timer/slavio_timer.c: " Peter Maydell
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Peter Maydell @ 2019-10-21 13:43 UTC (permalink / raw)
To: qemu-devel
Cc: Philippe Mathieu-Daudé, KONRAD Frederic, Richard Henderson,
Mark Cave-Ayland, Fabien Chouteau
Switch the grlib_gptimer 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
hw/timer/grlib_gptimer.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/hw/timer/grlib_gptimer.c b/hw/timer/grlib_gptimer.c
index bb09268ea14..7a9371c0e30 100644
--- a/hw/timer/grlib_gptimer.c
+++ b/hw/timer/grlib_gptimer.c
@@ -29,7 +29,6 @@
#include "hw/irq.h"
#include "hw/ptimer.h"
#include "hw/qdev-properties.h"
-#include "qemu/main-loop.h"
#include "qemu/module.h"
#include "trace.h"
@@ -63,7 +62,6 @@ typedef struct GPTimer GPTimer;
typedef struct GPTimerUnit GPTimerUnit;
struct GPTimer {
- QEMUBH *bh;
struct ptimer_state *ptimer;
qemu_irq irq;
@@ -93,6 +91,17 @@ struct GPTimerUnit {
uint32_t config;
};
+static void grlib_gptimer_tx_begin(GPTimer *timer)
+{
+ ptimer_transaction_begin(timer->ptimer);
+}
+
+static void grlib_gptimer_tx_commit(GPTimer *timer)
+{
+ ptimer_transaction_commit(timer->ptimer);
+}
+
+/* Must be called within grlib_gptimer_tx_begin/commit block */
static void grlib_gptimer_enable(GPTimer *timer)
{
assert(timer != NULL);
@@ -115,6 +124,7 @@ static void grlib_gptimer_enable(GPTimer *timer)
ptimer_run(timer->ptimer, 1);
}
+/* Must be called within grlib_gptimer_tx_begin/commit block */
static void grlib_gptimer_restart(GPTimer *timer)
{
assert(timer != NULL);
@@ -141,7 +151,9 @@ static void grlib_gptimer_set_scaler(GPTimerUnit *unit, uint32_t scaler)
trace_grlib_gptimer_set_scaler(scaler, value);
for (i = 0; i < unit->nr_timers; i++) {
+ ptimer_transaction_begin(unit->timers[i].ptimer);
ptimer_set_freq(unit->timers[i].ptimer, value);
+ ptimer_transaction_commit(unit->timers[i].ptimer);
}
}
@@ -266,8 +278,10 @@ static void grlib_gptimer_write(void *opaque, hwaddr addr,
switch (timer_addr) {
case COUNTER_OFFSET:
trace_grlib_gptimer_writel(id, addr, value);
+ grlib_gptimer_tx_begin(&unit->timers[id]);
unit->timers[id].counter = value;
grlib_gptimer_enable(&unit->timers[id]);
+ grlib_gptimer_tx_commit(&unit->timers[id]);
return;
case COUNTER_RELOAD_OFFSET:
@@ -291,6 +305,7 @@ static void grlib_gptimer_write(void *opaque, hwaddr addr,
/* gptimer_restart calls gptimer_enable, so if "enable" and "load"
bits are present, we just have to call restart. */
+ grlib_gptimer_tx_begin(&unit->timers[id]);
if (value & GPTIMER_LOAD) {
grlib_gptimer_restart(&unit->timers[id]);
} else if (value & GPTIMER_ENABLE) {
@@ -301,6 +316,7 @@ static void grlib_gptimer_write(void *opaque, hwaddr addr,
value &= ~(GPTIMER_LOAD & GPTIMER_DEBUG_HALT);
unit->timers[id].config = value;
+ grlib_gptimer_tx_commit(&unit->timers[id]);
return;
default:
@@ -344,9 +360,11 @@ static void grlib_gptimer_reset(DeviceState *d)
timer->counter = 0;
timer->reload = 0;
timer->config = 0;
+ ptimer_transaction_begin(timer->ptimer);
ptimer_stop(timer->ptimer);
ptimer_set_count(timer->ptimer, 0);
ptimer_set_freq(timer->ptimer, unit->freq_hz);
+ ptimer_transaction_commit(timer->ptimer);
}
}
@@ -365,14 +383,16 @@ static void grlib_gptimer_realize(DeviceState *dev, Error **errp)
GPTimer *timer = &unit->timers[i];
timer->unit = unit;
- timer->bh = qemu_bh_new(grlib_gptimer_hit, timer);
- timer->ptimer = ptimer_init_with_bh(timer->bh, PTIMER_POLICY_DEFAULT);
+ timer->ptimer = ptimer_init(grlib_gptimer_hit, timer,
+ PTIMER_POLICY_DEFAULT);
timer->id = i;
/* One IRQ line for each timer */
sysbus_init_irq(sbd, &timer->irq);
+ ptimer_transaction_begin(timer->ptimer);
ptimer_set_freq(timer->ptimer, unit->freq_hz);
+ ptimer_transaction_commit(timer->ptimer);
}
memory_region_init_io(&unit->iomem, OBJECT(unit), &grlib_gptimer_ops,
--
2.20.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] hw/timer/slavio_timer.c: Switch to transaction-based ptimer API
2019-10-21 13:43 [PATCH v2 0/3] Convert sparc devices to new ptimer API Peter Maydell
2019-10-21 13:43 ` [PATCH v2 1/3] hw/timer/slavio_timer: Remove useless check for NULL t->timer Peter Maydell
2019-10-21 13:43 ` [PATCH v2 2/3] hw/timer/grlib_gptimer.c: Switch to transaction-based ptimer API Peter Maydell
@ 2019-10-21 13:43 ` Peter Maydell
2019-10-21 14:06 ` [PATCH v2 0/3] Convert sparc devices to new " Philippe Mathieu-Daudé
2019-10-24 12:19 ` Peter Maydell
4 siblings, 0 replies; 11+ messages in thread
From: Peter Maydell @ 2019-10-21 13:43 UTC (permalink / raw)
To: qemu-devel
Cc: Philippe Mathieu-Daudé, KONRAD Frederic, Richard Henderson,
Mark Cave-Ayland, Fabien Chouteau
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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@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 890dd53f8d8..c55e8d0bf42 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;
@@ -234,6 +234,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(t->limit), 1);
}
}
+ ptimer_transaction_commit(t->timer);
break;
case TIMER_COUNTER:
if (slavio_timer_is_user(tc)) {
@@ -245,7 +246,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();
}
@@ -253,13 +256,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) {
@@ -271,6 +277,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) {
@@ -280,6 +287,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
@@ -306,6 +314,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();
@@ -365,10 +374,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;
@@ -378,7 +389,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;
@@ -390,9 +400,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);
--
2.20.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/3] Convert sparc devices to new ptimer API
2019-10-21 13:43 [PATCH v2 0/3] Convert sparc devices to new ptimer API Peter Maydell
` (2 preceding siblings ...)
2019-10-21 13:43 ` [PATCH v2 3/3] hw/timer/slavio_timer.c: " Peter Maydell
@ 2019-10-21 14:06 ` Philippe Mathieu-Daudé
2019-10-24 12:19 ` Peter Maydell
4 siblings, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-21 14:06 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Richard Henderson, KONRAD Frederic, Mark Cave-Ayland,
Fabien Chouteau
On 10/21/19 3:43 PM, Peter Maydell wrote:
> This patchset converts the devices used by sparc machines to the new
> ptimer API.
>
> 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. This was reported as a bug in a specific
> timer device but it's a problem with the generic ptimer code:
> https://bugs.launchpad.net/qemu/+bug/1777777
>
> The updates to the individual ptimer devices are straightforward:
> we need to add begin/commit calls around the various places that
> modify the ptimer state, and use the new ptimer_init() function
> to create the timer.
>
> Changes v1->v2:
> * patches 2 and 3 are the old 1 and 2 and have been reviewed
> * patch 1 is new and removes a pointless NULL check; without
> this we'd probably have got Coverity errors when patch 3
> added a use of t->timer before the check for it being NULL
>
> thanks
> --PMM
>
>
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> *** BLURB HERE ***
>
> Peter Maydell (3):
> hw/timer/slavio_timer: Remove useless check for NULL t->timer
> hw/timer/grlib_gptimer.c: Switch to transaction-based ptimer API
> hw/timer/slavio_timer.c: Switch to transaction-based ptimer API
Nitpicking, maybe reorder the grlib_gptimer patch last:
hw/timer/slavio_timer: Remove useless check for NULL t->timer
hw/timer/slavio_timer.c: Switch to transaction-based ptimer API
hw/timer/grlib_gptimer.c: Switch to transaction-based ptimer API
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/3] Convert sparc devices to new ptimer API
2019-10-21 13:43 [PATCH v2 0/3] Convert sparc devices to new ptimer API Peter Maydell
` (3 preceding siblings ...)
2019-10-21 14:06 ` [PATCH v2 0/3] Convert sparc devices to new " Philippe Mathieu-Daudé
@ 2019-10-24 12:19 ` Peter Maydell
2019-10-24 18:04 ` Mark Cave-Ayland
4 siblings, 1 reply; 11+ messages in thread
From: Peter Maydell @ 2019-10-24 12:19 UTC (permalink / raw)
To: QEMU Developers
Cc: Philippe Mathieu-Daudé, KONRAD Frederic, Richard Henderson,
Mark Cave-Ayland, Fabien Chouteau
On Mon, 21 Oct 2019 at 14:43, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset converts the devices used by sparc machines to the new
> ptimer API.
>
> 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. This was reported as a bug in a specific
> timer device but it's a problem with the generic ptimer code:
> https://bugs.launchpad.net/qemu/+bug/1777777
>
> The updates to the individual ptimer devices are straightforward:
> we need to add begin/commit calls around the various places that
> modify the ptimer state, and use the new ptimer_init() function
> to create the timer.
>
> Changes v1->v2:
> * patches 2 and 3 are the old 1 and 2 and have been reviewed
> * patch 1 is new and removes a pointless NULL check; without
> this we'd probably have got Coverity errors when patch 3
> added a use of t->timer before the check for it being NULL
I'm going to apply these to target-arm.next; I know they haven't
been on list long but the change since v1 is only minor and
they've all been reviewed.
thanks
-- PMM
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/3] Convert sparc devices to new ptimer API
2019-10-24 12:19 ` Peter Maydell
@ 2019-10-24 18:04 ` Mark Cave-Ayland
2019-10-24 18:17 ` Philippe Mathieu-Daudé
2019-10-25 7:32 ` Peter Maydell
0 siblings, 2 replies; 11+ messages in thread
From: Mark Cave-Ayland @ 2019-10-24 18:04 UTC (permalink / raw)
To: Peter Maydell, QEMU Developers
Cc: Richard Henderson, KONRAD Frederic, Philippe Mathieu-Daudé,
Fabien Chouteau
On 24/10/2019 13:19, Peter Maydell wrote:
> On Mon, 21 Oct 2019 at 14:43, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> This patchset converts the devices used by sparc machines to the new
>> ptimer API.
>>
>> 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. This was reported as a bug in a specific
>> timer device but it's a problem with the generic ptimer code:
>> https://bugs.launchpad.net/qemu/+bug/1777777
>>
>> The updates to the individual ptimer devices are straightforward:
>> we need to add begin/commit calls around the various places that
>> modify the ptimer state, and use the new ptimer_init() function
>> to create the timer.
>>
>> Changes v1->v2:
>> * patches 2 and 3 are the old 1 and 2 and have been reviewed
>> * patch 1 is new and removes a pointless NULL check; without
>> this we'd probably have got Coverity errors when patch 3
>> added a use of t->timer before the check for it being NULL
>
> I'm going to apply these to target-arm.next; I know they haven't
> been on list long but the change since v1 is only minor and
> they've all been reviewed.
Thanks Peter! Not sure if you saw my Tested-by tag last week for the slavio (sun4m)
parts, but there were no obvious regressions that I could see under qemu-system-sparc.
ATB,
Mark.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/3] Convert sparc devices to new ptimer API
2019-10-24 18:04 ` Mark Cave-Ayland
@ 2019-10-24 18:17 ` Philippe Mathieu-Daudé
2019-10-25 7:32 ` Peter Maydell
1 sibling, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-24 18:17 UTC (permalink / raw)
To: Mark Cave-Ayland, Peter Maydell, QEMU Developers
Cc: KONRAD Frederic, Richard Henderson, Fabien Chouteau
On 10/24/19 8:04 PM, Mark Cave-Ayland wrote:
> On 24/10/2019 13:19, Peter Maydell wrote:
>
>> On Mon, 21 Oct 2019 at 14:43, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>
>>> This patchset converts the devices used by sparc machines to the new
>>> ptimer API.
>>>
>>> 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. This was reported as a bug in a specific
>>> timer device but it's a problem with the generic ptimer code:
>>> https://bugs.launchpad.net/qemu/+bug/1777777
>>>
>>> The updates to the individual ptimer devices are straightforward:
>>> we need to add begin/commit calls around the various places that
>>> modify the ptimer state, and use the new ptimer_init() function
>>> to create the timer.
>>>
>>> Changes v1->v2:
>>> * patches 2 and 3 are the old 1 and 2 and have been reviewed
>>> * patch 1 is new and removes a pointless NULL check; without
>>> this we'd probably have got Coverity errors when patch 3
>>> added a use of t->timer before the check for it being NULL
>>
>> I'm going to apply these to target-arm.next; I know they haven't
>> been on list long but the change since v1 is only minor and
>> they've all been reviewed.
>
> Thanks Peter! Not sure if you saw my Tested-by tag last week for the slavio (sun4m)
> parts, but there were no obvious regressions that I could see under qemu-system-sparc.
This was on v1:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg653861.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/3] Convert sparc devices to new ptimer API
2019-10-24 18:04 ` Mark Cave-Ayland
2019-10-24 18:17 ` Philippe Mathieu-Daudé
@ 2019-10-25 7:32 ` Peter Maydell
1 sibling, 0 replies; 11+ messages in thread
From: Peter Maydell @ 2019-10-25 7:32 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: Richard Henderson, KONRAD Frederic, Philippe Mathieu-Daudé,
QEMU Developers, Fabien Chouteau
On Thu, 24 Oct 2019 at 19:10, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 24/10/2019 13:19, Peter Maydell wrote:
> > I'm going to apply these to target-arm.next; I know they haven't
> > been on list long but the change since v1 is only minor and
> > they've all been reviewed.
>
> Thanks Peter! Not sure if you saw my Tested-by tag last week for the slavio (sun4m)
> parts, but there were no obvious regressions that I could see under qemu-system-sparc.
Yeah, I saw that, thanks for the testing. I decided that since
I'd added patch 1 I didn't quite feel comfortable carrying the
tested-by tag across.
thanks
-- PMM
^ permalink raw reply [flat|nested] 11+ messages in thread