diff mbox

[03/12] pseries: Move XICS initialization before cpu initialization

Message ID 1352774820-22804-4-git-send-email-david@gibson.dropbear.id.au
State New
Headers show

Commit Message

David Gibson Nov. 13, 2012, 2:46 a.m. UTC 
From: Ben Herrenschmidt <benh@kernel.crashing.org>

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
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/spapr.c |   12 +++++++-----
 hw/xics.c  |   52 +++++++++++++++++++++++-----------------------------
 hw/xics.h  |    3 ++-
 3 files changed, 32 insertions(+), 35 deletions(-)

Comments

Alexander Graf Nov. 19, 2012, 4:22 p.m. UTC | #1 
On 13.11.2012, at 03:46, David Gibson wrote:

> From: Ben Herrenschmidt <benh@kernel.crashing.org>
> 
> 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
> 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.

Does this still hold true with the new in-kernel interrupt controller workflow?


Alex

> 
> 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/spapr.c |   12 +++++++-----
> hw/xics.c  |   52 +++++++++++++++++++++++-----------------------------
> hw/xics.h  |    3 ++-
> 3 files changed, 32 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/spapr.c b/hw/spapr.c
> index eafee03..dc2349c 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -747,6 +747,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";
> @@ -759,6 +764,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>         }
>         env = &cpu->env;
> 
> +        xics_cpu_setup(spapr->icp, env);
> +
>         /* Set time-base frequency to 512 MHz */
>         cpu_ppc_tb_init(env, TIMEBASE_FREQ);
> 
> @@ -798,11 +805,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/xics.c b/hw/xics.c
> index b8887cd..f72dfae 100644
> --- a/hw/xics.c
> +++ b/hw/xics.c
> @@ -510,42 +510,36 @@ static void xics_reset(void *opaque)
>     }
> }
> 
> -struct icp_state *xics_system_init(int nr_irqs)
> +void xics_cpu_setup(struct icp_state *icp, CPUPPCState *env)
> {
> -    CPUPPCState *env;
> -    int max_server_num;
> -    struct icp_state *icp;
> -    struct ics_state *ics;
> +    struct icp_server_state *ss = &icp->ss[env->cpu_index];
> 
> -    max_server_num = -1;
> -    for (env = first_cpu; env != NULL; env = env->next_cpu) {
> -        if (env->cpu_index > max_server_num) {
> -            max_server_num = env->cpu_index;
> -        }
> -    }
> +    assert(env->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) {
> -        struct icp_server_state *ss = &icp->ss[env->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..b43678a 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, CPUPPCState *env);
> 
> #endif /* __XICS_H__ */
> -- 
> 1.7.10.4
>
Benjamin Herrenschmidt Nov. 19, 2012, 7:54 p.m. UTC | #2 
On Mon, 2012-11-19 at 17:22 +0100, Alexander Graf 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
> > 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.
> 
> Does this still hold true with the new in-kernel interrupt controller workflow?

We need to look into this. The in-kernel ICPs will still certainly be
created early along with the VCPUs, however we might be able to delay
the creation of the qemu emulation when not using the former.

Cheers,
Ben.
David Gibson Nov. 19, 2012, 10:47 p.m. UTC | #3 
On Tue, Nov 20, 2012 at 06:54:20AM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2012-11-19 at 17:22 +0100, Alexander Graf 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
> > > 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.
> > 
> > Does this still hold true with the new in-kernel interrupt controller workflow?
> 
> We need to look into this. The in-kernel ICPs will still certainly be
> created early along with the VCPUs, however we might be able to delay
> the creation of the qemu emulation when not using the former.

I'd really prefer not to have two different initialization orders in
qemu though.  I think we'll want the ICP initialization first for CPU
hotplug one day anyway.
Alexander Graf Nov. 20, 2012, 9:26 a.m. UTC | #4 
On 19.11.2012, at 23:47, David Gibson wrote:

> On Tue, Nov 20, 2012 at 06:54:20AM +1100, Benjamin Herrenschmidt wrote:
>> On Mon, 2012-11-19 at 17:22 +0100, Alexander Graf 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
>>>> 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.
>>> 
>>> Does this still hold true with the new in-kernel interrupt controller workflow?
>> 
>> We need to look into this. The in-kernel ICPs will still certainly be
>> created early along with the VCPUs, however we might be able to delay
>> the creation of the qemu emulation when not using the former.
> 
> I'd really prefer not to have two different initialization orders in
> qemu though.  I think we'll want the ICP initialization first for CPU
> hotplug one day anyway.

I'd just leave this patch out until you have an implementation of the new in-kernel interrupt controller model as discussed during KVM Forum. If it still makes sense by then, we can always apply it along with it :). You most likely want to reshuffle code by then anyway, so this patch would just be needless churn.


Alex
David Gibson Nov. 21, 2012, 1:10 a.m. UTC | #5 
On Tue, Nov 20, 2012 at 10:26:06AM +0100, Alexander Graf wrote:
> 
> On 19.11.2012, at 23:47, David Gibson wrote:
> 
> > On Tue, Nov 20, 2012 at 06:54:20AM +1100, Benjamin Herrenschmidt wrote:
> >> On Mon, 2012-11-19 at 17:22 +0100, Alexander Graf 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
> >>>> 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.
> >>> 
> >>> Does this still hold true with the new in-kernel interrupt controller workflow?
> >> 
> >> We need to look into this. The in-kernel ICPs will still certainly be
> >> created early along with the VCPUs, however we might be able to delay
> >> the creation of the qemu emulation when not using the former.
> > 
> > I'd really prefer not to have two different initialization orders in
> > qemu though.  I think we'll want the ICP initialization first for CPU
> > hotplug one day anyway.
> 
> I'd just leave this patch out until you have an implementation of
> the new in-kernel interrupt controller model as discussed during KVM
> Forum. If it still makes sense by then, we can always apply it along
> with it :). You most likely want to reshuffle code by then anyway,
> so this patch would just be needless churn.

Ok.
diff mbox

Patch

diff --git a/hw/spapr.c b/hw/spapr.c
index eafee03..dc2349c 100644
--- a/hw/spapr.c
+++ b/hw/spapr.c
@@ -747,6 +747,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";
@@ -759,6 +764,8 @@  static void ppc_spapr_init(QEMUMachineInitArgs *args)
         }
         env = &cpu->env;
 
+        xics_cpu_setup(spapr->icp, env);
+
         /* Set time-base frequency to 512 MHz */
         cpu_ppc_tb_init(env, TIMEBASE_FREQ);
 
@@ -798,11 +805,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/xics.c b/hw/xics.c
index b8887cd..f72dfae 100644
--- a/hw/xics.c
+++ b/hw/xics.c
@@ -510,42 +510,36 @@  static void xics_reset(void *opaque)
     }
 }
 
-struct icp_state *xics_system_init(int nr_irqs)
+void xics_cpu_setup(struct icp_state *icp, CPUPPCState *env)
 {
-    CPUPPCState *env;
-    int max_server_num;
-    struct icp_state *icp;
-    struct ics_state *ics;
+    struct icp_server_state *ss = &icp->ss[env->cpu_index];
 
-    max_server_num = -1;
-    for (env = first_cpu; env != NULL; env = env->next_cpu) {
-        if (env->cpu_index > max_server_num) {
-            max_server_num = env->cpu_index;
-        }
-    }
+    assert(env->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) {
-        struct icp_server_state *ss = &icp->ss[env->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..b43678a 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, CPUPPCState *env);
 
 #endif /* __XICS_H__ */