Re: [Qemu-devel] [PATCH 2/6] target/m68k: add MC68040 MMU

[Richard Henderson]

On 01/08/2018 03:10 PM, Laurent Vivier wrote:
> +static int get_physical_address(CPUM68KState *env, hwaddr *physical,
> +                                int *prot, target_ulong address,
> +                                int access_type, target_ulong *page_size)
> +{
> +    M68kCPU *cpu = m68k_env_get_cpu(env);
> +    CPUState *cs = CPU(cpu);
> +    uint32_t page_offset;
> +    uint32_t entry;
> +    uint32_t next;
> +
> +    /* Page Table Root Pointer */
> +    *prot = PAGE_READ | PAGE_WRITE;
> +    if (access_type & ACCESS_CODE) {
> +        *prot |= PAGE_EXEC;
> +    }
> +    if (access_type & ACCESS_SUPER) {
> +        next = env->mmu.srp;
> +    } else {
> +        next = env->mmu.urp;
> +    }
> +
> +    /* Root Index */
> +    entry = M68K_POINTER_BASE(next) | M68K_ROOT_INDEX(address);
> +
> +    next = ldl_phys(cs->as, entry);
> +    if (!M68K_UDT_VALID(next)) {
> +        return -1;
> +    }
> +    if (!(next & M68K_DESC_USED)) {
> +        stl_phys(cs->as, entry, next | M68K_DESC_USED);
> +    }

You may want to add ACCESS_DEBUG or some such so that probes from gdb and the
monitor interface do not alter the cpu state.

> +    if (env->mmu.tcr & M68K_TCR_PAGE_8K) {
> +        *page_size = 8192;
> +        page_offset = address & 0x1fff;
> +        *physical = (next & ~0x1fff) + page_offset;
> +    } else {
> +        *page_size = 4096;
> +        page_offset = address & 0x0fff;
> +        *physical = (next & ~0x0fff) + page_offset;
> +    }

So...

> +    if (ret == 0) {
> +        tlb_set_page(cs, address & TARGET_PAGE_MASK,
> +                     physical & TARGET_PAGE_MASK,
> +                     prot, mmu_idx, page_size);

... this is going to go through the tlb_add_large_page path every time, since
both 4K and 8K are larger than the default 1K page size.

Using the large page path by default means that any single-page tlb flush will
quickly devolve to flushing the entire tlb.

Also, using page_size and TARGET_PAGE_MASK looks wrong.  I think you would have
needed address & -page_size.

That said, you may want to compare the performance of passing page_size vs
TARGET_PAGE_SIZE to tlb_set_page.

> +++ b/target/m68k/op_helper.c
> @@ -360,7 +360,50 @@ static void m68k_interrupt_all(CPUM68KState *env, int 
> is_hw)
>      sp = env->aregs[7];
>  
>      sp &= ~1;
> -    if (cs->exception_index == EXCP_ADDRESS) {
> +    if (cs->exception_index == EXCP_ACCESS) {
> +        static int mmu_fault;
> +        if (mmu_fault) {
> +            cpu_abort(cs, "DOUBLE MMU FAULT\n");
> +        }
> +        mmu_fault = 1;

This variable shouldn't be static -- it should be in CPUM68KState or M68kCPU.


r~