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