| Message ID | alpine.DEB.2.00.1111151457450.3519@kaball-desktop |
|---|---|
| State | New |
| Headers | show |
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); > >
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
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); >> >> >
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.
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.
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
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
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);