Re: [PATCH RFC v2 1/3] x86/hvm/rtc: Don't run the vpt timer when !REG_B.PIE.

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 14/02/2014 23:03, Tim Deegan wrote:
> At 19:51 +0000 on 14 Feb (1392403919), Andrew Cooper wrote:
>> From: Tim Deegan <tim@xen.org>
>>
>> If the guest has not asked for interrupts, don't run the vpt timer
>> to generate them.  This is a prerequisite for a patch to simplify how
>> the vpt interacts with the RTC, and also gets rid of a timer series in
>> Xen in a case where it's unlikely to be needed.
>>
>> Instead, calculate the correct value for REG_C.PF whenever REG_C is
>> read or PIE is enabled.  This allow a guest to poll for the PF bit
>> while not asking for actual timer interrupts.  Such a guest would no
>> longer get the benefit of the vpt's timer modes.
>>
>> Signed-off-by: Tim Deegan <tim@xen.org>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> ---
>>
>> Changes in v2:
>>  * Reduce scope of `now` in rtc_timer_update()
>>  * Merge PIE logic in REG_B write
>>  * Tightly couple setting s->period with creating/destroying timers, so the
>>    timer change gets properly recreated when the guest sets REG_B.PIE
> Thanks for sorting out that bug, but I think in this version the !PIE
> case won't work.  check_for_pf_ticks() uses s->period to figure out
> whether to set PF, so it needs to be set whenever the REG_A selector
> is configured, even if the timer's not running.
>
> It looks like always setting s->period == 0 just before the call to
> rtc_timer_update in the REG_B write (i.e. not just in the !PIE case)
> would DTRT, but is that what you tried earlier?

I tried the "careful clobbering" which resulted in the fragment below,
and altering the position of s->period in rtc_timer_update() to match,
but that does indeed lead to issues of s->period being 0 if !PIE.

I clearly should have been less careful.

>
> Er, that is, here:
>
>>          rtc_update_irq(s);
>> -        if ( (data & RTC_PIE) && !(orig & RTC_PIE) )
>> +        if ( (data ^ orig) & RTC_PIE )
>> +        {
>> +            if ( !(data & RTC_PIE) )
>> +            {
>> +                destroy_periodic_time(&s->pt);
>> +                s->period = 0;
>> +            }
>>              rtc_timer_update(s);
>> +        }
>>          if ( (data ^ orig) & RTC_SET )
> do this:
>
>> +        if ( (data ^ orig) & RTC_PIE )
>> +        {
>> +            destroy_periodic_time(&s->pt);
>> +            s->period = 0;
>>              rtc_timer_update(s);
>> +        }
> Tim.

... and undo the changes in rtc_timer_update().

~Andrew