Re: [Qemu-devel] [PATCH] sparc32: fix per cpu counter/timer

[Robert Reif]

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.