Re: [Qemu-devel] [PATCH] sparc32: fix per cpu counter/timer

[Blue Swirl]

On 1/5/08, Robert Reif <address@hidden> wrote:
> Sun4m SMP machines support a maximum of 4 CPUs.  Linux
> knows this and uses fixed size arrays for per-cpu counter/timers
> and interrupt controllers.  Sun4m uni-processor machines use
> the slaveio chip which has a single per-cpu counter/timer
> and interrupt controller.  However it does not fully decode the
> address so the same counter/timer or interrupt controller can
> be accesses from multiple addresses.
>
> This patch changes the per-cpu counter/timer to work the way
> the real hardware works: 4 per-cpu counter/timers for SMP and
> 1 counter/timer for UP mapped at multiple addresses.

The idea for this part is OK, but I don't like some parts of the
implementation, see below.

> This patch also fixes a number of per-cpu user timer bugs:
> limit bit set when limit reached, count saved and used when
> written, limit bit reset on count write and system timer configuration
> register updated properly for per-cpu user timer mode.

These changes should be in separate patches.

> Sun4d currently uses the sun4m counter/timer code.  They are
> simular but not the same.  This patch will break the broken
> sun4d implementation further.  The real fix is to create a proper
> sun4d counter/timer implementation.  Since the sun4d implementation
> doesn't currently work anyway, this shouldn't be an issue.

Well, why bother then?

> -    unsigned int num_slaves;
> -    struct SLAVIO_TIMERState *slave[MAX_CPUS];
> +    int smp;
> +    struct SLAVIO_TIMERState *slave[MAX_SUN4M_CPUS];

I don't think the change from num_slaves to smp is needed.

> +static int slavio_timer_is_mapped(SLAVIO_TIMERState *s)
> +{
> +    return s->master && (!s->master->smp && s->slave_index > 1);
> +}

These kind of helpers should be introduced so that the logic is not
changed, now it's hard to see what changes and what doesn't.

> -    if (s->timer)
> -        count = limit - PERIODS_TO_LIMIT(ptimer_get_count(s->timer));
> -    else
> -        count = 0;
> +    count = limit - PERIODS_TO_LIMIT(ptimer_get_count(s->timer));

Same here.

> +    if (slavio_timer_is_mapped(s))
> +        s = s->master->slave[0];
> +

And here.

> -            s->limit = TIMER_MAX_COUNT64;
> -            DPRINTF("processor %d user timer reset\n", s->slave_index);
> -            if (s->timer)
> -                ptimer_set_limit(s->timer, LIMIT_TO_PERIODS(s->limit), 1);
> +            s->reached = 0;
> +            s->counthigh = val & (TIMER_MAX_COUNT64 >> 32);
> +            count = s->count | (uint64_t)s->counthigh << 32;
> +            DPRINTF("processor %d user timer set to %016llx\n",
> +                    original->slave_index, count);
> +            ptimer_set_count(s->timer, LIMIT_TO_PERIODS(s->limit - count));
>          } else {
>              // set limit, reset counter
>              qemu_irq_lower(s->irq);
>              s->limit = val & TIMER_MAX_COUNT32;
> -            if (s->timer) {
> -                if (s->limit == 0) /* free-run */
> -                    ptimer_set_limit(s->timer, 
> LIMIT_TO_PERIODS(TIMER_MAX_COUNT32), 1);
> -                else
> -                    ptimer_set_limit(s->timer, LIMIT_TO_PERIODS(s->limit), 
> 1);
> -            }
> +            if (s->limit == 0) /* free-run */
> +                ptimer_set_limit(s->timer, 
> LIMIT_TO_PERIODS(TIMER_MAX_COUNT32),
> +                                 1);
> +            else
> +                ptimer_set_limit(s->timer, LIMIT_TO_PERIODS(s->limit), 1);
>          }
>          break;
>      case TIMER_COUNTER:
>          if (slavio_timer_is_user(s)) {
> +            uint64_t count;
>              // set user counter LSW, reset counter
>              qemu_irq_lower(s->irq);
> -            s->limit = TIMER_MAX_COUNT64;
> -            DPRINTF("processor %d user timer reset\n", s->slave_index);
> -            if (s->timer)
> -                ptimer_set_limit(s->timer, LIMIT_TO_PERIODS(s->limit), 1);
> +            s->reached = 0;
> +            s->count = val & TIMER_MAX_COUNT64;
> +            count = s->count | (uint64_t)s->counthigh << 32;
> +            DPRINTF("processor %d user timer set to %016llx\n",
> +                    original->slave_index, count);
> +            ptimer_set_count(s->timer, LIMIT_TO_PERIODS(s->limit - count));

These are separate.

> -            for (i = 0; i < s->num_slaves; i++) {
> -                if (val & (1 << i)) {
> -                    qemu_irq_lower(s->slave[i]->irq);
> -                    s->slave[i]->limit = -1ULL;
> -                } else {
> -                    ptimer_stop(s->slave[i]->timer);
> -                }
> -                if ((val & (1 << i)) != (s->slave_mode & (1 << i))) {
> -                    ptimer_stop(s->slave[i]->timer);
> -                    ptimer_set_limit(s->slave[i]->timer,
> -                                     LIMIT_TO_PERIODS(s->slave[i]->limit), 
> 1);
> -                    DPRINTF("processor %d timer changed\n",
> -                            s->slave[i]->slave_index);
> -                    ptimer_run(s->slave[i]->timer, 0);
> +            for (i = 0; i < num_slaves; i++) {
> +                unsigned int processor = 1 << i;
> +                // check for a change in timer mode for this processor
> +                if ((val & processor) != (s->slave_mode & processor)) {
> +                    if (val & processor) { // counter -> user timer
> +                        qemu_irq_lower(s->slave[i]->irq);
> +                        // counters are always running
> +                        ptimer_stop(s->slave[i]->timer);
> +                        s->slave[i]->running = 0;
> +                        // user timer limit is always the same
> +                        s->slave[i]->limit = TIMER_MAX_COUNT64;
> +                        ptimer_set_limit(s->slave[i]->timer,
> +                                         
> LIMIT_TO_PERIODS(s->slave[i]->limit), 1);
> +                        // set this processors user timer bit in config
> +                        // register
> +                        s->slave_mode |= processor;
> +                        DPRINTF("processor %d changed from counter to user "
> +                                "timer\n", s->slave[i]->slave_index);
> +                    } else { // user timer -> counter
> +                        // stop the user timer if it is running
> +                        if (s->slave[i]->running)
> +                            ptimer_stop(s->slave[i]->timer);
> +                        // start the counter
> +                        ptimer_run(s->slave[i]->timer, 0);
> +                        s->slave[i]->running = 1;
> +                        // clear this processors user timer bit in config
> +                        // register
> +                        s->slave_mode &= ~processor;
> +                        DPRINTF("processor %d changed from user timer to "
> +                                "counter\n", s->slave[i]->slave_index);
> +                    }

Too many changes at once.

>  static SLAVIO_TIMERState *slavio_timer_init(target_phys_addr_t addr,
>                                              qemu_irq irq,
>                                              SLAVIO_TIMERState *master,
> -                                            int slave_index)
> +                                            int slave_index, int mapped)
>  {
>      int slavio_timer_io_memory;
>      SLAVIO_TIMERState *s;
> @@ -349,7 +375,7 @@ static SLAVIO_TIMERState *slavio_timer_i
>      s->irq = irq;
>      s->master = master;
>      s->slave_index = slave_index;
> -    if (!master || slave_index < master->num_slaves) {
> +    if (!mapped) { /* don't create a qemu timer for mapped devices */
>          bh = qemu_bh_new(slavio_timer_irq, s);
>          s->timer = ptimer_init(bh);
>          ptimer_set_period(s->timer, TIMER_PERIOD);
> @@ -363,27 +389,30 @@ static SLAVIO_TIMERState *slavio_timer_i
>      else
>          cpu_register_physical_memory(addr, SYS_TIMER_SIZE,
>                                       slavio_timer_io_memory);
> -    register_savevm("slavio_timer", addr, 3, slavio_timer_save,
> -                    slavio_timer_load, s);
> -    qemu_register_reset(slavio_timer_reset, s);
> -    slavio_timer_reset(s);
> +    if (!mapped) { /* don't register mapped devices */
> +        register_savevm("slavio_timer", addr, 3, slavio_timer_save,
> +                        slavio_timer_load, s);
> +        qemu_register_reset(slavio_timer_reset, s);
> +        slavio_timer_reset(s);
> +    }
>
>      return s;
>  }
>
>  void slavio_timer_init_all(target_phys_addr_t base, qemu_irq master_irq,
> -                           qemu_irq *cpu_irqs, unsigned int num_cpus)
> +                           qemu_irq *cpu_irqs, int smp)
>  {
>      SLAVIO_TIMERState *master;
>      unsigned int i;
>
> -    master = slavio_timer_init(base + SYS_TIMER_OFFSET, master_irq, NULL, 0);
> +    master = slavio_timer_init(base + SYS_TIMER_OFFSET, master_irq, NULL, 0, 
> 0);
>
> -    master->num_slaves = num_cpus;
> +    master->smp = smp;
>
> -    for (i = 0; i < MAX_CPUS; i++) {
> +    for (i = 0; i < MAX_SUN4M_CPUS; i++) {
>          master->slave[i] = slavio_timer_init(base + (target_phys_addr_t)
>                                               CPU_TIMER_OFFSET(i),
> -                                             cpu_irqs[i], master, i);
> +                                             cpu_irqs[i], master, i,
> +                                             !smp && i != 0);

This part is not OK. "mapped" should be "disabled" or something more
descriptive. Currently a functioning device is created for units that
have a corresponding CPU, and a non-functioning device for the other
slots. I think that a non-functioning device is still needed for other
slots for SMP kernels to work.

> +    if ((hwdef->machine_id == 0x80 && smp_cpus > 1) ||
> +        (hwdef->machine_id != 0x80 && smp_cpus > MAX_SUN4M_CPUS)) {
> +        fprintf(stderr,
> +                "qemu: Too many CPUs for this machine: %d maiximum %d\n",
> +                smp_cpus, hwdef->machine_id == 0x80 ? 1 : MAX_SUN4M_CPUS);
> +        exit(1);

I don't want to limit the CPUs, so a warning is enough. A cleaner
implementation is to put the CPU limit and extra timer parameters to
hwdef.

> -            fprintf(stderr, "Unable to find Sparc CPU definition\n");
> +            fprintf(stderr, "qemu: Unable to find Sparc CPU definition\n");

Unrelated, like the next few wrapping changes.