From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:50069) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RRPq4-0003b0-BJ for qemu-devel@nongnu.org; Fri, 18 Nov 2011 09:54:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RRPq0-0006kN-6Q for qemu-devel@nongnu.org; Fri, 18 Nov 2011 09:54:48 -0500 Received: from mail-yw0-f45.google.com ([209.85.213.45]:61448) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RRPq0-0006kH-3r for qemu-devel@nongnu.org; Fri, 18 Nov 2011 09:54:44 -0500 Received: by ywa17 with SMTP id 17so3004840ywa.4 for ; Fri, 18 Nov 2011 06:54:43 -0800 (PST) Message-ID: <4EC671AF.5030805@codemonkey.ws> Date: Fri, 18 Nov 2011 08:54:39 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <1321368671-1134-1-git-send-email-stefano.stabellini@eu.citrix.com> <4EC27D0D.3040204@codemonkey.ws> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/4] xen: introduce mc146818rtcxen List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: "xen-devel@lists.xensource.com" , "qemu-devel@nongnu.org" , "agraf@suse.de" 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 >>>> >>>> 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 >>> >>> 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); >> >> >