Re: [Qemu-ppc] [PATCH 01/10] ppc: Fix rfi/rfid/hrfi/... emulation

[David Gibson]

On Thu, Jun 16, 2016 at 11:07:02AM +1000, David Gibson wrote:
> On Mon, Jun 13, 2016 at 07:24:47AM +0200, Cédric Le Goater wrote:
> > From: Benjamin Herrenschmidt <address@hidden>
> > 
> > This reworks emulation of the various "rfi" variants. I removed
> > some masking bits that I couldn't make sense of, the only bit that
> > I am aware we should mask here is POW, the CPU's MSR mask should
> > take care of the rest.
> > 
> > This also fixes some problems when running 32-bit userspace under
> > a 64-bit kernel.
> > 
> > Signed-off-by: Benjamin Herrenschmidt <address@hidden>
> > Reviewed-by: David Gibson <address@hidden>
> 
> I've merged this patch to ppc-for-2.7.

..and now I've removed it again.  It seems that this breaks Thomas'
new test that OpenBIOS runs on the mac machine types.  Not sure why,
but we need to figure that out before I apply.

> The reset of the series I'd like to see a respin for, even if it's
> just to clean up the commit messages.
> 
> > ---
> >  target-ppc/excp_helper.c | 51 
> > +++++++++++++++++++-----------------------------
> >  target-ppc/translate.c   |  7 +++++++
> >  2 files changed, 27 insertions(+), 31 deletions(-)
> > 
> > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> > index 30e960e30b63..aa0b63f4b0de 100644
> > --- a/target-ppc/excp_helper.c
> > +++ b/target-ppc/excp_helper.c
> > @@ -922,25 +922,20 @@ void helper_store_msr(CPUPPCState *env, target_ulong 
> > val)
> >      }
> >  }
> >  
> > -static inline void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong 
> > msr,
> > -                          target_ulong msrm, int keep_msrh)
> > +static inline void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong 
> > msr)
> >  {
> >      CPUState *cs = CPU(ppc_env_get_cpu(env));
> >  
> > +    /* MSR:POW cannot be set by any form of rfi */
> > +    msr &= ~(1ULL << MSR_POW);
> > +
> >  #if defined(TARGET_PPC64)
> > -    if (msr_is_64bit(env, msr)) {
> > -        nip = (uint64_t)nip;
> > -        msr &= (uint64_t)msrm;
> > -    } else {
> > +    /* Switching to 32-bit ? Crop the nip */
> > +    if (!msr_is_64bit(env, msr)) {
> >          nip = (uint32_t)nip;
> > -        msr = (uint32_t)(msr & msrm);
> > -        if (keep_msrh) {
> > -            msr |= env->msr & ~((uint64_t)0xFFFFFFFF);
> > -        }
> >      }
> >  #else
> >      nip = (uint32_t)nip;
> > -    msr &= (uint32_t)msrm;
> >  #endif
> >      /* XXX: beware: this is false if VLE is supported */
> >      env->nip = nip & ~((target_ulong)0x00000003);
> > @@ -959,26 +954,24 @@ static inline void do_rfi(CPUPPCState *env, 
> > target_ulong nip, target_ulong msr,
> >  
> >  void helper_rfi(CPUPPCState *env)
> >  {
> > -    if (env->excp_model == POWERPC_EXCP_BOOKE) {
> > -        do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1],
> > -               ~((target_ulong)0), 0);
> > -    } else {
> > -        do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1],
> > -               ~((target_ulong)0x783F0000), 1);
> > -    }
> > +    do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1] & 0xfffffffful);
> >  }
> >  
> > +#define MSR_BOOK3S_MASK
> >  #if defined(TARGET_PPC64)
> >  void helper_rfid(CPUPPCState *env)
> >  {
> > -    do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1],
> > -           ~((target_ulong)0x783F0000), 0);
> > +    /* The architeture defines a number of rules for which bits
> > +     * can change but in practice, we handle this in hreg_store_msr()
> > +     * which will be called by do_rfi(), so there is no need to filter
> > +     * here
> > +     */
> > +    do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1]);
> >  }
> >  
> >  void helper_hrfid(CPUPPCState *env)
> >  {
> > -    do_rfi(env, env->spr[SPR_HSRR0], env->spr[SPR_HSRR1],
> > -           ~((target_ulong)0x783F0000), 0);
> > +    do_rfi(env, env->spr[SPR_HSRR0], env->spr[SPR_HSRR1]);
> >  }
> >  #endif
> >  
> > @@ -986,28 +979,24 @@ void helper_hrfid(CPUPPCState *env)
> >  /* Embedded PowerPC specific helpers */
> >  void helper_40x_rfci(CPUPPCState *env)
> >  {
> > -    do_rfi(env, env->spr[SPR_40x_SRR2], env->spr[SPR_40x_SRR3],
> > -           ~((target_ulong)0xFFFF0000), 0);
> > +    do_rfi(env, env->spr[SPR_40x_SRR2], env->spr[SPR_40x_SRR3]);
> >  }
> >  
> >  void helper_rfci(CPUPPCState *env)
> >  {
> > -    do_rfi(env, env->spr[SPR_BOOKE_CSRR0], env->spr[SPR_BOOKE_CSRR1],
> > -           ~((target_ulong)0), 0);
> > +    do_rfi(env, env->spr[SPR_BOOKE_CSRR0], env->spr[SPR_BOOKE_CSRR1]);
> >  }
> >  
> >  void helper_rfdi(CPUPPCState *env)
> >  {
> >      /* FIXME: choose CSRR1 or DSRR1 based on cpu type */
> > -    do_rfi(env, env->spr[SPR_BOOKE_DSRR0], env->spr[SPR_BOOKE_DSRR1],
> > -           ~((target_ulong)0), 0);
> > +    do_rfi(env, env->spr[SPR_BOOKE_DSRR0], env->spr[SPR_BOOKE_DSRR1]);
> >  }
> >  
> >  void helper_rfmci(CPUPPCState *env)
> >  {
> >      /* FIXME: choose CSRR1 or MCSRR1 based on cpu type */
> > -    do_rfi(env, env->spr[SPR_BOOKE_MCSRR0], env->spr[SPR_BOOKE_MCSRR1],
> > -           ~((target_ulong)0), 0);
> > +    do_rfi(env, env->spr[SPR_BOOKE_MCSRR0], env->spr[SPR_BOOKE_MCSRR1]);
> >  }
> >  #endif
> >  
> > @@ -1045,7 +1034,7 @@ void helper_td(CPUPPCState *env, target_ulong arg1, 
> > target_ulong arg2,
> >  
> >  void helper_rfsvc(CPUPPCState *env)
> >  {
> > -    do_rfi(env, env->lr, env->ctr, 0x0000FFFF, 0);
> > +    do_rfi(env, env->lr, env->ctr & 0x0000FFFF);
> >  }
> >  
> >  /* Embedded.Processor Control */
> > diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> > index b6894751e8df..a02ddf52bfe6 100644
> > --- a/target-ppc/translate.c
> > +++ b/target-ppc/translate.c
> > @@ -4087,6 +4087,13 @@ static void gen_rfi(DisasContext *ctx)
> >  #if defined(CONFIG_USER_ONLY)
> >      gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >  #else
> > +    /* This instruction doesn't exist anymore on 64-bit server
> > +     * processors compliant with arch 2.x
> > +     */
> > +    if (ctx->insns_flags & PPC_SEGMENT_64B) {
> > +        gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
> > +        return;
> > +    }
> >      /* Restore CPU state */
> >      if (unlikely(ctx->pr)) {
> >          gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> 



-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature