Re: [Qemu-devel] [PATCH 5/5] pseries: Move XICS initialization before cpu initialization

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Fri, 2013-03-15 at 13:33 +0100, Alexander Graf wrote:
> On 14.03.2013, at 02:53, David Gibson wrote:
> 
> > Currently, the pseries machine initializes the cpus, then the XICS
> > interrupt controller.  However, to support the upcoming in-kernel XICS
> > implementation we will need to initialize the irq controller before the
> > vcpus.  This patch makes the necesssary rearrangement.  This means the
> 
> We're changing that notion in the in-kernel XICS discussions.  The flow will look like this:
> 
>   * create vcpus
>   * create XICS
>   * foreach (vcpu)
>       * enable_cap(vcpu, CAP_XICS_SERVER, xics_handle)

This is stupid. Why have the VCPU initialize itself for non-kernel
interrupts and *then* switch it over ?

You guys are tiring me of needing about 2 years to iron out a simple
API just to end up with the worst possible crap in the end.

Ben.

> However, that means we still need to know the maximum number of supported vcpus during the create phase. That number can be bigger than smp_cpus though, since you probably want to support hotplug add of CPUs later on.
> 
> Can't we just make the number of supported "interrupt servers" a constant?
> 
> 
> Alex
> 
> > xics init code can no longer auto-detect the number of cpus ("interrupt
> > servers" in XICS terminology) and so we must pass that in explicitly from
> > the platform code.
> > 
> > Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> > Signed-off-by: Ben Herrenschmidt <benh@kernel.crashing.org>
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > hw/ppc/spapr.c |   12 +++++++-----
> > hw/ppc/xics.c  |   57 +++++++++++++++++++++++++-------------------------------
> > hw/xics.h      |    3 ++-
> > 3 files changed, 34 insertions(+), 38 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 7293082..b2c9b42 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -791,6 +791,11 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
> >         spapr->htab_shift++;
> >     }
> > 
> > +    /* Set up Interrupt Controller before we create the VCPUs */
> > +    spapr->icp = xics_system_init(smp_cpus * kvmppc_smt_threads() / smp_threads,
> > +                                  XICS_IRQS);
> > +    spapr->next_irq = XICS_IRQ_BASE;
> > +
> >     /* init CPUs */
> >     if (cpu_model == NULL) {
> >         cpu_model = kvm_enabled() ? "host" : "POWER7";
> > @@ -803,6 +808,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
> >         }
> >         env = &cpu->env;
> > 
> > +        xics_cpu_setup(spapr->icp, cpu);
> > +
> >         /* Set time-base frequency to 512 MHz */
> >         cpu_ppc_tb_init(env, TIMEBASE_FREQ);
> > 
> > @@ -842,11 +849,6 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
> >     }
> >     g_free(filename);
> > 
> > -
> > -    /* Set up Interrupt Controller */
> > -    spapr->icp = xics_system_init(XICS_IRQS);
> > -    spapr->next_irq = XICS_IRQ_BASE;
> > -
> >     /* Set up EPOW events infrastructure */
> >     spapr_events_init(spapr);
> > 
> > diff --git a/hw/ppc/xics.c b/hw/ppc/xics.c
> > index c3ef12f..374da5b 100644
> > --- a/hw/ppc/xics.c
> > +++ b/hw/ppc/xics.c
> > @@ -521,45 +521,38 @@ static void xics_reset(void *opaque)
> >     }
> > }
> > 
> > -struct icp_state *xics_system_init(int nr_irqs)
> > +void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu)
> > {
> > -    CPUPPCState *env;
> > -    CPUState *cpu;
> > -    int max_server_num;
> > -    struct icp_state *icp;
> > -    struct ics_state *ics;
> > +    CPUState *cs = CPU(cpu);
> > +    CPUPPCState *env = &cpu->env;
> > +    struct icp_server_state *ss = &icp->ss[cs->cpu_index];
> > 
> > -    max_server_num = -1;
> > -    for (env = first_cpu; env != NULL; env = env->next_cpu) {
> > -        cpu = CPU(ppc_env_get_cpu(env));
> > -        if (cpu->cpu_index > max_server_num) {
> > -            max_server_num = cpu->cpu_index;
> > -        }
> > -    }
> > +    assert(cs->cpu_index < icp->nr_servers);
> > 
> > -    icp = g_malloc0(sizeof(*icp));
> > -    icp->nr_servers = max_server_num + 1;
> > -    icp->ss = g_malloc0(icp->nr_servers*sizeof(struct icp_server_state));
> > +    switch (PPC_INPUT(env)) {
> > +    case PPC_FLAGS_INPUT_POWER7:
> > +        ss->output = env->irq_inputs[POWER7_INPUT_INT];
> > +        break;
> > 
> > -    for (env = first_cpu; env != NULL; env = env->next_cpu) {
> > -        cpu = CPU(ppc_env_get_cpu(env));
> > -        struct icp_server_state *ss = &icp->ss[cpu->cpu_index];
> > +    case PPC_FLAGS_INPUT_970:
> > +        ss->output = env->irq_inputs[PPC970_INPUT_INT];
> > +        break;
> > 
> > -        switch (PPC_INPUT(env)) {
> > -        case PPC_FLAGS_INPUT_POWER7:
> > -            ss->output = env->irq_inputs[POWER7_INPUT_INT];
> > -            break;
> > +    default:
> > +        fprintf(stderr, "XICS interrupt controller does not support this CPU "
> > +                "bus model\n");
> > +        abort();
> > +    }
> > +}
> > 
> > -        case PPC_FLAGS_INPUT_970:
> > -            ss->output = env->irq_inputs[PPC970_INPUT_INT];
> > -            break;
> > +struct icp_state *xics_system_init(int nr_servers, int nr_irqs)
> > +{
> > +    struct icp_state *icp;
> > +    struct ics_state *ics;
> > 
> > -        default:
> > -            hw_error("XICS interrupt model does not support this CPU bus "
> > -                     "model\n");
> > -            exit(1);
> > -        }
> > -    }
> > +    icp = g_malloc0(sizeof(*icp));
> > +    icp->nr_servers = nr_servers;
> > +    icp->ss = g_malloc0(icp->nr_servers*sizeof(struct icp_server_state));
> > 
> >     ics = g_malloc0(sizeof(*ics));
> >     ics->nr_irqs = nr_irqs;
> > diff --git a/hw/xics.h b/hw/xics.h
> > index c3bf008..6bce042 100644
> > --- a/hw/xics.h
> > +++ b/hw/xics.h
> > @@ -35,6 +35,7 @@ struct icp_state;
> > qemu_irq xics_get_qirq(struct icp_state *icp, int irq);
> > void xics_set_irq_type(struct icp_state *icp, int irq, bool lsi);
> > 
> > -struct icp_state *xics_system_init(int nr_irqs);
> > +struct icp_state *xics_system_init(int nr_servers, int nr_irqs);
> > +void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu);
> > 
> > #endif /* __XICS_H__ */
> > -- 
> > 1.7.10.4
> >