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

[Aurelien Jarno <aurelien@aurel32.net>]

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