diff mbox series

What's the correct way to implement rfi and related instruction.

Message ID CAE2XoE84K6vdQ23upRa1MaCNWSycUGKja9DrTpVCQ4bdY7bZuQ@mail.gmail.com (mailing list archive)
State New, archived
Headers show
Series What's the correct way to implement rfi and related instruction. | expand

Commit Message

罗勇刚(Yonggang Luo) Jan. 7, 2021, 7:14 p.m. UTC
This is the first patch,:
It's store MSR bits differntly for different rfi instructions:
[Qemu-devel] [PATCH] target-ppc: fix RFI by clearing some bits of MSR
https://lists.gnu.org/archive/html/qemu-devel/2010-05/msg02999.html
Comes from  target-ppc: fix RFI by clearing some bits of MSR
SHA-1: c3d420ead1aee9fcfd12be11cbdf6b1620134773
 target-ppc/op_helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
```
 #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 */
```

And of cause, the second patch fixes some problem, but also cause new
problem, how to implement these instruction properly?



--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

Comments

Cédric Le Goater Jan. 7, 2021, 9:54 p.m. UTC | #1
On 1/7/21 8:14 PM, 罗勇刚(Yonggang Luo) wrote:
> This is the first patch,:
> It's store MSR bits differntly for different rfi instructions:
> [Qemu-devel] [PATCH] target-ppc: fix RFI by clearing some bits of MSR
> https://lists.gnu.org/archive/html/qemu-devel/2010-05/msg02999.html <https://lists.gnu.org/archive/html/qemu-devel/2010-05/msg02999.html>
> Comes from  target-ppc: fix RFI by clearing some bits of MSR
> SHA-1: c3d420ead1aee9fcfd12be11cbdf6b1620134773
>  target-ppc/op_helper.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> ```
> diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
> index 8f2ee986bb..3c3aa60bc3 100644
> --- a/target-ppc/op_helper.c
> +++ b/target-ppc/op_helper.c
> @@ -1646,20 +1646,20 @@ static inline void do_rfi(target_ulong nip, target_ulong msr,
>  void helper_rfi (void)
>  {
>      do_rfi(env->spr[SPR_SRR0], env->spr[SPR_SRR1],
> -           ~((target_ulong)0x0), 1);
> +           ~((target_ulong)0x783F0000), 1);
>  }
>  
>  #if defined(TARGET_PPC64)
>  void helper_rfid (void)
>  {
>      do_rfi(env->spr[SPR_SRR0], env->spr[SPR_SRR1],
> -           ~((target_ulong)0x0), 0);
> +           ~((target_ulong)0x783F0000), 0);
>  }
>  
>  void helper_hrfid (void)
>  {
>      do_rfi(env->spr[SPR_HSRR0], env->spr[SPR_HSRR1],
> -           ~((target_ulong)0x0), 0);
> +           ~((target_ulong)0x783F0000), 0);
>  }
>  #endif
>  #endif
> ```
> 
> This is the second patch,:
> it's remove the parameter  `target_ulong msrm, int keep_msrh`
> Comes from ppc: Fix rfi/rfid/hrfi/... emulation
> SHA-1: a2e71b28e832346409efc795ecd1f0a2bcb705a3
> ```
>  target-ppc/excp_helper.c | 51 +++++++++++++++++++-----------------------------
>  1 file changed, 20 insertions(+), 31 deletions(-)
> 
> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> index 30e960e30b..aa0b63f4b0 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 */
> ```
> 
> And of cause, the second patch fixes some problem, but also cause new problem, 
> how to implement these instruction properly?

What are the new problems  ? 

C.

> 
> 
> 
> --
>          此致
> 礼
> 罗勇刚
> Yours
>     sincerely,
> Yonggang Luo
罗勇刚(Yonggang Luo) Jan. 8, 2021, 4:21 a.m. UTC | #2
On Fri, Jan 8, 2021 at 5:54 AM Cédric Le Goater <clg@kaod.org> wrote:
>
> On 1/7/21 8:14 PM, 罗勇刚(Yonggang Luo) wrote:
> > This is the first patch,:
> > It's store MSR bits differntly for different rfi instructions:
> > [Qemu-devel] [PATCH] target-ppc: fix RFI by clearing some bits of MSR
> > https://lists.gnu.org/archive/html/qemu-devel/2010-05/msg02999.html <
https://lists.gnu.org/archive/html/qemu-devel/2010-05/msg02999.html>
> > Comes from  target-ppc: fix RFI by clearing some bits of MSR
> > SHA-1: c3d420ead1aee9fcfd12be11cbdf6b1620134773
> >  target-ppc/op_helper.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > ```
> > diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
> > index 8f2ee986bb..3c3aa60bc3 100644
> > --- a/target-ppc/op_helper.c
> > +++ b/target-ppc/op_helper.c
> > @@ -1646,20 +1646,20 @@ static inline void do_rfi(target_ulong nip,
target_ulong msr,
> >  void helper_rfi (void)
> >  {
> >      do_rfi(env->spr[SPR_SRR0], env->spr[SPR_SRR1],
> > -           ~((target_ulong)0x0), 1);
> > +           ~((target_ulong)0x783F0000), 1);
> >  }
> >
> >  #if defined(TARGET_PPC64)
> >  void helper_rfid (void)
> >  {
> >      do_rfi(env->spr[SPR_SRR0], env->spr[SPR_SRR1],
> > -           ~((target_ulong)0x0), 0);
> > +           ~((target_ulong)0x783F0000), 0);
> >  }
> >
> >  void helper_hrfid (void)
> >  {
> >      do_rfi(env->spr[SPR_HSRR0], env->spr[SPR_HSRR1],
> > -           ~((target_ulong)0x0), 0);
> > +           ~((target_ulong)0x783F0000), 0);
> >  }
> >  #endif
> >  #endif
> > ```
> >
> > This is the second patch,:
> > it's remove the parameter  `target_ulong msrm, int keep_msrh`
> > Comes from ppc: Fix rfi/rfid/hrfi/... emulation
> > SHA-1: a2e71b28e832346409efc795ecd1f0a2bcb705a3
> > ```
> >  target-ppc/excp_helper.c | 51
+++++++++++++++++++-----------------------------
> >  1 file changed, 20 insertions(+), 31 deletions(-)
> >
> > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> > index 30e960e30b..aa0b63f4b0 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 */
> > ```
> >
> > And of cause, the second patch fixes some problem, but also cause new
problem,
> > how to implement these instruction properly?
>
> What are the new problems  ?
Before this patch, VxWorks can working, but after this, VxWorks can not
boot anymore.

>
> C.
>
> >
> >
> >
> > --
> >          此致
> > 礼
> > 罗勇刚
> > Yours
> >     sincerely,
> > Yonggang Luo
>


--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo
Cédric Le Goater Jan. 8, 2021, 10:02 a.m. UTC | #3
On 1/8/21 5:21 AM, 罗勇刚(Yonggang Luo) wrote:
> 
> 
> On Fri, Jan 8, 2021 at 5:54 AM Cédric Le Goater <clg@kaod.org <mailto:clg@kaod.org>> wrote:
>>
>> On 1/7/21 8:14 PM, 罗勇刚(Yonggang Luo) wrote:
>> > This is the first patch,:
>> > It's store MSR bits differntly for different rfi instructions:
>> > [Qemu-devel] [PATCH] target-ppc: fix RFI by clearing some bits of MSR
>> > https://lists.gnu.org/archive/html/qemu-devel/2010-05/msg02999.html <https://lists.gnu.org/archive/html/qemu-devel/2010-05/msg02999.html> <https://lists.gnu.org/archive/html/qemu-devel/2010-05/msg02999.html <https://lists.gnu.org/archive/html/qemu-devel/2010-05/msg02999.html>>
>> > Comes from  target-ppc: fix RFI by clearing some bits of MSR
>> > SHA-1: c3d420ead1aee9fcfd12be11cbdf6b1620134773
>> >  target-ppc/op_helper.c | 6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> > ```
>> > diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
>> > index 8f2ee986bb..3c3aa60bc3 100644
>> > --- a/target-ppc/op_helper.c
>> > +++ b/target-ppc/op_helper.c
>> > @@ -1646,20 +1646,20 @@ static inline void do_rfi(target_ulong nip, target_ulong msr,
>> >  void helper_rfi (void)
>> >  {
>> >      do_rfi(env->spr[SPR_SRR0], env->spr[SPR_SRR1],
>> > -           ~((target_ulong)0x0), 1);
>> > +           ~((target_ulong)0x783F0000), 1);
>> >  }
>> >  
>> >  #if defined(TARGET_PPC64)
>> >  void helper_rfid (void)
>> >  {
>> >      do_rfi(env->spr[SPR_SRR0], env->spr[SPR_SRR1],
>> > -           ~((target_ulong)0x0), 0);
>> > +           ~((target_ulong)0x783F0000), 0);
>> >  }
>> >  
>> >  void helper_hrfid (void)
>> >  {
>> >      do_rfi(env->spr[SPR_HSRR0], env->spr[SPR_HSRR1],
>> > -           ~((target_ulong)0x0), 0);
>> > +           ~((target_ulong)0x783F0000), 0);
>> >  }
>> >  #endif
>> >  #endif
>> > ```
>> >
>> > This is the second patch,:
>> > it's remove the parameter  `target_ulong msrm, int keep_msrh`
>> > Comes from ppc: Fix rfi/rfid/hrfi/... emulation
>> > SHA-1: a2e71b28e832346409efc795ecd1f0a2bcb705a3
>> > ```
>> >  target-ppc/excp_helper.c | 51 +++++++++++++++++++-----------------------------
>> >  1 file changed, 20 insertions(+), 31 deletions(-)
>> >
>> > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
>> > index 30e960e30b..aa0b63f4b0 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 */
>> > ```
>> >
>> > And of cause, the second patch fixes some problem, but also cause new problem,
>> > how to implement these instruction properly?
>>
>> What are the new problems  ?
>
>
> Before this patch, VxWorks can working, but after this, VxWorks can not boot anymore.

I suppose you did a bisect to reach this patch. 

Which QEMU machine is impacted ? Which CPU ? What are the symptoms ? 

Did you try to run with -d exec or -d in_asm to identify the exact
instruction ? 

From there, you could try to revert partially the patch above to 
fix the problem. 

Thanks,

C.
罗勇刚(Yonggang Luo) Jan. 10, 2021, 3 p.m. UTC | #4
On Fri, Jan 8, 2021 at 2:02 AM Cédric Le Goater <clg@kaod.org> wrote:
>
> On 1/8/21 5:21 AM, 罗勇刚(Yonggang Luo) wrote:
> >
> >
> > On Fri, Jan 8, 2021 at 5:54 AM Cédric Le Goater <clg@kaod.org <mailto:
clg@kaod.org>> wrote:
> >>
> >> On 1/7/21 8:14 PM, 罗勇刚(Yonggang Luo) wrote:
> >> > This is the first patch,:
> >> > It's store MSR bits differntly for different rfi instructions:
> >> > [Qemu-devel] [PATCH] target-ppc: fix RFI by clearing some bits of MSR
> >> > https://lists.gnu.org/archive/html/qemu-devel/2010-05/msg02999.html <
https://lists.gnu.org/archive/html/qemu-devel/2010-05/msg02999.html> <
https://lists.gnu.org/archive/html/qemu-devel/2010-05/msg02999.html <
https://lists.gnu.org/archive/html/qemu-devel/2010-05/msg02999.html>>
> >> > Comes from  target-ppc: fix RFI by clearing some bits of MSR
> >> > SHA-1: c3d420ead1aee9fcfd12be11cbdf6b1620134773
> >> >  target-ppc/op_helper.c | 6 +++---
> >> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >> > ```
> >> > diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
> >> > index 8f2ee986bb..3c3aa60bc3 100644
> >> > --- a/target-ppc/op_helper.c
> >> > +++ b/target-ppc/op_helper.c
> >> > @@ -1646,20 +1646,20 @@ static inline void do_rfi(target_ulong nip,
target_ulong msr,
> >> >  void helper_rfi (void)
> >> >  {
> >> >      do_rfi(env->spr[SPR_SRR0], env->spr[SPR_SRR1],
> >> > -           ~((target_ulong)0x0), 1);
> >> > +           ~((target_ulong)0x783F0000), 1);
> >> >  }
> >> >
> >> >  #if defined(TARGET_PPC64)
> >> >  void helper_rfid (void)
> >> >  {
> >> >      do_rfi(env->spr[SPR_SRR0], env->spr[SPR_SRR1],
> >> > -           ~((target_ulong)0x0), 0);
> >> > +           ~((target_ulong)0x783F0000), 0);
> >> >  }
> >> >
> >> >  void helper_hrfid (void)
> >> >  {
> >> >      do_rfi(env->spr[SPR_HSRR0], env->spr[SPR_HSRR1],
> >> > -           ~((target_ulong)0x0), 0);
> >> > +           ~((target_ulong)0x783F0000), 0);
> >> >  }
> >> >  #endif
> >> >  #endif
> >> > ```
> >> >
> >> > This is the second patch,:
> >> > it's remove the parameter  `target_ulong msrm, int keep_msrh`
> >> > Comes from ppc: Fix rfi/rfid/hrfi/... emulation
> >> > SHA-1: a2e71b28e832346409efc795ecd1f0a2bcb705a3
> >> > ```
> >> >  target-ppc/excp_helper.c | 51
+++++++++++++++++++-----------------------------
> >> >  1 file changed, 20 insertions(+), 31 deletions(-)
> >> >
> >> > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> >> > index 30e960e30b..aa0b63f4b0 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 */
> >> > ```
> >> >
> >> > And of cause, the second patch fixes some problem, but also cause
new problem,
> >> > how to implement these instruction properly?
> >>
> >> What are the new problems  ?
> >
> >
> > Before this patch, VxWorks can working, but after this, VxWorks can not
boot anymore.
>
> I suppose you did a bisect to reach this patch.
>
> Which QEMU machine is impacted ? Which CPU ? What are the symptoms ?
>
> Did you try to run with -d exec or -d in_asm to identify the exact
> instruction ?
>
> From there, you could try to revert partially the patch above to
> fix the problem.
>
> Thanks,
>
> C.
>
>
>
QEMU 5.2.x, an e300 based machine ppc603 are impacted.
Here is my fix, narrowed down to  MSR_TGPR and  MSR_ILE
```
From 42ce41671f1e6c4dd44e6fb481bbda9df09320bd Mon Sep 17 00:00:00 2001
From: Yonggang Luo <luoyonggang@gmail.com>
Date: Sun, 10 Jan 2021 00:08:00 -0800
Subject: [PATCH] ppc: Fix rfi/rfid/hrfi/... emulation again

This revert part mask bits for ppc603/ppc4x that disabled in
 a2e71b28e832346409efc795ecd1f0a2bcb705a3.
Remove redundant macro MSR_BOOK3S_MASK.
Fixes boot VxWorks on e300

Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
---
 target/ppc/excp_helper.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 1c48b9fdf6..df70c5a4e8 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1156,8 +1156,10 @@ static inline void do_rfi(CPUPPCState *env,
target_ulong nip, target_ulong msr)
 {
     CPUState *cs = env_cpu(env);

-    /* MSR:POW cannot be set by any form of rfi */
+    /* MSR:POW,TGPR,ILE cannot be set by any form of rfi */
     msr &= ~(1ULL << MSR_POW);
+    msr &= ~(1ULL << MSR_TGPR);
+    msr &= ~(1ULL << MSR_ILE);

 #if defined(TARGET_PPC64)
     /* Switching to 32-bit ? Crop the nip */
@@ -1190,7 +1192,6 @@ void helper_rfi(CPUPPCState *env)
     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)
 {
Cédric Le Goater Jan. 12, 2021, 9:23 a.m. UTC | #5
> QEMU 5.2.x, an e300 based machine ppc603 are impacted.
> Here is my fix, narrowed down to  MSR_TGPR and  MSR_ILE
> ```
> From 42ce41671f1e6c4dd44e6fb481bbda9df09320bd Mon Sep 17 00:00:00 2001
> From: Yonggang Luo <luoyonggang@gmail.com <mailto:luoyonggang@gmail.com>>
> Date: Sun, 10 Jan 2021 00:08:00 -0800
> Subject: [PATCH] ppc: Fix rfi/rfid/hrfi/... emulation again
> 
> This revert part mask bits for ppc603/ppc4x that disabled in  a2e71b28e832346409efc795ecd1f0a2bcb705a3.
> Remove redundant macro MSR_BOOK3S_MASK.
> Fixes boot VxWorks on e300
> 
> Signed-off-by: Yonggang Luo <luoyonggang@gmail.com <mailto:luoyonggang@gmail.com>>
> ---
>  target/ppc/excp_helper.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 1c48b9fdf6..df70c5a4e8 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1156,8 +1156,10 @@ static inline void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr)
>  {
>      CPUState *cs = env_cpu(env);
>  
> -    /* MSR:POW cannot be set by any form of rfi */
> +    /* MSR:POW,TGPR,ILE cannot be set by any form of rfi */
>      msr &= ~(1ULL << MSR_POW);
> +    msr &= ~(1ULL << MSR_TGPR);

Indeed. The e300 user manual says that TGPR is cleared by rfi. We should 
add a per-cpu family mask and not a global setting.

> +    msr &= ~(1ULL << MSR_ILE);

that's curious. I am still trying to understand that part. May be this is 
due to the lack of HID2 modeling which contains a "True little-endian" bit.

Is your image Little endian ? 

C. 

>  
>  #if defined(TARGET_PPC64)
>      /* Switching to 32-bit ? Crop the nip */
> @@ -1190,7 +1192,6 @@ void helper_rfi(CPUPPCState *env)
>      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)
>  {
> -- 
> 2.29.2.windows.3
> 
> ```
> 
> --
>          此致
> 礼
> 罗勇刚
> Yours
>     sincerely,
> Yonggang Luo
罗勇刚(Yonggang Luo) Jan. 12, 2021, 1:52 p.m. UTC | #6
On Tue, Jan 12, 2021 at 5:23 PM Cédric Le Goater <clg@kaod.org> wrote:
>
> > QEMU 5.2.x, an e300 based machine ppc603 are impacted.
> > Here is my fix, narrowed down to  MSR_TGPR and  MSR_ILE
> > ```
> > From 42ce41671f1e6c4dd44e6fb481bbda9df09320bd Mon Sep 17 00:00:00 2001
> > From: Yonggang Luo <luoyonggang@gmail.com <mailto:luoyonggang@gmail.com
>>
> > Date: Sun, 10 Jan 2021 00:08:00 -0800
> > Subject: [PATCH] ppc: Fix rfi/rfid/hrfi/... emulation again
> >
> > This revert part mask bits for ppc603/ppc4x that disabled in
 a2e71b28e832346409efc795ecd1f0a2bcb705a3.
> > Remove redundant macro MSR_BOOK3S_MASK.
> > Fixes boot VxWorks on e300
> >
> > Signed-off-by: Yonggang Luo <luoyonggang@gmail.com <mailto:
luoyonggang@gmail.com>>
> > ---
> >  target/ppc/excp_helper.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > index 1c48b9fdf6..df70c5a4e8 100644
> > --- a/target/ppc/excp_helper.c
> > +++ b/target/ppc/excp_helper.c
> > @@ -1156,8 +1156,10 @@ static inline void do_rfi(CPUPPCState *env,
target_ulong nip, target_ulong msr)
> >  {
> >      CPUState *cs = env_cpu(env);
> >
> > -    /* MSR:POW cannot be set by any form of rfi */
> > +    /* MSR:POW,TGPR,ILE cannot be set by any form of rfi */
> >      msr &= ~(1ULL << MSR_POW);
> > +    msr &= ~(1ULL << MSR_TGPR);
>
> Indeed. The e300 user manual says that TGPR is cleared by rfi. We should
> add a per-cpu family mask and not a global setting.
Refer to https://www.nxp.com/docs/en/reference-manual/e300coreRM.pdf

`Table 2-4. MSR Bit Settings`

```
  Temporary GPR remapping (implementation-specific) 0 Normal operation 1
TGPR mode. GPR0–GPR3 are remapped to TGPR0–TGPR3 for use by TLB miss
routines. The contents of GPR0–GPR3 remain unchanged while MSR[TGPR] = 1.
Attempts to use GPR4–GPR31 with MSR[TGPR] = 1 yield undefined results.
Temporarily replaces TGPR0–TGPR3 with GPR0–GPR3 for use by TLB miss
routines. The TGPR bit is set when either an instruction TLB miss, data
read miss, or data write miss interrupt is taken. The TGPR bit is cleared
by an rfi instruction.
```

>
> > +    msr &= ~(1ULL << MSR_ILE);
>
> that's curious. I am still trying to understand that part. May be this is
> due to the lack of HID2 modeling which contains a "True little-endian"
bit.

Don't understand this part, I am running VxWorks 6.9 on MPC8349EA
https://www.nxp.com/docs/en/reference-manual/MPC8349EARM.pdf

Didn't got any idea about why  MSR_ILE are set

>
> Is your image Little endian ?
>
Big Endian vxworks image.

> C.
>
> >
> >  #if defined(TARGET_PPC64)
> >      /* Switching to 32-bit ? Crop the nip */
> > @@ -1190,7 +1192,6 @@ void helper_rfi(CPUPPCState *env)
> >      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)
> >  {
> > --
> > 2.29.2.windows.3
> >
> > ```
> >
> > --
> >          此致
> > 礼
> > 罗勇刚
> > Yours
> >     sincerely,
> > Yonggang Luo
>


--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo
罗勇刚(Yonggang Luo) Jan. 13, 2021, 10:02 a.m. UTC | #7
On Tue, Jan 12, 2021 at 1:23 AM Cédric Le Goater <clg@kaod.org> wrote:
>
> > QEMU 5.2.x, an e300 based machine ppc603 are impacted.
> > Here is my fix, narrowed down to  MSR_TGPR and  MSR_ILE
> > ```
> > From 42ce41671f1e6c4dd44e6fb481bbda9df09320bd Mon Sep 17 00:00:00 2001
> > From: Yonggang Luo <luoyonggang@gmail.com <mailto:luoyonggang@gmail.com
>>
> > Date: Sun, 10 Jan 2021 00:08:00 -0800
> > Subject: [PATCH] ppc: Fix rfi/rfid/hrfi/... emulation again
> >
> > This revert part mask bits for ppc603/ppc4x that disabled in
 a2e71b28e832346409efc795ecd1f0a2bcb705a3.
> > Remove redundant macro MSR_BOOK3S_MASK.
> > Fixes boot VxWorks on e300
> >
> > Signed-off-by: Yonggang Luo <luoyonggang@gmail.com <mailto:
luoyonggang@gmail.com>>
> > ---
> >  target/ppc/excp_helper.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > index 1c48b9fdf6..df70c5a4e8 100644
> > --- a/target/ppc/excp_helper.c
> > +++ b/target/ppc/excp_helper.c
> > @@ -1156,8 +1156,10 @@ static inline void do_rfi(CPUPPCState *env,
target_ulong nip, target_ulong msr)
> >  {
> >      CPUState *cs = env_cpu(env);
> >
> > -    /* MSR:POW cannot be set by any form of rfi */
> > +    /* MSR:POW,TGPR,ILE cannot be set by any form of rfi */
> >      msr &= ~(1ULL << MSR_POW);
> > +    msr &= ~(1ULL << MSR_TGPR);
>
> Indeed. The e300 user manual says that TGPR is cleared by rfi. We should
> add a per-cpu family mask and not a global setting.
>
> > +    msr &= ~(1ULL << MSR_ILE);
>
> that's curious. I am still trying to understand that part. May be this is
> due to the lack of HID2 modeling which contains a "True little-endian"
bit.
>
> Is your image Little endian ?

Hi, According to ` Table 2-4. MSR Bit Settings (continued) ` in
https://www.nxp.com/docs/en/reference-manual/e300coreRM.pdf
```
  Interrupt little-endian mode. When an interrupt occurs, this bit is
copied into MSR[LE] to select the endian mode for the context established
by the interrupt.
```
Does this means MSR[LE] =  MSR[LE]
>
> C.
>
> >
> >  #if defined(TARGET_PPC64)
> >      /* Switching to 32-bit ? Crop the nip */
> > @@ -1190,7 +1192,6 @@ void helper_rfi(CPUPPCState *env)
> >      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)
> >  {
> > --
> > 2.29.2.windows.3
> >
> > ```
> >
> > --
> >          此致
> > 礼
> > 罗勇刚
> > Yours
> >     sincerely,
> > Yonggang Luo
>


--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo
Cédric Le Goater Jan. 13, 2021, 2:19 p.m. UTC | #8
On 1/12/21 2:52 PM, 罗勇刚(Yonggang Luo) wrote:
> 
> 
> On Tue, Jan 12, 2021 at 5:23 PM Cédric Le Goater <clg@kaod.org <mailto:clg@kaod.org>> wrote:
>>
>> > QEMU 5.2.x, an e300 based machine ppc603 are impacted.
>> > Here is my fix, narrowed down to  MSR_TGPR and  MSR_ILE
>> > ```
>> > From 42ce41671f1e6c4dd44e6fb481bbda9df09320bd Mon Sep 17 00:00:00 2001
>> > From: Yonggang Luo <luoyonggang@gmail.com <mailto:luoyonggang@gmail.com> <mailto:luoyonggang@gmail.com <mailto:luoyonggang@gmail.com>>>
>> > Date: Sun, 10 Jan 2021 00:08:00 -0800
>> > Subject: [PATCH] ppc: Fix rfi/rfid/hrfi/... emulation again
>> >
>> > This revert part mask bits for ppc603/ppc4x that disabled in  a2e71b28e832346409efc795ecd1f0a2bcb705a3.
>> > Remove redundant macro MSR_BOOK3S_MASK.
>> > Fixes boot VxWorks on e300
>> >
>> > Signed-off-by: Yonggang Luo <luoyonggang@gmail.com <mailto:luoyonggang@gmail.com> <mailto:luoyonggang@gmail.com <mailto:luoyonggang@gmail.com>>>
>> > ---
>> >  target/ppc/excp_helper.c | 5 +++--
>> >  1 file changed, 3 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> > index 1c48b9fdf6..df70c5a4e8 100644
>> > --- a/target/ppc/excp_helper.c
>> > +++ b/target/ppc/excp_helper.c
>> > @@ -1156,8 +1156,10 @@ static inline void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr)
>> >  {
>> >      CPUState *cs = env_cpu(env);
>> >  
>> > -    /* MSR:POW cannot be set by any form of rfi */
>> > +    /* MSR:POW,TGPR,ILE cannot be set by any form of rfi */
>> >      msr &= ~(1ULL << MSR_POW);
>> > +    msr &= ~(1ULL << MSR_TGPR);
>>
>> Indeed. The e300 user manual says that TGPR is cleared by rfi. We should
>> add a per-cpu family mask and not a global setting.
> Refer to https://www.nxp.com/docs/en/reference-manual/e300coreRM.pdf <https://www.nxp.com/docs/en/reference-manual/e300coreRM.pdf>
> 
> `Table 2-4. MSR Bit Settings`
> 
> ```
>   Temporary GPR remapping (implementation-specific) 0 Normal operation 1 TGPR mode. GPR0–GPR3 are remapped to TGPR0–TGPR3 for use by TLB miss routines. The contents of GPR0–GPR3 remain unchanged while MSR[TGPR] = 1. Attempts to use GPR4–GPR31 with MSR[TGPR] = 1 yield undefined results. Temporarily replaces TGPR0–TGPR3 with GPR0–GPR3 for use by TLB miss routines. The TGPR bit is set when either an instruction TLB miss, data read miss, or data write miss interrupt is taken. The TGPR bit is cleared by an rfi instruction.  
> ```
>   
>>
>> > +    msr &= ~(1ULL << MSR_ILE);
>>
>> that's curious. I am still trying to understand that part. May be this is
>> due to the lack of HID2 modeling which contains a "True little-endian" bit.
> 
> Don't understand this part, I am running VxWorks 6.9 on MPC8349EA
> https://www.nxp.com/docs/en/reference-manual/MPC8349EARM.pdf <https://www.nxp.com/docs/en/reference-manual/MPC8349EARM.pdf>
> 
> Didn't got any idea about why  MSR_ILE are set
> 
>>
>> Is your image Little endian ?
>>
> Big Endian vxworks image.


Can you share the image ? and the QEMU command line ?

Thanks,

C.
diff mbox series

Patch

diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
index 8f2ee986bb..3c3aa60bc3 100644
--- a/target-ppc/op_helper.c
+++ b/target-ppc/op_helper.c
@@ -1646,20 +1646,20 @@  static inline void do_rfi(target_ulong nip,
target_ulong msr,
 void helper_rfi (void)
 {
     do_rfi(env->spr[SPR_SRR0], env->spr[SPR_SRR1],
-           ~((target_ulong)0x0), 1);
+           ~((target_ulong)0x783F0000), 1);
 }

 #if defined(TARGET_PPC64)
 void helper_rfid (void)
 {
     do_rfi(env->spr[SPR_SRR0], env->spr[SPR_SRR1],
-           ~((target_ulong)0x0), 0);
+           ~((target_ulong)0x783F0000), 0);
 }

 void helper_hrfid (void)
 {
     do_rfi(env->spr[SPR_HSRR0], env->spr[SPR_HSRR1],
-           ~((target_ulong)0x0), 0);
+           ~((target_ulong)0x783F0000), 0);
 }
 #endif
 #endif
```

This is the second patch,:
it's remove the parameter  `target_ulong msrm, int keep_msrh`
Comes from ppc: Fix rfi/rfid/hrfi/... emulation
SHA-1: a2e71b28e832346409efc795ecd1f0a2bcb705a3
```
 target-ppc/excp_helper.c | 51
+++++++++++++++++++-----------------------------
 1 file changed, 20 insertions(+), 31 deletions(-)

diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
index 30e960e30b..aa0b63f4b0 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;