On 1/5/08, Robert Reif <[EMAIL PROTECTED]> 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.