Patchwork [1/4] xen: introduce mc146818rtcxen

login
register
mail settings
Submitter Stefano Stabellini
Date Nov. 15, 2011, 4:57 p.m.
Message ID <alpine.DEB.2.00.1111151457450.3519@kaball-desktop>
Download mbox | patch
Permalink /patch/125822/
State New
Headers show

Comments

Stefano Stabellini - Nov. 15, 2011, 4:57 p.m.
On Tue, 15 Nov 2011, Anthony Liguori wrote:
> On 11/15/2011 08:51 AM, stefano.stabellini@eu.citrix.com wrote:
> > From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> >
> > Xen doesn't need full RTC emulation in Qemu because the RTC is already
> > emulated by the hypervisor. In particular we want to avoid the timers
> > initialization so that Qemu doesn't need to wake up needlessly.
> >
> > Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> 
> Yuck.  There's got to be a better way to do this.

Yeah, it is pretty ugly, I was hoping in some good suggestions to
improve this patch :)


> I think it would be better to name timers and then in Xen specific machine code, 
> disable the RTC timers.

Good idea!
I was thinking that I could implement an rtc_stop function in
mc146818rtc.c that stops and frees the timers.

Now the problem is that from xen-all.c I cannot easily find the
ISADevice instance to pass to rtc_stop. Do you think it would be
reasonable to call rtc_stop from pc_basic_device_init, inside the same
if (!xen_available()) introduce by the next patch?

Otherwise I could implement functions to walk the isa bus, similarly to
pci_for_each_device.


This is just an example:
Stefano Stabellini - Nov. 18, 2011, 11:46 a.m.
On Tue, 15 Nov 2011, Stefano Stabellini wrote:
> On Tue, 15 Nov 2011, Anthony Liguori wrote:
> > On 11/15/2011 08:51 AM, stefano.stabellini@eu.citrix.com wrote:
> > > From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> > >
> > > Xen doesn't need full RTC emulation in Qemu because the RTC is already
> > > emulated by the hypervisor. In particular we want to avoid the timers
> > > initialization so that Qemu doesn't need to wake up needlessly.
> > >
> > > Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> > 
> > Yuck.  There's got to be a better way to do this.
> 
> Yeah, it is pretty ugly, I was hoping in some good suggestions to
> improve this patch :)
> 
> 
> > I think it would be better to name timers and then in Xen specific machine code, 
> > disable the RTC timers.
> 
> Good idea!
> I was thinking that I could implement an rtc_stop function in
> mc146818rtc.c that stops and frees the timers.
> 
> Now the problem is that from xen-all.c I cannot easily find the
> ISADevice instance to pass to rtc_stop. Do you think it would be
> reasonable to call rtc_stop from pc_basic_device_init, inside the same
> if (!xen_available()) introduce by the next patch?
> 
> Otherwise I could implement functions to walk the isa bus, similarly to
> pci_for_each_device.
> 

ping?


> This is just an example:
> 
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index 2aaca2f..568c540 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -667,6 +667,28 @@ ISADevice *rtc_init(int base_year, qemu_irq intercept_irq)
>      return dev;
>  }
>  
> +void rtc_stop(ISADevice *dev)
> +{
> +    RTCState *s = DO_UPCAST(RTCState, dev, dev);
> +
> +    qemu_del_timer(s->periodic_timer);
> +    qemu_del_timer(s->second_timer);
> +    qemu_del_timer(s->second_timer2);
> +#ifdef TARGET_I386
> +    if (rtc_td_hack) {
> +        qemu_del_timer(s->coalesced_timer);
> +    }
> +#endif
> +    qemu_free_timer(s->periodic_timer);
> +    qemu_free_timer(s->second_timer);
> +    qemu_free_timer(s->second_timer2);
> +#ifdef TARGET_I386
> +    if (rtc_td_hack) {
> +        qemu_free_timer(s->coalesced_timer);
> +    }
> +#endif
> +}
> +
>  static ISADeviceInfo mc146818rtc_info = {
>      .qdev.name     = "mc146818rtc",
>      .qdev.size     = sizeof(RTCState),
> diff --git a/hw/mc146818rtc.h b/hw/mc146818rtc.h
> index 575968c..aa2b8ab 100644
> --- a/hw/mc146818rtc.h
> +++ b/hw/mc146818rtc.h
> @@ -8,5 +8,6 @@
>  ISADevice *rtc_init(int base_year, qemu_irq intercept_irq);
>  void rtc_set_memory(ISADevice *dev, int addr, int val);
>  void rtc_set_date(ISADevice *dev, const struct tm *tm);
> +void rtc_stop(ISADevice *dev);
>  
>  #endif /* !MC146818RTC_H */
> diff --git a/hw/pc.c b/hw/pc.c
> index a0ae981..d734f75 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -1145,6 +1145,8 @@ void pc_basic_device_init(qemu_irq *gsi,
>  
>      if (!xen_available()) {
>          pit = pit_init(0x40, 0);
> +    } else {
> +        rtc_stop(*rtc_state);
>      }
>      pcspk_init(pit);
>  
>
Anthony Liguori - Nov. 18, 2011, 1:58 p.m.
On 11/18/2011 05:46 AM, Stefano Stabellini wrote:
> On Tue, 15 Nov 2011, Stefano Stabellini wrote:
>> On Tue, 15 Nov 2011, Anthony Liguori wrote:
>>> On 11/15/2011 08:51 AM, stefano.stabellini@eu.citrix.com wrote:
>>>> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>>>
>>>> Xen doesn't need full RTC emulation in Qemu because the RTC is already
>>>> emulated by the hypervisor. In particular we want to avoid the timers
>>>> initialization so that Qemu doesn't need to wake up needlessly.
>>>>
>>>> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>>
>>> Yuck.  There's got to be a better way to do this.
>>
>> Yeah, it is pretty ugly, I was hoping in some good suggestions to
>> improve this patch :)
>>
>>
>>> I think it would be better to name timers and then in Xen specific machine code,
>>> disable the RTC timers.
>>
>> Good idea!
>> I was thinking that I could implement an rtc_stop function in
>> mc146818rtc.c that stops and frees the timers.

You could also just stop the rtc_clock.  The rtc is the only device that makes 
use of the rtc_clock.

Regards,

Anthony Liguori
Anthony Liguori - Nov. 18, 2011, 2:54 p.m.
On 11/18/2011 05:46 AM, Stefano Stabellini wrote:
> On Tue, 15 Nov 2011, Stefano Stabellini wrote:
>> On Tue, 15 Nov 2011, Anthony Liguori wrote:
>>> On 11/15/2011 08:51 AM, stefano.stabellini@eu.citrix.com wrote:
>>>> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>>>
>>>> Xen doesn't need full RTC emulation in Qemu because the RTC is already
>>>> emulated by the hypervisor. In particular we want to avoid the timers
>>>> initialization so that Qemu doesn't need to wake up needlessly.
>>>>
>>>> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>>
>>> Yuck.  There's got to be a better way to do this.
>>
>> Yeah, it is pretty ugly, I was hoping in some good suggestions to
>> improve this patch :)
>>
>>
>>> I think it would be better to name timers and then in Xen specific machine code,
>>> disable the RTC timers.
>>
>> Good idea!
>> I was thinking that I could implement an rtc_stop function in
>> mc146818rtc.c that stops and frees the timers.
>>
>> Now the problem is that from xen-all.c I cannot easily find the
>> ISADevice instance to pass to rtc_stop. Do you think it would be
>> reasonable to call rtc_stop from pc_basic_device_init, inside the same
>> if (!xen_available()) introduce by the next patch?
>>
>> Otherwise I could implement functions to walk the isa bus, similarly to
>> pci_for_each_device.
>>
>
> ping?

Thinking more about it, I think this entire line of thinking is wrong (including 
mine) :-)

The problem you're trying to solve is that the RTC fires two 1 second timers 
regardless of whether the guest is reading the wall clock time, right?  And 
since wall clock time is never read from the QEMU RTC in Xen, it's a huge waste?

The Right Solution would be to modify the RTC emulation such that it did a 
qemu_get_clock() during read of the CMOS registers in order to ensure the time 
was up to date (instead of using 1 second timers).

Then the timers wouldn't even exist anymore.

Regards,

Anthony Liguori

>
>
>> This is just an example:
>>
>> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
>> index 2aaca2f..568c540 100644
>> --- a/hw/mc146818rtc.c
>> +++ b/hw/mc146818rtc.c
>> @@ -667,6 +667,28 @@ ISADevice *rtc_init(int base_year, qemu_irq intercept_irq)
>>       return dev;
>>   }
>>
>> +void rtc_stop(ISADevice *dev)
>> +{
>> +    RTCState *s = DO_UPCAST(RTCState, dev, dev);
>> +
>> +    qemu_del_timer(s->periodic_timer);
>> +    qemu_del_timer(s->second_timer);
>> +    qemu_del_timer(s->second_timer2);
>> +#ifdef TARGET_I386
>> +    if (rtc_td_hack) {
>> +        qemu_del_timer(s->coalesced_timer);
>> +    }
>> +#endif
>> +    qemu_free_timer(s->periodic_timer);
>> +    qemu_free_timer(s->second_timer);
>> +    qemu_free_timer(s->second_timer2);
>> +#ifdef TARGET_I386
>> +    if (rtc_td_hack) {
>> +        qemu_free_timer(s->coalesced_timer);
>> +    }
>> +#endif
>> +}
>> +
>>   static ISADeviceInfo mc146818rtc_info = {
>>       .qdev.name     = "mc146818rtc",
>>       .qdev.size     = sizeof(RTCState),
>> diff --git a/hw/mc146818rtc.h b/hw/mc146818rtc.h
>> index 575968c..aa2b8ab 100644
>> --- a/hw/mc146818rtc.h
>> +++ b/hw/mc146818rtc.h
>> @@ -8,5 +8,6 @@
>>   ISADevice *rtc_init(int base_year, qemu_irq intercept_irq);
>>   void rtc_set_memory(ISADevice *dev, int addr, int val);
>>   void rtc_set_date(ISADevice *dev, const struct tm *tm);
>> +void rtc_stop(ISADevice *dev);
>>
>>   #endif /* !MC146818RTC_H */
>> diff --git a/hw/pc.c b/hw/pc.c
>> index a0ae981..d734f75 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -1145,6 +1145,8 @@ void pc_basic_device_init(qemu_irq *gsi,
>>
>>       if (!xen_available()) {
>>           pit = pit_init(0x40, 0);
>> +    } else {
>> +        rtc_stop(*rtc_state);
>>       }
>>       pcspk_init(pit);
>>
>>
>
Avi Kivity - Nov. 20, 2011, 2:53 p.m.
On 11/18/2011 04:54 PM, Anthony Liguori wrote:
>
> Thinking more about it, I think this entire line of thinking is wrong
> (including mine) :-)
>
> The problem you're trying to solve is that the RTC fires two 1 second
> timers regardless of whether the guest is reading the wall clock time,
> right?  And since wall clock time is never read from the QEMU RTC in
> Xen, it's a huge waste?
>
> The Right Solution would be to modify the RTC emulation such that it
> did a qemu_get_clock() during read of the CMOS registers in order to
> ensure the time was up to date (instead of using 1 second timers).
>
> Then the timers wouldn't even exist anymore.

That would make host time adjustments (suspend/resume) be reflected in
the guest.

Not sure if that's good or bad, but it's different.
Stefano Stabellini - Nov. 21, 2011, 11:05 a.m.
On Fri, 18 Nov 2011, Anthony Liguori wrote:
> On 11/18/2011 05:46 AM, Stefano Stabellini wrote:
> > On Tue, 15 Nov 2011, Stefano Stabellini wrote:
> >> On Tue, 15 Nov 2011, Anthony Liguori wrote:
> >>> On 11/15/2011 08:51 AM, stefano.stabellini@eu.citrix.com wrote:
> >>>> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> >>>>
> >>>> Xen doesn't need full RTC emulation in Qemu because the RTC is already
> >>>> emulated by the hypervisor. In particular we want to avoid the timers
> >>>> initialization so that Qemu doesn't need to wake up needlessly.
> >>>>
> >>>> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> >>>
> >>> Yuck.  There's got to be a better way to do this.
> >>
> >> Yeah, it is pretty ugly, I was hoping in some good suggestions to
> >> improve this patch :)
> >>
> >>
> >>> I think it would be better to name timers and then in Xen specific machine code,
> >>> disable the RTC timers.
> >>
> >> Good idea!
> >> I was thinking that I could implement an rtc_stop function in
> >> mc146818rtc.c that stops and frees the timers.
> >>
> >> Now the problem is that from xen-all.c I cannot easily find the
> >> ISADevice instance to pass to rtc_stop. Do you think it would be
> >> reasonable to call rtc_stop from pc_basic_device_init, inside the same
> >> if (!xen_available()) introduce by the next patch?
> >>
> >> Otherwise I could implement functions to walk the isa bus, similarly to
> >> pci_for_each_device.
> >>
> >
> > ping?
> 
> Thinking more about it, I think this entire line of thinking is wrong (including 
> mine) :-)

Actually I quite liked your suggestion of stopping the rtc_clock: the
patch becomes a one-liner in xen-all!


> The problem you're trying to solve is that the RTC fires two 1 second timers 
> regardless of whether the guest is reading the wall clock time, right?  And 
> since wall clock time is never read from the QEMU RTC in Xen, it's a huge waste?

The real problem I am trying to solve is that I don't need an RTC clock
in Qemu. However it is not easy to disentangle the RTC emulation from
the rest of the system (see rtc_state in pc.c and pc_piix.c). So I would
be happy enough with just getting rid of the timers.


> The Right Solution would be to modify the RTC emulation such that it did a 
> qemu_get_clock() during read of the CMOS registers in order to ensure the time 
> was up to date (instead of using 1 second timers).
> 
> Then the timers wouldn't even exist anymore.

That would be one way of doing it, however the timers don't only update
a clock variable, so removing them is certainly non-trivial and could
have unintended consequences. I am not sure it is worth it in this
context.
BTW the RTC emulation in Xen also has two timers.
Paolo Bonzini - Nov. 21, 2011, 1:21 p.m.
On 11/18/2011 03:54 PM, Anthony Liguori wrote:
>
> The Right Solution would be to modify the RTC emulation such that it did
> a qemu_get_clock() during read of the CMOS registers in order to ensure
> the time was up to date (instead of using 1 second timers).

True, but you also have to handle UIP and the UF/AF interrupts. 
Basically you have to track the various pieces of state and trigger a 
timer when the routine would do something interesting.  It's not hard, 
but it's also a decent amount of programming.

Paolo
Anthony Liguori - Nov. 21, 2011, 8:49 p.m.
On 11/20/2011 08:53 AM, Avi Kivity wrote:
> On 11/18/2011 04:54 PM, Anthony Liguori wrote:
>>
>> Thinking more about it, I think this entire line of thinking is wrong
>> (including mine) :-)
>>
>> The problem you're trying to solve is that the RTC fires two 1 second
>> timers regardless of whether the guest is reading the wall clock time,
>> right?  And since wall clock time is never read from the QEMU RTC in
>> Xen, it's a huge waste?
>>
>> The Right Solution would be to modify the RTC emulation such that it
>> did a qemu_get_clock() during read of the CMOS registers in order to
>> ensure the time was up to date (instead of using 1 second timers).
>>
>> Then the timers wouldn't even exist anymore.
>
> That would make host time adjustments (suspend/resume) be reflected in
> the guest.

qemu_get_clock(rtc_clock)

It depends on what clock rtc_clock is tied too.  If it's tied to vm_clock, it 
won't be affected.

> Not sure if that's good or bad, but it's different.

Doing this wouldn't change the behavior.  I presume you were confusing 
qemu_get_clock() with qemu_get_timedate().

But our current default behavior has the characteristic that you're concerned 
about.  It's was a conscious decision.  See:

commit 6875204c782e7c9aa5c28f96b2583fd31c50468f
Author: Jan Kiszka <jan.kiszka@siemens.com>
Date:   Tue Sep 15 13:36:04 2009 +0200

     Enable host-clock-based RTC

Regards,

Anthony Liguori

Patch

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 2aaca2f..568c540 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -667,6 +667,28 @@  ISADevice *rtc_init(int base_year, qemu_irq intercept_irq)
     return dev;
 }
 
+void rtc_stop(ISADevice *dev)
+{
+    RTCState *s = DO_UPCAST(RTCState, dev, dev);
+
+    qemu_del_timer(s->periodic_timer);
+    qemu_del_timer(s->second_timer);
+    qemu_del_timer(s->second_timer2);
+#ifdef TARGET_I386
+    if (rtc_td_hack) {
+        qemu_del_timer(s->coalesced_timer);
+    }
+#endif
+    qemu_free_timer(s->periodic_timer);
+    qemu_free_timer(s->second_timer);
+    qemu_free_timer(s->second_timer2);
+#ifdef TARGET_I386
+    if (rtc_td_hack) {
+        qemu_free_timer(s->coalesced_timer);
+    }
+#endif
+}
+
 static ISADeviceInfo mc146818rtc_info = {
     .qdev.name     = "mc146818rtc",
     .qdev.size     = sizeof(RTCState),
diff --git a/hw/mc146818rtc.h b/hw/mc146818rtc.h
index 575968c..aa2b8ab 100644
--- a/hw/mc146818rtc.h
+++ b/hw/mc146818rtc.h
@@ -8,5 +8,6 @@ 
 ISADevice *rtc_init(int base_year, qemu_irq intercept_irq);
 void rtc_set_memory(ISADevice *dev, int addr, int val);
 void rtc_set_date(ISADevice *dev, const struct tm *tm);
+void rtc_stop(ISADevice *dev);
 
 #endif /* !MC146818RTC_H */
diff --git a/hw/pc.c b/hw/pc.c
index a0ae981..d734f75 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1145,6 +1145,8 @@  void pc_basic_device_init(qemu_irq *gsi,
 
     if (!xen_available()) {
         pit = pit_init(0x40, 0);
+    } else {
+        rtc_stop(*rtc_state);
     }
     pcspk_init(pit);