| Message ID | 1466545735-2555-2-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Jun 21, 2016 at 11:48:46PM +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. > > This patch broke 32bit OpenBIOS when run under a 970 cpu. A fix was > proposed here : > > https://www.coreboot.org/pipermail/openbios/2016-June/009452.html > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > [clg: updated the commit log with the reference of the openbios fix ] > Signed-off-by: Cédric Le Goater <clg@kaod.org> I'm not comfortable merging this until the openbios change is pulled back into the qemu tree (submodule and pre-built binary). Again - sure you don't want to apply this with rfi still enabled for 64-bit for now, letting the rest of this series go in as well, then clean up the rfi/64 behaviour later? > --- > > target-ppc/excp_helper.c | 51 +++++++++++++++++++----------------------------- > target-ppc/translate.c | 8 ++++++++ > 2 files changed, 28 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..81481955a589 100644 > --- a/target-ppc/translate.c > +++ b/target-ppc/translate.c > @@ -4087,6 +4087,14 @@ 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/22/2016 04:46 AM, David Gibson wrote: > I'm not comfortable merging this until the openbios change is pulled > back into the qemu tree (submodule and pre-built binary). > > Again - sure you don't want to apply this with rfi still enabled for > 64-bit for now, letting the rest of this series go in as well, then > clean up the rfi/64 behaviour later? Sure. I don't think it has an impact on the serie anyway so we can keep it for later. Thanks, C.
On Wed, Jun 22, 2016 at 08:48:14AM +0200, Cédric Le Goater wrote: > On 06/22/2016 04:46 AM, David Gibson wrote: > > I'm not comfortable merging this until the openbios change is pulled > > back into the qemu tree (submodule and pre-built binary). > > > > Again - sure you don't want to apply this with rfi still enabled for > > 64-bit for now, letting the rest of this series go in as well, then > > clean up the rfi/64 behaviour later? > > Sure. I don't think it has an impact on the serie anyway so we can keep > it for later. Ok, I modified 1/10 to leave rfi in for 64-bit cpus for now, and applied the full series to ppc-for-2.7. I've also now sent a pull request including this.
On 06/23/2016 07:50 AM, David Gibson wrote: > On Wed, Jun 22, 2016 at 08:48:14AM +0200, Cédric Le Goater wrote: >> On 06/22/2016 04:46 AM, David Gibson wrote: >>> I'm not comfortable merging this until the openbios change is pulled >>> back into the qemu tree (submodule and pre-built binary). >>> >>> Again - sure you don't want to apply this with rfi still enabled for >>> 64-bit for now, letting the rest of this series go in as well, then >>> clean up the rfi/64 behaviour later? >> >> Sure. I don't think it has an impact on the serie anyway so we can keep >> it for later. > > Ok, I modified 1/10 to leave rfi in for 64-bit cpus for now, and > applied the full series to ppc-for-2.7. I've also now sent a pull > request including this. Perfect. I was wondering if we should not just remove the instruction from the 64bit set. Thanks, C.
On Thu, Jun 23, 2016 at 08:08:38AM +0200, Cédric Le Goater wrote: > On 06/23/2016 07:50 AM, David Gibson wrote: > > On Wed, Jun 22, 2016 at 08:48:14AM +0200, Cédric Le Goater wrote: > >> On 06/22/2016 04:46 AM, David Gibson wrote: > >>> I'm not comfortable merging this until the openbios change is pulled > >>> back into the qemu tree (submodule and pre-built binary). > >>> > >>> Again - sure you don't want to apply this with rfi still enabled for > >>> 64-bit for now, letting the rest of this series go in as well, then > >>> clean up the rfi/64 behaviour later? > >> > >> Sure. I don't think it has an impact on the serie anyway so we can keep > >> it for later. > > > > Ok, I modified 1/10 to leave rfi in for 64-bit cpus for now, and > > applied the full series to ppc-for-2.7. I've also now sent a pull > > request including this. > > Perfect. I was wondering if we should not just remove the instruction from > the 64bit set. We should, but I don't want to do so until OpenBIOS is updated to cope.
On 23/06/16 07:14, David Gibson wrote: > On Thu, Jun 23, 2016 at 08:08:38AM +0200, Cédric Le Goater wrote: >> On 06/23/2016 07:50 AM, David Gibson wrote: >>> On Wed, Jun 22, 2016 at 08:48:14AM +0200, Cédric Le Goater wrote: >>>> On 06/22/2016 04:46 AM, David Gibson wrote: >>>>> I'm not comfortable merging this until the openbios change is pulled >>>>> back into the qemu tree (submodule and pre-built binary). >>>>> >>>>> Again - sure you don't want to apply this with rfi still enabled for >>>>> 64-bit for now, letting the rest of this series go in as well, then >>>>> clean up the rfi/64 behaviour later? >>>> >>>> Sure. I don't think it has an impact on the serie anyway so we can keep >>>> it for later. >>> >>> Ok, I modified 1/10 to leave rfi in for 64-bit cpus for now, and >>> applied the full series to ppc-for-2.7. I've also now sent a pull >>> request including this. >> >> Perfect. I was wondering if we should not just remove the instruction from >> the 64bit set. > > We should, but I don't want to do so until OpenBIOS is updated to > cope. FWIW I've just sent a pull request to update the OpenBIOS images which contains Cédric's v4 patch as reviewed by Alex: https://www.coreboot.org/pipermail/openbios/2016-June/009461.html. ATB, Mark.
On 23.06.2016 07:50, David Gibson wrote: > On Wed, Jun 22, 2016 at 08:48:14AM +0200, Cédric Le Goater wrote: >> On 06/22/2016 04:46 AM, David Gibson wrote: >>> I'm not comfortable merging this until the openbios change is pulled >>> back into the qemu tree (submodule and pre-built binary). >>> >>> Again - sure you don't want to apply this with rfi still enabled for >>> 64-bit for now, letting the rest of this series go in as well, then >>> clean up the rfi/64 behaviour later? >> >> Sure. I don't think it has an impact on the serie anyway so we can keep >> it for later. > > Ok, I modified 1/10 to leave rfi in for 64-bit cpus for now, and > applied the full series to ppc-for-2.7. I've also now sent a pull > request including this. Shall we disable rfi now for QEMU 2.8 ? Cédric, could you maybe send a patch with that hunk again? Thomas
On 09/05/2016 10:25 PM, Thomas Huth wrote: > On 23.06.2016 07:50, David Gibson wrote: >> On Wed, Jun 22, 2016 at 08:48:14AM +0200, Cédric Le Goater wrote: >>> On 06/22/2016 04:46 AM, David Gibson wrote: >>>> I'm not comfortable merging this until the openbios change is pulled >>>> back into the qemu tree (submodule and pre-built binary). >>>> >>>> Again - sure you don't want to apply this with rfi still enabled for >>>> 64-bit for now, letting the rest of this series go in as well, then >>>> clean up the rfi/64 behaviour later? >>> >>> Sure. I don't think it has an impact on the serie anyway so we can keep >>> it for later. >> >> Ok, I modified 1/10 to leave rfi in for 64-bit cpus for now, and >> applied the full series to ppc-for-2.7. I've also now sent a pull >> request including this. > > Shall we disable rfi now for QEMU 2.8 ? Cédric, could you maybe send a > patch with that hunk again? Sure. I have kept it in a warm place here : https://github.com/legoater/qemu/commit/492a631e4e817863be312c1a34957cd8d679a56c Mark, is openbios at the right level now ? I have lost track of the recent changes. Thanks, C.
On 05/09/16 21:30, Cédric Le Goater wrote: >> Shall we disable rfi now for QEMU 2.8 ? Cédric, could you maybe send a >> patch with that hunk again? > > Sure. I have kept it in a warm place here : > > https://github.com/legoater/qemu/commit/492a631e4e817863be312c1a34957cd8d679a56c > > Mark, is openbios at the right level now ? I have lost track of the > recent changes. The following patch is already in the current OpenBIOS binaries: https://github.com/openbios/openbios/commit/b747b6acc272f6ab839728193042455c9b36e26a. Is that the one you're looking for? ATB, Mark.
On Mon, Sep 05, 2016 at 09:51:09PM +0100, Mark Cave-Ayland wrote: > On 05/09/16 21:30, Cédric Le Goater wrote: > > >> Shall we disable rfi now for QEMU 2.8 ? Cédric, could you maybe send a > >> patch with that hunk again? > > > > Sure. I have kept it in a warm place here : > > > > https://github.com/legoater/qemu/commit/492a631e4e817863be312c1a34957cd8d679a56c > > > > Mark, is openbios at the right level now ? I have lost track of the > > recent changes. > > The following patch is already in the current OpenBIOS binaries: > https://github.com/openbios/openbios/commit/b747b6acc272f6ab839728193042455c9b36e26a. > Is that the one you're looking for? Right, the relevant question is whether the updated openbios is in the qemu submodule and canned binary.
On 06/09/16 01:16, David Gibson wrote: > On Mon, Sep 05, 2016 at 09:51:09PM +0100, Mark Cave-Ayland wrote: >> On 05/09/16 21:30, Cédric Le Goater wrote: >> >>>> Shall we disable rfi now for QEMU 2.8 ? Cédric, could you maybe send a >>>> patch with that hunk again? >>> >>> Sure. I have kept it in a warm place here : >>> >>> https://github.com/legoater/qemu/commit/492a631e4e817863be312c1a34957cd8d679a56c >>> >>> Mark, is openbios at the right level now ? I have lost track of the >>> recent changes. >> >> The following patch is already in the current OpenBIOS binaries: >> https://github.com/openbios/openbios/commit/b747b6acc272f6ab839728193042455c9b36e26a. >> Is that the one you're looking for? > > Right, the relevant question is whether the updated openbios is in the > qemu submodule and canned binary. Yes, it was included in last merge for 2.7. ATB, Mark.
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..81481955a589 100644 --- a/target-ppc/translate.c +++ b/target-ppc/translate.c @@ -4087,6 +4087,14 @@ 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);