From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NicD0-0003Tr-Aa for qemu-devel@nongnu.org; Fri, 19 Feb 2010 18:24:30 -0500 Received: from [199.232.76.173] (port=53848 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NicCz-0003SI-GA for qemu-devel@nongnu.org; Fri, 19 Feb 2010 18:24:29 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NicCx-00006g-Af for qemu-devel@nongnu.org; Fri, 19 Feb 2010 18:24:29 -0500 Received: from mail-yw0-f197.google.com ([209.85.211.197]:36732) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NicCx-00006Y-0K for qemu-devel@nongnu.org; Fri, 19 Feb 2010 18:24:27 -0500 Received: by ywh35 with SMTP id 35so596119ywh.4 for ; Fri, 19 Feb 2010 15:24:26 -0800 (PST) Message-ID: <4B7F1DA7.7030007@codemonkey.ws> Date: Fri, 19 Feb 2010 17:24:23 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] rewrote timer implementation for rtl8139. References: <1266309345-2889-1-git-send-email-freddy77@gmail.com> In-Reply-To: <1266309345-2889-1-git-send-email-freddy77@gmail.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Frediano Ziglio Cc: qemu-devel@nongnu.org On 02/16/2010 02:35 AM, Frediano Ziglio wrote: > Add a QEMU timer only when needed (timeout status not set, timeout > irq wanted and timer set). > > This patch is required for Darwin. Patch has been tested under > FreeBSD, Darwin and Linux. > > Signed-off-by: Frediano Ziglio > --- > hw/rtl8139.c | 136 ++++++++++++++++++++++++++++++++++----------------------- > 1 files changed, 81 insertions(+), 55 deletions(-) > > diff --git a/hw/rtl8139.c b/hw/rtl8139.c > index f04dd54..e43c15c 100644 > --- a/hw/rtl8139.c > +++ b/hw/rtl8139.c > @@ -41,6 +41,10 @@ > * segmentation offloading > * Removed slirp.h dependency > * Added rx/tx buffer reset when enabling rx/tx operation > + * > + * 2010-Feb-04 Frediano Ziglio: Rewrote timer support using QEMU timer only > + * when strictly needed (required for for > + * Darwin) > */ > > #include "hw.h" > @@ -60,9 +64,6 @@ > /* Calculate CRCs properly on Rx packets */ > #define RTL8139_CALCULATE_RXCRC 1 > > -/* Uncomment to enable on-board timer interrupts */ > -//#define RTL8139_ONBOARD_TIMER 1 > - > #if defined(RTL8139_CALCULATE_RXCRC) > /* For crc32 */ > #include > @@ -491,9 +492,12 @@ typedef struct RTL8139State { > > /* PCI interrupt timer */ > QEMUTimer *timer; > + int64_t TimerExpire; > > } RTL8139State; > > +static void rtl8139_set_next_tctr_time(RTL8139State *s, int64_t current_time); > + > static void prom9346_decode_command(EEprom9346 *eeprom, uint8_t command) > { > DEBUG_PRINT(("RTL8139: eeprom command 0x%02x\n", command)); > @@ -2522,7 +2526,9 @@ static void rtl8139_IntrMask_write(RTL8139State *s, uint32_t val) > > s->IntrMask = val; > > + rtl8139_set_next_tctr_time(s, qemu_get_clock(vm_clock)); > rtl8139_update_irq(s); > + > } > > static uint32_t rtl8139_IntrMask_read(RTL8139State *s) > @@ -2555,12 +2561,22 @@ static void rtl8139_IntrStatus_write(RTL8139State *s, uint32_t val) > rtl8139_update_irq(s); > > s->IntrStatus = newStatus; > + /* > + * Computing if we miss an interrupt here is not that correct but > + * considered that we should have had already an interrupt > + * and probably emulated is slower is better to assume this resetting was > + * done before testing on previous rtl8139_update_irq lead to IRQ loosing > + */ > + rtl8139_set_next_tctr_time(s, qemu_get_clock(vm_clock)); > rtl8139_update_irq(s); > + > #endif > } > > static uint32_t rtl8139_IntrStatus_read(RTL8139State *s) > { > + rtl8139_set_next_tctr_time(s, qemu_get_clock(vm_clock)); > + > uint32_t ret = s->IntrStatus; > > DEBUG_PRINT(("RTL8139: IntrStatus read(w) val=0x%04x\n", ret)); > @@ -2739,6 +2755,43 @@ static void rtl8139_io_writew(void *opaque, uint8_t addr, uint32_t val) > } > } > > +static void rtl8139_set_next_tctr_time(RTL8139State *s, int64_t current_time) > +{ > + DEBUG_PRINT(("RTL8139: entered rtl8139_set_next_tctr_time\n")); > + > + if (s->TimerExpire&& current_time>= s->TimerExpire) { > + s->IntrStatus |= PCSTimeout; > + rtl8139_update_irq(s); > + } > + > + /* Set QEMU timer only if needed that is > + * - TimerInt<> 0 (we have a timer) > + * - mask = 1 (we want an interrupt timer) > + * - irq = 0 (irq is not already active) > + * If any of above change we need to compute timer again > + * Also we must check if timer is passed without QEMU timer > + */ > + s->TimerExpire = 0; > + if (!s->TimerInt) { > + return; > + } > + > + int64_t pci_time = muldiv64(current_time - s->TCTR_base, PCI_FREQUENCY, > + get_ticks_per_sec()); > + uint32_t low_pci = pci_time& 0xffffffff; > Please don't mix variable declarations in code. > + pci_time = pci_time - low_pci + s->TimerInt; > + if (low_pci>= s->TimerInt) { > + pci_time += 0x100000000LL; > + } > + int64_t next_time = s->TCTR_base + muldiv64(pci_time, get_ticks_per_sec(), > + PCI_FREQUENCY); > + s->TimerExpire = next_time; > + > + if ((s->IntrMask& PCSTimeout) != 0&& (s->IntrStatus& PCSTimeout) == 0) { > + qemu_mod_timer(s->timer, next_time); > + } > +} > + > static void rtl8139_io_writel(void *opaque, uint8_t addr, uint32_t val) > { > RTL8139State *s = opaque; > @@ -2784,13 +2837,16 @@ static void rtl8139_io_writel(void *opaque, uint8_t addr, uint32_t val) > > case Timer: > DEBUG_PRINT(("RTL8139: TCTR Timer reset on write\n")); > - s->TCTR = 0; > s->TCTR_base = qemu_get_clock(vm_clock); > + rtl8139_set_next_tctr_time(s, s->TCTR_base); > break; > > case FlashReg: > DEBUG_PRINT(("RTL8139: FlashReg TimerInt write val=0x%08x\n", val)); > - s->TimerInt = val; > + if (s->TimerInt != val) { > + s->TimerInt = val; > + rtl8139_set_next_tctr_time(s, qemu_get_clock(vm_clock)); > + } > break; > > default: > @@ -3000,7 +3056,8 @@ static uint32_t rtl8139_io_readl(void *opaque, uint8_t addr) > break; > > case Timer: > - ret = s->TCTR; > + ret = muldiv64(qemu_get_clock(vm_clock) - s->TCTR_base, > + PCI_FREQUENCY, get_ticks_per_sec()); > DEBUG_PRINT(("RTL8139: TCTR Timer read val=0x%08x\n", ret)); > break; > > @@ -3105,6 +3162,7 @@ static uint32_t rtl8139_mmio_readl(void *opaque, target_phys_addr_t addr) > static int rtl8139_post_load(void *opaque, int version_id) > { > RTL8139State* s = opaque; > + rtl8139_set_next_tctr_time(s, qemu_get_clock(vm_clock)); > if (version_id< 4) { > s->cplus_enabled = s->CpCmd != 0; > } > @@ -3112,12 +3170,24 @@ static int rtl8139_post_load(void *opaque, int version_id) > return 0; > } > > +static void rtl8139_pre_save(void *opaque) > +{ > + RTL8139State* s = opaque; > + > + // set IntrStatus correctly > Please avoid C99 comments. > + int64_t current_time = qemu_get_clock(vm_clock); > + rtl8139_set_next_tctr_time(s, current_time); > + s->TCTR = muldiv64(current_time - s->TCTR_base, PCI_FREQUENCY, > + get_ticks_per_sec()); > +} > Usually, timers have to be saved as part of save/restore in order for them to fire correctly when restarted. I suspect this breaks save/restore. I'd like to see some performance data too. I'm concerned that a change like this could have a significant impact on performance. Regards, Anthony Liguori