[Qemu-devel] [PATCH] SH: Fix TLB/MMU detection of code accesses.

[Qemu-devel] [PATCH] SH: Fix TLB/MMU detection of code accesses.

[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.

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

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

Re: [Qemu-devel] [PATCH] SH: Fix TLB/MMU detection of code accesses.

[Shin-ichiro KAWASAKI]

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 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.


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

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

     if (n >= 0) {
 	*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;


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


Regards,
Shin-ichiro KAWASAKI

Re: [Qemu-devel] [PATCH] SH: Fix TLB/MMU detection of code accesses.

[Aurelien Jarno]

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

Re: [Qemu-devel] [PATCH] SH: Fix TLB/MMU detection of code accesses.

[Aurelien Jarno]

On Fri, Nov 07, 2008 at 12:29:36AM +0900, Shin-ichiro KAWASAKI wrote:
> 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 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.
> 
> 
> > @@ -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;
> >  	}
> >      }
> 
> I think one more replace needed in get_mmu_address(), like following.
> Isn't it?
> 
>      if (n >= 0) {
>  	*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;
> 
> 
> > @@ -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.

Yes, this is what written in the SH manual. I have fixed that in the
patch I have just committed.

 

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net