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
 `. `'   address@hidden         | address@hidden
   `-    people.debian.org/~aurel32 | www.aurel32.net