[Qemu-devel] [PATCH 0/4] Fixes to the rtc device model

[Qemu-devel] [PATCH 0/4] Fixes to the rtc device model

[Paolo Bonzini]

Here are a bunch of fixes for the rtc that I found while reading
the code.  Might be for 1.0 or 1.1, at your choice.

I also have a fix for at least one relatively important bug: when writing
register B unmasks a pending interrupt, the IRQ line should be raised.
However, the fix is a bit invasive so I am not posting it yet.

Paolo Bonzini (4):
  rtc: fix 12-hour mode
  rtc: raise AF bit when the alarm is encountered but AIE=0
  rtc: raise PF bit when the periodic timer triggers but PIE=0
  rtc: clear non-PF bits when reinjecting on ack

 hw/mc146818rtc.c |   41 +++++++++++++++++++++++------------------
 1 files changed, 23 insertions(+), 18 deletions(-)

-- 
1.7.7.1

[Qemu-devel] [PATCH 1/4] rtc: fix 12-hour mode

[Paolo Bonzini]

Hours in 12-hour mode are in the 1-12 range, not 0-11.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/mc146818rtc.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 2aaca2f..14c8cb9 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -296,9 +296,11 @@ static void rtc_set_time(RTCState *s)
     tm->tm_sec = rtc_from_bcd(s, s->cmos_data[RTC_SECONDS]);
     tm->tm_min = rtc_from_bcd(s, s->cmos_data[RTC_MINUTES]);
     tm->tm_hour = rtc_from_bcd(s, s->cmos_data[RTC_HOURS] & 0x7f);
-    if (!(s->cmos_data[RTC_REG_B] & REG_B_24H) &&
-        (s->cmos_data[RTC_HOURS] & 0x80)) {
-        tm->tm_hour += 12;
+    if (!(s->cmos_data[RTC_REG_B] & REG_B_24H)) {
+        tm->tm_hour %= 12;
+        if (s->cmos_data[RTC_HOURS] & 0x80) {
+            tm->tm_hour += 12;
+        }
     }
     tm->tm_wday = rtc_from_bcd(s, s->cmos_data[RTC_DAY_OF_WEEK]) - 1;
     tm->tm_mday = rtc_from_bcd(s, s->cmos_data[RTC_DAY_OF_MONTH]);
@@ -320,7 +324,8 @@ static void rtc_copy_date(RTCState *s)
         s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour);
     } else {
         /* 12 hour format */
-        s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour % 12);
+        int h = (tm->tm_hour % 12) ? tm->tm_hour % 12 : 12;
+        s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, h);
         if (tm->tm_hour >= 12)
             s->cmos_data[RTC_HOURS] |= 0x80;
     }
-- 
1.7.7.1

Re: [Qemu-devel] [PATCH 1/4] rtc: fix 12-hour mode

[Mark Wu]

On 11/22/2011 02:00 AM, Paolo Bonzini wrote:
> Hours in 12-hour mode are in the 1-12 range, not 0-11.
Interesting. I would like to know how you could find this problem. It 
seems linux driver never changes the format  and 24-hour is default in 
rtc emulation code. So how did it expose and how to test it?

Thanks.
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   hw/mc146818rtc.c |   11 +++++++----
>   1 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index 2aaca2f..14c8cb9 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -296,9 +296,11 @@ static void rtc_set_time(RTCState *s)
>       tm->tm_sec = rtc_from_bcd(s, s->cmos_data[RTC_SECONDS]);
>       tm->tm_min = rtc_from_bcd(s, s->cmos_data[RTC_MINUTES]);
>       tm->tm_hour = rtc_from_bcd(s, s->cmos_data[RTC_HOURS]&  0x7f);
> -    if (!(s->cmos_data[RTC_REG_B]&  REG_B_24H)&&
> -        (s->cmos_data[RTC_HOURS]&  0x80)) {
> -        tm->tm_hour += 12;
> +    if (!(s->cmos_data[RTC_REG_B]&  REG_B_24H)) {
> +        tm->tm_hour %= 12;
> +        if (s->cmos_data[RTC_HOURS]&  0x80) {
> +            tm->tm_hour += 12;
> +        }
>       }
>       tm->tm_wday = rtc_from_bcd(s, s->cmos_data[RTC_DAY_OF_WEEK]) - 1;
>       tm->tm_mday = rtc_from_bcd(s, s->cmos_data[RTC_DAY_OF_MONTH]);
> @@ -320,7 +324,8 @@ static void rtc_copy_date(RTCState *s)
>           s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour);
>       } else {
>           /* 12 hour format */
> -        s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour % 12);
> +        int h = (tm->tm_hour % 12) ? tm->tm_hour % 12 : 12;
> +        s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, h);
>           if (tm->tm_hour>= 12)
>               s->cmos_data[RTC_HOURS] |= 0x80;
>       }

Re: [Qemu-devel] [PATCH 1/4] rtc: fix 12-hour mode

[Paolo Bonzini]

On 11/22/2011 07:39 AM, Mark Wu wrote:
>> Hours in 12-hour mode are in the 1-12 range, not 0-11.
> Interesting. I would like to know how you could find this problem. It
> seems linux driver never changes the format  and 24-hour is default in
> rtc emulation code. So how did it expose and how to test it?

It is exposed by reading the datasheet. :)

Honestly, to test it I just added two for loops, I didn't test it on a 
real VM.  But you could do all these tests using a DOS program for example.

Paolo

Re: [Qemu-devel] [PATCH 1/4] rtc: fix 12-hour mode

[Avi Kivity]

On 11/21/2011 08:00 PM, Paolo Bonzini wrote:
> Hours in 12-hour mode are in the 1-12 range, not 0-11.
>
> @@ -320,7 +324,8 @@ static void rtc_copy_date(RTCState *s)
>          s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour);
>      } else {
>          /* 12 hour format */
> -        s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour % 12);
> +        int h = (tm->tm_hour % 12) ? tm->tm_hour % 12 : 12;
> +        s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, h);
>          if (tm->tm_hour >= 12)
>              s->cmos_data[RTC_HOURS] |= 0x80;
>      }

Nitpick, don't update patch on this account:

I dislike seeing int-to-bool conversion on anything that is not a
true/false or count value.  Things like if (has_some_property) or if
(!nr_items) read well and are easily understood.  But here, you're not
checking for "are there any tm_hours, if yes, use them, if not, use
12".  You're testing against the value 0 which has a special encoding in
12 hour mode.

The is usually manifested in

   if (!strcmp(a, b)) ...

strcmp() does not return a bool or a count, and in fact it reads exactly
the opposite of the intent: "if not string compare".  strcmp() returns
an enumeration, or perhaps a mapping of string trichotomy to integer
trichotomy.

Sorry about the pontification, back to the regularly scheduled
whitespace discussion.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1/4] rtc: fix 12-hour mode

[Paolo Bonzini]

On 01/01/2012 03:53 PM, Avi Kivity wrote:
> On 11/21/2011 08:00 PM, Paolo Bonzini wrote:
>> Hours in 12-hour mode are in the 1-12 range, not 0-11.
>>
>> @@ -320,7 +324,8 @@ static void rtc_copy_date(RTCState *s)
>>           s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour);
>>       } else {
>>           /* 12 hour format */
>> -        s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour % 12);
>> +        int h = (tm->tm_hour % 12) ? tm->tm_hour % 12 : 12;
>> +        s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, h);
>>           if (tm->tm_hour>= 12)
>>               s->cmos_data[RTC_HOURS] |= 0x80;
>>       }
>
> Nitpick, don't update patch on this account:
>
> I dislike seeing int-to-bool conversion on anything that is not a
> true/false or count value.  Things like if (has_some_property) or if
> (!nr_items) read well and are easily understood.  But here, you're not
> checking for "are there any tm_hours, if yes, use them, if not, use
> 12".  You're testing against the value 0 which has a special encoding in
> 12 hour mode.
>
> The is usually manifested in
>
>     if (!strcmp(a, b)) ...
>
> strcmp() does not return a bool or a count, and in fact it reads exactly
> the opposite of the intent: "if not string compare".  strcmp() returns
> an enumeration, or perhaps a mapping of string trichotomy to integer
> trichotomy.
>
> Sorry about the pontification, back to the regularly scheduled
> whitespace discussion.

Yeah, I probably would have remarked the same if it was someone else's 
patch. :)

BTW, I have kvm-unit-tests tests for these and other RTC emulation 
problems.  Long term perhaps they should become qtests, but for now 
kvm-unit-tests is the best place.  However, without these four patches 
they hang which is a bit too heavy.  So I'll submit as soon as these are in.

Paolo

[Qemu-devel] [PATCH 2/4] rtc: raise AF bit when the alarm is encountered but AIE=0

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/mc146818rtc.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 14c8cb9..a5c533b 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -427,16 +427,17 @@ static void rtc_update_second2(void *opaque)
     }
 
     /* check alarm */
-    if (s->cmos_data[RTC_REG_B] & REG_B_AIE) {
-        if (((s->cmos_data[RTC_SECONDS_ALARM] & 0xc0) == 0xc0 ||
-             rtc_from_bcd(s, s->cmos_data[RTC_SECONDS_ALARM]) == s->current_tm.tm_sec) &&
-            ((s->cmos_data[RTC_MINUTES_ALARM] & 0xc0) == 0xc0 ||
-             rtc_from_bcd(s, s->cmos_data[RTC_MINUTES_ALARM]) == s->current_tm.tm_min) &&
-            ((s->cmos_data[RTC_HOURS_ALARM] & 0xc0) == 0xc0 ||
-             rtc_from_bcd(s, s->cmos_data[RTC_HOURS_ALARM]) == s->current_tm.tm_hour)) {
-
-            s->cmos_data[RTC_REG_C] |= 0xa0;
+    if (((s->cmos_data[RTC_SECONDS_ALARM] & 0xc0) == 0xc0 ||
+         rtc_from_bcd(s, s->cmos_data[RTC_SECONDS_ALARM]) == s->current_tm.tm_sec) &&
+        ((s->cmos_data[RTC_MINUTES_ALARM] & 0xc0) == 0xc0 ||
+         rtc_from_bcd(s, s->cmos_data[RTC_MINUTES_ALARM]) == s->current_tm.tm_min) &&
+        ((s->cmos_data[RTC_HOURS_ALARM] & 0xc0) == 0xc0 ||
+         rtc_from_bcd(s, s->cmos_data[RTC_HOURS_ALARM]) == s->current_tm.tm_hour)) {
+
+        s->cmos_data[RTC_REG_C] |= REG_C_AF;
+        if (s->cmos_data[RTC_REG_B] & REG_B_AIE) {
             qemu_irq_raise(s->irq);
+            s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
         }
     }
 
-- 
1.7.7.1

[Qemu-devel] [PATCH 3/4] rtc: raise PF bit when the periodic timer triggers but PIE=0

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/mc146818rtc.c |    4 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index a5c533b..b942ce2 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -179,8 +179,9 @@ static void rtc_periodic_timer(void *opaque)
     RTCState *s = opaque;
 
     rtc_timer_update(s, s->next_periodic_time);
+    s->cmos_data[RTC_REG_C] |= REG_C_PF;
     if (s->cmos_data[RTC_REG_B] & REG_B_PIE) {
-        s->cmos_data[RTC_REG_C] |= 0xc0;
+        s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
 #ifdef TARGET_I386
         if(rtc_td_hack) {
             if (s->irq_reinject_on_ack_count >= RTC_REINJECT_ON_ACK_COUNT)
-- 
1.7.7.1

[Qemu-devel] [PATCH 4/4] rtc: clear non-PF bits when reinjecting on ack

[Paolo Bonzini]

When an rtc interrupt is reinjected immediately after being acked,
other interrupts should not be reinjected, so do clear their bits.

Also, if the periodic interrupts have been disabled before acking,
do not reinject, as the guest might get very confused!

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/mc146818rtc.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index b942ce2..5d17cca 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -479,10 +479,13 @@ static uint32_t cmos_ioport_read(void *opaque, uint32_t addr)
         case RTC_REG_C:
             ret = s->cmos_data[s->cmos_index];
             qemu_irq_lower(s->irq);
+            s->cmos_data[RTC_REG_C] = 0x00;
 #ifdef TARGET_I386
             if(s->irq_coalesced &&
+                    (s->cmos_data[RTC_REG_B] & REG_B_PIE) &&
                     s->irq_reinject_on_ack_count < RTC_REINJECT_ON_ACK_COUNT) {
                 s->irq_reinject_on_ack_count++;
+                s->cmos_data[RTC_REG_C] |= REG_C_IRQF | REG_C_PF;
                 apic_reset_irq_delivered();
                 DPRINTF_C("cmos: injecting on ack\n");
                 qemu_irq_raise(s->irq);
@@ -491,11 +494,8 @@ static uint32_t cmos_ioport_read(void *opaque, uint32_t addr)
                     DPRINTF_C("cmos: coalesced irqs decreased to %d\n",
                               s->irq_coalesced);
                 }
-                break;
             }
 #endif
-
-            s->cmos_data[RTC_REG_C] = 0x00;
             break;
         default:
             ret = s->cmos_data[s->cmos_index];
-- 
1.7.7.1

Re: [Qemu-devel] [PATCH 0/4] Fixes to the rtc device model

[Paolo Bonzini]

On 11/21/2011 07:00 PM, Paolo Bonzini wrote:
> Here are a bunch of fixes for the rtc that I found while reading
> the code.  Might be for 1.0 or 1.1, at your choice.
>
> I also have a fix for at least one relatively important bug: when writing
> register B unmasks a pending interrupt, the IRQ line should be raised.
> However, the fix is a bit invasive so I am not posting it yet.
>
> Paolo Bonzini (4):
>    rtc: fix 12-hour mode
>    rtc: raise AF bit when the alarm is encountered but AIE=0
>    rtc: raise PF bit when the periodic timer triggers but PIE=0
>    rtc: clear non-PF bits when reinjecting on ack
>
>   hw/mc146818rtc.c |   41 +++++++++++++++++++++++------------------
>   1 files changed, 23 insertions(+), 18 deletions(-)
>

Ping.

Paolo

Re: [Qemu-devel] [PATCH 0/4] Fixes to the rtc device model

[Paolo Bonzini]

On 12/30/2011 04:58 PM, Paolo Bonzini wrote:
> On 11/21/2011 07:00 PM, Paolo Bonzini wrote:
>> Here are a bunch of fixes for the rtc that I found while reading
>> the code. Might be for 1.0 or 1.1, at your choice.
>>
>> I also have a fix for at least one relatively important bug: when writing
>> register B unmasks a pending interrupt, the IRQ line should be raised.
>> However, the fix is a bit invasive so I am not posting it yet.
>>
>> Paolo Bonzini (4):
>> rtc: fix 12-hour mode
>> rtc: raise AF bit when the alarm is encountered but AIE=0
>> rtc: raise PF bit when the periodic timer triggers but PIE=0
>> rtc: clear non-PF bits when reinjecting on ack
>>
>> hw/mc146818rtc.c | 41 +++++++++++++++++++++++------------------
>> 1 files changed, 23 insertions(+), 18 deletions(-)
>>
>
> Ping.

Ping^2

Paolo

Re: [Qemu-devel] [PATCH 0/4] Fixes to the rtc device model

[Anthony Liguori]

On 11/21/2011 12:00 PM, Paolo Bonzini wrote:
> Here are a bunch of fixes for the rtc that I found while reading
> the code.  Might be for 1.0 or 1.1, at your choice.
>
> I also have a fix for at least one relatively important bug: when writing
> register B unmasks a pending interrupt, the IRQ line should be raised.
> However, the fix is a bit invasive so I am not posting it yet.

Applied.  Thanks.

Regards,

Anthony Liguori

>
> Paolo Bonzini (4):
>    rtc: fix 12-hour mode
>    rtc: raise AF bit when the alarm is encountered but AIE=0
>    rtc: raise PF bit when the periodic timer triggers but PIE=0
>    rtc: clear non-PF bits when reinjecting on ack
>
>   hw/mc146818rtc.c |   41 +++++++++++++++++++++++------------------
>   1 files changed, 23 insertions(+), 18 deletions(-)
>