On 04/05/2017 05:25, Xiao Guangrong wrote:
>
>
> On 05/03/2017 11:39 PM, Paolo Bonzini wrote:
>>
>>
>> On 12/04/2017 11:51, guangrong.xiao@xxxxxxxxx wrote:
>>> From: Xiao Guangrong <xiaoguangrong@xxxxxxxxxxx>
>>>
>>> Move the x86 specific code in periodic_timer_update() to a common place,
>>> the actual logic is not changed
>>>
>>> Signed-off-by: Xiao Guangrong <xiaoguangrong@xxxxxxxxxxx>
>>> ---
>>> hw/timer/mc146818rtc.c | 112
>>> +++++++++++++++++++++++++++++--------------------
>>> 1 file changed, 66 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
>>> index 3bf559d..d7b7c56 100644
>>> --- a/hw/timer/mc146818rtc.c
>>> +++ b/hw/timer/mc146818rtc.c
>>> @@ -144,6 +144,63 @@ static void rtc_coalesced_timer(void *opaque)
>>> rtc_coalesced_timer_update(s);
>>> }
>>> +
>>> +static int64_t
>>> +arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
>>> +{
>>> + if (period != s->period) {
>>> + int64_t scale_lost_clock;
>>> + int current_irq_coalesced = s->irq_coalesced;
>>> +
>>> + s->irq_coalesced = (current_irq_coalesced * s->period) /
>>> period;
>>> +
>>> + /*
>>> + * calculate the lost clock after it is scaled which should be
>>> + * compensated in the next interrupt.
>>> + */
>>> + scale_lost_clock = current_irq_coalesced * s->period -
>>> + s->irq_coalesced * period;
>>> + DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld
>>> clocks "
>>> + "are compensated.\n", current_irq_coalesced,
>>> + s->irq_coalesced, scale_lost_clock);
>>> + lost_clock += scale_lost_clock;
>>> + s->period = period;
>>
>> This should be moved up to the caller.
>
> It should not. As you pointed out below, all these code are only needed
> for LOST_TICK_POLICY_SLEW that is x86 specific.
>
> Or use "if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW)" without
> "#ifdef TARGET_I386" is acceptable as only x86 can make it SLEW,
> Unnecessary branch checks will little slow down other architectures,
> but i think it is acceptable, right? :)
Yeah, the #ifdef TARGET_I386 is only needed because of the APIC
interface. This one doesn't really need the #ifdef. But you're right
that it shouldn't be moved to the caller.
Paolo
>>
>> Also, if s->lost_tick_policy is not SLEW, s->irq_coalesced on input is
>> zero. So I *think* what you get is equivalent to
>>
>> if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW) {
>> return;
>> }
>>
>> /* ... comment ... */
>> lost_interrupt = (s->irq_coalesced * s->period) / period;
>> lost_clock += (s->irq_coalesced * s->period) % period;
>> lost_interrupt += lost_clock / period;
>> lost_clock %= period;
>>
>> s->irq_coalesced = load_interrupt;
>> rtc_coalesced_timer_update(s);
>>
>> or equivalently:
>>
>> lost_clock += s->irq_coalesced * s->period;
>>
>> s->irq_coalesced = lost_clock / period;
>> lost_clock %= period;
>> rtc_coalesced_timer_update(s);
>>
>
> Exactly right, it is much better, will apply it.
>
>> I think you should probably merge these three patches and document the
>> resulting logic, because it's simpler than building it a patch at a time.
>
> Okay, i will consider it carefully in the next version.
>
> Thank you, Paolo!
>
