Re: [Xen-devel] [Qemu-devel] [PATCH 1/4] xen: introduce mc146818rtcxen

[Anthony Liguori <anthony@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx wrote:
> > > > From: Stefano Stabellini<stefano.stabellini@xxxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxxx>
> > > 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);

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel