Re: [Qemu-devel] [PATCH 01/10] ppc: Fix rfi/rfid/hrfi/... emulation
On 06/16/2016 03:07 AM, David Gibson wrote: > On Mon, Jun 13, 2016 at 07:24:47AM +0200, Cédric Le Goater wrote: >> From: Benjamin Herrenschmidt>> >> 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 >> Reviewed-by: David Gibson > > I've merged this patch to ppc-for-2.7. > > The reset of the series I'd like to see a respin for, even if it's > just to clean up the commit messages. Yes. I noted a couple of commit messages to rephrase, the "inline" removal patch to keep and a merge of Andrei patches for HV {I,D}SI. I will get that going once I have a better idea on a block I/O issue I see with another patchset. I am really pulling my hair on this one. Spent the week on it :/ The good thing is that it made me write a unit test ! Cheers, C. >> --- >> 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)0x); >> -} >> } >> #else >> nip = (uint32_t)nip; >> -msr &= (uint32_t)msrm; >> #endif >> /* XXX: beware: this is false if VLE is supported */ >> env->nip = nip & ~((target_ulong)0x0003); >> @@ -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)0x783F), 1); >> -} >> +do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1] & 0xul); >> } >> >> +#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)0x783F), 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)0x783F), 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)0x), 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,
Re: [Qemu-devel] [PATCH 01/10] ppc: Fix rfi/rfid/hrfi/... emulation
On Mon, Jun 13, 2016 at 07:24:47AM +0200, Cédric Le Goater wrote: > From: Benjamin Herrenschmidt> > 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 > Reviewed-by: David Gibson I've merged this patch to ppc-for-2.7. 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)0x); > -} > } > #else > nip = (uint32_t)nip; > -msr &= (uint32_t)msrm; > #endif > /* XXX: beware: this is false if VLE is supported */ > env->nip = nip & ~((target_ulong)0x0003); > @@ -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)0x783F), 1); > -} > +do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1] & 0xul); > } > > +#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)0x783F), 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)0x783F), 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)0x), 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, 0x, 0); > +do_rfi(env, env->lr, env->ctr & 0x); > } > > /* Embedded.Processor Control */ > diff --git a/target-ppc/translate.c
[Qemu-devel] [PATCH 01/10] ppc: Fix rfi/rfid/hrfi/... emulation
From: Benjamin HerrenschmidtThis 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 Reviewed-by: David Gibson --- 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)0x); -} } #else nip = (uint32_t)nip; -msr &= (uint32_t)msrm; #endif /* XXX: beware: this is false if VLE is supported */ env->nip = nip & ~((target_ulong)0x0003); @@ -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)0x783F), 1); -} +do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1] & 0xul); } +#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)0x783F), 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)0x783F), 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)0x), 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, 0x, 0); +do_rfi(env, env->lr, env->ctr & 0x); } /* 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); +