diff mbox

[RFC,v3,06/24] spapr: Consolidate cpu init code into a routine

Message ID 1429858066-12088-7-git-send-email-bharata@linux.vnet.ibm.com
State New
Headers show

Commit Message

Bharata B Rao April 24, 2015, 6:47 a.m. UTC 
Factor out bits of sPAPR specific CPU initialization code into
a separate routine so that it can be called from CPU hotplug
path too.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

Comments

Thomas Huth May 4, 2015, 4:10 p.m. UTC | #1 
On Fri, 24 Apr 2015 12:17:28 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> Factor out bits of sPAPR specific CPU initialization code into
> a separate routine so that it can be called from CPU hotplug
> path too.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++-------------------------
>  1 file changed, 29 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a56f9a1..5c8f2ff 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1440,6 +1440,34 @@ static void spapr_drc_reset(void *opaque)
>      }
>  }
>  
> +static void spapr_cpu_init(PowerPCCPU *cpu)
> +{
> +    CPUPPCState *env = &cpu->env;
> +
> +    /* Set time-base frequency to 512 MHz */
> +    cpu_ppc_tb_init(env, TIMEBASE_FREQ);
> +
> +    /* PAPR always has exception vectors in RAM not ROM. To ensure this,
> +     * MSR[IP] should never be set.
> +     */
> +    env->msr_mask &= ~(1 << 6);

While you're at it ... could we maybe get a proper #define for that MSR
bit? (just like the other ones in target-ppc/cpu.h)

> +    /* Tell KVM that we're in PAPR mode */
> +    if (kvm_enabled()) {
> +        kvmppc_set_papr(cpu);
> +    }
> +
> +    if (cpu->max_compat) {
> +        if (ppc_set_compat(cpu, cpu->max_compat) < 0) {
> +            exit(1);
> +        }
> +    }
> +
> +    xics_cpu_setup(spapr->icp, cpu);
> +
> +    qemu_register_reset(spapr_cpu_reset, cpu);
> +}

 Thomas
Bharata B Rao May 6, 2015, 4:28 a.m. UTC | #2 
On Mon, May 04, 2015 at 06:10:59PM +0200, Thomas Huth wrote:
> On Fri, 24 Apr 2015 12:17:28 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > Factor out bits of sPAPR specific CPU initialization code into
> > a separate routine so that it can be called from CPU hotplug
> > path too.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++-------------------------
> >  1 file changed, 29 insertions(+), 25 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index a56f9a1..5c8f2ff 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1440,6 +1440,34 @@ static void spapr_drc_reset(void *opaque)
> >      }
> >  }
> >  
> > +static void spapr_cpu_init(PowerPCCPU *cpu)
> > +{
> > +    CPUPPCState *env = &cpu->env;
> > +
> > +    /* Set time-base frequency to 512 MHz */
> > +    cpu_ppc_tb_init(env, TIMEBASE_FREQ);
> > +
> > +    /* PAPR always has exception vectors in RAM not ROM. To ensure this,
> > +     * MSR[IP] should never be set.
> > +     */
> > +    env->msr_mask &= ~(1 << 6);
> 
> While you're at it ... could we maybe get a proper #define for that MSR
> bit? (just like the other ones in target-ppc/cpu.h)

Sure will use MSR_EP here next time.

Regards,
Bharata.
Thomas Huth May 6, 2015, 6:32 a.m. UTC | #3 
On Wed, 6 May 2015 09:58:09 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Mon, May 04, 2015 at 06:10:59PM +0200, Thomas Huth wrote:
> > On Fri, 24 Apr 2015 12:17:28 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > Factor out bits of sPAPR specific CPU initialization code into
> > > a separate routine so that it can be called from CPU hotplug
> > > path too.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++-------------------------
> > >  1 file changed, 29 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index a56f9a1..5c8f2ff 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1440,6 +1440,34 @@ static void spapr_drc_reset(void *opaque)
> > >      }
> > >  }
> > >  
> > > +static void spapr_cpu_init(PowerPCCPU *cpu)
> > > +{
> > > +    CPUPPCState *env = &cpu->env;
> > > +
> > > +    /* Set time-base frequency to 512 MHz */
> > > +    cpu_ppc_tb_init(env, TIMEBASE_FREQ);
> > > +
> > > +    /* PAPR always has exception vectors in RAM not ROM. To ensure this,
> > > +     * MSR[IP] should never be set.
> > > +     */
> > > +    env->msr_mask &= ~(1 << 6);
> > 
> > While you're at it ... could we maybe get a proper #define for that MSR
> > bit? (just like the other ones in target-ppc/cpu.h)
> 
> Sure will use MSR_EP here next time.

According to the comment in cpu.h, the EP bit was for the 601 CPU only,
so I think it would be better to introduce a new #define MSR_IP 6 (or
so) here instead.

 Thomas
Bharata B Rao May 6, 2015, 8:45 a.m. UTC | #4 
On Wed, May 06, 2015 at 08:32:03AM +0200, Thomas Huth wrote:
> On Wed, 6 May 2015 09:58:09 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > On Mon, May 04, 2015 at 06:10:59PM +0200, Thomas Huth wrote:
> > > On Fri, 24 Apr 2015 12:17:28 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > 
> > > > Factor out bits of sPAPR specific CPU initialization code into
> > > > a separate routine so that it can be called from CPU hotplug
> > > > path too.
> > > > 
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > >  hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++-------------------------
> > > >  1 file changed, 29 insertions(+), 25 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index a56f9a1..5c8f2ff 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -1440,6 +1440,34 @@ static void spapr_drc_reset(void *opaque)
> > > >      }
> > > >  }
> > > >  
> > > > +static void spapr_cpu_init(PowerPCCPU *cpu)
> > > > +{
> > > > +    CPUPPCState *env = &cpu->env;
> > > > +
> > > > +    /* Set time-base frequency to 512 MHz */
> > > > +    cpu_ppc_tb_init(env, TIMEBASE_FREQ);
> > > > +
> > > > +    /* PAPR always has exception vectors in RAM not ROM. To ensure this,
> > > > +     * MSR[IP] should never be set.
> > > > +     */
> > > > +    env->msr_mask &= ~(1 << 6);
> > > 
> > > While you're at it ... could we maybe get a proper #define for that MSR
> > > bit? (just like the other ones in target-ppc/cpu.h)
> > 
> > Sure will use MSR_EP here next time.
> 
> According to the comment in cpu.h, the EP bit was for the 601 CPU only,
> so I think it would be better to introduce a new #define MSR_IP 6 (or
> so) here instead.

Kernel defines bit 6 as
#define MSR_IP_LG       6               /* Exception prefix 0x000/0xFFF */
(arch/powerpc/include/asm/reg.h)

QEMU defines it as
#define MSR_EP   6  /* Exception prefix on 601                               */

I can add MSR_IP in QEMU, but that will mean two defines for same bit position,
but I think MSR_IP_LG in kernel or MSR_EP in QEMU both mean the same, but
called differently.

Regards,
Bharata.
Thomas Huth May 6, 2015, 9:37 a.m. UTC | #5 
On Wed, 6 May 2015 14:15:37 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Wed, May 06, 2015 at 08:32:03AM +0200, Thomas Huth wrote:
> > On Wed, 6 May 2015 09:58:09 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > On Mon, May 04, 2015 at 06:10:59PM +0200, Thomas Huth wrote:
> > > > On Fri, 24 Apr 2015 12:17:28 +0530
> > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > 
> > > > > Factor out bits of sPAPR specific CPU initialization code into
> > > > > a separate routine so that it can be called from CPU hotplug
> > > > > path too.
> > > > > 
> > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > ---
> > > > >  hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++-------------------------
> > > > >  1 file changed, 29 insertions(+), 25 deletions(-)
> > > > > 
> > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > index a56f9a1..5c8f2ff 100644
> > > > > --- a/hw/ppc/spapr.c
> > > > > +++ b/hw/ppc/spapr.c
> > > > > @@ -1440,6 +1440,34 @@ static void spapr_drc_reset(void *opaque)
> > > > >      }
> > > > >  }
> > > > >  
> > > > > +static void spapr_cpu_init(PowerPCCPU *cpu)
> > > > > +{
> > > > > +    CPUPPCState *env = &cpu->env;
> > > > > +
> > > > > +    /* Set time-base frequency to 512 MHz */
> > > > > +    cpu_ppc_tb_init(env, TIMEBASE_FREQ);
> > > > > +
> > > > > +    /* PAPR always has exception vectors in RAM not ROM. To ensure this,
> > > > > +     * MSR[IP] should never be set.
> > > > > +     */
> > > > > +    env->msr_mask &= ~(1 << 6);
> > > > 
> > > > While you're at it ... could we maybe get a proper #define for that MSR
> > > > bit? (just like the other ones in target-ppc/cpu.h)
> > > 
> > > Sure will use MSR_EP here next time.
> > 
> > According to the comment in cpu.h, the EP bit was for the 601 CPU only,
> > so I think it would be better to introduce a new #define MSR_IP 6 (or
> > so) here instead.
> 
> Kernel defines bit 6 as
> #define MSR_IP_LG       6               /* Exception prefix 0x000/0xFFF */
> (arch/powerpc/include/asm/reg.h)
> 
> QEMU defines it as
> #define MSR_EP   6  /* Exception prefix on 601                               */
> 
> I can add MSR_IP in QEMU, but that will mean two defines for same bit position,
> but I think MSR_IP_LG in kernel or MSR_EP in QEMU both mean the same, but
> called differently.

Ok, so EP = IP ... then I also think it's fine to use the MSR_EP define
here. I first thought that EP and IP would mean something different,
since a lot of MSR bits are defined differently on the various
POWER/PowerPC chips (see the MSR bit 10 for example, it has three
defines in cpu.h), but in this case they really seem to be the same.

By the way, is this bit still used at all on recent chips (which are
used for the spapr machine)? It's apparently not defined in the PowerISA
spec anymore...

 Thomas
diff mbox

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a56f9a1..5c8f2ff 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1440,6 +1440,34 @@  static void spapr_drc_reset(void *opaque)
     }
 }
 
+static void spapr_cpu_init(PowerPCCPU *cpu)
+{
+    CPUPPCState *env = &cpu->env;
+
+    /* Set time-base frequency to 512 MHz */
+    cpu_ppc_tb_init(env, TIMEBASE_FREQ);
+
+    /* PAPR always has exception vectors in RAM not ROM. To ensure this,
+     * MSR[IP] should never be set.
+     */
+    env->msr_mask &= ~(1 << 6);
+
+    /* Tell KVM that we're in PAPR mode */
+    if (kvm_enabled()) {
+        kvmppc_set_papr(cpu);
+    }
+
+    if (cpu->max_compat) {
+        if (ppc_set_compat(cpu, cpu->max_compat) < 0) {
+            exit(1);
+        }
+    }
+
+    xics_cpu_setup(spapr->icp, cpu);
+
+    qemu_register_reset(spapr_cpu_reset, cpu);
+}
+
 /* pSeries LPAR / sPAPR hardware init */
 static void ppc_spapr_init(MachineState *machine)
 {
@@ -1451,7 +1479,6 @@  static void ppc_spapr_init(MachineState *machine)
     const char *initrd_filename = machine->initrd_filename;
     const char *boot_device = machine->boot_order;
     PowerPCCPU *cpu;
-    CPUPPCState *env;
     PCIHostState *phb;
     int i;
     MemoryRegion *sysmem = get_system_memory();
@@ -1549,30 +1576,7 @@  static void ppc_spapr_init(MachineState *machine)
             fprintf(stderr, "Unable to find PowerPC CPU definition\n");
             exit(1);
         }
-        env = &cpu->env;
-
-        /* Set time-base frequency to 512 MHz */
-        cpu_ppc_tb_init(env, TIMEBASE_FREQ);
-
-        /* PAPR always has exception vectors in RAM not ROM. To ensure this,
-         * MSR[IP] should never be set.
-         */
-        env->msr_mask &= ~(1 << 6);
-
-        /* Tell KVM that we're in PAPR mode */
-        if (kvm_enabled()) {
-            kvmppc_set_papr(cpu);
-        }
-
-        if (cpu->max_compat) {
-            if (ppc_set_compat(cpu, cpu->max_compat) < 0) {
-                exit(1);
-            }
-        }
-
-        xics_cpu_setup(spapr->icp, cpu);
-
-        qemu_register_reset(spapr_cpu_reset, cpu);
+        spapr_cpu_init(cpu);
     }
 
     /* allocate RAM */