Re: [Qemu-devel] [PATCH v5 1/2] hw/ptimer: Set delta to the original limit on reload in ptimer_set_limit()

[Dmitry Osipenko <digetx@gmail.com>]

19.10.2015 20:06, Peter Maydell пишет:
> On 14 October 2015 at 00:30, Dmitry Osipenko <digetx@gmail.com> wrote:
>> ptimer_get_count() returns incorrect value for the disabled timer after
>> reloading the counter with a small value, because corrected limit value
>> is used instead of the original.
>>
>> For instance:
>>      1) ptimer_stop(t)
>>      2) ptimer_set_period(t, 1)
>>      3) ptimer_set_limit(t, 0, 1)
>>      4) ptimer_get_count(t) <-- would return 10000 instead of 0
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>   hw/core/ptimer.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
>> index 8437bd6..abc3a20 100644
>> --- a/hw/core/ptimer.c
>> +++ b/hw/core/ptimer.c
>> @@ -180,6 +180,8 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq)
>>      count = limit.  */
>>   void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
>>   {
>> +    uint64_t count = limit;
>> +
>>       /*
>>        * Artificially limit timeout rate to something
>>        * achievable under QEMU.  Otherwise, QEMU spends all
>> @@ -195,7 +197,7 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
>>
>>       s->limit = limit;
>>       if (reload)
>> -        s->delta = limit;
>> +        s->delta = count;
>>       if (s->enabled && reload) {
>>           s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>>           ptimer_reload(s);
>
> Doesn't this defeat the rate limiting if the timer is enabled,
> though? ptimer_reload() sets the underlying timer based on
> s->delta, so if s->delta isn't the rate-limited value then the
> timer will be incorrectly set to a very close-in value.
>

Yes, it defeats the rate limiting for the first timer expire. As I understand, 
the idea of the rate limit correction aims periodic timer only.

"Otherwise, QEMU spends all its time generating timer interrupts, and there is 
no forward progress.", as stated in the comment.

I think it's not a problem to get "instant" timer trigger for the first expire.

> I think we'll return "incorrect" values from ptimer_get_count()
> in the "counter is running" case too, because we calculate those
> by looking at when the underlying timer's due to expire, and
> we set the expiry time based on the adjusted value.
>

That's a good point.

> What's the underlying model we should have for what values
> we return from reading the count if we've decided to adjust
> the actual timer expiry with the rate limit? Should the
> count go down from the specified value and then just hang
> at 1 until the extended timer expiry time hits? Or something
> else? Clearly defining what we want to happen ought to make
> it easier to review attempts to fix it...
>

What about the following:

Add additional ptimer struct member, say "limit_corrected", to check whether the 
limit was corrected or not.

ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
{
	.limit_corrected = 0;

	// on the limit correction:
	.limit_corrected = (limit == 0) ? 1 : 2;
	limit = 10000 / s->period;
}

ptimer_get_count()
{
	if (enabled) {
		if (expired || .limit_corrected == 1) {
			counter = 0;
		} else if (.limit_corrected == 2) {
			counter = 1;
		} else {
			// do the counter calculations ...
		}
	}
}

and clear .limit_corrected on the one-shot timer start. That would bump ptimer 
VMSD version, but keep .minimum_version_id.

> (Calling ptimer_set_count() also bypasses the ratelimiting at
> the moment, incidentally.)
>

Should be okay if my "aiming the periodic timer only" thought is correct.

> thanks
> -- PMM
>

-- 
Dmitry