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-ayl...@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 <c...@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 <c...@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-ayl...@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.
>>
>
