Re: [Qemu-devel] [RFC PATCH v3 06/24] spapr: Consolidate cpu init code into a routine

[Thomas Huth <thuth@redhat.com>]

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