| Message ID | 1447201710-10229-10-git-send-email-benh@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 11, 2015 at 11:27:22AM +1100, Benjamin Herrenschmidt wrote: > XXX This patch needs double checking... It fixed 32-bit userspace > but I'm not sure it's right. I wonder whether msr_is_64bit() should > be applied to env->msr, not msr, but I need to double check the > architecture. Hrm, I'm not really sure where I'd look in the arch, but msr_is_64bit(env->msr) seems like it would make more sense to me. The current logic means that rfi, ostensibly a 32-bit instruction will have different behaviour depending on the upper bits of SRR1, which seems a unexpected. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > --- > target-ppc/excp_helper.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c > index c1d6605..00fae60 100644 > --- a/target-ppc/excp_helper.c > +++ b/target-ppc/excp_helper.c > @@ -878,13 +878,13 @@ static inline void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr, > CPUState *cs = CPU(ppc_env_get_cpu(env)); > > #if defined(TARGET_PPC64) > + msr = msr & msrm; > if (msr_is_64bit(env, msr)) { > nip = (uint64_t)nip; > - msr &= (uint64_t)msrm; > } else { > nip = (uint32_t)nip; > - msr = (uint32_t)(msr & msrm); > if (keep_msrh) { > + msr &= 0xffffffff; > msr |= env->msr & ~((uint64_t)0xFFFFFFFF); > } > }
On Thu, 2015-11-19 at 17:19 +1100, David Gibson wrote: > On Wed, Nov 11, 2015 at 11:27:22AM +1100, Benjamin Herrenschmidt > wrote: > > XXX This patch needs double checking... It fixed 32-bit userspace > > but I'm not sure it's right. I wonder whether msr_is_64bit() should > > be applied to env->msr, not msr, but I need to double check the > > architecture. > > Hrm, I'm not really sure where I'd look in the arch, but > msr_is_64bit(env->msr) seems like it would make more sense to me. > The current logic means that rfi, ostensibly a 32-bit instruction > will > have different behaviour depending on the upper bits of SRR1, which > seems a unexpected. I only just discovered that rfi is actually gone from arch 2.07 :-) I'll dig a bit more tomorrow. Cheers, Ben. > > > > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > --- > > target-ppc/excp_helper.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c > > index c1d6605..00fae60 100644 > > --- a/target-ppc/excp_helper.c > > +++ b/target-ppc/excp_helper.c > > @@ -878,13 +878,13 @@ static inline void do_rfi(CPUPPCState *env, > > target_ulong nip, target_ulong msr, > > CPUState *cs = CPU(ppc_env_get_cpu(env)); > > > > #if defined(TARGET_PPC64) > > + msr = msr & msrm; > > if (msr_is_64bit(env, msr)) { > > nip = (uint64_t)nip; > > - msr &= (uint64_t)msrm; > > } else { > > nip = (uint32_t)nip; > > - msr = (uint32_t)(msr & msrm); > > if (keep_msrh) { > > + msr &= 0xffffffff; > > msr |= env->msr & ~((uint64_t)0xFFFFFFFF); > > } > > } >
On Thu, 2015-11-19 at 21:23 +1100, Benjamin Herrenschmidt wrote: > > I only just discovered that rfi is actually gone from arch 2.07 :-) > > I'll dig a bit more tomorrow. Ok, so I had a closer look and tore that stuff appart even more :-) If you are curious, feel free to check out github. I've removed the MSR mask completely, I can't figure out what it's supposed to be about. I've quickly tested 64-bit powernv/pseries, 32-bit userspace on 64-bit pseries kernel, and 32-bit Mac99 (ubuntu). Cheers, Ben. > Cheers, > Ben. > > > > > > > > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > > --- > > > target-ppc/excp_helper.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c > > > index c1d6605..00fae60 100644 > > > --- a/target-ppc/excp_helper.c > > > +++ b/target-ppc/excp_helper.c > > > @@ -878,13 +878,13 @@ static inline void do_rfi(CPUPPCState *env, > > > target_ulong nip, target_ulong msr, > > > CPUState *cs = CPU(ppc_env_get_cpu(env)); > > > > > > #if defined(TARGET_PPC64) > > > + msr = msr & msrm; > > > if (msr_is_64bit(env, msr)) { > > > nip = (uint64_t)nip; > > > - msr &= (uint64_t)msrm; > > > } else { > > > nip = (uint32_t)nip; > > > - msr = (uint32_t)(msr & msrm); > > > if (keep_msrh) { > > > + msr &= 0xffffffff; > > > msr |= env->msr & ~((uint64_t)0xFFFFFFFF); > > > } > > > }
diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c index c1d6605..00fae60 100644 --- a/target-ppc/excp_helper.c +++ b/target-ppc/excp_helper.c @@ -878,13 +878,13 @@ static inline void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr, CPUState *cs = CPU(ppc_env_get_cpu(env)); #if defined(TARGET_PPC64) + msr = msr & msrm; if (msr_is_64bit(env, msr)) { nip = (uint64_t)nip; - msr &= (uint64_t)msrm; } else { nip = (uint32_t)nip; - msr = (uint32_t)(msr & msrm); if (keep_msrh) { + msr &= 0xffffffff; msr |= env->msr & ~((uint64_t)0xFFFFFFFF); } }
XXX This patch needs double checking... It fixed 32-bit userspace but I'm not sure it's right. I wonder whether msr_is_64bit() should be applied to env->msr, not msr, but I need to double check the architecture. Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> --- target-ppc/excp_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)