On Tue, Oct 27, 2015 at 2:26 PM, Dmitry Osipenko <address@hidden
<mailto:address@hidden>> wrote:
25.10.2015 20:39, Peter Crosthwaite пишет:
On Sun, Oct 25, 2015 at 6:23 AM, Dmitry Osipenko <address@hidden
<mailto:address@hidden>> wrote:
25.10.2015 02:55, Peter Crosthwaite пишет:
On Sat, Oct 24, 2015 at 3:22 PM, Dmitry Osipenko
<address@hidden <mailto:address@hidden>> wrote:
24.10.2015 22:45, Peter Crosthwaite пишет:
This looks like a give-up without trying to get the
correct value. If
the calculated value (using the normal-path logic below)
is sane, you
should just use it. If it comes out bad then you should
clamp to 1.
I am wondering whether this clamping policy (as in
original code as
well) is correct at all though. The value of a
free-running
short-interval periodic timer (poor mans random number
generator)
without any actual interrupt generation will be affected
by QEMUs
asynchronous handling of timer events. So if I set limit
to 100, then
sample this timer every user keyboard stroke, I should
get a uniform
distribution on [0,100]. Instead in am going to get lots
of 1s. This
Right, that's a good example. What about to scale ptimer
period to match
adjusted timer_mod interval?
Thats just as incorrect as changing the limit IMO. The guest
could get
confused with a timer running at the wrong frequency.
is more broken in the original code (as you state), as I
will get >
100, but I think we have traded broken for slightly less
broken. I
think the correct semantic is to completely ignoring
rate limitin
except for the scheduling on event callbacks. That is,
the timer
I'm missing you here. What event callbacks?
when timer_mod() hits, and it turn triggers some device specific
event
(usually an interrupt).
There are two basic interactions for any QEMU timer. There are
synchronous events, i.e. the guest reading (polling) the counter
which
is what this patch tries to fix. The second is the common case
of
periodic interrupt generation. My proposal is that rate limiting
does
not affect synchronous operation, only asynchronous (so my
keystroke
RNG case works). In the current code, if ptimer_get_count() is
called
when the event has passed it returns 0 under the assumption
that the
timer_mod callback is about to happen. With rate-limiting that
may be
well in the future.
ptimer_tick() would happen on the next QEMU loop cycle, so it might
be more
reasonable to return counter = 1 here, wouldn't it?
interval is not rate limited, instead the timer_mod
interval
(next_event -last_event) just has a 10us clamp.
The means the original code semantic of returning
counter = 0 for an
already triggered timer is wrong. It should handle
in-the-past
wrap-arounds as wraparounds by triggering the timer and
redoing the
math with the new interval values. So instead the logic
would be
something like:
timer_val = -1;
for(;;) {
if (!enabled) {
return delta;
}
timer_val = (next-event - now) * scaling();
if (timer_val >= 0) {
return timer_val;
}
/* Timer has actually expired but we missed it,
reload it and try
again
*/
ptimer_tick();
}
Why do you think that ptimer_get_count() == 0 in case of the
running
periodic timer that was expired while QEMU was "on the way"
to ptimer
code
is bad and wrong?
Because you may have gone past the time when it was actually
zero and
reloaded and started counting again. It should return the real
(reloaded and resumed) value. This is made worse by rate
limiting as
the timer will spend a long time at the clamp value waiting for
the
rate-limited tick to fix it.
Following on from before, we don't want the async stuff to
affect
sync. As the async callbacks are heavily affected by rate
limiting we
don't want the polled timer having to rely on the callbacks
(async
path) at all for correct operation. That's what the current
implementation of ptimer_get_count assumes with that 0-clamp.
Alright, that make sense now. Thanks for clarifying.
From the guest point of view it's okay (no?), do we
really
need to overengineer that corner case?
Depends on your use case. Your bug report is correct in that the
timer
should never be outside the bounds of the limit. But you are
fixing
that very specifically with a minimal change rather than
correcting
the larger ptimer_get_count() logic which I think is more broken
than
you suggest it is.
ptimer_reload() then needs to be patched to make sure it
always
timer_mod()s in the future, otherwise this loop could
iterate a large
number of times.
This means that when the qemu_timer() actually ticks, a
large number
or cycles may have occured, but we can justify that in
that callback
event latency (usually interrupts) is undefined anyway
and coalescing
of multiples may have happened as part of that. This
usually matches
expectations of real guests where interrupt latency is
ultimately
undefined.
ptimer_tick() is re-arm'ing the qemu_timer() in case of
periodic mode.
Hope
I haven't missed your point here.
Yes. But it is also capable of doing the heavy lifting for our
already
expired case. Basic idea is, if the timer is in a bad state
(should
have hit) but hasn't, do the hit to put the timer into a good
state
(by calling ptimer_tick) then just do the right thing. That's
what the
loop does. It should also work for an in-the-past one-shot.
Summarizing what we have now:
There are two issues with ptimer:
1) ptimer_get_count() return wrong values with adjusted .limit
Patch V7 doesn't solve that issue, but makes it slightly better by
clamping
returned value to [0, 1]. That's not what we want, we need to return
counter
value in it's valid range [0, limit].
You are rejecting variant of scaling ptimer period, saying that it
might
affect software behavior inside the guest. But by adjusting the
timer, we
might already causing same misbehavior in case of blazing fast host
machine.
It is a different misbehaviour. We are modelling the polled timer
perfectly but limiting the frequency of callbacks (interrupts). I
think this is the lesser of two evils.
I'll scratch my head a bit more on it. If you have any better idea,
please
share.
2) ptimer_get_count() return fake 0 value in case of the expired
qemu_timer() without triggering ptimer_tick()
You're suggesting to solve it by running ptimer_tick(). So if
emulated
device uses ptimer tick event (scheduled qemu bh) to raise
interrupt, it
would do it by one QEMU loop cycle earlier.
Yes, this is ok, as even in a rate limited scenario there is no reason
to absolutely force the rate limit. If a poll happens it should just
flush the waiting interrupt.
My question here: is it always legal for the guest software to be
able to
get counter = 0 on poll while CPU interrupt on timer expire hasn't
happened
yet (would happen after one QEMU cycle).
Yes. And I am going a step further by saying it is ok for the guest
software to see the timer value wrapped around before the expire too.
Let's imagine a hardware with a such restriction: timer interrupt has
highest priority and CPU immediately switches to the interrupt handler in a
such way that it won't ever could see counter = 0 / wraparound (with
interrupt enabled) before entering the handler.
Is it unrealistic?
Yes. And if it is possible in real HW, I don't think this is valid for QEMU
outside of icount mode.
For instance (on QEMU):
CPU | Timer
---------------------------------------------------------------------
start_periodic_timer | timer starts ticking
.....
QEMU starts to execute |
translated block |
| QEMU timer expires
|
CPU reads the timer register, | ptimer_get_count() return
ptimer_get_count() called | wrapped around value
.....
CPU interrupt handler kicks in | timer continue ticking, so
| any value is valid here
CPU stops the timer and sets |
counter to 0, returns from the |
handler |
.....
Now, for some reason, software |
sees that timer is stopped |
and do something using read |
value |
Program code sketch:
timer_interrupt_handler()
{
write32(1, TIMER_STOP);
write32(0, TIMER_COUNTER);
write32(TIMER_IRQ_CLEAR, TIMER_STATUS);
return IRQ_HANDLED;
}
program()
{
.....
..... <--- timer expired here
..... <--- interrupt handler executed here on real HW
var1 = read32(TIMER_COUNTER); <--- Emulated got wrapped,
real got 0
..... <--- interrupt handler executed here on QEMU
if (read32(TIMER_STATUS) & TIMER_RUNNING) {
.....
} else {
.....
write(var1 >> 16, SOME_DEV_REGISTER);
}
.....
}
Might emulated program behave differently from the real HW after it?
Probably.
I want to mention that not only beefy generic CPU's are the ptimer users.
However, it seems that no one of the current ptimer users has a such
restriction since it would already return 0 on expire and ptimer_tick()
would happen after it. We can agree on keeping ptimer less universal in
favor of
the expire optimization, so somebody may improve it later if it would be
needed.
I think removing the rate limiter's and clamping-affect on the read value makes
it more universal if anything.
Do we agree?
I'm not sure, what are you referring to as the "expire optimizsation"?
I guess it might cause software
misbehavior if it assumes that the real hardware has CPU and timer
running
in the same clock domain, i.e. such situation might be not possible.
Assumptions about the CPU clocking only make sense in icount mode,
where the rate limiter is disable anyway.
Timer limiter has nothing to do with a returned value for the expired timer.
Clock cycle accurate execution isn't relevant to upstream QEMU, I meant
clocking in general. Emulated behaviour shouldn't diverge from the real HW.
But when the rate limiter is on for a short interval, it massively distorts
this.
Regards,
Peter
So I'm
suggesting to return counter = 1 and allow ptimer_tick() happen on
it's own.
My alternate suggestion is, if you detect that the tick should have
already happened, just make it happen. I don't see the need to rate
limit a polled timer.
Yes, I got your idea and it is absolutely correct if we agree on the above
tradeoff (if that tradeoff exists).