From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1JBAcC-0004vw-Mp for qemu-devel@nongnu.org; Sat, 05 Jan 2008 10:07:12 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1JBAcB-0004vW-V0 for qemu-devel@nongnu.org; Sat, 05 Jan 2008 10:07:12 -0500 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1JBAcB-0004vT-Qi for qemu-devel@nongnu.org; Sat, 05 Jan 2008 10:07:11 -0500 Received: from pop-satin.atl.sa.earthlink.net ([207.69.195.63]) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1JBAcB-0002pn-AI for qemu-devel@nongnu.org; Sat, 05 Jan 2008 10:07:11 -0500 Received: from user-142h2k8.cable.mindspring.com ([72.40.138.136] helo=earthlink.net) by pop-satin.atl.sa.earthlink.net with esmtp (Exim 3.36 #1) id 1JBAcA-0007MK-00 for qemu-devel@nongnu.org; Sat, 05 Jan 2008 10:07:10 -0500 Message-ID: <477F9D1F.5010809@earthlink.net> Date: Sat, 05 Jan 2008 10:07:11 -0500 From: Robert Reif MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] sparc32: fix per cpu counter/timer References: <477ED892.8060400@earthlink.net> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org This patch is trying to make qemu behave like real hardware. This is what the 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 very trusting 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. >>+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. > > 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 from a different address. >>- 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. > > 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 or saved. >>+ if (slavio_timer_is_mapped(s)) >>+ s = s->master->slave[0]; >>+ >> >> > >And here. > > Here we are redirecting accesses to mapped counter/timers to the single real one. >>- 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 immediately above 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. >> 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. > > 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. >>+ 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. > > 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 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't really support smp anyway. SMP instruction locking isn't implemented in qemu. >>- 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. > > > All but a few have qemu: in the error message. I'm just correcting an over site but that can go in a split up patch.