SH: Fix TLB/MMU detection of code accesses.

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.

target-sh4/helper.c +0 -0

@@ -406,11 +404,11 @@ static int get_mmu_address(CPUState * env, target_ulong * physical,
 	    case 3:		/* 011 */
 	    case 6:		/* 110 */
 	    case 7:		/* 111 */

Quoted 10 lines · Vladimir Prus

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.

Quoted 2 lines

This patch fixes this, by stopping cpu_sh4_handle_mmu_fault from erasing the data read/code read distinction.

I found that this patch still can be applied to the trunk HEAD, rev 5639, and it really stabilizes SH-Linux system emulation : some segmentation fault disappears. Thanks.

Quoted 4 lines

target-sh4/helper.c +0 -0

@@ -406,11 +404,11 @@ static int get_mmu_address(CPUState * env, target_ulong * physical,
 	    case 3:		/* 011 */
 	    case 6:		/* 110 */
 	    case 7:		/* 111 */

target-sh4/helper.c static int get_mmu_address(CPUState * env, target_ulong * physical, · line 414

Quoted 10 lines

target-sh4/helper.c +2 -2

-		*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;
 	}
     }

I think one more replace needed in get_mmu_address(), like following. Isn't it?

     if (n >= 0) {

(quoted diff) +1 -1

 	*physical = ((matching->ppn << 10) & ~(matching->size - 1)) |
 	    (address & (matching->size - 1));
-	if ((rw & PAGE_WRITE) & !matching->d)
+	if ((rw == 1) & !matching->d)
 	    n = MMU_DTLB_INITIAL_WRITE;
 	else
 	    n = MMU_OK;

target-sh4/helper.c int get_physical_address(CPUState * env, target_ulong * physical, · line 442

Quoted 12 lines

target-sh4/helper.c +6 -2

@@ -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;

To be more precise, these cases should not invoke TLB miss error exceptions but address error exceptions, whose exception codes are 0x0e0, or 0x100, I guess. It might be another small patch.

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.

- 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(-)

target-sh4/helper.c +14 -33

index f523f81..b1b02dd 100644
@@ -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;
 }
 

Signature · 1 line

1.5.3.5