Re: [Qemu-devel] [PATCH 0/3] ppc: complete the new HV mode

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

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.