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

[Alexander Graf]

Am 16.03.2013 um 04:14 schrieb Benjamin Herrenschmidt <address@hidden>:

> 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 ?

Because non-kernel initialization is a nop.

Alex

> 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 <address@hidden>
>>> Signed-off-by: Ben Herrenschmidt <address@hidden>
>>> Signed-off-by: David Gibson <address@hidden>
>>> ---
>>> 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
> 
>