Re: [Qemu-devel] [PATCH] sparc32: fix per cpu counter/timer
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);
-
Re: [Qemu-devel] [PATCH] sparc32: fix per cpu counter/timer
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]->s
[Qemu-devel] [PATCH] sparc32: fix per cpu counter/timer
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.
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.
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.
Index: hw/sbi.c
===
RCS file: /sources/qemu/qemu/hw/sbi.c,v
retrieving revision 1.2
diff -p -u -r1.2 sbi.c
--- hw/sbi.c1 Jan 2008 17:06:38 - 1.2
+++ hw/sbi.c5 Jan 2008 00:43:27 -
@@ -34,15 +34,13 @@ do { printf("IRQ: " fmt , ##args); } whi
#define DPRINTF(fmt, args...)
#endif
-#define MAX_CPUS 16
-
#define SBI_NREGS 16
typedef struct SBIState {
uint32_t regs[SBI_NREGS];
-uint32_t intreg_pending[MAX_CPUS];
-qemu_irq *cpu_irqs[MAX_CPUS];
-uint32_t pil_out[MAX_CPUS];
+uint32_t intreg_pending[MAX_SUN4D_CPUS];
+qemu_irq *cpu_irqs[MAX_SUN4D_CPUS];
+uint32_t pil_out[MAX_SUN4D_CPUS];
} SBIState;
#define SBI_SIZE (SBI_NREGS * 4)
@@ -107,7 +105,7 @@ static void sbi_save(QEMUFile *f, void *
SBIState *s = opaque;
unsigned int i;
-for (i = 0; i < MAX_CPUS; i++) {
+for (i = 0; i < MAX_SUN4D_CPUS; i++) {
qemu_put_be32s(f, &s->intreg_pending[i]);
}
}
@@ -120,7 +118,7 @@ static int sbi_load(QEMUFile *f, void *o
if (version_id != 1)
return -EINVAL;
-for (i = 0; i < MAX_CPUS; i++) {
+for (i = 0; i < MAX_SUN4D_CPUS; i++) {
qemu_get_be32s(f, &s->intreg_pending[i]);
}
sbi_check_interrupts(s);
@@ -133,7 +131,7 @@ static void sbi_reset(void *opaque)
SBIState *s = opaque;
unsigned int i;
-for (i = 0; i < MAX_CPUS; i++) {
+for (i = 0; i < MAX_SUN4D_CPUS; i++) {
s->intreg_pending[i] = 0;
}
sbi_check_interrupts(s);
@@ -150,7 +148,7 @@ void *sbi_init(target_phys_addr_t addr,
if (!s)
return NULL;
-for (i = 0; i < MAX_CPUS; i++) {
+for (i = 0; i < MAX_SUN4D_CPUS; i++) {
s->cpu_irqs[i] = parent_irq[i];
}
@@ -160,7 +158,7 @@ void *sbi_init(target_phys_addr_t addr,
register_savevm("sbi", addr, 1, sbi_save, sbi_load, s);
qemu_register_reset(sbi_reset, s);
*irq = qemu_allocate_irqs(sbi_set_irq, s, 32);
-*cpu_irq = qemu_allocate_irqs(sbi_set_timer_irq_cpu, s, MAX_CPUS);
+*cpu_irq = qemu_allocate_irqs(sbi_set_timer_irq_cpu, s, MAX_SUN4D_CPUS);
sbi_reset(s);
return s;
Index: hw/slavio_intctl.c
===
RCS file: /sources/qemu/qemu/hw/slavio_intctl.c,v
retrieving revision 1.29
diff -p -u -r1.29 slavio_intctl.c
--- hw/slavio_intctl.c 1 Jan 2008 20:57:25 - 1.29
+++ hw/slavio_intctl.c 5 Jan 2008 00:43:27 -
@@ -46,21 +46,20 @@ do { printf("IRQ: " fmt , ##args); } whi
*
*/
-#define MAX_CPUS 16
#define MAX_PILS 16
typedef struct SLAVIO_INTCTLState {
-uint32_t intreg_pending[MAX_CPUS];
+uint32_t intreg_pending[MAX_SUN4M_CPUS];
uint32_t intregm_pending;
uint32_t intregm_disabled;
uint32_t target_cpu;
#ifdef DEBUG_IRQ_COUNT
uint64_t irq_count[32];
#endif
-qemu_irq *cpu_irqs[MAX_CPUS];
+qemu_irq *cpu_irqs[MAX_SUN4M_CPUS];
const uint32_t *intbit_to_level;
uint32_t cputimer_lbit, cputimer_mbit;
-uint32_t pil_out[MAX_CPUS];
+uint32_t pil_out[MAX_SUN4M_CPUS];
} SLAVIO_INTCTLState;
#define INTCTL_MAXADDR 0xf
@@ -84,7 +83,7 @@ static uint32_t slavio_intctl_mem_readl(
uint32_t saddr, ret;
int cpu;
-cpu = (addr & (MAX_CPUS - 1) * TARGET_PAGE_SIZE) >> 12;
+cpu = (addr & (MAX_SUN4M_CPUS - 1) * TARGET_PAGE_SIZE) >> 12;
saddr = (addr & INTCTL_MAXADDR) >> 2;
switch (saddr) {
case 0:
@@ -105,7 +104,7 @@ static void slavio_intctl_mem_writel(voi
uint32_t saddr;
int cpu;
-cpu = (addr & (MAX_CPUS - 1) * TARGET_PAGE_SIZE) >> 12;
+cpu = (addr & (MAX_SUN4M_CPUS - 1) * TARGET_PAGE_SIZE) >> 12;
saddr = (addr & INTCTL_MAXADDR) >> 2;
DPRINTF("write cpu %d reg 0x" TARGET_FMT_plx " = %x\n", cpu, addr, val);
switch (saddr) {
@@ -190,7 +18
