[Qemu-devel] [PATCH V3] rtc: fix a infinite loop in windows vm startup

[Qemu-devel] [PATCH V3] rtc: fix a infinite loop in windows vm startup

[Peng Hao]

When a windows vm starts, periodic timer of rtc will stop several times.
windows kernel will check whether REG_A_UIP is changed. REG_C's interrupt
flags will not be cleared when periodic timer stops and the update timer
will switch to alarm timer. So the expiration time of alarm timer is very
long and REG_A_UIP will not vary.At last windows kernel will repeat to 
check REG_A_UIP all the time.

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
Signed-off-by: Liu Yi <liu.yi24@zte.com.cn>
---
 hw/timer/mc146818rtc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 1b8d3d7..aa55fae 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -457,6 +457,8 @@ static void rtc_update_timer(void *opaque)
     if ((new_irqs & s->cmos_data[RTC_REG_B]) != 0) {
         s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
         qemu_irq_raise(s->irq);
+    } else if ((s->cmos_data[RTC_REG_B] & REG_B_UIE) == 0) {
+        s->cmos_data[RTC_REG_C] &= ~REG_C_UF;
     }
     check_update_timer(s);
 }
@@ -559,7 +561,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
                 s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
                 qemu_irq_raise(s->irq);
             } else {
-                s->cmos_data[RTC_REG_C] &= ~REG_C_IRQF;
+                s->cmos_data[RTC_REG_C] &= ~(REG_C_UF | REG_C_IRQF);
                 qemu_irq_lower(s->irq);
             }
             s->cmos_data[RTC_REG_B] = data;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH V3] rtc: fix a infinite loop in windows vm startup

[Paolo Bonzini]

On 24/07/2017 20:35, Peng Hao wrote:
> When a windows vm starts, periodic timer of rtc will stop several times.
> windows kernel will check whether REG_A_UIP is changed. REG_C's interrupt
> flags will not be cleared when periodic timer stops and the update timer
> will switch to alarm timer. So the expiration time of alarm timer is very
> long and REG_A_UIP will not vary.At last windows kernel will repeat to 
> check REG_A_UIP all the time.

This should not happen.  REG_A_UIP is set and cleared in register A
every second, like this:

        case RTC_REG_A:
            if (update_in_progress(s)) {
                s->cmos_data[s->cmos_index] |= REG_A_UIP;
            } else {
                s->cmos_data[s->cmos_index] &= ~REG_A_UIP;
            }
            ret = s->cmos_data[s->cmos_index];
            break;

where update_in_progress does:

    guest_nsec = get_guest_rtc_ns(s);
    /* UIP bit will be set at last 244us of every second. */
    if ((guest_nsec % NANOSECONDS_PER_SECOND) >=
        (NANOSECONDS_PER_SECOND - UIP_HOLD_LENGTH)) {
        return 1;
    }
    return 0;

This is done even if the timer is not pending.

How can the bug be reproduced?

Paolo

> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> Signed-off-by: Liu Yi <liu.yi24@zte.com.cn>
> ---
>  hw/timer/mc146818rtc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 1b8d3d7..aa55fae 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -457,6 +457,8 @@ static void rtc_update_timer(void *opaque)
>      if ((new_irqs & s->cmos_data[RTC_REG_B]) != 0) {
>          s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
>          qemu_irq_raise(s->irq);
> +    } else if ((s->cmos_data[RTC_REG_B] & REG_B_UIE) == 0) {
> +        s->cmos_data[RTC_REG_C] &= ~REG_C_UF;
>      }
>      check_update_timer(s);
>  }
> @@ -559,7 +561,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>                  s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
>                  qemu_irq_raise(s->irq);
>              } else {
> -                s->cmos_data[RTC_REG_C] &= ~REG_C_IRQF;
> +                s->cmos_data[RTC_REG_C] &= ~(REG_C_UF | REG_C_IRQF);
>                  qemu_irq_lower(s->irq);
>              }
>              s->cmos_data[RTC_REG_B] = data;
> 

Re: [Qemu-devel] [PATCH V3] rtc: fix a infinite loop in windows vm startup

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

On 07/24/2017 01:35 PM, Peng Hao wrote:
> When a windows vm starts, periodic timer of rtc will stop several times.
> windows kernel will check whether REG_A_UIP is changed. REG_C's interrupt
> flags will not be cleared when periodic timer stops and the update timer
> will switch to alarm timer. So the expiration time of alarm timer is very
> long and REG_A_UIP will not vary.At last windows kernel will repeat to 
> check REG_A_UIP all the time.
> 
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> Signed-off-by: Liu Yi <liu.yi24@zte.com.cn>
> ---

When posting a v3, it's useful to tell us (after the --- separator) what
changed from v2, to help us focus on why a v3 was needed.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH V3] rtc: fix a infinite loop in windows vm startup

[peng.hao2]

>On 24/07/2017 20:35, Peng Hao wrote:





>> When a windows vm starts, periodic timer of rtc will stop several times.
>> windows kernel will check whether REG_A_UIP is changed. REG_C's interrupt
>> flags will not be cleared when periodic timer stops and the update timer
>> will switch to alarm timer. So the expiration time of alarm timer is very
>> long and REG_A_UIP will not vary.At last windows kernel will repeat to 
>> check REG_A_UIP all the time.

>This should not happen.  REG_A_UIP is set and cleared in register A
>every second, like this:
>        case RTC_REG_A:

>            if (update_in_progress(s)) {
>                s->cmos_data[s->cmos_index] |= REG_A_UIP
>            } else {
>                s->cmos_data[s->cmos_index] &= ~REG_A_UIP
>            }
>            ret = s->cmos_data[s->cmos_index]
>            break



when periodic timer stop, update timer is set to a long expire time (as alarm timer).

So update_in_progress always return 1 and REG_A_UIP is never  cleared.

>where update_in_progress does:

>    guest_nsec = get_guest_rtc_ns(s)
>    /* UIP bit will be set at last 244us of every second. */
>    if ((guest_nsec % NANOSECONDS_PER_SECOND) >=
>        (NANOSECONDS_PER_SECOND - UIP_HOLD_LENGTH)) {
>        return 1
>    }
>    return 0

>This is done even if the timer is not pending.

>How can the bug be reproduced?

many windows VMs reboot at the same time.some VM's vcpu thread go to a infinite loop 

at the starting stage.





>Paolo

> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> Signed-off-by: Liu Yi <liu.yi24@zte.com.cn>
> ---
>  hw/timer/mc146818rtc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 1b8d3d7..aa55fae 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -457,6 +457,8 @@ static void rtc_update_timer(void *opaque)
>      if ((new_irqs & s->cmos_data[RTC_REG_B]) != 0) {
>          s->cmos_data[RTC_REG_C] |= REG_C_IRQF
>          qemu_irq_raise(s->irq)
> +    } else if ((s->cmos_data[RTC_REG_B] & REG_B_UIE) == 0) {
> +        s->cmos_data[RTC_REG_C] &= ~REG_C_UF
>      }
>      check_update_timer(s)
>  }
> @@ -559,7 +561,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>                  s->cmos_data[RTC_REG_C] |= REG_C_IRQF
>                  qemu_irq_raise(s->irq)
>              } else {
> -                s->cmos_data[RTC_REG_C] &= ~REG_C_IRQF
> +                s->cmos_data[RTC_REG_C] &= ~(REG_C_UF | REG_C_IRQF)
>                  qemu_irq_lower(s->irq)
>              }
>              s->cmos_data[RTC_REG_B] = data
>

Re: [Qemu-devel] [PATCH V3] rtc: fix a infinite loop in windows vm startup

[Paolo Bonzini]

On 25/07/2017 06:14, peng.hao2@zte.com.cn wrote:
>> On 24/07/2017 20:35, Peng Hao wrote:
> 
> 
> 
> 
> 
>>> When a windows vm starts, periodic timer of rtc will stop several times.
>>> windows kernel will check whether REG_A_UIP is changed. REG_C's interrupt
>>> flags will not be cleared when periodic timer stops and the update timer
>>> will switch to alarm timer. So the expiration time of alarm timer is very
>>> long and REG_A_UIP will not vary.At last windows kernel will repeat to 
>>> check REG_A_UIP all the time.
> 
>> This should not happen.  REG_A_UIP is set and cleared in register A
>> every second, like this:
>>        case RTC_REG_A:
> 
>>            if (update_in_progress(s)) {
>>                s->cmos_data[s->cmos_index] |= REG_A_UIP
>>            } else {
>>                s->cmos_data[s->cmos_index] &= ~REG_A_UIP
>>            }
>>            ret = s->cmos_data[s->cmos_index]
>>            break
> 
> 
> 
> when periodic timer stop, update timer is set to a long expire time (as alarm timer).

I think I see the bug now:

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 1b8d3d7d4c..6184b4378e 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -321,9 +321,11 @@ static void check_update_timer(RTCState *s)
     s->next_alarm_time = next_update_time +
                          (next_alarm_sec - 1) * NANOSECONDS_PER_SECOND;
 
-    if (s->cmos_data[RTC_REG_C] & REG_C_UF) {
-        /* UF is set, but AF is clear.  Program the timer to target
-         * the alarm time.  */
+    if ((s->cmos_data[RTC_REG_C] & REG_C_UF) &&
+        !(s->cmos_data[RTC_REG_A] & REG_A_UIP)) {
+        /* If UIP was latched, we need to clear it at the next update.
+         * Otherwise, if UF is set we only need to program the timer to
+         * target the alarm time.  */
         next_update_time = s->next_alarm_time;
     }
     if (next_update_time != timer_expire_time_ns(s->update_timer)) {


but I would like to have a testcase for it in tests/rtc-test.c.

Can you check if the above works and try writing a testcase (that fails 
without the patch and succeeds with it)?

Thanks,

Paolo