[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] sparc32: fix per cpu counter/timer
From: |
Blue Swirl |
Subject: |
Re: [Qemu-devel] [PATCH] sparc32: fix per cpu counter/timer |
Date: |
Sat, 5 Jan 2008 13:30:43 +0200 |
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.