Re: [Qemu-devel] [PATCH 9/9] sparc64: reimplement tick timers

[Blue Swirl]

On Tue, Jan 5, 2010 at 11:47 PM, Igor Kovalenko
<address@hidden> 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 <address@hidden>
> ---
>  hw/sun4u.c |  182 
> ++++++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 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)
>     }
>  }
>
> +static void cpu_kick_irq(CPUState *env)
> +{
> +    env->halted = 0;
> +    cpu_check_irqs(env);
> +}
> +
>  static void cpu_set_irq(void *opaque, int irq, int level)
>  {
>     CPUState *env = opaque;
> @@ -302,6 +308,41 @@ typedef struct ResetData {
>     uint64_t prom_addr;
>  } ResetData;
>
> +struct sun4u_timer
> +{
> +    const char *name;
> +    uint32_t    frequency;
> +    int      disabled;
> +    uint64_t disabled_mask;
> +    QEMUTimer *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,
> +                                       QEMUBHFunc *cb, uint32_t frequency,
> +                                       int64_t period, uint64_t 
> disabled_mask)
> +{
> +    sun4u_timer *timer;
> +
> +    timer = qemu_mallocz(sizeof (sun4u_timer));
> +
> +    timer->name = name;
> +    timer->frequency = frequency;
> +    timer->disabled = 1;
> +    timer->disabled_mask = disabled_mask;
> +
> +    timer->qtimer = qemu_new_timer(vm_clock, cb, env);
> +
> +    return timer;
> +}

The parameter 'period' is not used.

> +
> +static void sun4u_timer_reset(sun4u_timer *timer)
> +{
> +    timer->disabled = 1;
> +    qemu_del_timer(timer->qtimer);
> +}
> +
>  static void main_cpu_reset(void *opaque)
>  {
>     ResetData *s = (ResetData *)opaque;
> @@ -309,15 +350,11 @@ static void main_cpu_reset(void *opaque)
>     static unsigned int nr_resets;
>
>     cpu_reset(env);
> -    env->tick_cmpr = TICK_INT_DIS | 0;
> -    ptimer_set_limit(env->tick, TICK_MAX, 1);
> -    ptimer_run(env->tick, 1);
> -    env->stick_cmpr = TICK_INT_DIS | 0;
> -    ptimer_set_limit(env->stick, TICK_MAX, 1);
> -    ptimer_run(env->stick, 1);
> -    env->hstick_cmpr = TICK_INT_DIS | 0;
> -    ptimer_set_limit(env->hstick, TICK_MAX, 1);
> -    ptimer_run(env->hstick, 1);
> +
> +    sun4u_timer_reset(env->tick);
> +    sun4u_timer_reset(env->stick);
> +    sun4u_timer_reset(env->hstick);
> +
>     env->gregs[1] = 0; // Memory start
>     env->gregs[2] = ram_size; // Memory size
>     env->gregs[3] = 0; // Machine description XXX
> @@ -334,44 +371,125 @@ static void tick_irq(void *opaque)
>  {
>     CPUState *env = opaque;
>
> -    if (!(env->tick_cmpr & TICK_INT_DIS)) {
> -        env->softint |= SOFTINT_TIMER;
> -        cpu_interrupt(env, CPU_INTERRUPT_TIMER);
> +    sun4u_timer* timer = (sun4u_timer*) env->tick;
> +
> +    if (timer->disabled)
> +    {

Coding style.

> +        fprintf(logfile, "tick_irq: softint disabled\n");
> +        return;
>     }
> +    else
> +    {

Coding style.

> +        fprintf(logfile, "tick: fire\n");
> +    }
> +
> +    env->softint |= SOFTINT_TM;
> +    cpu_kick_irq(env);
>  }
>
>  static void stick_irq(void *opaque)
>  {
>     CPUState *env = opaque;
>
> -    if (!(env->stick_cmpr & TICK_INT_DIS)) {
> -        env->softint |= SOFTINT_STIMER;
> -        cpu_interrupt(env, CPU_INTERRUPT_TIMER);
> +    sun4u_timer* timer = (sun4u_timer*) env->stick;
> +
> +    if (timer->disabled)
> +    {

Coding style.

> +        CPUIRQ_DPRINTF("stick_irq: softint disabled\n");
> +        return;
>     }
> +    else
> +    {

Coding style.

> +        CPUIRQ_DPRINTF("stick: fire\n");
> +    }
> +
> +    env->softint |= SOFTINT_SM;
> +    cpu_kick_irq(env);
>  }
>
>  static void hstick_irq(void *opaque)
>  {
>     CPUState *env = opaque;
>
> -    if (!(env->hstick_cmpr & TICK_INT_DIS)) {
> -        cpu_interrupt(env, CPU_INTERRUPT_TIMER);
> +    sun4u_timer* timer = (sun4u_timer*) env->hstick;
> +
> +    if (timer->disabled)
> +    {
> +        CPUIRQ_DPRINTF("hstick_irq: softint disabled\n");
> +        return;
> +    }
> +    else

Coding style.

> +    {
> +        CPUIRQ_DPRINTF("hstick: fire\n");
>     }
> +
> +    env->softint |= SOFTINT_SM;
> +    cpu_kick_irq(env);
>  }
>
>  void cpu_tick_set_count(void *opaque, uint64_t count)
>  {
> -    ptimer_set_count(opaque, -count);
> +    sun4u_timer *timer = opaque;
> +
> +    uint64_t real_count = count & ~timer->disabled_mask;
> +    timer->disabled = (count & timer->disabled_mask) ? 1 : 0;
> +
> +    fprintf(logfile, "%s (ignored) set_count count=0x%016lx (%s) p=%p\n",
> +           timer->name, real_count,
> timer->disabled?"disabled":"enabled", opaque);

I think logging this is not necessary, please create TICK_DPRINTF() or
something.

> +
> +    // TODO: save offset in our timer
>  }
>
>  uint64_t cpu_tick_get_count(void *opaque)
>  {
> -    return -ptimer_get_count(opaque);
> +    sun4u_timer *timer = opaque;
> +
> +    uint64_t real_count = muldiv64(qemu_get_clock(vm_clock),
> timer->frequency, get_ticks_per_sec());

The line is wrapped, perhaps the line is too long?

> +
> +    fprintf(logfile, "%s get_count count=0x%016lx (%s) p=%p\n",
> +           timer->name, real_count,
> timer->disabled?"disabled":"enabled", opaque);

DPRINTF()

> +
> +    if (timer->disabled)
> +        real_count |= timer->disabled_mask;
> +
> +    return real_count;
>  }
>
>  void cpu_tick_set_limit(void *opaque, uint64_t limit)
>  {
> -    ptimer_set_limit(opaque, -limit, 0);
> +    sun4u_timer *timer = opaque;
> +
> +    int64_t now = qemu_get_clock(vm_clock);
> +
> +    int64_t real_limit = limit & ~timer->disabled_mask;
> +    int64_t expires = muldiv64(now, timer->frequency,
> get_ticks_per_sec()) & ~timer->disabled_mask;

Line too long.

> +    int64_t current_tick = expires;
> +    int64_t delta = real_limit - current_tick;
> +    if (delta < 0)
> +        delta = 1;
> +
> +    timer->disabled = (limit & timer->disabled_mask) ? 1 : 0;
> +
> +    fprintf(logfile, "%s set_limit limit=0x%016lx (%s) p=%p "
> +            "called with limit=0x%016lx at 0x%016lx (delta=0x%016lx)\n",
> +           timer->name, real_limit, timer->disabled?"disabled":"enabled",
> +                           opaque, limit, current_tick, delta);

DPRINTF()

> +
> +    if (!real_limit)
> +    {

Coding style.

> +        fprintf(logfile, "%s set_limit limit=ZERO - not starting timer\n",
> +                timer->name);
> +        qemu_del_timer(timer->qtimer);
> +    }
> +    else if (timer->disabled)
> +    {

Coding style.

> +        qemu_del_timer(timer->qtimer);
> +    }
> +    else
> +    {

Coding style.

> +        qemu_mod_timer(timer->qtimer, now + muldiv64(delta,
> get_ticks_per_sec(),

Line too long.

> +                                                     timer->frequency));
> +    }
>  }
>
>  static void ebus_mmio_mapfunc(PCIDevice *pci_dev, int region_num,
> @@ -558,9 +676,12 @@ device_init(ram_register_devices);
>  static CPUState *cpu_devinit(const char *cpu_model, const struct hwdef 
> *hwdef)
>  {
>     CPUState *env;
> -    QEMUBH *bh;
>     ResetData *reset_info;
>
> +    uint32_t   tick_frequency = 10*1000000;
> +    uint32_t  stick_frequency = 10*1000000;
> +    uint32_t hstick_frequency = 10*1000000;
> +
>     if (!cpu_model)
>         cpu_model = hwdef->default_cpu_model;
>     env = cpu_init(cpu_model);
> @@ -568,17 +689,18 @@ static CPUState *cpu_devinit(const char
> *cpu_model, const struct hwdef *hwdef)
>         fprintf(stderr, "Unable to find Sparc CPU definition\n");
>         exit(1);
>     }
> -    bh = qemu_bh_new(tick_irq, env);
> -    env->tick = ptimer_init(bh);
> -    ptimer_set_period(env->tick, 1ULL);
>
> -    bh = qemu_bh_new(stick_irq, env);
> -    env->stick = ptimer_init(bh);
> -    ptimer_set_period(env->stick, 1ULL);
> +    env->tick = sun4u_timer_create("tick", env, tick_irq,
> +                                    tick_frequency, 1ULL,
> +                                    TICK_NPT_MASK);
> +
> +    env->stick = sun4u_timer_create("stick", env, stick_irq,
> +                                     stick_frequency, 1ULL,
> +                                     TICK_SOFTINT_DISABLE);
>
> -    bh = qemu_bh_new(hstick_irq, env);
> -    env->hstick = ptimer_init(bh);
> -    ptimer_set_period(env->hstick, 1ULL);
> +    env->hstick = sun4u_timer_create("hstick", env, hstick_irq,
> +                                     hstick_frequency, 1ULL,
> +                                     TICK_SOFTINT_DISABLE);
>
>     reset_info = qemu_mallocz(sizeof(ResetData));
>     reset_info->env = 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.