diff mbox

[0/3] ppc: complete the new HV mode

Message ID 5754645E.8080504@kaod.org (mailing list archive)
State New, archived
Headers show

Commit Message

Cédric Le Goater June 5, 2016, 5:41 p.m. UTC
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(-)

Comments

Mark Cave-Ayland June 5, 2016, 10:26 p.m. UTC | #1
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.
David Gibson June 6, 2016, 1:47 a.m. UTC | #2
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.
Benjamin Herrenschmidt June 6, 2016, 4:17 a.m. UTC | #3
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
>       *
>
Cédric Le Goater June 6, 2016, 6:27 a.m. UTC | #4
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.
>
Cédric Le Goater June 6, 2016, 6:30 a.m. UTC | #5
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.
>>
>
Mark Cave-Ayland June 6, 2016, 6:38 a.m. UTC | #6
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.
Cédric Le Goater June 6, 2016, 7:28 a.m. UTC | #7
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
>>       *
>>
Cédric Le Goater June 7, 2016, 7:04 a.m. UTC | #8
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.
Mark Cave-Ayland June 7, 2016, 8:24 a.m. UTC | #9
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.
diff mbox

Patch

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
      *