Re: [Qemu-devel] [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update

[Paolo Bonzini]

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.x...@gmail.com wrote:
>>> From: Xiao Guangrong 
>>>
>>> Move the x86 specific code in periodic_timer_update() to a common place,
>>> the actual logic is not changed
>>>
>>> Signed-off-by: Xiao Guangrong 
>>> ---
>>>   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!
> 

Re: [Qemu-devel] [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update

[Xiao Guangrong]

On 05/03/2017 11:39 PM, Paolo Bonzini wrote:



On 12/04/2017 11:51, guangrong.x...@gmail.com wrote:

From: Xiao Guangrong 

Move the x86 specific code in periodic_timer_update() to a common place,
the actual logic is not changed

Signed-off-by: Xiao Guangrong 
---
  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? :)



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!

Re: [Qemu-devel] [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update

[Paolo Bonzini]

On 12/04/2017 11:51, guangrong.x...@gmail.com wrote:
> From: Xiao Guangrong 
> 
> Move the x86 specific code in periodic_timer_update() to a common place,
> the actual logic is not changed
> 
> Signed-off-by: Xiao Guangrong 
> ---
>  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.

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);

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.

Thanks,

Paolo

> +}
> +
> +/*
> + * if more than period clocks were passed, i.e, the timer interrupt
> + * has been lost, we should catch up the time.
> + */
> +if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
> +   (lost_clock / period)) {
> +int lost_interrupt = lost_clock / period;
> +
> +s->irq_coalesced += lost_interrupt;
> +lost_clock -= lost_interrupt * period;
> +if (lost_interrupt) {
> +DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs "
> +  "increased to %d\n", lost_interrupt, s->irq_coalesced);
> +rtc_coalesced_timer_update(s);
> +}
> +}
> +
> +return lost_clock;
> +}
> +
> +static void arch_periodic_timer_disable(RTCState *s)
> +{
> +s->irq_coalesced = 0;
> +}
> +#else
> +static int64_t
> +arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
> +{
> +return lost_clock;
> +}
> +
> +static void arch_periodic_timer_disable(RTCState *s)
> +{
> +}
>  #endif
>  
>  static int period_code_to_clock(int period_code)
> @@ -175,24 +232,7 @@ periodic_timer_update(RTCState *s, int64_t current_time, 
> int old_period)
>  if (period_code != 0
>  && (s->cmos_data[RTC_REG_B] & REG_B_PIE)) {
>  period = period_code_to_clock(period_code);
> -#ifdef TARGET_I386
> -if (period != s->period) {
> -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.
> - */
> -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, lost_clock);
> -}
> -s->period = period;
> -#endif
>  /* compute 32 khz clock */
>  cur_clock =
>  muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
> @@ -219,42 +259,22 @@ periodic_timer_update(RTCState *s, int64_t 
> current_time, int old_period)
>  
>  /* calculate the clock since last interrupt. */
>  lost_clock += cur_clock - last_periodic_clock;
> -
> -#ifdef TARGET_I386
> -/*
> - * if more than period clocks were passed, i.e, the timer 
> interrupt

[Qemu-devel] [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update

[guangrong . xiao]

From: Xiao Guangrong 

Move the x86 specific code in periodic_timer_update() to a common place,
the actual logic is not changed

Signed-off-by: Xiao Guangrong 
---
 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;
+}
+
+/*
+ * if more than period clocks were passed, i.e, the timer interrupt
+ * has been lost, we should catch up the time.
+ */
+if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
+   (lost_clock / period)) {
+int lost_interrupt = lost_clock / period;
+
+s->irq_coalesced += lost_interrupt;
+lost_clock -= lost_interrupt * period;
+if (lost_interrupt) {
+DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs "
+  "increased to %d\n", lost_interrupt, s->irq_coalesced);
+rtc_coalesced_timer_update(s);
+}
+}
+
+return lost_clock;
+}
+
+static void arch_periodic_timer_disable(RTCState *s)
+{
+s->irq_coalesced = 0;
+}
+#else
+static int64_t
+arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
+{
+return lost_clock;
+}
+
+static void arch_periodic_timer_disable(RTCState *s)
+{
+}
 #endif
 
 static int period_code_to_clock(int period_code)
@@ -175,24 +232,7 @@ periodic_timer_update(RTCState *s, int64_t current_time, 
int old_period)
 if (period_code != 0
 && (s->cmos_data[RTC_REG_B] & REG_B_PIE)) {
 period = period_code_to_clock(period_code);
-#ifdef TARGET_I386
-if (period != s->period) {
-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.
- */
-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, lost_clock);
-}
-s->period = period;
-#endif
 /* compute 32 khz clock */
 cur_clock =
 muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
@@ -219,42 +259,22 @@ periodic_timer_update(RTCState *s, int64_t current_time, 
int old_period)
 
 /* calculate the clock since last interrupt. */
 lost_clock += cur_clock - last_periodic_clock;
-
-#ifdef TARGET_I386
-/*
- * if more than period clocks were passed, i.e, the timer interrupt
- * has been lost, we should catch up the time.
- */
-if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
-(lost_clock / period)) {
-int lost_interrupt = lost_clock / period;
-
-s->irq_coalesced += lost_interrupt;
-lost_clock -= lost_interrupt * period;
-if (lost_interrupt) {
-DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs "
-  "increased to %d\n", lost_interrupt,
-  s->irq_coalesced);
-rtc_coalesced_timer_update(s);
-}
-} else
-#endif
-/*
- * no way to compensate the interrupt if LOST_TICK_POLICY_SLEW
- * is not used, we should make the time progress anyway.
- */
-lost_clock = MIN(lost_clock, period);
-assert(lost_clock >= 0);
 }
 
+lost_clock = arch_periodic_timer_update(s, period, lost_clock);
+
+/*
+ * we should make the time progress anyway.
+ */
+lost_clock