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

[Cédric Le Goater]

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 <address@hidden>
>>>>>>
>>>>>> 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 <address@hidden>
>>>>> 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 <address@hidden>
>>>>> ---
>>>>>
>>>>>  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 <address@hidden>
>>>
>>> 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/address@hidden/address@hidden/
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.