Re: [qemu-s390x] [PATCH v2 18/27] target/s390x: Convert to CPUClass::tlb_fill

[David Hildenbrand]

On 09.05.19 08:02, Richard Henderson wrote:
> Cc: address@hidden
> Cc: Cornelia Huck <address@hidden>
> Cc: David Hildenbrand <address@hidden>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
> v2: Keep user-only and system tlb_fill separate.
> ---
>  target/s390x/internal.h    |  5 +--
>  target/s390x/cpu.c         |  5 ++-
>  target/s390x/excp_helper.c | 73 ++++++++++++++++++++++++++------------
>  target/s390x/mem_helper.c  | 16 ---------
>  4 files changed, 55 insertions(+), 44 deletions(-)
> 
> diff --git a/target/s390x/internal.h b/target/s390x/internal.h
> index 26575f2130..56534b38e0 100644
> --- a/target/s390x/internal.h
> +++ b/target/s390x/internal.h
> @@ -263,8 +263,9 @@ ObjectClass *s390_cpu_class_by_name(const char *name);
>  void s390x_cpu_debug_excp_handler(CPUState *cs);
>  void s390_cpu_do_interrupt(CPUState *cpu);
>  bool s390_cpu_exec_interrupt(CPUState *cpu, int int_req);
> -int s390_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int size, int rw,
> -                              int mmu_idx);
> +bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> +                       MMUAccessType access_type, int mmu_idx,
> +                       bool probe, uintptr_t retaddr);
>  void s390x_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
>                                     MMUAccessType access_type,
>                                     int mmu_idx, uintptr_t retaddr);
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index b58ef0a8ef..e28939032b 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -478,9 +478,8 @@ static void s390_cpu_class_init(ObjectClass *oc, void 
> *data)
>      cc->set_pc = s390_cpu_set_pc;
>      cc->gdb_read_register = s390_cpu_gdb_read_register;
>      cc->gdb_write_register = s390_cpu_gdb_write_register;
> -#ifdef CONFIG_USER_ONLY
> -    cc->handle_mmu_fault = s390_cpu_handle_mmu_fault;
> -#else
> +    cc->tlb_fill = s390_cpu_tlb_fill;
> +#ifndef CONFIG_USER_ONLY
>      cc->get_phys_page_debug = s390_cpu_get_phys_page_debug;
>      cc->vmsd = &vmstate_s390_cpu;
>      cc->write_elf64_note = s390_cpu_write_elf64_note;
> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
> index f84bfb1284..a4e134bcab 100644
> --- a/target/s390x/excp_helper.c
> +++ b/target/s390x/excp_helper.c
> @@ -74,8 +74,9 @@ void s390_cpu_do_interrupt(CPUState *cs)
>      cs->exception_index = -1;
>  }
>  
> -int s390_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size,
> -                              int rw, int mmu_idx)
> +bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> +                       MMUAccessType access_type, int mmu_idx,
> +                       bool probe, uintptr_t retaddr)
>  {
>      S390CPU *cpu = S390_CPU(cs);
>  
> @@ -83,7 +84,7 @@ int s390_cpu_handle_mmu_fault(CPUState *cs, vaddr address, 
> int size,
>      /* On real machines this value is dropped into LowMem.  Since this
>         is userland, simply put this someplace that cpu_loop can find it.  */
>      cpu->env.__excp_addr = address;
> -    return 1;
> +    cpu_loop_exit_restore(cs, retaddr);
>  }
>  
>  #else /* !CONFIG_USER_ONLY */
> @@ -102,19 +103,20 @@ static inline uint64_t cpu_mmu_idx_to_asc(int mmu_idx)
>      }
>  }
>  
> -int s390_cpu_handle_mmu_fault(CPUState *cs, vaddr orig_vaddr, int size,
> -                              int rw, int mmu_idx)
> +bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> +                       MMUAccessType access_type, int mmu_idx,
> +                       bool probe, uintptr_t retaddr)
>  {
>      S390CPU *cpu = S390_CPU(cs);
>      CPUS390XState *env = &cpu->env;
>      target_ulong vaddr, raddr;
>      uint64_t asc;
> -    int prot;
> +    int prot, fail;
>  
>      qemu_log_mask(CPU_LOG_MMU, "%s: addr 0x%" VADDR_PRIx " rw %d mmu_idx 
> %d\n",
> -                  __func__, orig_vaddr, rw, mmu_idx);
> +                  __func__, address, access_type, mmu_idx);
>  
> -    vaddr = orig_vaddr;
> +    vaddr = address;
>  
>      if (mmu_idx < MMU_REAL_IDX) {
>          asc = cpu_mmu_idx_to_asc(mmu_idx);
> @@ -122,39 +124,64 @@ int s390_cpu_handle_mmu_fault(CPUState *cs, vaddr 
> orig_vaddr, int size,
>          if (!(env->psw.mask & PSW_MASK_64)) {
>              vaddr &= 0x7fffffff;
>          }
> -        if (mmu_translate(env, vaddr, rw, asc, &raddr, &prot, true)) {
> -            return 1;
> -        }
> +        fail = mmu_translate(env, vaddr, access_type, asc, &raddr, &prot, 
> true);
>      } else if (mmu_idx == MMU_REAL_IDX) {
>          /* 31-Bit mode */
>          if (!(env->psw.mask & PSW_MASK_64)) {
>              vaddr &= 0x7fffffff;
>          }
> -        if (mmu_translate_real(env, vaddr, rw, &raddr, &prot)) {
> -            return 1;
> -        }
> +        fail = mmu_translate_real(env, vaddr, access_type, &raddr, &prot);
>      } else {
> -        abort();
> +        g_assert_not_reached();
>      }
>  
>      /* check out of RAM access */
> -    if (!address_space_access_valid(&address_space_memory, raddr,
> -                                    TARGET_PAGE_SIZE, rw,
> +    if (!fail &&
> +        !address_space_access_valid(&address_space_memory, raddr,
> +                                    TARGET_PAGE_SIZE, access_type,
>                                      MEMTXATTRS_UNSPECIFIED)) {
>          qemu_log_mask(CPU_LOG_MMU,
>                        "%s: raddr %" PRIx64 " > ram_size %" PRIx64 "\n",
>                        __func__, (uint64_t)raddr, (uint64_t)ram_size);
>          trigger_pgm_exception(env, PGM_ADDRESSING, ILEN_AUTO);
> -        return 1;
> +        fail = 1;
>      }
>  
> -    qemu_log_mask(CPU_LOG_MMU, "%s: set tlb %" PRIx64 " -> %" PRIx64 " 
> (%x)\n",
> -            __func__, (uint64_t)vaddr, (uint64_t)raddr, prot);
> +    if (!fail) {
> +        qemu_log_mask(CPU_LOG_MMU,
> +                      "%s: set tlb %" PRIx64 " -> %" PRIx64 " (%x)\n",
> +                      __func__, (uint64_t)vaddr, (uint64_t)raddr, prot);
> +        tlb_set_page(cs, address & TARGET_PAGE_MASK, raddr, prot,
> +                     mmu_idx, TARGET_PAGE_SIZE);
> +        return true;
> +    }
> +    if (probe) {
> +        return false;
> +    }
>  
> -    tlb_set_page(cs, orig_vaddr & TARGET_PAGE_MASK, raddr, prot,
> -                 mmu_idx, TARGET_PAGE_SIZE);
> +    cpu_restore_state(cs, retaddr, true);
>  
> -    return 0;
> +    /*
> +     * The ILC value for code accesses is undefined.  The important
> +     * thing here is to *not* leave env->int_pgm_ilen set to ILEN_AUTO,
> +     * which would cause do_program_interrupt to attempt to read from
> +     * env->psw.addr again.  C.f. the condition in trigger_page_fault,
> +     * but is not universally applied.
> +     *
> +     * ??? If we remove ILEN_AUTO, by moving the computation of ILEN
> +     * into cpu_restore_state, then we may remove this entirely.
> +     */
> +    if (access_type == MMU_INST_FETCH) {
> +        env->int_pgm_ilen = 2;
> +    }
> +
> +    cpu_loop_exit(cs);
> +}
> +
> +void tlb_fill(CPUState *cs, target_ulong addr, int size,
> +              MMUAccessType access_type, int mmu_idx, uintptr_t retaddr)
> +{
> +    s390_cpu_tlb_fill(cs, addr, size, access_type, mmu_idx, false, retaddr);
>  }
>  
>  static void do_program_interrupt(CPUS390XState *env)
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index 3f76a8abfd..ffd5f02fbe 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -33,22 +33,6 @@
>  
>  
> /*****************************************************************************/
>  /* Softmmu support */
> -#if !defined(CONFIG_USER_ONLY)
> -
> -/* try to fill the TLB and return an exception if error. If retaddr is
> -   NULL, it means that the function was called in C code (i.e. not
> -   from generated code or from helper.c) */
> -/* XXX: fix it to restore all registers */
> -void tlb_fill(CPUState *cs, target_ulong addr, int size,
> -              MMUAccessType access_type, int mmu_idx, uintptr_t retaddr)
> -{
> -    int ret = s390_cpu_handle_mmu_fault(cs, addr, size, access_type, 
> mmu_idx);
> -    if (unlikely(ret != 0)) {
> -        cpu_loop_exit_restore(cs, retaddr);
> -    }
> -}
> -
> -#endif
>  
>  /* #define DEBUG_HELPER */
>  #ifdef DEBUG_HELPER
> 


Reviewed-by: David Hildenbrand <address@hidden>

-- 

Thanks,

David / dhildenb