Re: [Qemu-devel] [PATCH v2 20/27] target/sparc: Convert to CPUClass::tlb_fill

[Peter Maydell]

On Thu, 9 May 2019 at 07:17, Richard Henderson
<address@hidden> wrote:
>
> Cc: Artyom Tarasenko <address@hidden>
> Cc: Mark Cave-Ayland <address@hidden>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
> v2: Keep user-only, sparc32, and sparc64 tlb_fill separate.
> ---
>  target/sparc/cpu.h         |  5 ++-
>  target/sparc/cpu.c         |  5 +--
>  target/sparc/ldst_helper.c | 11 +-----
>  target/sparc/mmu_helper.c  | 78 ++++++++++++++++++++++----------------
>  4 files changed, 51 insertions(+), 48 deletions(-)

>  /* Perform address translation */
> -int sparc_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw,
> -                               int mmu_idx)
> +bool sparc_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> +                        MMUAccessType access_type, int mmu_idx,
> +                        bool probe, uintptr_t retaddr)
>  {
>      SPARCCPU *cpu = SPARC_CPU(cs);
>      CPUSPARCState *env = &cpu->env;
> @@ -220,22 +222,18 @@ int sparc_cpu_handle_mmu_fault(CPUState *cs, vaddr 
> address, int size, int rw,
>
>      address &= TARGET_PAGE_MASK;
>      error_code = get_physical_address(env, &paddr, &prot, &access_index,
> -                                      address, rw, mmu_idx, &page_size);
> +                                      address, access_type,
> +                                      mmu_idx, &page_size);
>      vaddr = address;
> -    if (error_code == 0) {
> +    if (likely(error_code == 0)) {
>          qemu_log_mask(CPU_LOG_MMU,
> -                "Translate at %" VADDR_PRIx " -> " TARGET_FMT_plx ", vaddr "
> -                TARGET_FMT_lx "\n", address, paddr, vaddr);
> +                      "Translate at %" VADDR_PRIx " -> "
> +                      TARGET_FMT_plx ", vaddr " TARGET_FMT_lx "\n",
> +                      address, paddr, vaddr);
>          tlb_set_page(cs, vaddr, paddr, prot, mmu_idx, page_size);
> -        return 0;
> +        return true;
>      }
>
> -    if (env->mmuregs[3]) { /* Fault status register */
> -        env->mmuregs[3] = 1; /* overflow (not read before another fault) */
> -    }
> -    env->mmuregs[3] |= (access_index << 5) | error_code | 2;
> -    env->mmuregs[4] = address; /* Fault address register */
> -

In the old code, we set these MMU registers before checking
for the MMU_NF case...

>      if ((env->mmuregs[0] & MMU_NF) || env->psret == 0)  {
>          /* No fault mode: if a mapping is available, just override
>             permissions. If no mapping is available, redirect accesses to
> @@ -243,15 +241,25 @@ int sparc_cpu_handle_mmu_fault(CPUState *cs, vaddr 
> address, int size, int rw,
>             switching to normal mode. */
>          prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>          tlb_set_page(cs, vaddr, paddr, prot, mmu_idx, TARGET_PAGE_SIZE);
> -        return 0;
> -    } else {
> -        if (rw & 2) {
> -            cs->exception_index = TT_TFAULT;
> -        } else {
> -            cs->exception_index = TT_DFAULT;
> -        }
> -        return 1;
> +        return true;
>      }
> +
> +    if (probe) {
> +        return false;
> +    }
> +
> +    if (env->mmuregs[3]) { /* Fault status register */
> +        env->mmuregs[3] = 1; /* overflow (not read before another fault) */
> +    }
> +    env->mmuregs[3] |= (access_index << 5) | error_code | 2;
> +    env->mmuregs[4] = address; /* Fault address register */
> +
> +    if (access_type == MMU_INST_FETCH) {
> +        cs->exception_index = TT_TFAULT;
> +    } else {
> +        cs->exception_index = TT_DFAULT;
> +    }
> +    cpu_loop_exit_restore(cs, retaddr);
>  }

...but in the new code we only set them if we're really
going to fault.

The v8 SPARC architecture manual appending H says that
when the NF bit is 1 faults detected by the MMU cause
FSR and FAR to be updated even though no fault is generated
to the processor. So I think the change here is not correct.

(The spec also says that ASI 9 is supposed to be special and
not affected by NF==1; and I think that since we put entries in
the TLB for the NF case we won't correctly set the fault address
register if the CPU makes two successive accesses to the same
page, because the second access won't take the slow path and
won't update the FAR. But those are pre-existing bugs.)

thanks
-- PMM