From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1L3eZe-000447-Uz for qemu-devel@nongnu.org; Fri, 21 Nov 2008 17:34:03 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1L3eZe-00043l-H9 for qemu-devel@nongnu.org; Fri, 21 Nov 2008 17:34:02 -0500 Received: from [199.232.76.173] (port=35927 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1L3eZe-00043h-4V for qemu-devel@nongnu.org; Fri, 21 Nov 2008 17:34:02 -0500 Received: from hall.aurel32.net ([88.191.82.174]:41122) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1L3eZd-0001KA-Jz for qemu-devel@nongnu.org; Fri, 21 Nov 2008 17:34:02 -0500 Received: from volta-wlan.aurel32.net ([2002:52e8:2fb:ffff:21d:e0ff:fe49:1047] helo=volta.aurel32.net) by hall.aurel32.net with esmtpsa (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.63) (envelope-from ) id 1L3eZb-0002zR-Us for qemu-devel@nongnu.org; Fri, 21 Nov 2008 23:34:00 +0100 Received: from aurel32 by volta.aurel32.net with local (Exim 4.69) (envelope-from ) id 1L3eZb-0001MS-EC for qemu-devel@nongnu.org; Fri, 21 Nov 2008 23:33:59 +0100 Date: Fri, 21 Nov 2008 23:33:59 +0100 From: Aurelien Jarno Subject: Re: [Qemu-devel] [PATCH] SH: Fix TLB/MMU detection of code accesses. Message-ID: <20081121223359.GA4884@volta.aurel32.net> References: <200810240004.41414.vladimir@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <200810240004.41414.vladimir@codesourcery.com> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org On Fri, Oct 24, 2008 at 12:04:41AM +0400, Vladimir Prus wrote: > > Current SH4 TLB emulation does strange thing about code accesses. For > code accesses, tlb_fill will have 2 passed for is_write parameter. > In SH case, tlb_fill calls cpu_sh4_handle_mmu_fault, which treats > data read and code read identically -- that is, the same value is > passed for the 'rw' parameter for get_physical_address. The latter > function then calls get_mmu_address -- which tries to figure if we're > doing code address or not -- by comparing env->pc with the address > being accessed. The code comment say "Hack", and in fact this sometimes > gets wrong results, which causes random crashes in the simulated program. > > This patch fixes this, by stopping cpu_sh4_handle_mmu_fault from > erasing the data read/code read distinction. I have applied a slightly different patch, using comments from Shin-ichiro KAWASAKI. Thanks. > - Volodya > > * target-sh4/helper.c (get_mmu_address): Don't use > hacks to determine if access is to code. Just check 'rw'. > (get_physical_address): Adjust to the fact that 'rw' can have > 3 values. > (cpu_sh4_handle_mmu_fault): Don't treat code and data reads the > same. > --- > target-sh4/helper.c | 47 ++++++++++++++--------------------------------- > 1 files changed, 14 insertions(+), 33 deletions(-) > > diff --git a/target-sh4/helper.c b/target-sh4/helper.c > index f523f81..b1b02dd 100644 > --- a/target-sh4/helper.c > +++ b/target-sh4/helper.c > @@ -369,14 +369,12 @@ static int get_mmu_address(CPUState * env, target_ulong * physical, > int *prot, target_ulong address, > int rw, int access_type) > { > - int use_asid, is_code, n; > + int use_asid, n; > tlb_t *matching = NULL; > > - use_asid = (env->mmucr & MMUCR_SV) == 0 || (env->sr & SR_MD) == 0; > - is_code = env->pc == address; /* Hack */ > + use_asid = (env->mmucr & MMUCR_SV) == 0 || (env->sr & SR_MD) == 0; > > - /* Use a hack to find if this is an instruction or data access */ > - if (env->pc == address && !(rw & PAGE_WRITE)) { > + if (rw == 2) { > n = find_itlb_entry(env, address, use_asid, 1); > if (n >= 0) { > matching = &env->itlb[n]; > @@ -392,13 +390,13 @@ static int get_mmu_address(CPUState * env, target_ulong * physical, > switch ((matching->pr << 1) | ((env->sr & SR_MD) ? 1 : 0)) { > case 0: /* 000 */ > case 2: /* 010 */ > - n = (rw & PAGE_WRITE) ? MMU_DTLB_VIOLATION_WRITE : > + n = (rw == 1) ? MMU_DTLB_VIOLATION_WRITE : > MMU_DTLB_VIOLATION_READ; > break; > case 1: /* 001 */ > case 4: /* 100 */ > case 5: /* 101 */ > - if (rw & PAGE_WRITE) > + if (rw == 1) > n = MMU_DTLB_VIOLATION_WRITE; > else > *prot = PAGE_READ; > @@ -406,11 +404,11 @@ static int get_mmu_address(CPUState * env, target_ulong * physical, > case 3: /* 011 */ > case 6: /* 110 */ > case 7: /* 111 */ > - *prot = rw & (PAGE_READ | PAGE_WRITE); > + *prot = (rw == 1)? PAGE_WRITE : PAGE_READ; > break; > } > } else if (n == MMU_DTLB_MISS) { > - n = (rw & PAGE_WRITE) ? MMU_DTLB_MISS_WRITE : > + n = (rw == 1) ? MMU_DTLB_MISS_WRITE : > MMU_DTLB_MISS_READ; > } > } > @@ -436,8 +434,12 @@ int get_physical_address(CPUState * env, target_ulong * physical, > && (address < 0xe0000000 || address > 0xe4000000)) { > /* Unauthorized access in user mode (only store queues are available) */ > fprintf(stderr, "Unauthorized access\n"); > - return (rw & PAGE_WRITE) ? MMU_DTLB_MISS_WRITE : > - MMU_DTLB_MISS_READ; > + if (rw == 0) > + return MMU_DTLB_MISS_READ; > + else if (rw == 1) > + return MMU_DTLB_MISS_WRITE; > + else > + return MMU_ITLB_MISS; > } > if (address >= 0x80000000 && address < 0xc0000000) { > /* Mask upper 3 bits for P1 and P2 areas */ > @@ -475,27 +477,6 @@ int cpu_sh4_handle_mmu_fault(CPUState * env, target_ulong address, int rw, > target_ulong physical, page_offset, page_size; > int prot, ret, access_type; > > - switch (rw) { > - case 0: > - rw = PAGE_READ; > - break; > - case 1: > - rw = PAGE_WRITE; > - break; > - case 2: /* READ_ACCESS_TYPE == 2 defined in softmmu_template.h */ > - rw = PAGE_READ; > - break; > - default: > - /* fatal error */ > - assert(0); > - } > - > - /* XXXXX */ > -#if 0 > - fprintf(stderr, "%s pc %08x ad %08x rw %d mmu_idx %d smmu %d\n", > - __func__, env->pc, address, rw, mmu_idx, is_softmmu); > -#endif > - > access_type = ACCESS_INT; > ret = > get_physical_address(env, &physical, &prot, address, rw, > @@ -547,7 +528,7 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState * env, target_ulong addr) > target_ulong physical; > int prot; > > - get_physical_address(env, &physical, &prot, addr, PAGE_READ, 0); > + get_physical_address(env, &physical, &prot, addr, 0, 0); > return physical; > } > > -- > 1.5.3.5 > > > > -- .''`. Aurelien Jarno | GPG: 1024D/F1BCDB73 : :' : Debian developer | Electrical Engineer `. `' aurel32@debian.org | aurelien@aurel32.net `- people.debian.org/~aurel32 | www.aurel32.net