[Qemu-devel] [PATCHv2] Add rtc reset function.

[Qemu-devel] [PATCHv2] Add rtc reset function.

[Gleb Natapov]

On reset:
Periodic Interrupt Enable (PIE) bit is cleared to zero
Alarm Interrupt Enable (AIE) bit is cleared to zero
Update ended Interrupt Flag (UF) bit is cleared to zero
Interrupt Request status Flag (IRQF) bit is cleared to zero
Periodic Interrupt Flag (PF) bit is cleared to zero
Alarm Interrupt Flag (AF) bit is cleared to zero
Square Wave output Enable (SQWE) zero

Signed-off-by: Gleb Natapov <gleb@redhat.com>
diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 888b85a..8c446e1 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -57,6 +57,11 @@
 #define REG_B_SQWE 0x08
 #define REG_B_DM   0x04
 
+#define REG_C_UF   0x10
+#define REG_C_IRQF 0x80
+#define REG_C_PF   0x40
+#define REG_C_AF   0x20
+
 struct RTCState {
     uint8_t cmos_data[128];
     uint8_t cmos_index;
@@ -568,6 +573,21 @@ static int rtc_load_td(QEMUFile *f, void *opaque, int version_id)
 }
 #endif
 
+static void rtc_reset(void *opaque)
+{
+    RTCState *s = opaque;
+
+    s->cmos_data[RTC_REG_B] &= ~(REG_B_PIE | REG_B_AIE | REG_B_SQWE);
+    s->cmos_data[RTC_REG_C] &= ~(REG_C_UF | REG_C_IRQF | REG_C_PF | REG_C_AF);
+
+    qemu_irq_lower(s->irq);
+
+#ifdef TARGET_I386
+    if (rtc_td_hack)
+	    s->irq_coalesced = 0;
+#endif
+}
+
 RTCState *rtc_init_sqw(int base, qemu_irq irq, qemu_irq sqw_irq, int base_year)
 {
     RTCState *s;
@@ -606,6 +626,8 @@ RTCState *rtc_init_sqw(int base, qemu_irq irq, qemu_irq sqw_irq, int base_year)
     if (rtc_td_hack)
         register_savevm("mc146818rtc-td", base, 1, rtc_save_td, rtc_load_td, s);
 #endif
+    qemu_register_reset(rtc_reset, 0, s);
+
     return s;
 }
 
@@ -721,5 +743,6 @@ RTCState *rtc_mm_init(target_phys_addr_t base, int it_shift, qemu_irq irq,
     if (rtc_td_hack)
         register_savevm("mc146818rtc-td", base, 1, rtc_save_td, rtc_load_td, s);
 #endif
+    qemu_register_reset(rtc_reset, 0, s);
     return s;
 }
--
			Gleb.

[Qemu-devel] Re: [PATCHv2] Add rtc reset function.

[Jan Kiszka]

Gleb Natapov wrote:
> On reset:
> Periodic Interrupt Enable (PIE) bit is cleared to zero
> Alarm Interrupt Enable (AIE) bit is cleared to zero
> Update ended Interrupt Flag (UF) bit is cleared to zero
> Interrupt Request status Flag (IRQF) bit is cleared to zero
> Periodic Interrupt Flag (PF) bit is cleared to zero
> Alarm Interrupt Flag (AF) bit is cleared to zero
> Square Wave output Enable (SQWE) zero
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index 888b85a..8c446e1 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -57,6 +57,11 @@
>  #define REG_B_SQWE 0x08
>  #define REG_B_DM   0x04
>  
> +#define REG_C_UF   0x10
> +#define REG_C_IRQF 0x80
> +#define REG_C_PF   0x40
> +#define REG_C_AF   0x20
> +
>  struct RTCState {
>      uint8_t cmos_data[128];
>      uint8_t cmos_index;
> @@ -568,6 +573,21 @@ static int rtc_load_td(QEMUFile *f, void *opaque, int version_id)
>  }
>  #endif
>  
> +static void rtc_reset(void *opaque)
> +{
> +    RTCState *s = opaque;
> +
> +    s->cmos_data[RTC_REG_B] &= ~(REG_B_PIE | REG_B_AIE | REG_B_SQWE);
> +    s->cmos_data[RTC_REG_C] &= ~(REG_C_UF | REG_C_IRQF | REG_C_PF | REG_C_AF);
> +
> +    qemu_irq_lower(s->irq);
> +
> +#ifdef TARGET_I386
> +    if (rtc_td_hack)
> +	    s->irq_coalesced = 0;

Sorry, missed that on first run: Isn't it safer, clearer, more beautiful
to reset this unconditionally?

Jan

[Qemu-devel] Re: [PATCHv2] Add rtc reset function.

[Gleb Natapov]

On Tue, Jun 09, 2009 at 12:23:21PM +0200, Jan Kiszka wrote:
> > +static void rtc_reset(void *opaque)
> > +{
> > +    RTCState *s = opaque;
> > +
> > +    s->cmos_data[RTC_REG_B] &= ~(REG_B_PIE | REG_B_AIE | REG_B_SQWE);
> > +    s->cmos_data[RTC_REG_C] &= ~(REG_C_UF | REG_C_IRQF | REG_C_PF | REG_C_AF);
> > +
> > +    qemu_irq_lower(s->irq);
> > +
> > +#ifdef TARGET_I386
> > +    if (rtc_td_hack)
> > +	    s->irq_coalesced = 0;
> 
> Sorry, missed that on first run: Isn't it safer, clearer, more beautiful
> to reset this unconditionally?
> 
It doesn't really matter. From safety point of view the code is safe.

--
			Gleb.

Re: [Qemu-devel] Re: [PATCHv2] Add rtc reset function.

[Dor Laor]

Gleb Natapov wrote:
> On Tue, Jun 09, 2009 at 12:23:21PM +0200, Jan Kiszka wrote:
>   
>>> +static void rtc_reset(void *opaque)
>>> +{
>>> +    RTCState *s = opaque;
>>> +
>>> +    s->cmos_data[RTC_REG_B] &= ~(REG_B_PIE | REG_B_AIE | REG_B_SQWE);
>>> +    s->cmos_data[RTC_REG_C] &= ~(REG_C_UF | REG_C_IRQF | REG_C_PF | REG_C_AF);
>>> +
>>> +    qemu_irq_lower(s->irq);
>>> +
>>> +#ifdef TARGET_I386
>>> +    if (rtc_td_hack)
>>> +	    s->irq_coalesced = 0;
>>>       
>> Sorry, missed that on first run: Isn't it safer, clearer, more beautiful
>> to reset this unconditionally?
>>
>>     
> It doesn't really matter. From safety point of view the code is safe.
>
>   
Actually we should drop this rtc-td-hack flag entirely.
The time drift fix is now only compiled for x86 so it will be harmless 
for other arch.
Users are affected by not calling it although it is a must for every OS 
that uses rtc clock source
and cannot deal with drifts (aka all windows, and some of the Linux guests).

Anthony, will you accept such a patch to remove rtc-td-hack or at least 
change it's default?
> --
> 			Gleb.
>
>
>   

Re: [Qemu-devel] [PATCHv2] Add rtc reset function.

[Marcelo Tosatti]

On Tue, Jun 09, 2009 at 12:26:05PM +0300, Gleb Natapov wrote:
> On reset:
> Periodic Interrupt Enable (PIE) bit is cleared to zero
> Alarm Interrupt Enable (AIE) bit is cleared to zero
> Update ended Interrupt Flag (UF) bit is cleared to zero
> Interrupt Request status Flag (IRQF) bit is cleared to zero
> Periodic Interrupt Flag (PF) bit is cleared to zero
> Alarm Interrupt Flag (AF) bit is cleared to zero
> Square Wave output Enable (SQWE) zero
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index 888b85a..8c446e1 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -57,6 +57,11 @@
>  #define REG_B_SQWE 0x08
>  #define REG_B_DM   0x04
>  
> +#define REG_C_UF   0x10
> +#define REG_C_IRQF 0x80
> +#define REG_C_PF   0x40
> +#define REG_C_AF   0x20
> +
>  struct RTCState {
>      uint8_t cmos_data[128];
>      uint8_t cmos_index;
> @@ -568,6 +573,21 @@ static int rtc_load_td(QEMUFile *f, void *opaque, int version_id)
>  }
>  #endif
>  
> +static void rtc_reset(void *opaque)
> +{
> +    RTCState *s = opaque;
> +
> +    s->cmos_data[RTC_REG_B] &= ~(REG_B_PIE | REG_B_AIE | REG_B_SQWE);
> +    s->cmos_data[RTC_REG_C] &= ~(REG_C_UF | REG_C_IRQF | REG_C_PF | REG_C_AF);
> +
> +    qemu_irq_lower(s->irq);
> +
> +#ifdef TARGET_I386
> +    if (rtc_td_hack)
> +	    s->irq_coalesced = 0;
> +#endif
> +}

Don't you need to cancel the timer before clearing cmos_data[]?

Re: [Qemu-devel] [PATCHv2] Add rtc reset function.

[Gleb Natapov]

On Tue, Jun 09, 2009 at 12:45:33PM -0300, Marcelo Tosatti wrote:
> On Tue, Jun 09, 2009 at 12:26:05PM +0300, Gleb Natapov wrote:
> > On reset:
> > Periodic Interrupt Enable (PIE) bit is cleared to zero
> > Alarm Interrupt Enable (AIE) bit is cleared to zero
> > Update ended Interrupt Flag (UF) bit is cleared to zero
> > Interrupt Request status Flag (IRQF) bit is cleared to zero
> > Periodic Interrupt Flag (PF) bit is cleared to zero
> > Alarm Interrupt Flag (AF) bit is cleared to zero
> > Square Wave output Enable (SQWE) zero
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> > index 888b85a..8c446e1 100644
> > --- a/hw/mc146818rtc.c
> > +++ b/hw/mc146818rtc.c
> > @@ -57,6 +57,11 @@
> >  #define REG_B_SQWE 0x08
> >  #define REG_B_DM   0x04
> >  
> > +#define REG_C_UF   0x10
> > +#define REG_C_IRQF 0x80
> > +#define REG_C_PF   0x40
> > +#define REG_C_AF   0x20
> > +
> >  struct RTCState {
> >      uint8_t cmos_data[128];
> >      uint8_t cmos_index;
> > @@ -568,6 +573,21 @@ static int rtc_load_td(QEMUFile *f, void *opaque, int version_id)
> >  }
> >  #endif
> >  
> > +static void rtc_reset(void *opaque)
> > +{
> > +    RTCState *s = opaque;
> > +
> > +    s->cmos_data[RTC_REG_B] &= ~(REG_B_PIE | REG_B_AIE | REG_B_SQWE);
> > +    s->cmos_data[RTC_REG_C] &= ~(REG_C_UF | REG_C_IRQF | REG_C_PF | REG_C_AF);
> > +
> > +    qemu_irq_lower(s->irq);
> > +
> > +#ifdef TARGET_I386
> > +    if (rtc_td_hack)
> > +	    s->irq_coalesced = 0;
> > +#endif
> > +}
> 
> Don't you need to cancel the timer before clearing cmos_data[]?
I believe when this callback runs guest is already stopped, so there is
not difference.

--
			Gleb.