From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NScsf-0004yj-Sx for qemu-devel@nongnu.org; Wed, 06 Jan 2010 15:53:26 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NScsZ-0004uY-7f for qemu-devel@nongnu.org; Wed, 06 Jan 2010 15:53:24 -0500 Received: from [199.232.76.173] (port=33864 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NScsY-0004uN-Tz for qemu-devel@nongnu.org; Wed, 06 Jan 2010 15:53:18 -0500 Received: from mail-bw0-f212.google.com ([209.85.218.212]:44056) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NScsY-00030E-4Q for qemu-devel@nongnu.org; Wed, 06 Jan 2010 15:53:18 -0500 Received: by bwz4 with SMTP id 4so11161944bwz.2 for ; Wed, 06 Jan 2010 12:53:17 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: Date: Wed, 6 Jan 2010 23:53:16 +0300 Message-ID: Subject: Re: [Qemu-devel] [PATCH 9/9] sparc64: reimplement tick timers From: Igor Kovalenko Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl , qemu-devel On Wed, Jan 6, 2010 at 8:31 PM, Blue Swirl wrote: > On Tue, Jan 5, 2010 at 11:47 PM, Igor Kovalenko > wrote: >> sparc64 timer has tick counter which can be set and read, >> and tick compare value used as deadline to fire timer interrupt. >> The timer is not used as periodic timer, instead deadline >> is set each time new timer interrupt is needed. >> >> This change implements sparc64 timers without >> periodic timers. It is not complete yet, >> cpu_tick_set_count does not really set counter value. >> >> Signed-off-by: Igor V. Kovalenko >> --- >> =A0hw/sun4u.c | =A0182 +++++++++++++++++++++++++++++++++++++++++++++++++= +---------- >> =A01 files changed, 152 insertions(+), 30 deletions(-) >> >> diff --git a/hw/sun4u.c b/hw/sun4u.c >> index 84a8043..35f4c6b 100644 >> --- a/hw/sun4u.c >> +++ b/hw/sun4u.c >> @@ -281,6 +281,12 @@ void cpu_check_irqs(CPUState *env) >> =A0 =A0 } >> =A0} >> >> +static void cpu_kick_irq(CPUState *env) >> +{ >> + =A0 =A0env->halted =3D 0; >> + =A0 =A0cpu_check_irqs(env); >> +} >> + >> =A0static void cpu_set_irq(void *opaque, int irq, int level) >> =A0{ >> =A0 =A0 CPUState *env =3D opaque; >> @@ -302,6 +308,41 @@ typedef struct ResetData { >> =A0 =A0 uint64_t prom_addr; >> =A0} ResetData; >> >> +struct sun4u_timer >> +{ >> + =A0 =A0const char *name; >> + =A0 =A0uint32_t =A0 =A0frequency; >> + =A0 =A0int =A0 =A0 =A0disabled; >> + =A0 =A0uint64_t disabled_mask; >> + =A0 =A0QEMUTimer *qtimer; >> +}; > > The formatting seems to be off. Please don't use tabs. The prefix > 'sun4u_' does not fit hstick and it's not CamelCase, how about > CPUTimer? > >> + >> +typedef struct sun4u_timer sun4u_timer; >> + >> +static sun4u_timer* sun4u_timer_create(const char* name, CPUState *env, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 QEMUBHFunc *cb, uint32_t frequency, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 int64_t period, uint64_t disabled_mask) >> +{ >> + =A0 =A0sun4u_timer *timer; >> + >> + =A0 =A0timer =3D qemu_mallocz(sizeof (sun4u_timer)); >> + >> + =A0 =A0timer->name =3D name; >> + =A0 =A0timer->frequency =3D frequency; >> + =A0 =A0timer->disabled =3D 1; >> + =A0 =A0timer->disabled_mask =3D disabled_mask; >> + >> + =A0 =A0timer->qtimer =3D qemu_new_timer(vm_clock, cb, env); >> + >> + =A0 =A0return timer; >> +} > > The parameter 'period' is not used. > >> + >> +static void sun4u_timer_reset(sun4u_timer *timer) >> +{ >> + =A0 =A0timer->disabled =3D 1; >> + =A0 =A0qemu_del_timer(timer->qtimer); >> +} >> + >> =A0static void main_cpu_reset(void *opaque) >> =A0{ >> =A0 =A0 ResetData *s =3D (ResetData *)opaque; >> @@ -309,15 +350,11 @@ static void main_cpu_reset(void *opaque) >> =A0 =A0 static unsigned int nr_resets; >> >> =A0 =A0 cpu_reset(env); >> - =A0 =A0env->tick_cmpr =3D TICK_INT_DIS | 0; >> - =A0 =A0ptimer_set_limit(env->tick, TICK_MAX, 1); >> - =A0 =A0ptimer_run(env->tick, 1); >> - =A0 =A0env->stick_cmpr =3D TICK_INT_DIS | 0; >> - =A0 =A0ptimer_set_limit(env->stick, TICK_MAX, 1); >> - =A0 =A0ptimer_run(env->stick, 1); >> - =A0 =A0env->hstick_cmpr =3D TICK_INT_DIS | 0; >> - =A0 =A0ptimer_set_limit(env->hstick, TICK_MAX, 1); >> - =A0 =A0ptimer_run(env->hstick, 1); >> + >> + =A0 =A0sun4u_timer_reset(env->tick); >> + =A0 =A0sun4u_timer_reset(env->stick); >> + =A0 =A0sun4u_timer_reset(env->hstick); >> + >> =A0 =A0 env->gregs[1] =3D 0; // Memory start >> =A0 =A0 env->gregs[2] =3D ram_size; // Memory size >> =A0 =A0 env->gregs[3] =3D 0; // Machine description XXX >> @@ -334,44 +371,125 @@ static void tick_irq(void *opaque) >> =A0{ >> =A0 =A0 CPUState *env =3D opaque; >> >> - =A0 =A0if (!(env->tick_cmpr & TICK_INT_DIS)) { >> - =A0 =A0 =A0 =A0env->softint |=3D SOFTINT_TIMER; >> - =A0 =A0 =A0 =A0cpu_interrupt(env, CPU_INTERRUPT_TIMER); >> + =A0 =A0sun4u_timer* timer =3D (sun4u_timer*) env->tick; >> + >> + =A0 =A0if (timer->disabled) >> + =A0 =A0{ > > Coding style. > >> + =A0 =A0 =A0 =A0fprintf(logfile, "tick_irq: softint disabled\n"); >> + =A0 =A0 =A0 =A0return; >> =A0 =A0 } >> + =A0 =A0else >> + =A0 =A0{ > > Coding style. > >> + =A0 =A0 =A0 =A0fprintf(logfile, "tick: fire\n"); >> + =A0 =A0} >> + >> + =A0 =A0env->softint |=3D SOFTINT_TM; >> + =A0 =A0cpu_kick_irq(env); >> =A0} >> >> =A0static void stick_irq(void *opaque) >> =A0{ >> =A0 =A0 CPUState *env =3D opaque; >> >> - =A0 =A0if (!(env->stick_cmpr & TICK_INT_DIS)) { >> - =A0 =A0 =A0 =A0env->softint |=3D SOFTINT_STIMER; >> - =A0 =A0 =A0 =A0cpu_interrupt(env, CPU_INTERRUPT_TIMER); >> + =A0 =A0sun4u_timer* timer =3D (sun4u_timer*) env->stick; >> + >> + =A0 =A0if (timer->disabled) >> + =A0 =A0{ > > Coding style. > >> + =A0 =A0 =A0 =A0CPUIRQ_DPRINTF("stick_irq: softint disabled\n"); >> + =A0 =A0 =A0 =A0return; >> =A0 =A0 } >> + =A0 =A0else >> + =A0 =A0{ > > Coding style. > >> + =A0 =A0 =A0 =A0CPUIRQ_DPRINTF("stick: fire\n"); >> + =A0 =A0} >> + >> + =A0 =A0env->softint |=3D SOFTINT_SM; >> + =A0 =A0cpu_kick_irq(env); >> =A0} >> >> =A0static void hstick_irq(void *opaque) >> =A0{ >> =A0 =A0 CPUState *env =3D opaque; >> >> - =A0 =A0if (!(env->hstick_cmpr & TICK_INT_DIS)) { >> - =A0 =A0 =A0 =A0cpu_interrupt(env, CPU_INTERRUPT_TIMER); >> + =A0 =A0sun4u_timer* timer =3D (sun4u_timer*) env->hstick; >> + >> + =A0 =A0if (timer->disabled) >> + =A0 =A0{ >> + =A0 =A0 =A0 =A0CPUIRQ_DPRINTF("hstick_irq: softint disabled\n"); >> + =A0 =A0 =A0 =A0return; >> + =A0 =A0} >> + =A0 =A0else > > Coding style. > >> + =A0 =A0{ >> + =A0 =A0 =A0 =A0CPUIRQ_DPRINTF("hstick: fire\n"); >> =A0 =A0 } >> + >> + =A0 =A0env->softint |=3D SOFTINT_SM; >> + =A0 =A0cpu_kick_irq(env); >> =A0} >> >> =A0void cpu_tick_set_count(void *opaque, uint64_t count) >> =A0{ >> - =A0 =A0ptimer_set_count(opaque, -count); >> + =A0 =A0sun4u_timer *timer =3D opaque; >> + >> + =A0 =A0uint64_t real_count =3D count & ~timer->disabled_mask; >> + =A0 =A0timer->disabled =3D (count & timer->disabled_mask) ? 1 : 0; >> + >> + =A0 =A0fprintf(logfile, "%s (ignored) set_count count=3D0x%016lx (%s) = p=3D%p\n", >> + =A0 =A0 =A0 =A0 =A0 timer->name, real_count, >> timer->disabled?"disabled":"enabled", opaque); > > I think logging this is not necessary, please create TICK_DPRINTF() or > something. > >> + >> + =A0 =A0// TODO: save offset in our timer >> =A0} >> >> =A0uint64_t cpu_tick_get_count(void *opaque) >> =A0{ >> - =A0 =A0return -ptimer_get_count(opaque); >> + =A0 =A0sun4u_timer *timer =3D opaque; >> + >> + =A0 =A0uint64_t real_count =3D muldiv64(qemu_get_clock(vm_clock), >> timer->frequency, get_ticks_per_sec()); > > The line is wrapped, perhaps the line is too long? > >> + >> + =A0 =A0fprintf(logfile, "%s get_count count=3D0x%016lx (%s) p=3D%p\n", >> + =A0 =A0 =A0 =A0 =A0 timer->name, real_count, >> timer->disabled?"disabled":"enabled", opaque); > > DPRINTF() > >> + >> + =A0 =A0if (timer->disabled) >> + =A0 =A0 =A0 =A0real_count |=3D timer->disabled_mask; >> + >> + =A0 =A0return real_count; >> =A0} >> >> =A0void cpu_tick_set_limit(void *opaque, uint64_t limit) >> =A0{ >> - =A0 =A0ptimer_set_limit(opaque, -limit, 0); >> + =A0 =A0sun4u_timer *timer =3D opaque; >> + >> + =A0 =A0int64_t now =3D qemu_get_clock(vm_clock); >> + >> + =A0 =A0int64_t real_limit =3D limit & ~timer->disabled_mask; >> + =A0 =A0int64_t expires =3D muldiv64(now, timer->frequency, >> get_ticks_per_sec()) & ~timer->disabled_mask; > > Line too long. > >> + =A0 =A0int64_t current_tick =3D expires; >> + =A0 =A0int64_t delta =3D real_limit - current_tick; >> + =A0 =A0if (delta < 0) >> + =A0 =A0 =A0 =A0delta =3D 1; >> + >> + =A0 =A0timer->disabled =3D (limit & timer->disabled_mask) ? 1 : 0; >> + >> + =A0 =A0fprintf(logfile, "%s set_limit limit=3D0x%016lx (%s) p=3D%p " >> + =A0 =A0 =A0 =A0 =A0 =A0"called with limit=3D0x%016lx at 0x%016lx (delt= a=3D0x%016lx)\n", >> + =A0 =A0 =A0 =A0 =A0 timer->name, real_limit, timer->disabled?"disabled= ":"enabled", >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 opaque, limit, cur= rent_tick, delta); > > DPRINTF() > >> + >> + =A0 =A0if (!real_limit) >> + =A0 =A0{ > > Coding style. > >> + =A0 =A0 =A0 =A0fprintf(logfile, "%s set_limit limit=3DZERO - not start= ing timer\n", >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0timer->name); >> + =A0 =A0 =A0 =A0qemu_del_timer(timer->qtimer); >> + =A0 =A0} >> + =A0 =A0else if (timer->disabled) >> + =A0 =A0{ > > Coding style. > >> + =A0 =A0 =A0 =A0qemu_del_timer(timer->qtimer); >> + =A0 =A0} >> + =A0 =A0else >> + =A0 =A0{ > > Coding style. > >> + =A0 =A0 =A0 =A0qemu_mod_timer(timer->qtimer, now + muldiv64(delta, >> get_ticks_per_sec(), > > Line too long. > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 timer->frequency)); >> + =A0 =A0} >> =A0} >> >> =A0static void ebus_mmio_mapfunc(PCIDevice *pci_dev, int region_num, >> @@ -558,9 +676,12 @@ device_init(ram_register_devices); >> =A0static CPUState *cpu_devinit(const char *cpu_model, const struct hwde= f *hwdef) >> =A0{ >> =A0 =A0 CPUState *env; >> - =A0 =A0QEMUBH *bh; >> =A0 =A0 ResetData *reset_info; >> >> + =A0 =A0uint32_t =A0 tick_frequency =3D 10*1000000; >> + =A0 =A0uint32_t =A0stick_frequency =3D 10*1000000; >> + =A0 =A0uint32_t hstick_frequency =3D 10*1000000; >> + >> =A0 =A0 if (!cpu_model) >> =A0 =A0 =A0 =A0 cpu_model =3D hwdef->default_cpu_model; >> =A0 =A0 env =3D cpu_init(cpu_model); >> @@ -568,17 +689,18 @@ static CPUState *cpu_devinit(const char >> *cpu_model, const struct hwdef *hwdef) >> =A0 =A0 =A0 =A0 fprintf(stderr, "Unable to find Sparc CPU definition\n")= ; >> =A0 =A0 =A0 =A0 exit(1); >> =A0 =A0 } >> - =A0 =A0bh =3D qemu_bh_new(tick_irq, env); >> - =A0 =A0env->tick =3D ptimer_init(bh); >> - =A0 =A0ptimer_set_period(env->tick, 1ULL); >> >> - =A0 =A0bh =3D qemu_bh_new(stick_irq, env); >> - =A0 =A0env->stick =3D ptimer_init(bh); >> - =A0 =A0ptimer_set_period(env->stick, 1ULL); >> + =A0 =A0env->tick =3D sun4u_timer_create("tick", env, tick_irq, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0tick_frequency, 1ULL, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0TICK_NPT_MASK); >> + >> + =A0 =A0env->stick =3D sun4u_timer_create("stick", env, stick_irq, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 stick_frequency, 1ULL, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 TICK_SOFTINT_DISABLE); >> >> - =A0 =A0bh =3D qemu_bh_new(hstick_irq, env); >> - =A0 =A0env->hstick =3D ptimer_init(bh); >> - =A0 =A0ptimer_set_period(env->hstick, 1ULL); >> + =A0 =A0env->hstick =3D sun4u_timer_create("hstick", env, hstick_irq, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 hstick_frequency, 1ULL, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 TICK_SOFTINT_DISABLE); >> >> =A0 =A0 reset_info =3D qemu_mallocz(sizeof(ResetData)); >> =A0 =A0 reset_info->env =3D env; >> >> >> > > After this, are ptimers not going to be used anymore for Sparc64? Then > Makefile etc. changes should be included. > > Anyway, thank you for your efforts. Thanks for the review! I'm not sure at the moment we need to discard ptimer implementation for sparc64. I'll work with the rest of this series according to review comments, assuming those are otherwise not bad to be committed. We can deal with (the only, at the moment) interrupt source separately. --=20 Kind regards, Igor V. Kovalenko