| Message ID | 5754645E.8080504@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/06/16 18:41, Cédric Le Goater wrote: > Hello Mark, > > On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote: >> On 03/06/16 13:11, Cédric Le Goater wrote: >> >>> This is follow up to complete the serie "ppc: preparing pnv landing >>> (round 2)" plus a little fix on instruction privileges. >>> >>> Tested on a POWER8 pserie guest and on mac99. >>> >>> Benjamin Herrenschmidt (2): >>> ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV >>> ppc: Better figure out if processor has HV mode >>> >>> Cédric Le Goater (1): >>> ppc: fix hrfid, tlbia and slbia privilege >>> >>> target-ppc/cpu.h | 4 ++++ >>> target-ppc/excp_helper.c | 8 ++++++-- >>> target-ppc/helper_regs.h | 4 ++-- >>> target-ppc/translate.c | 10 ++++++---- >>> target-ppc/translate_init.c | 19 +++++++++++++++---- >>> 5 files changed, 33 insertions(+), 12 deletions(-) >> >> Hi Cédric, >> >> I can confirm that this patchset fixes starting up OpenBIOS for both >> g3beige/mac99 in my tests here. With the escc fix also applied, the only >> outstanding issue is the removal of the tlb_flush() statements which >> causes Darwin, MacOS X and HelenOS 0.60 to panic on boot >> >> My only request is if it would be possible to move patch 2 "ppc: Better >> figure out if processor has HV mode" to the front of this patchset which >> will make the remaining patches bisectable for the Mac machines. With that: >> >> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> >> Does anyone know if Ben has any ideas as to why the MMU tlb_flush >> changes patch is causing such problems? > > > Here is a fix I think. Could you give it a try ? > > Cheers, > > C. > > From: Cédric Le Goater <clg@kaod.org> > Subject: [PATCH] ppc: fix tlb flushes for 32bit environment > Date: Sun, 05 Jun 2016 18:46:19 +0200 > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > commit cd0c6f473532 ('ppc: Do some batching of TCG tlb flushes') > introduced an optimisation to flush TLBs only when a context > synchronizing event is reached (interrupt, rfi). This was done for > ppc64 but 32bit was forgotten on the way. > > Tested on mac99 and g3beige with > > qemu-system-ppc -cdrom darwinppc-602.cdr -boot d > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > > I think the hunk in powerpc_excp() is needed if we don't generate a > context synchronizing event. what is best to do ? > > target-ppc/cpu.h | 2 +- > target-ppc/excp_helper.c | 10 ++++++++++ > target-ppc/helper_regs.h | 9 ++++++++- > target-ppc/translate.c | 2 +- > 4 files changed, 20 insertions(+), 3 deletions(-) > > Index: qemu-dgibson-for-2.7.git/target-ppc/translate.c > =================================================================== > --- qemu-dgibson-for-2.7.git.orig/target-ppc/translate.c > +++ qemu-dgibson-for-2.7.git/target-ppc/translate.c > @@ -3290,7 +3290,7 @@ static void gen_eieio(DisasContext *ctx) > { > } > > -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64) > +#if !defined(CONFIG_USER_ONLY) > static inline void gen_check_tlb_flush(DisasContext *ctx) > { > TCGv_i32 t = tcg_temp_new_i32(); > Index: qemu-dgibson-for-2.7.git/target-ppc/cpu.h > =================================================================== > --- qemu-dgibson-for-2.7.git.orig/target-ppc/cpu.h > +++ qemu-dgibson-for-2.7.git/target-ppc/cpu.h > @@ -958,9 +958,9 @@ struct CPUPPCState { > /* PowerPC 64 SLB area */ > ppc_slb_t slb[MAX_SLB_ENTRIES]; > int32_t slb_nr; > +#endif > /* tcg TLB needs flush (deferred slb inval instruction typically) */ > uint32_t tlb_need_flush; > -#endif > /* segment registers */ > hwaddr htab_base; > /* mask used to normalize hash value to PTEG index */ > Index: qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h > =================================================================== > --- qemu-dgibson-for-2.7.git.orig/target-ppc/helper_regs.h > +++ qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h > @@ -121,6 +121,13 @@ static inline int hreg_store_msr(CPUPPCS > } > if (((value >> MSR_IR) & 1) != msr_ir || > ((value >> MSR_DR) & 1) != msr_dr) { > + /* A change of the instruction relocation bit in the MSR can > + * cause an implicit branch in the address space. This > + * requires a tlb flush. > + */ > + if (env->mmu_model & POWERPC_MMU_32B) { > + env->tlb_need_flush = 1; > + } > cs->interrupt_request |= CPU_INTERRUPT_EXITTB; > } > if ((env->mmu_model & POWERPC_MMU_BOOKE) && > @@ -151,7 +158,7 @@ static inline int hreg_store_msr(CPUPPCS > return excp; > } > > -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64) > +#if !defined(CONFIG_USER_ONLY) > static inline void check_tlb_flush(CPUPPCState *env) > { > CPUState *cs = CPU(ppc_env_get_cpu(env)); > Index: qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c > =================================================================== > --- qemu-dgibson-for-2.7.git.orig/target-ppc/excp_helper.c > +++ qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c > @@ -709,6 +709,16 @@ static inline void powerpc_excp(PowerPCC > } > } > #endif > + if (((new_msr >> MSR_IR) & 1) != msr_ir || > + ((new_msr >> MSR_DR) & 1) != msr_dr) { > + /* A change of the instruction relocation bit in the MSR can > + * cause an implicit branch in the address space. This > + * requires a tlb flush. > + */ > + if (env->mmu_model & POWERPC_MMU_32B) { > + env->tlb_need_flush = 1; > + } > + } > /* We don't use hreg_store_msr here as already have treated > * any special case that could occur. Just store MSR and update hflags > * > > Hi Cédric, I've just tried this patch on a selection of images here with both g3beige and mac99 and as far as I can tell the crash has now gone away: Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Thanks for getting this sorted so quickly. ATB, Mark.
On Sun, Jun 05, 2016 at 07:41:50PM +0200, Cédric Le Goater wrote: > Hello Mark, > > On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote: > > On 03/06/16 13:11, Cédric Le Goater wrote: > > > >> This is follow up to complete the serie "ppc: preparing pnv landing > >> (round 2)" plus a little fix on instruction privileges. > >> > >> Tested on a POWER8 pserie guest and on mac99. > >> > >> Benjamin Herrenschmidt (2): > >> ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV > >> ppc: Better figure out if processor has HV mode > >> > >> Cédric Le Goater (1): > >> ppc: fix hrfid, tlbia and slbia privilege > >> > >> target-ppc/cpu.h | 4 ++++ > >> target-ppc/excp_helper.c | 8 ++++++-- > >> target-ppc/helper_regs.h | 4 ++-- > >> target-ppc/translate.c | 10 ++++++---- > >> target-ppc/translate_init.c | 19 +++++++++++++++---- > >> 5 files changed, 33 insertions(+), 12 deletions(-) > > > > Hi Cédric, > > > > I can confirm that this patchset fixes starting up OpenBIOS for both > > g3beige/mac99 in my tests here. With the escc fix also applied, the only > > outstanding issue is the removal of the tlb_flush() statements which > > causes Darwin, MacOS X and HelenOS 0.60 to panic on boot > > > > My only request is if it would be possible to move patch 2 "ppc: Better > > figure out if processor has HV mode" to the front of this patchset which > > will make the remaining patches bisectable for the Mac machines. With that: > > > > Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > > > Does anyone know if Ben has any ideas as to why the MMU tlb_flush > > changes patch is causing such problems? > > > Here is a fix I think. Could you give it a try ? So, I had applied this to ppc-for-2.7, but I've now removed it again. BenH correctly pointed out that it basically just removes any benefit of his original tlb_flush() patch, in a slightly more subtle way that the last "fix". You just set the need_flush flag whenever IR or DR are changed, whereas the whole point of BenH's patch is that the translation on and off modes are now different MM contexts, which should be flagged in qemu's TLB and so not require a full flush. We need to work out what the real problem is here.
On Sun, 2016-06-05 at 19:41 +0200, Cédric Le Goater wrote: > > Here is a fix I think. Could you give it a try ? This is somewhat wrong... > commit cd0c6f473532 ('ppc: Do some batching of TCG tlb flushes') > introduced an optimisation to flush TLBs only when a context > synchronizing event is reached (interrupt, rfi). This was done for > ppc64 but 32bit was forgotten on the way. No it didn't. That commit only delays flushes on ppc64. ppc32 is unaffected, unless I missed something. IE. It will delay flushes caused by slb instructions (which don't exist on 32-bit) and ppc_tlb_invalidate_one() only in the 64-bit cases. Also what your patch does in practice is not really change that, though you seem to try to somewhat extend the batching to 32-bit (but incompletely), you also introduce something which effectively reverts part of 9fb044911444fdd09f5f072ad0ca269d7f8b841d (split I/D mode). I think that's more what's "fixing" your problem, ie, the flush in IR/DR changes. However it shouldn't be needed. I suspect all of that is papering over another bug somewhere else which got exposed by the split I/D mode, since we no longer over-flush on transitions to/from real-mode. So we must be missing flushes elsewhere, possibly some G3 specific stuff, or there always was some kind of bug in the TLB flushing on 32-bit that got somewhat masked by the over- flushing we used to do. I need a repro-case. Cheers, Ben. > Tested on mac99 and g3beige with > > qemu-system-ppc -cdrom darwinppc-602.cdr -boot d > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > > I think the hunk in powerpc_excp() is needed if we don't generate a > context synchronizing event. what is best to do ? > > target-ppc/cpu.h | 2 +- > target-ppc/excp_helper.c | 10 ++++++++++ > target-ppc/helper_regs.h | 9 ++++++++- > target-ppc/translate.c | 2 +- > 4 files changed, 20 insertions(+), 3 deletions(-) > > Index: qemu-dgibson-for-2.7.git/target-ppc/translate.c > =================================================================== > --- qemu-dgibson-for-2.7.git.orig/target-ppc/translate.c > +++ qemu-dgibson-for-2.7.git/target-ppc/translate.c > @@ -3290,7 +3290,7 @@ static void gen_eieio(DisasContext *ctx) > { > } > > -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64) > +#if !defined(CONFIG_USER_ONLY) > static inline void gen_check_tlb_flush(DisasContext *ctx) > { > TCGv_i32 t = tcg_temp_new_i32(); > Index: qemu-dgibson-for-2.7.git/target-ppc/cpu.h > =================================================================== > --- qemu-dgibson-for-2.7.git.orig/target-ppc/cpu.h > +++ qemu-dgibson-for-2.7.git/target-ppc/cpu.h > @@ -958,9 +958,9 @@ struct CPUPPCState { > /* PowerPC 64 SLB area */ > ppc_slb_t slb[MAX_SLB_ENTRIES]; > int32_t slb_nr; > +#endif > /* tcg TLB needs flush (deferred slb inval instruction > typically) */ > uint32_t tlb_need_flush; > -#endif > /* segment registers */ > hwaddr htab_base; > /* mask used to normalize hash value to PTEG index */ > Index: qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h > =================================================================== > --- qemu-dgibson-for-2.7.git.orig/target-ppc/helper_regs.h > +++ qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h > @@ -121,6 +121,13 @@ static inline int hreg_store_msr(CPUPPCS > } > if (((value >> MSR_IR) & 1) != msr_ir || > ((value >> MSR_DR) & 1) != msr_dr) { > + /* A change of the instruction relocation bit in the MSR can > + * cause an implicit branch in the address space. This > + * requires a tlb flush. > + */ > + if (env->mmu_model & POWERPC_MMU_32B) { > + env->tlb_need_flush = 1; > + } > cs->interrupt_request |= CPU_INTERRUPT_EXITTB; > } > if ((env->mmu_model & POWERPC_MMU_BOOKE) && > @@ -151,7 +158,7 @@ static inline int hreg_store_msr(CPUPPCS > return excp; > } > > -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64) > +#if !defined(CONFIG_USER_ONLY) > static inline void check_tlb_flush(CPUPPCState *env) > { > CPUState *cs = CPU(ppc_env_get_cpu(env)); > Index: qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c > =================================================================== > --- qemu-dgibson-for-2.7.git.orig/target-ppc/excp_helper.c > +++ qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c > @@ -709,6 +709,16 @@ static inline void powerpc_excp(PowerPCC > } > } > #endif > + if (((new_msr >> MSR_IR) & 1) != msr_ir || > + ((new_msr >> MSR_DR) & 1) != msr_dr) { > + /* A change of the instruction relocation bit in the MSR can > + * cause an implicit branch in the address space. This > + * requires a tlb flush. > + */ > + if (env->mmu_model & POWERPC_MMU_32B) { > + env->tlb_need_flush = 1; > + } > + } > /* We don't use hreg_store_msr here as already have treated > * any special case that could occur. Just store MSR and update > hflags > * >
On 06/06/2016 12:26 AM, Mark Cave-Ayland wrote: > On 05/06/16 18:41, Cédric Le Goater wrote: > >> Hello Mark, >> >> On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote: >>> On 03/06/16 13:11, Cédric Le Goater wrote: >>> >>>> This is follow up to complete the serie "ppc: preparing pnv landing >>>> (round 2)" plus a little fix on instruction privileges. >>>> >>>> Tested on a POWER8 pserie guest and on mac99. >>>> >>>> Benjamin Herrenschmidt (2): >>>> ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV >>>> ppc: Better figure out if processor has HV mode >>>> >>>> Cédric Le Goater (1): >>>> ppc: fix hrfid, tlbia and slbia privilege >>>> >>>> target-ppc/cpu.h | 4 ++++ >>>> target-ppc/excp_helper.c | 8 ++++++-- >>>> target-ppc/helper_regs.h | 4 ++-- >>>> target-ppc/translate.c | 10 ++++++---- >>>> target-ppc/translate_init.c | 19 +++++++++++++++---- >>>> 5 files changed, 33 insertions(+), 12 deletions(-) >>> >>> Hi Cédric, >>> >>> I can confirm that this patchset fixes starting up OpenBIOS for both >>> g3beige/mac99 in my tests here. With the escc fix also applied, the only >>> outstanding issue is the removal of the tlb_flush() statements which >>> causes Darwin, MacOS X and HelenOS 0.60 to panic on boot >>> >>> My only request is if it would be possible to move patch 2 "ppc: Better >>> figure out if processor has HV mode" to the front of this patchset which >>> will make the remaining patches bisectable for the Mac machines. With that: >>> >>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>> >>> Does anyone know if Ben has any ideas as to why the MMU tlb_flush >>> changes patch is causing such problems? >> >> >> Here is a fix I think. Could you give it a try ? >> >> Cheers, >> >> C. >> >> From: Cédric Le Goater <clg@kaod.org> >> Subject: [PATCH] ppc: fix tlb flushes for 32bit environment >> Date: Sun, 05 Jun 2016 18:46:19 +0200 >> MIME-Version: 1.0 >> Content-Type: text/plain; charset=UTF-8 >> Content-Transfer-Encoding: 8bit >> >> commit cd0c6f473532 ('ppc: Do some batching of TCG tlb flushes') >> introduced an optimisation to flush TLBs only when a context >> synchronizing event is reached (interrupt, rfi). This was done for >> ppc64 but 32bit was forgotten on the way. >> >> Tested on mac99 and g3beige with >> >> qemu-system-ppc -cdrom darwinppc-602.cdr -boot d >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> >> I think the hunk in powerpc_excp() is needed if we don't generate a >> context synchronizing event. what is best to do ? >> >> target-ppc/cpu.h | 2 +- >> target-ppc/excp_helper.c | 10 ++++++++++ >> target-ppc/helper_regs.h | 9 ++++++++- >> target-ppc/translate.c | 2 +- >> 4 files changed, 20 insertions(+), 3 deletions(-) >> >> Index: qemu-dgibson-for-2.7.git/target-ppc/translate.c >> =================================================================== >> --- qemu-dgibson-for-2.7.git.orig/target-ppc/translate.c >> +++ qemu-dgibson-for-2.7.git/target-ppc/translate.c >> @@ -3290,7 +3290,7 @@ static void gen_eieio(DisasContext *ctx) >> { >> } >> >> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64) >> +#if !defined(CONFIG_USER_ONLY) >> static inline void gen_check_tlb_flush(DisasContext *ctx) >> { >> TCGv_i32 t = tcg_temp_new_i32(); >> Index: qemu-dgibson-for-2.7.git/target-ppc/cpu.h >> =================================================================== >> --- qemu-dgibson-for-2.7.git.orig/target-ppc/cpu.h >> +++ qemu-dgibson-for-2.7.git/target-ppc/cpu.h >> @@ -958,9 +958,9 @@ struct CPUPPCState { >> /* PowerPC 64 SLB area */ >> ppc_slb_t slb[MAX_SLB_ENTRIES]; >> int32_t slb_nr; >> +#endif >> /* tcg TLB needs flush (deferred slb inval instruction typically) */ >> uint32_t tlb_need_flush; >> -#endif >> /* segment registers */ >> hwaddr htab_base; >> /* mask used to normalize hash value to PTEG index */ >> Index: qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h >> =================================================================== >> --- qemu-dgibson-for-2.7.git.orig/target-ppc/helper_regs.h >> +++ qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h >> @@ -121,6 +121,13 @@ static inline int hreg_store_msr(CPUPPCS >> } >> if (((value >> MSR_IR) & 1) != msr_ir || >> ((value >> MSR_DR) & 1) != msr_dr) { >> + /* A change of the instruction relocation bit in the MSR can >> + * cause an implicit branch in the address space. This >> + * requires a tlb flush. >> + */ >> + if (env->mmu_model & POWERPC_MMU_32B) { >> + env->tlb_need_flush = 1; >> + } >> cs->interrupt_request |= CPU_INTERRUPT_EXITTB; >> } >> if ((env->mmu_model & POWERPC_MMU_BOOKE) && >> @@ -151,7 +158,7 @@ static inline int hreg_store_msr(CPUPPCS >> return excp; >> } >> >> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64) >> +#if !defined(CONFIG_USER_ONLY) >> static inline void check_tlb_flush(CPUPPCState *env) >> { >> CPUState *cs = CPU(ppc_env_get_cpu(env)); >> Index: qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c >> =================================================================== >> --- qemu-dgibson-for-2.7.git.orig/target-ppc/excp_helper.c >> +++ qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c >> @@ -709,6 +709,16 @@ static inline void powerpc_excp(PowerPCC >> } >> } >> #endif >> + if (((new_msr >> MSR_IR) & 1) != msr_ir || >> + ((new_msr >> MSR_DR) & 1) != msr_dr) { >> + /* A change of the instruction relocation bit in the MSR can >> + * cause an implicit branch in the address space. This >> + * requires a tlb flush. >> + */ >> + if (env->mmu_model & POWERPC_MMU_32B) { >> + env->tlb_need_flush = 1; >> + } >> + } >> /* We don't use hreg_store_msr here as already have treated >> * any special case that could occur. Just store MSR and update hflags >> * >> >> > > Hi Cédric, > > I've just tried this patch on a selection of images here with both > g3beige and mac99 and as far as I can tell the crash has now gone away: > > Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Super. I still need to sort out the exit path of hreg_store_msr(). If we have a change in IR/DR, this is a case to transform mtmsr in a context-synchronize instruction, for a tlb flush. This is problably the reason behind the POWERPC_EXCP_NONE I believe, which is then handled in powerpc_excp(). I need to look at that path closer. And, now that I have a darwin guest, I have a few questions : 1. How do I get X running ? 2. I have an old ibook G4 from which I dd'ed the disk. openbios complains for some invalid state. is that supported ? Thanks, C. > Thanks for getting this sorted so quickly. > > > ATB, > > Mark. >
On 06/06/2016 08:27 AM, Cédric Le Goater wrote: > On 06/06/2016 12:26 AM, Mark Cave-Ayland wrote: >> On 05/06/16 18:41, Cédric Le Goater wrote: >> >>> Hello Mark, >>> >>> On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote: >>>> On 03/06/16 13:11, Cédric Le Goater wrote: >>>> >>>>> This is follow up to complete the serie "ppc: preparing pnv landing >>>>> (round 2)" plus a little fix on instruction privileges. >>>>> >>>>> Tested on a POWER8 pserie guest and on mac99. >>>>> >>>>> Benjamin Herrenschmidt (2): >>>>> ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV >>>>> ppc: Better figure out if processor has HV mode >>>>> >>>>> Cédric Le Goater (1): >>>>> ppc: fix hrfid, tlbia and slbia privilege >>>>> >>>>> target-ppc/cpu.h | 4 ++++ >>>>> target-ppc/excp_helper.c | 8 ++++++-- >>>>> target-ppc/helper_regs.h | 4 ++-- >>>>> target-ppc/translate.c | 10 ++++++---- >>>>> target-ppc/translate_init.c | 19 +++++++++++++++---- >>>>> 5 files changed, 33 insertions(+), 12 deletions(-) >>>> >>>> Hi Cédric, >>>> >>>> I can confirm that this patchset fixes starting up OpenBIOS for both >>>> g3beige/mac99 in my tests here. With the escc fix also applied, the only >>>> outstanding issue is the removal of the tlb_flush() statements which >>>> causes Darwin, MacOS X and HelenOS 0.60 to panic on boot >>>> >>>> My only request is if it would be possible to move patch 2 "ppc: Better >>>> figure out if processor has HV mode" to the front of this patchset which >>>> will make the remaining patches bisectable for the Mac machines. With that: >>>> >>>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>> >>>> Does anyone know if Ben has any ideas as to why the MMU tlb_flush >>>> changes patch is causing such problems? >>> >>> >>> Here is a fix I think. Could you give it a try ? >>> >>> Cheers, >>> >>> C. >>> >>> From: Cédric Le Goater <clg@kaod.org> >>> Subject: [PATCH] ppc: fix tlb flushes for 32bit environment >>> Date: Sun, 05 Jun 2016 18:46:19 +0200 >>> MIME-Version: 1.0 >>> Content-Type: text/plain; charset=UTF-8 >>> Content-Transfer-Encoding: 8bit >>> >>> commit cd0c6f473532 ('ppc: Do some batching of TCG tlb flushes') >>> introduced an optimisation to flush TLBs only when a context >>> synchronizing event is reached (interrupt, rfi). This was done for >>> ppc64 but 32bit was forgotten on the way. >>> >>> Tested on mac99 and g3beige with >>> >>> qemu-system-ppc -cdrom darwinppc-602.cdr -boot d >>> >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> --- >>> >>> I think the hunk in powerpc_excp() is needed if we don't generate a >>> context synchronizing event. what is best to do ? >>> >>> target-ppc/cpu.h | 2 +- >>> target-ppc/excp_helper.c | 10 ++++++++++ >>> target-ppc/helper_regs.h | 9 ++++++++- >>> target-ppc/translate.c | 2 +- >>> 4 files changed, 20 insertions(+), 3 deletions(-) >>> >>> Index: qemu-dgibson-for-2.7.git/target-ppc/translate.c >>> =================================================================== >>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/translate.c >>> +++ qemu-dgibson-for-2.7.git/target-ppc/translate.c >>> @@ -3290,7 +3290,7 @@ static void gen_eieio(DisasContext *ctx) >>> { >>> } >>> >>> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64) >>> +#if !defined(CONFIG_USER_ONLY) >>> static inline void gen_check_tlb_flush(DisasContext *ctx) >>> { >>> TCGv_i32 t = tcg_temp_new_i32(); >>> Index: qemu-dgibson-for-2.7.git/target-ppc/cpu.h >>> =================================================================== >>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/cpu.h >>> +++ qemu-dgibson-for-2.7.git/target-ppc/cpu.h >>> @@ -958,9 +958,9 @@ struct CPUPPCState { >>> /* PowerPC 64 SLB area */ >>> ppc_slb_t slb[MAX_SLB_ENTRIES]; >>> int32_t slb_nr; >>> +#endif >>> /* tcg TLB needs flush (deferred slb inval instruction typically) */ >>> uint32_t tlb_need_flush; >>> -#endif >>> /* segment registers */ >>> hwaddr htab_base; >>> /* mask used to normalize hash value to PTEG index */ >>> Index: qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h >>> =================================================================== >>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/helper_regs.h >>> +++ qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h >>> @@ -121,6 +121,13 @@ static inline int hreg_store_msr(CPUPPCS >>> } >>> if (((value >> MSR_IR) & 1) != msr_ir || >>> ((value >> MSR_DR) & 1) != msr_dr) { >>> + /* A change of the instruction relocation bit in the MSR can >>> + * cause an implicit branch in the address space. This >>> + * requires a tlb flush. >>> + */ >>> + if (env->mmu_model & POWERPC_MMU_32B) { >>> + env->tlb_need_flush = 1; >>> + } >>> cs->interrupt_request |= CPU_INTERRUPT_EXITTB; >>> } >>> if ((env->mmu_model & POWERPC_MMU_BOOKE) && >>> @@ -151,7 +158,7 @@ static inline int hreg_store_msr(CPUPPCS >>> return excp; >>> } >>> >>> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64) >>> +#if !defined(CONFIG_USER_ONLY) >>> static inline void check_tlb_flush(CPUPPCState *env) >>> { >>> CPUState *cs = CPU(ppc_env_get_cpu(env)); >>> Index: qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c >>> =================================================================== >>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/excp_helper.c >>> +++ qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c >>> @@ -709,6 +709,16 @@ static inline void powerpc_excp(PowerPCC >>> } >>> } >>> #endif >>> + if (((new_msr >> MSR_IR) & 1) != msr_ir || >>> + ((new_msr >> MSR_DR) & 1) != msr_dr) { >>> + /* A change of the instruction relocation bit in the MSR can >>> + * cause an implicit branch in the address space. This >>> + * requires a tlb flush. >>> + */ >>> + if (env->mmu_model & POWERPC_MMU_32B) { >>> + env->tlb_need_flush = 1; >>> + } >>> + } >>> /* We don't use hreg_store_msr here as already have treated >>> * any special case that could occur. Just store MSR and update hflags >>> * >>> >>> >> >> Hi Cédric, >> >> I've just tried this patch on a selection of images here with both >> g3beige and mac99 and as far as I can tell the crash has now gone away: >> >> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > Super. I still need to sort out the exit path of hreg_store_msr(). > > If we have a change in IR/DR, this is a case to transform mtmsr in a > context-synchronize instruction, for a tlb flush. This is problably > the reason behind the POWERPC_EXCP_NONE I believe, which is then > handled in powerpc_excp(). I need to look at that path closer. Forget the above as Ben just passed by :) I still interested by what is below : > And, now that I have a darwin guest, I have a few questions : > > 1. How do I get X running ? > 2. I have an old ibook G4 from which I dd'ed the disk. openbios > complains for some invalid state. is that supported ? > > Thanks, > > C. > > > >> Thanks for getting this sorted so quickly. >> >> >> ATB, >> >> Mark. >> >
On 06/06/16 07:30, Cedric Le Goater wrote: > On 06/06/2016 08:27 AM, Cédric Le Goater wrote: >> On 06/06/2016 12:26 AM, Mark Cave-Ayland wrote: >>> On 05/06/16 18:41, Cédric Le Goater wrote: >>> >>>> Hello Mark, >>>> >>>> On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote: >>>>> On 03/06/16 13:11, Cédric Le Goater wrote: >>>>> >>>>>> This is follow up to complete the serie "ppc: preparing pnv landing >>>>>> (round 2)" plus a little fix on instruction privileges. >>>>>> >>>>>> Tested on a POWER8 pserie guest and on mac99. >>>>>> >>>>>> Benjamin Herrenschmidt (2): >>>>>> ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV >>>>>> ppc: Better figure out if processor has HV mode >>>>>> >>>>>> Cédric Le Goater (1): >>>>>> ppc: fix hrfid, tlbia and slbia privilege >>>>>> >>>>>> target-ppc/cpu.h | 4 ++++ >>>>>> target-ppc/excp_helper.c | 8 ++++++-- >>>>>> target-ppc/helper_regs.h | 4 ++-- >>>>>> target-ppc/translate.c | 10 ++++++---- >>>>>> target-ppc/translate_init.c | 19 +++++++++++++++---- >>>>>> 5 files changed, 33 insertions(+), 12 deletions(-) >>>>> >>>>> Hi Cédric, >>>>> >>>>> I can confirm that this patchset fixes starting up OpenBIOS for both >>>>> g3beige/mac99 in my tests here. With the escc fix also applied, the only >>>>> outstanding issue is the removal of the tlb_flush() statements which >>>>> causes Darwin, MacOS X and HelenOS 0.60 to panic on boot >>>>> >>>>> My only request is if it would be possible to move patch 2 "ppc: Better >>>>> figure out if processor has HV mode" to the front of this patchset which >>>>> will make the remaining patches bisectable for the Mac machines. With that: >>>>> >>>>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>> >>>>> Does anyone know if Ben has any ideas as to why the MMU tlb_flush >>>>> changes patch is causing such problems? >>>> >>>> >>>> Here is a fix I think. Could you give it a try ? >>>> >>>> Cheers, >>>> >>>> C. >>>> >>>> From: Cédric Le Goater <clg@kaod.org> >>>> Subject: [PATCH] ppc: fix tlb flushes for 32bit environment >>>> Date: Sun, 05 Jun 2016 18:46:19 +0200 >>>> MIME-Version: 1.0 >>>> Content-Type: text/plain; charset=UTF-8 >>>> Content-Transfer-Encoding: 8bit >>>> >>>> commit cd0c6f473532 ('ppc: Do some batching of TCG tlb flushes') >>>> introduced an optimisation to flush TLBs only when a context >>>> synchronizing event is reached (interrupt, rfi). This was done for >>>> ppc64 but 32bit was forgotten on the way. >>>> >>>> Tested on mac99 and g3beige with >>>> >>>> qemu-system-ppc -cdrom darwinppc-602.cdr -boot d >>>> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>> --- >>>> >>>> I think the hunk in powerpc_excp() is needed if we don't generate a >>>> context synchronizing event. what is best to do ? >>>> >>>> target-ppc/cpu.h | 2 +- >>>> target-ppc/excp_helper.c | 10 ++++++++++ >>>> target-ppc/helper_regs.h | 9 ++++++++- >>>> target-ppc/translate.c | 2 +- >>>> 4 files changed, 20 insertions(+), 3 deletions(-) >>>> >>>> Index: qemu-dgibson-for-2.7.git/target-ppc/translate.c >>>> =================================================================== >>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/translate.c >>>> +++ qemu-dgibson-for-2.7.git/target-ppc/translate.c >>>> @@ -3290,7 +3290,7 @@ static void gen_eieio(DisasContext *ctx) >>>> { >>>> } >>>> >>>> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64) >>>> +#if !defined(CONFIG_USER_ONLY) >>>> static inline void gen_check_tlb_flush(DisasContext *ctx) >>>> { >>>> TCGv_i32 t = tcg_temp_new_i32(); >>>> Index: qemu-dgibson-for-2.7.git/target-ppc/cpu.h >>>> =================================================================== >>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/cpu.h >>>> +++ qemu-dgibson-for-2.7.git/target-ppc/cpu.h >>>> @@ -958,9 +958,9 @@ struct CPUPPCState { >>>> /* PowerPC 64 SLB area */ >>>> ppc_slb_t slb[MAX_SLB_ENTRIES]; >>>> int32_t slb_nr; >>>> +#endif >>>> /* tcg TLB needs flush (deferred slb inval instruction typically) */ >>>> uint32_t tlb_need_flush; >>>> -#endif >>>> /* segment registers */ >>>> hwaddr htab_base; >>>> /* mask used to normalize hash value to PTEG index */ >>>> Index: qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h >>>> =================================================================== >>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/helper_regs.h >>>> +++ qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h >>>> @@ -121,6 +121,13 @@ static inline int hreg_store_msr(CPUPPCS >>>> } >>>> if (((value >> MSR_IR) & 1) != msr_ir || >>>> ((value >> MSR_DR) & 1) != msr_dr) { >>>> + /* A change of the instruction relocation bit in the MSR can >>>> + * cause an implicit branch in the address space. This >>>> + * requires a tlb flush. >>>> + */ >>>> + if (env->mmu_model & POWERPC_MMU_32B) { >>>> + env->tlb_need_flush = 1; >>>> + } >>>> cs->interrupt_request |= CPU_INTERRUPT_EXITTB; >>>> } >>>> if ((env->mmu_model & POWERPC_MMU_BOOKE) && >>>> @@ -151,7 +158,7 @@ static inline int hreg_store_msr(CPUPPCS >>>> return excp; >>>> } >>>> >>>> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64) >>>> +#if !defined(CONFIG_USER_ONLY) >>>> static inline void check_tlb_flush(CPUPPCState *env) >>>> { >>>> CPUState *cs = CPU(ppc_env_get_cpu(env)); >>>> Index: qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c >>>> =================================================================== >>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/excp_helper.c >>>> +++ qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c >>>> @@ -709,6 +709,16 @@ static inline void powerpc_excp(PowerPCC >>>> } >>>> } >>>> #endif >>>> + if (((new_msr >> MSR_IR) & 1) != msr_ir || >>>> + ((new_msr >> MSR_DR) & 1) != msr_dr) { >>>> + /* A change of the instruction relocation bit in the MSR can >>>> + * cause an implicit branch in the address space. This >>>> + * requires a tlb flush. >>>> + */ >>>> + if (env->mmu_model & POWERPC_MMU_32B) { >>>> + env->tlb_need_flush = 1; >>>> + } >>>> + } >>>> /* We don't use hreg_store_msr here as already have treated >>>> * any special case that could occur. Just store MSR and update hflags >>>> * >>>> >>>> >>> >>> Hi Cédric, >>> >>> I've just tried this patch on a selection of images here with both >>> g3beige and mac99 and as far as I can tell the crash has now gone away: >>> >>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> >> Super. I still need to sort out the exit path of hreg_store_msr(). >> >> If we have a change in IR/DR, this is a case to transform mtmsr in a >> context-synchronize instruction, for a tlb flush. This is problably >> the reason behind the POWERPC_EXCP_NONE I believe, which is then >> handled in powerpc_excp(). I need to look at that path closer. > > Forget the above as Ben just passed by :) > > I still interested by what is below : > >> And, now that I have a darwin guest, I have a few questions : >> >> 1. How do I get X running ? It's not something I really test here, but shouldn't startx work? You may need to do some configuration work if X is configured to directly use the ATI driver. >> 2. I have an old ibook G4 from which I dd'ed the disk. openbios >> complains for some invalid state. is that supported ? Yes, OpenBIOS should boot most things these days (MorphOS is the only execption I know of where the bootloader won't execute correctly as it assumes real mode). The above message means that OpenBIOS couldn't find a bootloader, or it could but was unable to execute it, e.g. due to incompatible architecture - which OS is your image running? Have you tried both mac99 and g3beige machines? ATB, Mark.
On 06/06/2016 06:17 AM, Benjamin Herrenschmidt wrote: > On Sun, 2016-06-05 at 19:41 +0200, Cédric Le Goater wrote: >> >> Here is a fix I think. Could you give it a try ? > > This is somewhat wrong... > >> commit cd0c6f473532 ('ppc: Do some batching of TCG tlb flushes') >> introduced an optimisation to flush TLBs only when a context >> synchronizing event is reached (interrupt, rfi). This was done for >> ppc64 but 32bit was forgotten on the way. > > No it didn't. That commit only delays flushes on ppc64. ppc32 is > unaffected, unless I missed something. IE. It will delay flushes caused > by slb instructions (which don't exist on 32-bit) > and ppc_tlb_invalidate_one() only in the 64-bit cases. > > Also what your patch does in practice is not really change that, though > you seem to try to somewhat extend the batching to 32-bit (but > incompletely), you also introduce something which effectively reverts > part of 9fb044911444fdd09f5f072ad0ca269d7f8b841d (split I/D mode). > > I think that's more what's "fixing" your problem, ie, the flush in > IR/DR changes. However it shouldn't be needed. OK. I thought that was needed because of what the 32b specs say in "Synchronization Requirements for Special Registers and for Lookaside Buffers", a "Context-synchronizing instruction" is required after a mtmsr({I,D}R) and also because changing IR can break implicit branching. But I might just misunderstand all of it as I am discovering. > I suspect all of that is papering over another bug somewhere else which > got exposed by the split I/D mode, since we no longer over-flush on > transitions to/from real-mode. So we must be missing flushes elsewhere, > possibly some G3 specific stuff, or there always was some kind of bug > in the TLB flushing on 32-bit that got somewhat masked by the over- > flushing we used to do. From what I see, darwin loops on tlbie : 0x000952fc: mtctr r0 0x00095300: tlbie r6 0x00095304: addi r6,r6,4096 0x00095308: bdnz+ 0x95300 0x0009530c: mtcrf 128,r11 0x00095310: sync 0x00095314: eieio 0x00095318: bns- 0x95328 and this is done on the G4, but not necessarily on the G3, it depends on r11 which contains some bits of SPRG2 : 0x0009531c: tlbsync 0x00095320: sync 0x00095324: isync HID0 is also read and written to but to control cache bits. > I need a repro-case. Booting the darwin CD is enough. Cheers, C. > Cheers, > Ben. > >> Tested on mac99 and g3beige with >> >> qemu-system-ppc -cdrom darwinppc-602.cdr -boot d >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> >> I think the hunk in powerpc_excp() is needed if we don't generate a >> context synchronizing event. what is best to do ? >> >> target-ppc/cpu.h | 2 +- >> target-ppc/excp_helper.c | 10 ++++++++++ >> target-ppc/helper_regs.h | 9 ++++++++- >> target-ppc/translate.c | 2 +- >> 4 files changed, 20 insertions(+), 3 deletions(-) >> >> Index: qemu-dgibson-for-2.7.git/target-ppc/translate.c >> =================================================================== >> --- qemu-dgibson-for-2.7.git.orig/target-ppc/translate.c >> +++ qemu-dgibson-for-2.7.git/target-ppc/translate.c >> @@ -3290,7 +3290,7 @@ static void gen_eieio(DisasContext *ctx) >> { >> } >> >> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64) >> +#if !defined(CONFIG_USER_ONLY) >> static inline void gen_check_tlb_flush(DisasContext *ctx) >> { >> TCGv_i32 t = tcg_temp_new_i32(); >> Index: qemu-dgibson-for-2.7.git/target-ppc/cpu.h >> =================================================================== >> --- qemu-dgibson-for-2.7.git.orig/target-ppc/cpu.h >> +++ qemu-dgibson-for-2.7.git/target-ppc/cpu.h >> @@ -958,9 +958,9 @@ struct CPUPPCState { >> /* PowerPC 64 SLB area */ >> ppc_slb_t slb[MAX_SLB_ENTRIES]; >> int32_t slb_nr; >> +#endif >> /* tcg TLB needs flush (deferred slb inval instruction >> typically) */ >> uint32_t tlb_need_flush; >> -#endif >> /* segment registers */ >> hwaddr htab_base; >> /* mask used to normalize hash value to PTEG index */ >> Index: qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h >> =================================================================== >> --- qemu-dgibson-for-2.7.git.orig/target-ppc/helper_regs.h >> +++ qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h >> @@ -121,6 +121,13 @@ static inline int hreg_store_msr(CPUPPCS >> } >> if (((value >> MSR_IR) & 1) != msr_ir || >> ((value >> MSR_DR) & 1) != msr_dr) { >> + /* A change of the instruction relocation bit in the MSR can >> + * cause an implicit branch in the address space. This >> + * requires a tlb flush. >> + */ >> + if (env->mmu_model & POWERPC_MMU_32B) { >> + env->tlb_need_flush = 1; >> + } >> cs->interrupt_request |= CPU_INTERRUPT_EXITTB; >> } >> if ((env->mmu_model & POWERPC_MMU_BOOKE) && >> @@ -151,7 +158,7 @@ static inline int hreg_store_msr(CPUPPCS >> return excp; >> } >> >> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64) >> +#if !defined(CONFIG_USER_ONLY) >> static inline void check_tlb_flush(CPUPPCState *env) >> { >> CPUState *cs = CPU(ppc_env_get_cpu(env)); >> Index: qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c >> =================================================================== >> --- qemu-dgibson-for-2.7.git.orig/target-ppc/excp_helper.c >> +++ qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c >> @@ -709,6 +709,16 @@ static inline void powerpc_excp(PowerPCC >> } >> } >> #endif >> + if (((new_msr >> MSR_IR) & 1) != msr_ir || >> + ((new_msr >> MSR_DR) & 1) != msr_dr) { >> + /* A change of the instruction relocation bit in the MSR can >> + * cause an implicit branch in the address space. This >> + * requires a tlb flush. >> + */ >> + if (env->mmu_model & POWERPC_MMU_32B) { >> + env->tlb_need_flush = 1; >> + } >> + } >> /* We don't use hreg_store_msr here as already have treated >> * any special case that could occur. Just store MSR and update >> hflags >> * >>
On 06/06/2016 08:38 AM, Mark Cave-Ayland wrote: > On 06/06/16 07:30, Cedric Le Goater wrote: > >> On 06/06/2016 08:27 AM, Cédric Le Goater wrote: >>> On 06/06/2016 12:26 AM, Mark Cave-Ayland wrote: >>>> On 05/06/16 18:41, Cédric Le Goater wrote: >>>> >>>>> Hello Mark, >>>>> >>>>> On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote: >>>>>> On 03/06/16 13:11, Cédric Le Goater wrote: >>>>>> >>>>>>> This is follow up to complete the serie "ppc: preparing pnv landing >>>>>>> (round 2)" plus a little fix on instruction privileges. >>>>>>> >>>>>>> Tested on a POWER8 pserie guest and on mac99. >>>>>>> >>>>>>> Benjamin Herrenschmidt (2): >>>>>>> ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV >>>>>>> ppc: Better figure out if processor has HV mode >>>>>>> >>>>>>> Cédric Le Goater (1): >>>>>>> ppc: fix hrfid, tlbia and slbia privilege >>>>>>> >>>>>>> target-ppc/cpu.h | 4 ++++ >>>>>>> target-ppc/excp_helper.c | 8 ++++++-- >>>>>>> target-ppc/helper_regs.h | 4 ++-- >>>>>>> target-ppc/translate.c | 10 ++++++---- >>>>>>> target-ppc/translate_init.c | 19 +++++++++++++++---- >>>>>>> 5 files changed, 33 insertions(+), 12 deletions(-) >>>>>> >>>>>> Hi Cédric, >>>>>> >>>>>> I can confirm that this patchset fixes starting up OpenBIOS for both >>>>>> g3beige/mac99 in my tests here. With the escc fix also applied, the only >>>>>> outstanding issue is the removal of the tlb_flush() statements which >>>>>> causes Darwin, MacOS X and HelenOS 0.60 to panic on boot >>>>>> >>>>>> My only request is if it would be possible to move patch 2 "ppc: Better >>>>>> figure out if processor has HV mode" to the front of this patchset which >>>>>> will make the remaining patches bisectable for the Mac machines. With that: >>>>>> >>>>>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>>> >>>>>> Does anyone know if Ben has any ideas as to why the MMU tlb_flush >>>>>> changes patch is causing such problems? >>>>> >>>>> >>>>> Here is a fix I think. Could you give it a try ? >>>>> >>>>> Cheers, >>>>> >>>>> C. >>>>> >>>>> From: Cédric Le Goater <clg@kaod.org> >>>>> Subject: [PATCH] ppc: fix tlb flushes for 32bit environment >>>>> Date: Sun, 05 Jun 2016 18:46:19 +0200 >>>>> MIME-Version: 1.0 >>>>> Content-Type: text/plain; charset=UTF-8 >>>>> Content-Transfer-Encoding: 8bit >>>>> >>>>> commit cd0c6f473532 ('ppc: Do some batching of TCG tlb flushes') >>>>> introduced an optimisation to flush TLBs only when a context >>>>> synchronizing event is reached (interrupt, rfi). This was done for >>>>> ppc64 but 32bit was forgotten on the way. >>>>> >>>>> Tested on mac99 and g3beige with >>>>> >>>>> qemu-system-ppc -cdrom darwinppc-602.cdr -boot d >>>>> >>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>>> --- >>>>> >>>>> I think the hunk in powerpc_excp() is needed if we don't generate a >>>>> context synchronizing event. what is best to do ? >>>>> >>>>> target-ppc/cpu.h | 2 +- >>>>> target-ppc/excp_helper.c | 10 ++++++++++ >>>>> target-ppc/helper_regs.h | 9 ++++++++- >>>>> target-ppc/translate.c | 2 +- >>>>> 4 files changed, 20 insertions(+), 3 deletions(-) >>>>> >>>>> Index: qemu-dgibson-for-2.7.git/target-ppc/translate.c >>>>> =================================================================== >>>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/translate.c >>>>> +++ qemu-dgibson-for-2.7.git/target-ppc/translate.c >>>>> @@ -3290,7 +3290,7 @@ static void gen_eieio(DisasContext *ctx) >>>>> { >>>>> } >>>>> >>>>> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64) >>>>> +#if !defined(CONFIG_USER_ONLY) >>>>> static inline void gen_check_tlb_flush(DisasContext *ctx) >>>>> { >>>>> TCGv_i32 t = tcg_temp_new_i32(); >>>>> Index: qemu-dgibson-for-2.7.git/target-ppc/cpu.h >>>>> =================================================================== >>>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/cpu.h >>>>> +++ qemu-dgibson-for-2.7.git/target-ppc/cpu.h >>>>> @@ -958,9 +958,9 @@ struct CPUPPCState { >>>>> /* PowerPC 64 SLB area */ >>>>> ppc_slb_t slb[MAX_SLB_ENTRIES]; >>>>> int32_t slb_nr; >>>>> +#endif >>>>> /* tcg TLB needs flush (deferred slb inval instruction typically) */ >>>>> uint32_t tlb_need_flush; >>>>> -#endif >>>>> /* segment registers */ >>>>> hwaddr htab_base; >>>>> /* mask used to normalize hash value to PTEG index */ >>>>> Index: qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h >>>>> =================================================================== >>>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/helper_regs.h >>>>> +++ qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h >>>>> @@ -121,6 +121,13 @@ static inline int hreg_store_msr(CPUPPCS >>>>> } >>>>> if (((value >> MSR_IR) & 1) != msr_ir || >>>>> ((value >> MSR_DR) & 1) != msr_dr) { >>>>> + /* A change of the instruction relocation bit in the MSR can >>>>> + * cause an implicit branch in the address space. This >>>>> + * requires a tlb flush. >>>>> + */ >>>>> + if (env->mmu_model & POWERPC_MMU_32B) { >>>>> + env->tlb_need_flush = 1; >>>>> + } >>>>> cs->interrupt_request |= CPU_INTERRUPT_EXITTB; >>>>> } >>>>> if ((env->mmu_model & POWERPC_MMU_BOOKE) && >>>>> @@ -151,7 +158,7 @@ static inline int hreg_store_msr(CPUPPCS >>>>> return excp; >>>>> } >>>>> >>>>> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64) >>>>> +#if !defined(CONFIG_USER_ONLY) >>>>> static inline void check_tlb_flush(CPUPPCState *env) >>>>> { >>>>> CPUState *cs = CPU(ppc_env_get_cpu(env)); >>>>> Index: qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c >>>>> =================================================================== >>>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/excp_helper.c >>>>> +++ qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c >>>>> @@ -709,6 +709,16 @@ static inline void powerpc_excp(PowerPCC >>>>> } >>>>> } >>>>> #endif >>>>> + if (((new_msr >> MSR_IR) & 1) != msr_ir || >>>>> + ((new_msr >> MSR_DR) & 1) != msr_dr) { >>>>> + /* A change of the instruction relocation bit in the MSR can >>>>> + * cause an implicit branch in the address space. This >>>>> + * requires a tlb flush. >>>>> + */ >>>>> + if (env->mmu_model & POWERPC_MMU_32B) { >>>>> + env->tlb_need_flush = 1; >>>>> + } >>>>> + } >>>>> /* We don't use hreg_store_msr here as already have treated >>>>> * any special case that could occur. Just store MSR and update hflags >>>>> * >>>>> >>>>> >>>> >>>> Hi Cédric, >>>> >>>> I've just tried this patch on a selection of images here with both >>>> g3beige and mac99 and as far as I can tell the crash has now gone away: >>>> >>>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>> >>> Super. I still need to sort out the exit path of hreg_store_msr(). >>> >>> If we have a change in IR/DR, this is a case to transform mtmsr in a >>> context-synchronize instruction, for a tlb flush. This is problably >>> the reason behind the POWERPC_EXCP_NONE I believe, which is then >>> handled in powerpc_excp(). I need to look at that path closer. >> >> Forget the above as Ben just passed by :) >> >> I still interested by what is below : >> >>> And, now that I have a darwin guest, I have a few questions : >>> >>> 1. How do I get X running ? > > It's not something I really test here, but shouldn't startx work? You > may need to do some configuration work if X is configured to directly > use the ATI driver. > >>> 2. I have an old ibook G4 from which I dd'ed the disk. openbios >>> complains for some invalid state. is that supported ? > > Yes, OpenBIOS should boot most things these days (MorphOS is the only > execption I know of where the bootloader won't execute correctly as it > assumes real mode). The above message means that OpenBIOS couldn't find > a bootloader, or it could but was unable to execute it, e.g. due to > incompatible architecture - which OS is your image running? I am pretty sure it is a Mac OS X v10.5. I still have the hardware but it is running Linux now : # cat /proc/cpuinfo processor : 0 cpu : 7447A, altivec supported clock : 1333.333000MHz revision : 1.5 (pvr 8003 0105) bogomips : 36.86 total bogomips : 36.86 timebase : 18432000 platform : PowerMac model : PowerBook6,7 machine : PowerBook6,7 motherboard : PowerBook6,7 MacRISC3 Power Macintosh detected as : 287 (iBook G4) pmac flags : 0000001a L2 cache : 512K unified pmac-generation : NewWorld Memory : 1024 MB # lsprop /proc/device-tree/rom@ff800000/boot-rom@fff00000/ reg fff00000 00100000 info fff00000 00003f00 000493f0 20050705 815fda85 fff08000 00078001 000493f0 20050705 23765c6c fff80000 00080002 000493f0 20050705 b3364dca fff03f00 00000083 000493f0 20050705 c2b72d61 fff03f80 00000084 e24a68ca 15a82001 ffffffff fff04000 00004005 6e767261 6d000000 00000000 00000000 00000000 [140 bytes total] name "boot-rom" security-modes 6e6f6e65 2c206675 6c6c2c20 636f6d6d 616e642c 206e6f2d 70617373 776f7264 image 00080000 (524288) model "Apple PowerBook6,7 4.9.3f0 BootROM built on 07/05/05 at 11:14:11" write-characteristic "flash" hwi-flags 402a1220 (1076498976) BootROM-version "$0004.93f0" BootROM-build-date "07/05/05 at 11:14:11" linux,phandle ff89cb08 has-config-block > Have you tried both mac99 and g3beige machines? yes. C.
On 07/06/16 08:04, Cédric Le Goater wrote: >>>> 2. I have an old ibook G4 from which I dd'ed the disk. openbios >>>> complains for some invalid state. is that supported ? >> >> Yes, OpenBIOS should boot most things these days (MorphOS is the only >> execption I know of where the bootloader won't execute correctly as it >> assumes real mode). The above message means that OpenBIOS couldn't find >> a bootloader, or it could but was unable to execute it, e.g. due to >> incompatible architecture - which OS is your image running? > > I am pretty sure it is a Mac OS X v10.5. I still have the hardware but it > is running Linux now : > > # cat /proc/cpuinfo > processor : 0 > cpu : 7447A, altivec supported > clock : 1333.333000MHz > revision : 1.5 (pvr 8003 0105) > bogomips : 36.86 > > total bogomips : 36.86 > timebase : 18432000 > platform : PowerMac > model : PowerBook6,7 > machine : PowerBook6,7 > motherboard : PowerBook6,7 MacRISC3 Power Macintosh > detected as : 287 (iBook G4) > pmac flags : 0000001a > L2 cache : 512K unified > pmac-generation : NewWorld > Memory : 1024 MB > > # lsprop /proc/device-tree/rom@ff800000/boot-rom@fff00000/ > reg fff00000 00100000 > info fff00000 00003f00 000493f0 20050705 > 815fda85 fff08000 00078001 000493f0 > 20050705 23765c6c fff80000 00080002 > 000493f0 20050705 b3364dca fff03f00 > 00000083 000493f0 20050705 c2b72d61 > fff03f80 00000084 e24a68ca 15a82001 > ffffffff fff04000 00004005 6e767261 > 6d000000 00000000 00000000 00000000 > [140 bytes total] > name "boot-rom" > security-modes 6e6f6e65 2c206675 6c6c2c20 636f6d6d > 616e642c 206e6f2d 70617373 776f7264 > image 00080000 (524288) > model "Apple PowerBook6,7 4.9.3f0 BootROM built on 07/05/05 at 11:14:11" > write-characteristic > "flash" > hwi-flags 402a1220 (1076498976) > BootROM-version "$0004.93f0" > BootROM-build-date > "07/05/05 at 11:14:11" > linux,phandle ff89cb08 > has-config-block > > >> Have you tried both mac99 and g3beige machines? > > yes. (I'm wondering if we should start a new thread here either just on qemu-ppc or over on the OpenBIOS list) OpenBIOS will read hfs/hfsplus filesystems fine - I wonder if maybe it can't locate a suitable partition in the Apple Partition Map? If you boot to the Forth prompt with -prom-env 'auto-boot?=false' can you try the following to see if you can list the disk contents: dir hd:,\ If that doesn't work it means that the partition auto-detection is failing, so try manually forcing the partition number until you find one that works e.g. dir hd:0,\ dir hd:1,\ etc. up until around partition 10? Once you find one that works it should be possible to boot that partition directly e.g. boot hd:1 ATB, Mark.
Index: qemu-dgibson-for-2.7.git/target-ppc/translate.c =================================================================== --- qemu-dgibson-for-2.7.git.orig/target-ppc/translate.c +++ qemu-dgibson-for-2.7.git/target-ppc/translate.c @@ -3290,7 +3290,7 @@ static void gen_eieio(DisasContext *ctx) { } -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64) +#if !defined(CONFIG_USER_ONLY) static inline void gen_check_tlb_flush(DisasContext *ctx) { TCGv_i32 t = tcg_temp_new_i32(); Index: qemu-dgibson-for-2.7.git/target-ppc/cpu.h =================================================================== --- qemu-dgibson-for-2.7.git.orig/target-ppc/cpu.h +++ qemu-dgibson-for-2.7.git/target-ppc/cpu.h @@ -958,9 +958,9 @@ struct CPUPPCState { /* PowerPC 64 SLB area */ ppc_slb_t slb[MAX_SLB_ENTRIES]; int32_t slb_nr; +#endif /* tcg TLB needs flush (deferred slb inval instruction typically) */ uint32_t tlb_need_flush; -#endif /* segment registers */ hwaddr htab_base; /* mask used to normalize hash value to PTEG index */ Index: qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h =================================================================== --- qemu-dgibson-for-2.7.git.orig/target-ppc/helper_regs.h +++ qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h @@ -121,6 +121,13 @@ static inline int hreg_store_msr(CPUPPCS } if (((value >> MSR_IR) & 1) != msr_ir || ((value >> MSR_DR) & 1) != msr_dr) { + /* A change of the instruction relocation bit in the MSR can + * cause an implicit branch in the address space. This + * requires a tlb flush. + */ + if (env->mmu_model & POWERPC_MMU_32B) { + env->tlb_need_flush = 1; + } cs->interrupt_request |= CPU_INTERRUPT_EXITTB; } if ((env->mmu_model & POWERPC_MMU_BOOKE) && @@ -151,7 +158,7 @@ static inline int hreg_store_msr(CPUPPCS return excp; } -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64) +#if !defined(CONFIG_USER_ONLY) static inline void check_tlb_flush(CPUPPCState *env) { CPUState *cs = CPU(ppc_env_get_cpu(env)); Index: qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c =================================================================== --- qemu-dgibson-for-2.7.git.orig/target-ppc/excp_helper.c +++ qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c @@ -709,6 +709,16 @@ static inline void powerpc_excp(PowerPCC } } #endif + if (((new_msr >> MSR_IR) & 1) != msr_ir || + ((new_msr >> MSR_DR) & 1) != msr_dr) { + /* A change of the instruction relocation bit in the MSR can + * cause an implicit branch in the address space. This + * requires a tlb flush. + */ + if (env->mmu_model & POWERPC_MMU_32B) { + env->tlb_need_flush = 1; + } + } /* We don't use hreg_store_msr here as already have treated * any special case that could occur. Just store MSR and update hflags *