|
From: | Robert Reif |
Subject: | Re: [Qemu-devel] [PATCH] sparc32: fix per cpu counter/timer |
Date: | Sat, 05 Jan 2008 10:07:11 -0500 |
User-agent: | Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4.2) Gecko/20040308 |
This patch is trying to make qemu behave like real hardware. This is whatthe OSs expect. The ability to create hardware that never existed and can't
exist due to real hardware limitations is cool but it's not going to work properly with existing OSs. At best you will have the OS never accessing the extra hardware because of known hardwired limits in the OS or worse, you will have the OS accessing off the end of fixed size arrays. Neither are fixable without changing the OS. I know that the boot prom provides a layer of abstraction between the hardware and the OS and linux is verytrusting of what the prom provides, even when it makes no sense. However there are some assumptions that the OS writers knew about and
do ignore what the prom says. You can have the prom tell linux that there a a million CPUs and it will happily print out that the prom said there are a million CPUs but it knows that there are 4 and has fixed size arrays for just those 4 :-) Blue Swirl wrote:
- 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.
The number is a constant, 1 for UP and 4 for SMP. That's defined by the real hardware. The OS writers knew that.
This doesn't change any logic. It's just a helper to determine if the counter/timer being accessed is really the one at that address or another one being accessed+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.
from a different address.
There are never any counter/timers created with s->timer being NULL. We are creating alternate address ranges for the mapped counter/timers so qemu is happy but we redirect access to those spaces to a single real counter/timer. The alternate address spaces are never accessed. Thats why they and not initialized, reset, loaded- 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.
or saved.
Here we are redirecting accesses to mapped counter/timers to the single real one.+ 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.
The original code was really broken and not salvagable. This code only makes changes when something actually changes and makes the proper changes to reflect how the hardware actually works. The code immediatelyabove this also makes sure that that only real hardware is accessed. I could break this up into less stages of brokenness until it's finally fixed but those
intermediate patches would still be broken, just less so.
The counter/timer is not disabled. It is not there. The memory space is allocated for it in qemu but all accesses are redirected (mapped) to the single real counter/timer.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.
That's the whole point of this patch. There never were any sun4m SMP machines with more than 4 CPUs and there never will be. OS writers know that. That's why linux has fixed size arrays for per-cpu resources. It's a a hardware and+ 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.
architectural limitation for sun4m machines. You would need to patch linux to make more than 4 CPU work. What's the point of doing that. qemu doesn'treally support smp anyway. SMP instruction locking isn't implemented in qemu.
All but a few have qemu: in the error message. I'm just correcting an over site but that- 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.
can go in a split up patch.
[Prev in Thread] | Current Thread | [Next in Thread] |