[Qemu-devel] [Bug] Timer bugs in hw/m48t59.c?

[Qemu-devel] [Bug] Timer bugs in hw/m48t59.c?

[Stefan Weil]

Hi,

could SPARC or PPC users please check whether the timer code
in hw/m48t59.c is really correct?

I expect a crash in qemu_mod_timer after wd_timer = NULL and
a call to qemu_mod_timer with this NULL value.

The same applies to alrm_timer.

I wrote a quick-and-dirty patch, but think that even more
old code could be removed.

Stefan



diff -u -b -B -r1.8 m48t59.c
--- hw/m48t59.c 14 Jun 2006 12:41:34 -0000      1.8
+++ hw/m48t59.c 22 Mar 2007 20:29:15 -0000
@@ -155,7 +155,6 @@
     NVRAM->alarm = mktime(tm);
     if (NVRAM->alrm_timer != NULL) {
         qemu_del_timer(NVRAM->alrm_timer);
-       NVRAM->alrm_timer = NULL;
     }
     if (NVRAM->alarm - time(NULL) > 0)
        qemu_mod_timer(NVRAM->alrm_timer, NVRAM->alarm * 1000);
@@ -184,7 +183,6 @@

     if (NVRAM->wd_timer != NULL) {
         qemu_del_timer(NVRAM->wd_timer);
-       NVRAM->wd_timer = NULL;
     }
     NVRAM->buffer[0x1FF0] &= ~0x80;
     if (value != 0) {

RE: [Qemu-devel] [Bug] Timer bugs in hw/m48t59.c?

[Blue Swirl]

>could SPARC or PPC users please check whether the timer code
>in hw/m48t59.c is really correct?
>
>I expect a crash in qemu_mod_timer after wd_timer = NULL and
>a call to qemu_mod_timer with this NULL value.

Looking inside qemu_mod_timer shows why that's no surprise, so your patch 
should be correct. But the timer isn't available on Sparc32, so can't 
comment otherwise.

_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today it's FREE! 
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/

Re: [Qemu-devel] [Bug] Timer bugs in hw/m48t59.c?

[Stefan Weil]

Hello,

the bug mentioned in my previous mail is still open.
Could someone please add the patch to CVS HEAD
(or find another solution)?

Thank you
Stefan

Stefan Weil schrieb:
> Hi,
>
> could SPARC or PPC users please check whether the timer code
> in hw/m48t59.c is really correct?
>
> I expect a crash in qemu_mod_timer after wd_timer = NULL and
> a call to qemu_mod_timer with this NULL value.
>
> The same applies to alrm_timer.
>
> I wrote a quick-and-dirty patch, but think that even more
> old code could be removed.
>
> Stefan
>
>
>
> diff -u -b -B -r1.8 m48t59.c
> --- hw/m48t59.c 14 Jun 2006 12:41:34 -0000 1.8
> +++ hw/m48t59.c 22 Mar 2007 20:29:15 -0000
> @@ -155,7 +155,6 @@
> NVRAM->alarm = mktime(tm);
> if (NVRAM->alrm_timer != NULL) {
> qemu_del_timer(NVRAM->alrm_timer);
> - NVRAM->alrm_timer = NULL;
> }
> if (NVRAM->alarm - time(NULL) > 0)
> qemu_mod_timer(NVRAM->alrm_timer, NVRAM->alarm * 1000);
> @@ -184,7 +183,6 @@
>
> if (NVRAM->wd_timer != NULL) {
> qemu_del_timer(NVRAM->wd_timer);
> - NVRAM->wd_timer = NULL;
> }
> NVRAM->buffer[0x1FF0] &= ~0x80;
> if (value != 0) {
>
>
>
> _______________________________________________
> Qemu-devel mailing list
> Qemu-devel@nongnu.org
> http://lists.nongnu.org/mailman/listinfo/qemu-devel
>

Re: [Qemu-devel] [Bug] Timer bugs in hw/m48t59.c?

[J. Mayer]

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

On Mon, 2007-09-24 at 19:24 +0200, Stefan Weil wrote:
> Hello,

Hi,

> the bug mentioned in my previous mail is still open.
> Could someone please add the patch to CVS HEAD
> (or find another solution)?

Seems you're right, this code would lead to crashes. I think there are
other issues to address in the same code because alrm_timer and wd_timer
are always NULL if the RTC model is a m48t08.
Could you please take a look at this updated patch ?

Regards.

> 
> Thank you
> Stefan
> 
> Stefan Weil schrieb:
> > Hi,
> >
> > could SPARC or PPC users please check whether the timer code
> > in hw/m48t59.c is really correct?
> >
> > I expect a crash in qemu_mod_timer after wd_timer = NULL and
> > a call to qemu_mod_timer with this NULL value.
> >
> > The same applies to alrm_timer.
> >
> > I wrote a quick-and-dirty patch, but think that even more
> > old code could be removed.
> >
> > Stefan
> >
[...]

-- 
J. Mayer <l_indien@magic.fr>
Never organized

[-- Attachment #2: m48t59.c.diff --]
[-- Type: text/x-patch, Size: 1483 bytes --]

Index: hw/m48t59.c
===================================================================
RCS file: /sources/qemu/qemu/hw/m48t59.c,v
retrieving revision 1.14
diff -u -d -d -p -r1.14 m48t59.c
--- hw/m48t59.c	17 Sep 2007 08:09:47 -0000	1.14
+++ hw/m48t59.c	24 Sep 2007 20:24:01 -0000
@@ -161,10 +161,9 @@ static void set_alarm (m48t59_t *NVRAM, 
     NVRAM->alarm = mktime(tm);
     if (NVRAM->alrm_timer != NULL) {
         qemu_del_timer(NVRAM->alrm_timer);
-	NVRAM->alrm_timer = NULL;
+        if (NVRAM->alarm - time(NULL) > 0)
+            qemu_mod_timer(NVRAM->alrm_timer, NVRAM->alarm * 1000);
     }
-    if (NVRAM->alarm - time(NULL) > 0)
-	qemu_mod_timer(NVRAM->alrm_timer, NVRAM->alarm * 1000);
 }
 
 /* Watchdog management */
@@ -188,15 +187,14 @@ static void set_up_watchdog (m48t59_t *N
 {
     uint64_t interval; /* in 1/16 seconds */
 
+    NVRAM->buffer[0x1FF0] &= ~0x80;
     if (NVRAM->wd_timer != NULL) {
         qemu_del_timer(NVRAM->wd_timer);
-	NVRAM->wd_timer = NULL;
-    }
-    NVRAM->buffer[0x1FF0] &= ~0x80;
-    if (value != 0) {
-	interval = (1 << (2 * (value & 0x03))) * ((value >> 2) & 0x1F);
-	qemu_mod_timer(NVRAM->wd_timer, ((uint64_t)time(NULL) * 1000) +
-		       ((interval * 1000) >> 4));
+        if (value != 0) {
+            interval = (1 << (2 * (value & 0x03))) * ((value >> 2) & 0x1F);
+            qemu_mod_timer(NVRAM->wd_timer, ((uint64_t)time(NULL) * 1000) +
+                           ((interval * 1000) >> 4));
+        }
     }
 }