From patchwork Thu Jun 2 05:32:29 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Cave-Ayland X-Patchwork-Id: 628962 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3rKwrh6m4Dz9t4Z for ; Thu, 2 Jun 2016 15:34:12 +1000 (AEST) Received: from localhost ([::1]:45287 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b8LGk-0001kz-Kg for incoming@patchwork.ozlabs.org; Thu, 02 Jun 2016 01:34:10 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34696) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b8LG2-0001Ns-SS for qemu-devel@nongnu.org; Thu, 02 Jun 2016 01:33:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b8LFz-0005VB-Gp for qemu-devel@nongnu.org; Thu, 02 Jun 2016 01:33:26 -0400 Received: from chuckie.co.uk ([82.165.15.123]:51262 helo=s16892447.onlinehome-server.info) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b8LFz-0005Ss-5p; Thu, 02 Jun 2016 01:33:23 -0400 Received: from host31-50-169-25.range31-50.btcentralplus.com ([31.50.169.25] helo=[192.168.1.65]) by s16892447.onlinehome-server.info with esmtpsa (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from ) id 1b8LFo-0004zh-Em; Thu, 02 Jun 2016 06:33:15 +0100 To: David Gibson References: <1464655277-14748-1-git-send-email-david@gibson.dropbear.id.au> <1464655277-14748-3-git-send-email-david@gibson.dropbear.id.au> <574F388A.3020201@ilande.co.uk> <20160602031509.GI15455@voom.fritz.box> From: Mark Cave-Ayland Message-ID: <574FC4ED.4080605@ilande.co.uk> Date: Thu, 2 Jun 2016 06:32:29 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.8.0 MIME-Version: 1.0 In-Reply-To: <20160602031509.GI15455@voom.fritz.box> X-SA-Exim-Connect-IP: 31.50.169.25 X-SA-Exim-Mail-From: mark.cave-ayland@ilande.co.uk X-SA-Exim-Version: 4.2.1 (built Sun, 08 Jan 2012 02:45:44 +0000) X-SA-Exim-Scanned: Yes (on s16892447.onlinehome-server.info) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 82.165.15.123 Subject: Re: [Qemu-devel] [Qemu-ppc] [PULL 02/12] ppc: Use split I/D mmu modes to avoid flushes on interrupts X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: peter.maydell@linaro.org, bharata.rao@gmail.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org, pbonzini@redhat.com Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" On 02/06/16 04:15, David Gibson wrote: > On Wed, Jun 01, 2016 at 08:33:30PM +0100, Mark Cave-Ayland wrote: >> On 31/05/16 01:41, David Gibson wrote: >> >>> From: Benjamin Herrenschmidt >>> >>> We rework the way the MMU indices are calculated, providing separate >>> indices for I and D side based on MSR:IR and MSR:DR respectively, >>> and thus no longer need to flush the TLB on context changes. This also >>> adds correct support for HV as a separate address space. >>> >>> Signed-off-by: Benjamin Herrenschmidt >>> Signed-off-by: David Gibson >>> --- >>> target-ppc/cpu.h | 11 +++++++--- >>> target-ppc/excp_helper.c | 11 ---------- >>> target-ppc/helper_regs.h | 54 +++++++++++++++++++++++++++++++++++++++++------- >>> target-ppc/machine.c | 5 ++++- >>> target-ppc/translate.c | 7 ++++--- >>> 5 files changed, 63 insertions(+), 25 deletions(-) >>> >>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h >>> index 02e71ea..2c8c8c0 100644 >>> --- a/target-ppc/cpu.h >>> +++ b/target-ppc/cpu.h >>> @@ -359,6 +359,8 @@ struct ppc_slb_t { >>> #define MSR_EP 6 /* Exception prefix on 601 */ >>> #define MSR_IR 5 /* Instruction relocate */ >>> #define MSR_DR 4 /* Data relocate */ >>> +#define MSR_IS 5 /* Instruction address space (BookE) */ >>> +#define MSR_DS 4 /* Data address space (BookE) */ >>> #define MSR_PE 3 /* Protection enable on 403 */ >>> #define MSR_PX 2 /* Protection exclusive on 403 x */ >>> #define MSR_PMM 2 /* Performance monitor mark on POWER x */ >>> @@ -410,6 +412,8 @@ struct ppc_slb_t { >>> #define msr_ep ((env->msr >> MSR_EP) & 1) >>> #define msr_ir ((env->msr >> MSR_IR) & 1) >>> #define msr_dr ((env->msr >> MSR_DR) & 1) >>> +#define msr_is ((env->msr >> MSR_IS) & 1) >>> +#define msr_ds ((env->msr >> MSR_DS) & 1) >>> #define msr_pe ((env->msr >> MSR_PE) & 1) >>> #define msr_px ((env->msr >> MSR_PX) & 1) >>> #define msr_pmm ((env->msr >> MSR_PMM) & 1) >>> @@ -889,7 +893,7 @@ struct ppc_segment_page_sizes { >>> >>> /*****************************************************************************/ >>> /* The whole PowerPC CPU context */ >>> -#define NB_MMU_MODES 3 >>> +#define NB_MMU_MODES 8 >>> >>> #define PPC_CPU_OPCODES_LEN 0x40 >>> #define PPC_CPU_INDIRECT_OPCODES_LEN 0x20 >>> @@ -1053,7 +1057,8 @@ struct CPUPPCState { >>> /* Those resources are used only in QEMU core */ >>> target_ulong hflags; /* hflags is a MSR & HFLAGS_MASK */ >>> target_ulong hflags_nmsr; /* specific hflags, not coming from MSR */ >>> - int mmu_idx; /* precomputed MMU index to speed up mem accesses */ >>> + int immu_idx; /* precomputed MMU index to speed up insn access */ >>> + int dmmu_idx; /* precomputed MMU index to speed up data accesses */ >>> >>> /* Power management */ >>> int (*check_pow)(CPUPPCState *env); >>> @@ -1245,7 +1250,7 @@ int ppc_dcr_write (ppc_dcr_t *dcr_env, int dcrn, uint32_t val); >>> #define MMU_USER_IDX 0 >>> static inline int cpu_mmu_index (CPUPPCState *env, bool ifetch) >>> { >>> - return env->mmu_idx; >>> + return ifetch ? env->immu_idx : env->dmmu_idx; >>> } >>> >>> #include "exec/cpu-all.h" >>> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c >>> index 288903e..ba3caec 100644 >>> --- a/target-ppc/excp_helper.c >>> +++ b/target-ppc/excp_helper.c >>> @@ -646,9 +646,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >>> >>> if (env->spr[SPR_LPCR] & LPCR_AIL) { >>> new_msr |= (1 << MSR_IR) | (1 << MSR_DR); >>> - } else if (msr & ((1 << MSR_IR) | (1 << MSR_DR))) { >>> - /* If we disactivated any translation, flush TLBs */ >>> - tlb_flush(cs, 1); >>> } >>> >>> #ifdef TARGET_PPC64 >>> @@ -721,14 +718,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >>> /* Reset exception state */ >>> cs->exception_index = POWERPC_EXCP_NONE; >>> env->error_code = 0; >>> - >>> - if ((env->mmu_model == POWERPC_MMU_BOOKE) || >>> - (env->mmu_model == POWERPC_MMU_BOOKE206)) { >>> - /* XXX: The BookE changes address space when switching modes, >>> - we should probably implement that as different MMU indexes, >>> - but for the moment we do it the slow way and flush all. */ >>> - tlb_flush(cs, 1); >>> - } >>> } >>> >>> void ppc_cpu_do_interrupt(CPUState *cs) >>> diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h >>> index 271fddf..f7edd5b 100644 >>> --- a/target-ppc/helper_regs.h >>> +++ b/target-ppc/helper_regs.h >>> @@ -41,11 +41,50 @@ static inline void hreg_swap_gpr_tgpr(CPUPPCState *env) >>> >>> static inline void hreg_compute_mem_idx(CPUPPCState *env) >>> { >>> - /* Precompute MMU index */ >>> - if (msr_pr == 0 && msr_hv != 0) { >>> - env->mmu_idx = 2; >>> + /* This is our encoding for server processors >>> + * >>> + * 0 = Guest User space virtual mode >>> + * 1 = Guest Kernel space virtual mode >>> + * 2 = Guest Kernel space real mode >>> + * 3 = HV User space virtual mode >>> + * 4 = HV Kernel space virtual mode >>> + * 5 = HV Kernel space real mode >>> + * >>> + * The combination PR=1 IR&DR=0 is invalid, we will treat >>> + * it as IR=DR=1 >>> + * >>> + * For BookE, we need 8 MMU modes as follow: >>> + * >>> + * 0 = AS 0 HV User space >>> + * 1 = AS 0 HV Kernel space >>> + * 2 = AS 1 HV User space >>> + * 3 = AS 1 HV Kernel space >>> + * 4 = AS 0 Guest User space >>> + * 5 = AS 0 Guest Kernel space >>> + * 6 = AS 1 Guest User space >>> + * 7 = AS 1 Guest Kernel space >>> + */ >>> + if (env->mmu_model & POWERPC_MMU_BOOKE) { >>> + env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1; >>> + env->immu_idx += msr_is ? 2 : 0; >>> + env->dmmu_idx += msr_ds ? 2 : 0; >>> + env->immu_idx += msr_gs ? 4 : 0; >>> + env->dmmu_idx += msr_gs ? 4 : 0; >>> } else { >>> - env->mmu_idx = 1 - msr_pr; >>> + /* First calucalte a base value independent of HV */ >>> + if (msr_pr != 0) { >>> + /* User space, ignore IR and DR */ >>> + env->immu_idx = env->dmmu_idx = 0; >>> + } else { >>> + /* Kernel, setup a base I/D value */ >>> + env->immu_idx = msr_ir ? 1 : 2; >>> + env->dmmu_idx = msr_dr ? 1 : 2; >>> + } >>> + /* Then offset it for HV */ >>> + if (msr_hv) { >>> + env->immu_idx += 3; >>> + env->dmmu_idx += 3; >>> + } >>> } >>> } >>> >>> @@ -82,9 +121,10 @@ static inline int hreg_store_msr(CPUPPCState *env, target_ulong value, >>> } >>> if (((value >> MSR_IR) & 1) != msr_ir || >>> ((value >> MSR_DR) & 1) != msr_dr) { >>> - /* Flush all tlb when changing translation mode */ >>> - tlb_flush(cs, 1); >>> - excp = POWERPC_EXCP_NONE; >>> + cs->interrupt_request |= CPU_INTERRUPT_EXITTB; >>> + } >>> + if ((env->mmu_model & POWERPC_MMU_BOOKE) && >>> + ((value >> MSR_GS) & 1) != msr_gs) { >>> cs->interrupt_request |= CPU_INTERRUPT_EXITTB; >>> } >>> if (unlikely((env->flags & POWERPC_FLAG_TGPR) && >>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c >>> index f6c7256..4820f22 100644 >>> --- a/target-ppc/machine.c >>> +++ b/target-ppc/machine.c >>> @@ -97,9 +97,12 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int version_id) >>> qemu_get_betls(f, &env->nip); >>> qemu_get_betls(f, &env->hflags); >>> qemu_get_betls(f, &env->hflags_nmsr); >>> - qemu_get_sbe32s(f, &env->mmu_idx); >>> + qemu_get_sbe32(f); /* Discard unused mmu_idx */ >>> qemu_get_sbe32(f); /* Discard unused power_mode */ >>> >>> + /* Recompute mmu indices */ >>> + hreg_compute_mem_idx(env); >>> + >>> return 0; >>> } >>> >>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c >>> index f5ceae5..b757634 100644 >>> --- a/target-ppc/translate.c >>> +++ b/target-ppc/translate.c >>> @@ -11220,8 +11220,9 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf, >>> env->nip, env->lr, env->ctr, cpu_read_xer(env), >>> cs->cpu_index); >>> cpu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx " HF " >>> - TARGET_FMT_lx " idx %d\n", env->msr, env->spr[SPR_HID0], >>> - env->hflags, env->mmu_idx); >>> + TARGET_FMT_lx " iidx %d didx %d\n", >>> + env->msr, env->spr[SPR_HID0], >>> + env->hflags, env->immu_idx, env->dmmu_idx); >>> #if !defined(NO_TIMER_DUMP) >>> cpu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64 >>> #if !defined(CONFIG_USER_ONLY) >>> @@ -11428,7 +11429,7 @@ void gen_intermediate_code(CPUPPCState *env, struct TranslationBlock *tb) >>> ctx.spr_cb = env->spr_cb; >>> ctx.pr = msr_pr; >>> ctx.hv = !msr_pr && msr_hv; >>> - ctx.mem_idx = env->mmu_idx; >>> + ctx.mem_idx = env->dmmu_idx; >>> ctx.insns_flags = env->insns_flags; >>> ctx.insns_flags2 = env->insns_flags2; >>> ctx.access_type = -1; >> >> Apologies to have to report another regression, however this patch >> causes a kernel panic in Darwin PPC under TCG: >> >> ./qemu-system-ppc -cdrom darwinppc-602.iso -boot d >> >> The panic backtrace on-screen points towards one of the SCSI modules, so >> perhaps this patch doesn't handle DMA correctly when trying to access >> the CDROM? > > Bother. It's a lot less obvious to me how this one would cause > trouble. Possibly a bug in the CD-ROM device which was masked by the > pervious mem access stuff. Some more testing shows that HelenOS also fails with this patch applied. I'm not sure exactly how helpful this is, however the minimal diff below in conjunction with a revert of tlbia patch is enough to allow both Darwin and HelenOS to boot once again (note that I've tried each flush individually, and both are required): if ((env->mmu_model & POWERPC_MMU_BOOKE) && ATB, Mark. diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c index a37009e..6b81a09 100644 --- a/target-ppc/excp_helper.c +++ b/target-ppc/excp_helper.c @@ -646,6 +646,9 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) if (env->spr[SPR_LPCR] & LPCR_AIL) { new_msr |= (1 << MSR_IR) | (1 << MSR_DR); + } else if (msr & ((1 << MSR_IR) | (1 << MSR_DR))) { + /* If we disactivated any translation, flush TLBs */ + tlb_flush(cs, 1); } #ifdef TARGET_PPC64 diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h index 57da931..8437211 100644 --- a/target-ppc/helper_regs.h +++ b/target-ppc/helper_regs.h @@ -121,6 +121,9 @@ static inline int hreg_store_msr(CPUPPCState *env, target_ulong value, } if (((value >> MSR_IR) & 1) != msr_ir || ((value >> MSR_DR) & 1) != msr_dr) { + /* Flush all tlb when changing translation mode */ + tlb_flush(cs, 1); + excp = POWERPC_EXCP_NONE; cs->interrupt_request |= CPU_INTERRUPT_EXITTB; }