[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 2/2] target/arm: only set the nexttick timer if !ISTATUS
From: |
Alex Bennée |
Subject: |
Re: [PATCH v1 2/2] target/arm: only set the nexttick timer if !ISTATUS |
Date: |
Tue, 28 Jul 2020 17:11:14 +0100 |
User-agent: |
mu4e 1.5.5; emacs 28.0.50 |
Peter Maydell <peter.maydell@linaro.org> writes:
> On Tue, 28 Jul 2020 at 15:10, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Otherwise we have an unfortunate interaction with -count sleep=off
>> which means we fast forward time when we don't need to. The easiest
>> way to trigger it was to attach to the gdbstub and place a break point
>> at the timers IRQ routine. Once the timer fired setting the next event
>> at INT_MAX then qemu_start_warp_timer would skip to the end.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>> target/arm/helper.c | 35 ++++++++++++++++++++++-------------
>> 1 file changed, 22 insertions(+), 13 deletions(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index c69a2baf1d3..ec1b84cf0fd 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -2683,7 +2683,7 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
>> uint64_t count = gt_get_countervalue(&cpu->env);
>> /* Note that this must be unsigned 64 bit arithmetic: */
>> int istatus = count - offset >= gt->cval;
>> - uint64_t nexttick;
>> + uint64_t nexttick = 0;
>> int irqstate;
>>
>> gt->ctl = deposit32(gt->ctl, 2, 1, istatus);
>> @@ -2692,21 +2692,30 @@ static void gt_recalc_timer(ARMCPU *cpu, int
>> timeridx)
>> qemu_set_irq(cpu->gt_timer_outputs[timeridx], irqstate);
>>
>> if (istatus) {
>> - /* Next transition is when count rolls back over to zero */
>> - nexttick = UINT64_MAX;
>> + /*
>> + * The IRQ status of the timer will persist until:
>> + * - CVAL is changed or
>> + * - ENABLE is changed
>> + *
>> + * There is no point re-arming the timer for some far
>> + * flung future - currently it just is.
>> + */
>> + timer_del(cpu->gt_timer[timeridx]);
>
> Why do we delete the timer for this case of "next time we need to
> know is massively in the future"...
It's not really - it's happening now and it will continue to happen
until the IRQ is serviced or we change the CVAL at which point we can
calculate the next time we need it.
>
>> } else {
>> /* Next transition is when we hit cval */
>> nexttick = gt->cval + offset;
>> - }
>> - /* Note that the desired next expiry time might be beyond the
>> - * signed-64-bit range of a QEMUTimer -- in this case we just
>> - * set the timer for as far in the future as possible. When the
>> - * timer expires we will reset the timer for any remaining period.
>> - */
>> - if (nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu)) {
>> - timer_mod_ns(cpu->gt_timer[timeridx], INT64_MAX);
>> - } else {
>> - timer_mod(cpu->gt_timer[timeridx], nexttick);
>> +
>> + /*
>> + * It is possible the next tick is beyond the
>> + * signed-64-bit range of a QEMUTimer but currently the
>> + * timer system doesn't support a run time of more the 292
>> + * odd years so we set it to INT_MAX in this case.
>> + */
>> + if (nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu)) {
>> + timer_mod_ns(cpu->gt_timer[timeridx], INT64_MAX);
>
> ...but here we handle the similar case by "set a timeout for
> INT64_MAX" ?
Yeah we could just swallow it up and report something to say it's not
going to happen because it's beyond the horizon of what QEMUTimer can
deal with.
>
>> + } else {
>> + timer_mod(cpu->gt_timer[timeridx], nexttick);
>> + }
>> }
>> trace_arm_gt_recalc(timeridx, irqstate, nexttick);
>> } else {
>> --
>
> thanks
> -- PMM
--
Alex Bennée