Re: [Qemu-devel] [PATCH 02/26] tcg: Add CPUClass::tlb_fill

[Peter Maydell]

On Wed, 3 Apr 2019 at 04:49, Richard Henderson
<address@hidden> wrote:
>
> This hook will replace the (user-only mode specific) handle_mmu_fault
> hook, and the (system mode specific) tlb_fill function.
>
> The handle_mmu_fault hook was written as if there was a valid
> way to recover from an mmu fault, and had 3 possible return states.
> In reality, the only valid action is to raise an exception,
> return to the main loop, and delver the SIGSEGV to the guest.

"deliver"

You might also mention here that all of the implementations
of handle_mmu_fault for guest architectures which support
linux-user do in fact only ever return 1.

>
> Using the hook for system mode requires that all targets be converted,
> so for now the hook is (optionally) used only from user-only mode.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  include/qom/cpu.h     |  9 +++++++++
>  accel/tcg/user-exec.c | 42 ++++++++++++++----------------------------
>  2 files changed, 23 insertions(+), 28 deletions(-)
>
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 1d6099e5d4..7e96a0aed3 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -119,6 +119,12 @@ struct TranslationBlock;
>   *       will need to do more. If this hook is not implemented then the
>   *       default is to call @set_pc(tb->pc).
>   * @handle_mmu_fault: Callback for handling an MMU fault.
> + * @tlb_fill: Callback for handling a softmmu tlb miss or user-only
> + *       address fault.  For system mode, if the access is valid, call
> + *       tlb_set_page and return true; if the access is invalid, and
> + *       probe is true, return false; otherwise raise an exception and
> + *       do not return.  For user-only mode, always raise an exception
> + *       and do not return.
>   * @get_phys_page_debug: Callback for obtaining a physical address.
>   * @get_phys_page_attrs_debug: Callback for obtaining a physical address and 
> the
>   *       associated memory transaction attributes to use for the access.
> @@ -194,6 +200,9 @@ typedef struct CPUClass {
>      void (*synchronize_from_tb)(CPUState *cpu, struct TranslationBlock *tb);
>      int (*handle_mmu_fault)(CPUState *cpu, vaddr address, int size, int rw,
>                              int mmu_index);
> +    bool (*tlb_fill)(CPUState *cpu, vaddr address, int size,
> +                     MMUAccessType access_type, int mmu_idx,
> +                     bool probe, uintptr_t retaddr);
>      hwaddr (*get_phys_page_debug)(CPUState *cpu, vaddr addr);
>      hwaddr (*get_phys_page_attrs_debug)(CPUState *cpu, vaddr addr,
>                                          MemTxAttrs *attrs);
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index fa9380a380..f13c0b2b67 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -65,6 +65,7 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t 
> *info,
>      CPUClass *cc;
>      int ret;
>      unsigned long address = (unsigned long)info->si_addr;
> +    MMUAccessType access_type;
>
>      /* We must handle PC addresses from two different sources:
>       * a call return address and a signal frame address.
> @@ -151,40 +152,25 @@ static inline int handle_cpu_signal(uintptr_t pc, 
> siginfo_t *info,
>  #if TARGET_LONG_BITS == 32 && HOST_LONG_BITS == 64
>      g_assert(h2g_valid(address));
>  #endif
> -
> -    /* Convert forcefully to guest address space, invalid addresses
> -       are still valid segv ones */

This comment is still valid so I don't think it should be deleted.

>      address = h2g_nocheck(address);

Otherwise

Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM