| Message ID | 1465795496-15071-2-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 13, 2016 at 07:24:47AM +0200, Cédric Le Goater wrote: > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > 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 <benh@kernel.crashing.org> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> 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)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);
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 <benh@kernel.crashing.org> > > > > 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 <benh@kernel.crashing.org> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > 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); >
On 06/17/2016 04:27 AM, David Gibson wrote: > 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 <benh@kernel.crashing.org> >>> >>> 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 <benh@kernel.crashing.org> >>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >> >> 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. Just this patch ? I booted a macosx image with it. but maybe just a mac99. I will check today. C. >> 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); >> > > >
On 06/17/2016 07:54 AM, Cédric Le Goater wrote: > On 06/17/2016 04:27 AM, David Gibson wrote: >> 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 <benh@kernel.crashing.org> >>>> >>>> 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 <benh@kernel.crashing.org> >>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >>> >>> 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. > > Just this patch ? I booted a macosx image with it. but maybe just a mac99. > I will check today. With your branch ppc-for-2.7 (at commit aba2e6258d86) + the "ppc: Fix rfi/rfid/hrfi/... emulation" patch, these guests : qemu-system-ppc -cdrom ./darwinppc-602.cdr -boot d qemu-system-ppc -M mac99 -cdrom ./darwinppc-602.cdr -boot d reach the installer macosx installer. C. >>> 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); >>> >> >> >> >
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 <benh@kernel.crashing.org> >> >> 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 <benh@kernel.crashing.org> >> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > 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)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); >
On Fri, Jun 17, 2016 at 08:03:29AM +0200, Cédric Le Goater wrote: > On 06/17/2016 07:54 AM, Cédric Le Goater wrote: > > On 06/17/2016 04:27 AM, David Gibson wrote: > >> 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 <benh@kernel.crashing.org> > >>>> > >>>> 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 <benh@kernel.crashing.org> > >>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > >>> > >>> 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. > > > > Just this patch ? I booted a macosx image with it. but maybe just a mac99. > > I will check today. > > With your branch ppc-for-2.7 (at commit aba2e6258d86) + the > "ppc: Fix rfi/rfid/hrfi/... emulation" patch, these guests : > > qemu-system-ppc -cdrom ./darwinppc-602.cdr -boot d > qemu-system-ppc -M mac99 -cdrom ./darwinppc-602.cdr -boot d > > reach the installer macosx installer. But the prom-env-test from make check fails :(.
On 06/17/2016 08:28 AM, David Gibson wrote: > On Fri, Jun 17, 2016 at 08:03:29AM +0200, Cédric Le Goater wrote: >> On 06/17/2016 07:54 AM, Cédric Le Goater wrote: >>> On 06/17/2016 04:27 AM, David Gibson wrote: >>>> 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 <benh@kernel.crashing.org> >>>>>> >>>>>> 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 <benh@kernel.crashing.org> >>>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >>>>> >>>>> 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. >>> >>> Just this patch ? I booted a macosx image with it. but maybe just a mac99. >>> I will check today. >> >> With your branch ppc-for-2.7 (at commit aba2e6258d86) + the >> "ppc: Fix rfi/rfid/hrfi/... emulation" patch, these guests : >> >> qemu-system-ppc -cdrom ./darwinppc-602.cdr -boot d >> qemu-system-ppc -M mac99 -cdrom ./darwinppc-602.cdr -boot d >> >> reach the installer macosx installer. > > But the prom-env-test from make check fails :(. > I do not see the issue on ppc-for-2.7 (commit aba2e6258d86) + the "ppc: Fix rfi/rfid/hrfi/... emulation" patch. I will update during the day and let you know. Cheers, C.
On 17.06.2016 08:03, Cédric Le Goater wrote: > On 06/17/2016 07:54 AM, Cédric Le Goater wrote: >> On 06/17/2016 04:27 AM, David Gibson wrote: >>> 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 <benh@kernel.crashing.org> >>>>> >>>>> 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 <benh@kernel.crashing.org> >>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >>>> >>>> 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. >> >> Just this patch ? I booted a macosx image with it. but maybe just a mac99. >> I will check today. > > With your branch ppc-for-2.7 (at commit aba2e6258d86) + the > "ppc: Fix rfi/rfid/hrfi/... emulation" patch, these guests : > > qemu-system-ppc -cdrom ./darwinppc-602.cdr -boot d > qemu-system-ppc -M mac99 -cdrom ./darwinppc-602.cdr -boot d ^ | You're missing the "64" here ;-) > reach the installer macosx installer. It seems to be only failing for the 64-bit builds - and there only for the PPC970 CPU (which is the default for the mac99 machine in 64-bit builds): qemu-system-ppc64 -nographic -cpu 750 -M mac99 ==> works fine qemu-system-ppc64 -nographic -cpu 970 -M mac99 ==> hangs Thomas
On 06/17/2016 09:10 AM, Thomas Huth wrote: > On 17.06.2016 08:03, Cédric Le Goater wrote: >> On 06/17/2016 07:54 AM, Cédric Le Goater wrote: >>> On 06/17/2016 04:27 AM, David Gibson wrote: >>>> 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 <benh@kernel.crashing.org> >>>>>> >>>>>> 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 <benh@kernel.crashing.org> >>>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >>>>> >>>>> 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. >>> >>> Just this patch ? I booted a macosx image with it. but maybe just a mac99. >>> I will check today. >> >> With your branch ppc-for-2.7 (at commit aba2e6258d86) + the >> "ppc: Fix rfi/rfid/hrfi/... emulation" patch, these guests : >> >> qemu-system-ppc -cdrom ./darwinppc-602.cdr -boot d >> qemu-system-ppc -M mac99 -cdrom ./darwinppc-602.cdr -boot d > ^ > | > You're missing the "64" here ;-) ah ! I never use the 64 bit version for these but I should. I will give a try. Thanks, C. >> reach the installer macosx installer. > > It seems to be only failing for the 64-bit builds - and there only for > the PPC970 CPU (which is the default for the mac99 machine in 64-bit > builds): > > qemu-system-ppc64 -nographic -cpu 750 -M mac99 ==> works fine > > qemu-system-ppc64 -nographic -cpu 970 -M mac99 ==> hangs > > Thomas >
On 06/17/2016 09:10 AM, Thomas Huth wrote: > On 17.06.2016 08:03, Cédric Le Goater wrote: >> On 06/17/2016 07:54 AM, Cédric Le Goater wrote: >>> On 06/17/2016 04:27 AM, David Gibson wrote: >>>> 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 <benh@kernel.crashing.org> >>>>>> >>>>>> 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. he. >>>>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >>>>> >>>>> 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. >>> >>> Just this patch ? I booted a macosx image with it. but maybe just a mac99. >>> I will check today. >> >> With your branch ppc-for-2.7 (at commit aba2e6258d86) + the >> "ppc: Fix rfi/rfid/hrfi/... emulation" patch, these guests : >> >> qemu-system-ppc -cdrom ./darwinppc-602.cdr -boot d >> qemu-system-ppc -M mac99 -cdrom ./darwinppc-602.cdr -boot d > ^ > | > You're missing the "64" here ;-) > >> reach the installer macosx installer. > > It seems to be only failing for the 64-bit builds - and there only for > the PPC970 CPU (which is the default for the mac99 machine in 64-bit > builds): > > qemu-system-ppc64 -nographic -cpu 750 -M mac99 ==> works fine > > qemu-system-ppc64 -nographic -cpu 970 -M mac99 ==> hangs This is too brutal : + /* 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; + } There are a couple of instructions which have been deleted from ISA 2.x. rfi is one of them. Could we use a insn_flag to filter them ? Thanks, C.
On 17.06.2016 12:41, Cédric Le Goater wrote: > On 06/17/2016 09:10 AM, Thomas Huth wrote: >> On 17.06.2016 08:03, Cédric Le Goater wrote: >>> On 06/17/2016 07:54 AM, Cédric Le Goater wrote: >>>> On 06/17/2016 04:27 AM, David Gibson wrote: >>>>> 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 <benh@kernel.crashing.org> >>>>>>> >>>>>>> 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. > > he. > >>>>>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>>>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >>>>>> >>>>>> 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. >>>> >>>> Just this patch ? I booted a macosx image with it. but maybe just a mac99. >>>> I will check today. >>> >>> With your branch ppc-for-2.7 (at commit aba2e6258d86) + the >>> "ppc: Fix rfi/rfid/hrfi/... emulation" patch, these guests : >>> >>> qemu-system-ppc -cdrom ./darwinppc-602.cdr -boot d >>> qemu-system-ppc -M mac99 -cdrom ./darwinppc-602.cdr -boot d >> ^ >> | >> You're missing the "64" here ;-) >> >>> reach the installer macosx installer. >> >> It seems to be only failing for the 64-bit builds - and there only for >> the PPC970 CPU (which is the default for the mac99 machine in 64-bit >> builds): >> >> qemu-system-ppc64 -nographic -cpu 750 -M mac99 ==> works fine >> >> qemu-system-ppc64 -nographic -cpu 970 -M mac99 ==> hangs > > > This is too brutal : > > + /* 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; > + } > > There are a couple of instructions which have been deleted from > ISA 2.x. rfi is one of them. Could we use a insn_flag to filter > them ? According to the PPC970FX user manual that I have: "The 970FX does not provide support for the following optional or obsolete instructions (or instruction forms). Attempted use of these will result in an illegal instruction type program interrupt. [...] · rfi - Return from interrupt (obsolete) " So if OpenBIOS is using this instruction in 970 mode, it's maybe OpenBIOS that should be fixed instead? Thomas
On 17.06.16 13:02, Thomas Huth wrote: > On 17.06.2016 12:41, Cédric Le Goater wrote: >> On 06/17/2016 09:10 AM, Thomas Huth wrote: >>> On 17.06.2016 08:03, Cédric Le Goater wrote: >>>> On 06/17/2016 07:54 AM, Cédric Le Goater wrote: >>>>> On 06/17/2016 04:27 AM, David Gibson wrote: >>>>>> 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 <benh@kernel.crashing.org> >>>>>>>> >>>>>>>> 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. >> >> he. >> >>>>>>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>>>>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >>>>>>> >>>>>>> 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. >>>>> >>>>> Just this patch ? I booted a macosx image with it. but maybe just a mac99. >>>>> I will check today. >>>> >>>> With your branch ppc-for-2.7 (at commit aba2e6258d86) + the >>>> "ppc: Fix rfi/rfid/hrfi/... emulation" patch, these guests : >>>> >>>> qemu-system-ppc -cdrom ./darwinppc-602.cdr -boot d >>>> qemu-system-ppc -M mac99 -cdrom ./darwinppc-602.cdr -boot d >>> ^ >>> | >>> You're missing the "64" here ;-) >>> >>>> reach the installer macosx installer. >>> >>> It seems to be only failing for the 64-bit builds - and there only for >>> the PPC970 CPU (which is the default for the mac99 machine in 64-bit >>> builds): >>> >>> qemu-system-ppc64 -nographic -cpu 750 -M mac99 ==> works fine >>> >>> qemu-system-ppc64 -nographic -cpu 970 -M mac99 ==> hangs >> >> >> This is too brutal : >> >> + /* 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; >> + } >> >> There are a couple of instructions which have been deleted from >> ISA 2.x. rfi is one of them. Could we use a insn_flag to filter >> them ? > > According to the PPC970FX user manual that I have: > > "The 970FX does not provide support for the following optional or > obsolete instructions (or instruction forms). > Attempted use of these will result in an illegal instruction type > program interrupt. > [...] > · rfi - Return from interrupt (obsolete) " > > So if OpenBIOS is using this instruction in 970 mode, it's maybe > OpenBIOS that should be fixed instead? If 970 doesn't implement rfi, OpenBIOS shouldn't use it, yes :). Alex
On 06/17/2016 01:02 PM, Thomas Huth wrote: > On 17.06.2016 12:41, Cédric Le Goater wrote: >> On 06/17/2016 09:10 AM, Thomas Huth wrote: >>> On 17.06.2016 08:03, Cédric Le Goater wrote: >>>> On 06/17/2016 07:54 AM, Cédric Le Goater wrote: >>>>> On 06/17/2016 04:27 AM, David Gibson wrote: >>>>>> 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 <benh@kernel.crashing.org> >>>>>>>> >>>>>>>> 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. >> >> he. >> >>>>>>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>>>>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >>>>>>> >>>>>>> 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. >>>>> >>>>> Just this patch ? I booted a macosx image with it. but maybe just a mac99. >>>>> I will check today. >>>> >>>> With your branch ppc-for-2.7 (at commit aba2e6258d86) + the >>>> "ppc: Fix rfi/rfid/hrfi/... emulation" patch, these guests : >>>> >>>> qemu-system-ppc -cdrom ./darwinppc-602.cdr -boot d >>>> qemu-system-ppc -M mac99 -cdrom ./darwinppc-602.cdr -boot d >>> ^ >>> | >>> You're missing the "64" here ;-) >>> >>>> reach the installer macosx installer. >>> >>> It seems to be only failing for the 64-bit builds - and there only for >>> the PPC970 CPU (which is the default for the mac99 machine in 64-bit >>> builds): >>> >>> qemu-system-ppc64 -nographic -cpu 750 -M mac99 ==> works fine >>> >>> qemu-system-ppc64 -nographic -cpu 970 -M mac99 ==> hangs >> >> >> This is too brutal : >> >> + /* 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; >> + } >> >> There are a couple of instructions which have been deleted from >> ISA 2.x. rfi is one of them. Could we use a insn_flag to filter >> them ? > > According to the PPC970FX user manual that I have: > > "The 970FX does not provide support for the following optional or > obsolete instructions (or instruction forms). > Attempted use of these will result in an illegal instruction type > program interrupt. > [...] > · rfi - Return from interrupt (obsolete) " > > So if OpenBIOS is using this instruction in 970 mode, it's maybe > OpenBIOS that should be fixed instead? Both, then :) rfi is considered implemented for all cpu. But it should not. The instruction set PPC_POWER_BR contains nearly all the deleted instructions from isa2. rfi is not part of it and should. Also, only the cpus "PowerPC 601*" make a use of it in their insns_flags. So, we would want this set to be in all the "PowerPC {6,7}*" cpus. Are there more ? Thanks, C. dc->desc = "PowerPC 401"; dc->desc = "PowerPC 401x2"; dc->desc = "PowerPC 401x3"; dc->desc = "IOP480"; dc->desc = "PowerPC 403"; dc->desc = "PowerPC 403 GCX"; dc->desc = "PowerPC 405"; dc->desc = "PowerPC 440 EP"; dc->desc = "PowerPC 440 GP"; dc->desc = "PowerPC 440x4"; dc->desc = "PowerPC 440x5"; dc->desc = "PowerPC 440x5 with double precision FPU"; dc->desc = "PowerPC 460 (guessed)"; dc->desc = "PowerPC 460F (guessed)"; dc->desc = "Freescale 5xx cores (aka RCPU)"; dc->desc = "Freescale 8xx cores (aka PowerQUICC)"; dc->desc = "PowerPC G2"; dc->desc = "PowerPC G2LE"; dc->desc = "e200 core"; dc->desc = "e300 core"; dc->desc = "e500v1 core"; dc->desc = "e500v2 core"; dc->desc = "e500mc core"; dc->desc = "e5500 core"; dc->desc = "POWER"; dc->desc = "PowerPC 601"; pcc->insns_flags = PPC_INSNS_BASE | PPC_STRING | PPC_POWER_BR | dc->desc = "PowerPC 601v"; pcc->insns_flags = PPC_INSNS_BASE | PPC_STRING | PPC_POWER_BR | dc->desc = "PowerPC 602"; dc->desc = "PowerPC 603"; dc->desc = "PowerPC 603e"; dc->desc = "PowerPC 604"; dc->desc = "PowerPC 604E"; dc->desc = "PowerPC 740"; dc->desc = "PowerPC 750"; dc->desc = "PowerPC 750 CL"; dc->desc = "PowerPC 750CX"; dc->desc = "PowerPC 750FX"; dc->desc = "PowerPC 750GX"; dc->desc = "PowerPC 745"; dc->desc = "PowerPC 755"; dc->desc = "PowerPC 7400 (aka G4)"; dc->desc = "PowerPC 7410 (aka G4)"; dc->desc = "PowerPC 7440 (aka G4)"; dc->desc = "PowerPC 7450 (aka G4)"; dc->desc = "PowerPC 7445 (aka G4)"; dc->desc = "PowerPC 7455 (aka G4)"; dc->desc = "PowerPC 7457 (aka G4)"; dc->desc = "PowerPC e600"; dc->desc = "PowerPC 970"; dc->desc = "POWER5+"; dc->desc = "POWER7"; dc->desc = "POWER8";
<1465795496-15071-2-git-send-email-clg@kaod.org> <20160616010702.GI28087@voom.fritz.box> <20160617022731.GA19581@voom.fritz.box> <57639095.5010305@kaod.org> <576392B1.6030204@kaod.org> <5763A258.2010408@redhat.com> <5763D3EF.6060305@kaod.org> Organization: IBM Australia Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.3 (3.20.3-1.fc24) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit On Fri, 2016-06-17 at 12:41 +0200, Cédric Le Goater wrote: > > This is too brutal : > > + /* 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; > + } > > > There are a couple of instructions which have been deleted from > ISA 2.x. rfi is one of them. Could we use a insn_flag to filter > them ? Why is it too brutal ? We don't really support pre-arch 2.0 64-bit processors do we ? Cheers, Ben.
On Fri, 2016-06-17 at 13:02 +0200, Thomas Huth wrote: > According to the PPC970FX user manual that I have: > > "The 970FX does not provide support for the following optional or > obsolete instructions (or instruction forms). > Attempted use of these will result in an illegal instruction type > program interrupt. > [...] > · rfi - Return from interrupt (obsolete) " > > So if OpenBIOS is using this instruction in 970 mode, it's maybe > OpenBIOS that should be fixed instead? Right, I was about to say that ... This instruction *might* have existed on POWER3 which we don't emulate, but definitely not on POWER4 and later. Cheers, Ben.
On Fri, 2016-06-17 at 16:32 +0200, Cédric Le Goater wrote: > The instruction set PPC_POWER_BR contains nearly all the deleted > instructions from isa2. rfi is not part of it and should. Also, only > the cpus "PowerPC 601*" make a use of it in their insns_flags. Are you sure those arent the old POWER instructions as in pre-powerPC architecture that 601 (and only 601) supports ? > So, we would want this set to be in all the "PowerPC {6,7}*" cpus. > Are there more ? All 32-bit hash based CPUs are arch 1.x and support rfi All 64-bit hash based CPUs we support (ie, POWER4 and later) are architecture 2.x and later. So my test is correct in the context of what we emulate today. Cheers, Ben.
On 06/19/2016 01:35 AM, Benjamin Herrenschmidt wrote: > On Fri, 2016-06-17 at 16:32 +0200, Cédric Le Goater wrote: >> The instruction set PPC_POWER_BR contains nearly all the deleted >> instructions from isa2. rfi is not part of it and should. Also, only >> the cpus "PowerPC 601*" make a use of it in their insns_flags. > > Are you sure those arent the old POWER instructions as in pre-powerPC > architecture that 601 (and only 601) supports ? OK. I get it now. All the deleted instructions from POWER are/should be under the set PPC_POWER. All the deleted instructions from POWER2 are under the set PPC_POWER2. None of these sets are in use. For the "PowerPC 601*" cpus, we moved a couple from set PPC_POWER to subset PPC_POWER_BR. rfi is special. it is under PPC_FLOW and all CPUs can use it >> So, we would want this set to be in all the "PowerPC {6,7}*" cpus. >> Are there more ? > > All 32-bit hash based CPUs are arch 1.x and support rfi > > All 64-bit hash based CPUs we support (ie, POWER4 and later) are > architecture 2.x and later. > > So my test is correct in the context of what we emulate today. OK. so this is an openbios issue when run under a ppc64. shouldn't we be using an openbios-ppc64 in that case ? We could be more strict with the rfi instruction. C.
> Am 19.06.2016 um 14:49 schrieb Cédric Le Goater <clg@kaod.org>: > >> On 06/19/2016 01:35 AM, Benjamin Herrenschmidt wrote: >>> On Fri, 2016-06-17 at 16:32 +0200, Cédric Le Goater wrote: >>> The instruction set PPC_POWER_BR contains nearly all the deleted >>> instructions from isa2. rfi is not part of it and should. Also, only >>> the cpus "PowerPC 601*" make a use of it in their insns_flags. >> >> Are you sure those arent the old POWER instructions as in pre-powerPC >> architecture that 601 (and only 601) supports ? > > OK. I get it now. > > All the deleted instructions from POWER are/should be under the set > PPC_POWER. > > All the deleted instructions from POWER2 are under the set PPC_POWER2. > > None of these sets are in use. > > > For the "PowerPC 601*" cpus, we moved a couple from set PPC_POWER to > subset PPC_POWER_BR. > > rfi is special. it is under PPC_FLOW and all CPUs can use it > >>> So, we would want this set to be in all the "PowerPC {6,7}*" cpus. >>> Are there more ? >> >> All 32-bit hash based CPUs are arch 1.x and support rfi >> >> All 64-bit hash based CPUs we support (ie, POWER4 and later) are >> architecture 2.x and later. >> >> So my test is correct in the context of what we emulate today. > > OK. so this is an openbios issue when run under a ppc64. shouldn't we > be using an openbios-ppc64 in that case ? No, openbios can run on both. Just add a runtime check in openbios for rfi/rfid. Alex > > > We could be more strict with the rfi instruction. > > > C. >
On Sun, 2016-06-19 at 14:49 +0200, Cédric Le Goater wrote: > > So my test is correct in the context of what we emulate today. > > OK. so this is an openbios issue when run under a ppc64. shouldn't we > be using an openbios-ppc64 in that case ? > > > We could be more strict with the rfi instruction. Well, ppc64 isn't backward compatible with ppc32 at the supervisor level, at least not on server chips (it is on bookE chips). So yes it's an OpenBIOS problem but that doesn't mean we have to use openbios-ppc64 :-) You can run a 32-bit OS or firmware on ppc64, but it needs to know that it's running on a 64-bit chip and do a few things differently. Cheers, Ben.
On 06/19/2016 04:08 PM, Benjamin Herrenschmidt wrote: > On Sun, 2016-06-19 at 14:49 +0200, Cédric Le Goater wrote: >>> So my test is correct in the context of what we emulate today. >> >> OK. so this is an openbios issue when run under a ppc64. shouldn't we >> be using an openbios-ppc64 in that case ? >> >> >> We could be more strict with the rfi instruction. > > Well, ppc64 isn't backward compatible with ppc32 at the supervisor > level, at least not on server chips (it is on bookE chips). So yes it's > an OpenBIOS problem but that doesn't mean we have to use openbios-ppc64 > :-) > > You can run a 32-bit OS or firmware on ppc64, but it needs to know that > it's running on a 64-bit chip and do a few things differently. yes sure but qemu would still allow rfi under 64bit CPUs, that is what I was concerned about. Is that ok ? C.
On Sun, 2016-06-19 at 19:23 +0200, Cédric Le Goater wrote: > > You can run a 32-bit OS or firmware on ppc64, but it needs to know that > > it's running on a 64-bit chip and do a few things differently. > > yes sure but qemu would still allow rfi under 64bit CPUs, that is what > I was concerned about. Is that ok ? Why ? A real CPU won't allow it, why should we ? Cheers, Ben.
On Mon, Jun 20, 2016 at 07:12:38AM +1000, Benjamin Herrenschmidt wrote: > On Sun, 2016-06-19 at 19:23 +0200, Cédric Le Goater wrote: > > > You can run a 32-bit OS or firmware on ppc64, but it needs to know that > > > it's running on a 64-bit chip and do a few things differently. > > > > yes sure but qemu would still allow rfi under 64bit CPUs, that is what > > I was concerned about. Is that ok ? > > Why ? A real CPU won't allow it, why should we ? We shouldn't. However, I'm inclined to in for now, until we have an OpenBIOS fix actually committed. I'd prefer to keep the existing setup sorta-working when the current situation is unlikely to break working code, even though it's definitely wrong. BenH or Cédric, if you want to resend the hrfi fix patch with the 64-bit rfi support left in for no, that would be good.
On 06/19/2016 11:12 PM, Benjamin Herrenschmidt wrote: > On Sun, 2016-06-19 at 19:23 +0200, Cédric Le Goater wrote: >>> You can run a 32-bit OS or firmware on ppc64, but it needs to know that >>> it's running on a 64-bit chip and do a few things differently. >> >> yes sure but qemu would still allow rfi under 64bit CPUs, that is what >> I was concerned about. Is that ok ? > > Why ? A real CPU won't allow it, why should we ? That is how I feel also. So, why don't we just remove the op code in the instruction sets from the 32bit CPUs instead of leaving it and testing for PPC_SEGMENT_64B ? or is there some reasons we want to keep it around ? Cheers, C.
On 06/20/2016 04:19 AM, David Gibson wrote: > On Mon, Jun 20, 2016 at 07:12:38AM +1000, Benjamin Herrenschmidt wrote: >> On Sun, 2016-06-19 at 19:23 +0200, Cédric Le Goater wrote: >>>> You can run a 32-bit OS or firmware on ppc64, but it needs to know that >>>> it's running on a 64-bit chip and do a few things differently. >>> >>> yes sure but qemu would still allow rfi under 64bit CPUs, that is what >>> I was concerned about. Is that ok ? >> >> Why ? A real CPU won't allow it, why should we ? > > We shouldn't. However, I'm inclined to in for now, until we have an > OpenBIOS fix actually committed. I'd prefer to keep the existing > setup sorta-working when the current situation is unlikely to break > working code, even though it's definitely wrong. > > BenH or Cédric, if you want to resend the hrfi fix patch with the > 64-bit rfi support left in for no, that would be good. The current patch does not need fixing. I will send the OpenBIOS patch shortly after I have looked at the FPU exception. Linux ppc behaves the same on a 970. So we will need to fix the 'rfi's there also. C.
On 20.06.2016 08:17, Cédric Le Goater wrote: > On 06/20/2016 04:19 AM, David Gibson wrote: >> On Mon, Jun 20, 2016 at 07:12:38AM +1000, Benjamin Herrenschmidt wrote: >>> On Sun, 2016-06-19 at 19:23 +0200, Cédric Le Goater wrote: >>>>> You can run a 32-bit OS or firmware on ppc64, but it needs to know that >>>>> it's running on a 64-bit chip and do a few things differently. >>>> >>>> yes sure but qemu would still allow rfi under 64bit CPUs, that is what >>>> I was concerned about. Is that ok ? >>> >>> Why ? A real CPU won't allow it, why should we ? >> >> We shouldn't. However, I'm inclined to in for now, until we have an >> OpenBIOS fix actually committed. I'd prefer to keep the existing >> setup sorta-working when the current situation is unlikely to break >> working code, even though it's definitely wrong. >> >> BenH or Cédric, if you want to resend the hrfi fix patch with the >> 64-bit rfi support left in for no, that would be good. > > The current patch does not need fixing. I will send the OpenBIOS patch > shortly after I have looked at the FPU exception. > > Linux ppc behaves the same on a 970. So we will need to fix the 'rfi's > there also. Really? Wow, that surprises me. That OpenBIOS code likely never ran on a real 970 hardware, so that's not too much surprising that the "rfi" sneaked in there, but the Linux kernel certainly ran on a real 970 once, so I wonder why it's not properly using rfid for the 970 yet? Thomas
On Mon, 2016-06-20 at 08:10 +0200, Cédric Le Goater wrote: > That is how I feel also. So, why don't we just remove the op code in the > instruction sets from the 32bit CPUs instead of leaving it and testing > for PPC_SEGMENT_64B ? or is there some reasons we want to keep it around ? Ah no Ben.
On Mon, 2016-06-20 at 08:17 +0200, Cédric Le Goater wrote: > > The current patch does not need fixing. I will send the OpenBIOS > patch > shortly after I have looked at the FPU exception. > > Linux ppc behaves the same on a 970. So we will need to fix the > 'rfi's there also. What do you mean ? where ? Linux should be only using rfid on 970... Cheers, Ben.
On Mon, 2016-06-20 at 09:47 +0200, Thomas Huth wrote: > > Linux ppc behaves the same on a 970. So we will need to fix the 'rfi's > > there also. > > Really? Wow, that surprises me. That OpenBIOS code likely never ran on a > real 970 hardware, so that's not too much surprising that the "rfi" > sneaked in there, but the Linux kernel certainly ran on a real 970 once, > so I wonder why it's not properly using rfid for the 970 yet? It's working, Cedric was looking at the 32-bit kernel :-) Cheers, Ben.
On 06/20/2016 09:47 AM, Thomas Huth wrote: > On 20.06.2016 08:17, Cédric Le Goater wrote: >> On 06/20/2016 04:19 AM, David Gibson wrote: >>> On Mon, Jun 20, 2016 at 07:12:38AM +1000, Benjamin Herrenschmidt wrote: >>>> On Sun, 2016-06-19 at 19:23 +0200, Cédric Le Goater wrote: >>>>>> You can run a 32-bit OS or firmware on ppc64, but it needs to know that >>>>>> it's running on a 64-bit chip and do a few things differently. >>>>> >>>>> yes sure but qemu would still allow rfi under 64bit CPUs, that is what >>>>> I was concerned about. Is that ok ? >>>> >>>> Why ? A real CPU won't allow it, why should we ? >>> >>> We shouldn't. However, I'm inclined to in for now, until we have an >>> OpenBIOS fix actually committed. I'd prefer to keep the existing >>> setup sorta-working when the current situation is unlikely to break >>> working code, even though it's definitely wrong. >>> >>> BenH or Cédric, if you want to resend the hrfi fix patch with the >>> 64-bit rfi support left in for no, that would be good. >> >> The current patch does not need fixing. I will send the OpenBIOS patch >> shortly after I have looked at the FPU exception. >> >> Linux ppc behaves the same on a 970. So we will need to fix the 'rfi's >> there also. > > Really? Wow, that surprises me. That OpenBIOS code likely never ran on a > real 970 hardware, so that's not too much surprising that the "rfi" > sneaked in there, but the Linux kernel certainly ran on a real 970 once, > so I wonder why it's not properly using rfid for the 970 yet? I just learned that the support for 970 was removed from the 32bit kernel long time ago. False alarm, my bad. C.
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);