Re: [Qemu-devel] [PATCH v3 5/9] target-arm: Add ARMMMUFaultInfo

["Alex Bennée" <alex.bennee@linaro.org>]

Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Introduce ARMMMUFaultInfo to propagate MMU Fault information
> across the MMU translation code path. This is in preparation for
> adding State-2 translation.

s/State/stage/?

>
> No functional changes.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/helper.c    | 32 ++++++++++++++++++++------------
>  target-arm/internals.h | 15 ++++++++++++++-
>  target-arm/op_helper.c |  3 ++-
>  3 files changed, 36 insertions(+), 14 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index cbc1570..a429ff2 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -18,7 +18,8 @@
>  static bool get_phys_addr(CPUARMState *env, target_ulong address,
>                            int access_type, ARMMMUIdx mmu_idx,
>                            hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
> -                          target_ulong *page_size, uint32_t *fsr);
> +                          target_ulong *page_size, uint32_t *fsr,
> +                          ARMMMUFaultInfo *fi);
>  
>  /* Definitions for the PMCCNTR and PMCR registers */
>  #define PMCRD   0x8
> @@ -1774,9 +1775,10 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>      bool ret;
>      uint64_t par64;
>      MemTxAttrs attrs = {};
> +    ARMMMUFaultInfo fi = {};
>  
>      ret = get_phys_addr(env, value, access_type, mmu_idx,
> -                        &phys_addr, &attrs, &prot, &page_size, &fsr);
> +                        &phys_addr, &attrs, &prot, &page_size, &fsr, &fi);
>      if (extended_addresses_enabled(env)) {
>          /* fsr is a DFSR/IFSR value for the long descriptor
>           * translation table format, but with WnR always clear.
> @@ -6167,7 +6169,8 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure)
>  static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
>                               int access_type, ARMMMUIdx mmu_idx,
>                               hwaddr *phys_ptr, int *prot,
> -                             target_ulong *page_size, uint32_t *fsr)
> +                             target_ulong *page_size, uint32_t *fsr,
> +                             ARMMMUFaultInfo *fi)
>  {
>      CPUState *cs = CPU(arm_env_get_cpu(env));
>      int code;
> @@ -6280,7 +6283,8 @@ do_fault:
>  static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
>                               int access_type, ARMMMUIdx mmu_idx,
>                               hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
> -                             target_ulong *page_size, uint32_t *fsr)
> +                             target_ulong *page_size, uint32_t *fsr,
> +                             ARMMMUFaultInfo *fi)
>  {
>      CPUState *cs = CPU(arm_env_get_cpu(env));
>      int code;
> @@ -6431,7 +6435,8 @@ typedef enum {
>  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>                                 int access_type, ARMMMUIdx mmu_idx,
>                                 hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
> -                               target_ulong *page_size_ptr, uint32_t *fsr)
> +                               target_ulong *page_size_ptr, uint32_t *fsr,
> +                               ARMMMUFaultInfo *fi)
>  {
>      CPUState *cs = CPU(arm_env_get_cpu(env));
>      /* Read an LPAE long-descriptor translation table. */
> @@ -6975,7 +6980,8 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
>  static bool get_phys_addr(CPUARMState *env, target_ulong address,
>                            int access_type, ARMMMUIdx mmu_idx,
>                            hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
> -                          target_ulong *page_size, uint32_t *fsr)
> +                          target_ulong *page_size, uint32_t *fsr,
> +                          ARMMMUFaultInfo *fi)
>  {
>      if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
>          /* TODO: when we support EL2 we should here call ourselves recursively
> @@ -7034,13 +7040,13 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
>  
>      if (regime_using_lpae_format(env, mmu_idx)) {
>          return get_phys_addr_lpae(env, address, access_type, mmu_idx, phys_ptr,
> -                                  attrs, prot, page_size, fsr);
> +                                  attrs, prot, page_size, fsr, fi);
>      } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
>          return get_phys_addr_v6(env, address, access_type, mmu_idx, phys_ptr,
> -                                attrs, prot, page_size, fsr);
> +                                attrs, prot, page_size, fsr, fi);
>      } else {
>          return get_phys_addr_v5(env, address, access_type, mmu_idx, phys_ptr,
> -                                prot, page_size, fsr);
> +                                prot, page_size, fsr, fi);
>      }
>  }
>  
> @@ -7049,7 +7055,8 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
>   * fsr with ARM DFSR/IFSR fault register format value on failure.
>   */
>  bool arm_tlb_fill(CPUState *cs, vaddr address,
> -                  int access_type, int mmu_idx, uint32_t *fsr)
> +                  int access_type, int mmu_idx, uint32_t *fsr,
> +                  ARMMMUFaultInfo *fi)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
> @@ -7060,7 +7067,7 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
>      MemTxAttrs attrs = {};
>  
>      ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr,
> -                        &attrs, &prot, &page_size, fsr);
> +                        &attrs, &prot, &page_size, fsr, fi);
>      if (!ret) {
>          /* Map a single [sub]page.  */
>          phys_addr &= TARGET_PAGE_MASK;
> @@ -7083,9 +7090,10 @@ hwaddr arm_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
>      bool ret;
>      uint32_t fsr;
>      MemTxAttrs attrs = {};
> +    ARMMMUFaultInfo fi = {};
>  
>      ret = get_phys_addr(env, addr, 0, cpu_mmu_index(env, false), &phys_addr,
> -                        &attrs, &prot, &page_size, &fsr);
> +                        &attrs, &prot, &page_size, &fsr, &fi);
>  
>      if (ret) {
>          return -1;
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index 36a56aa..3be23be 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -389,8 +389,21 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type);
>  void arm_handle_psci_call(ARMCPU *cpu);
>  #endif
>  
> +/**
> + * ARMMMUFaultInfo: Information describing an ARM MMU Fault
> + * @s2addr: Address that caused a fault at stage 2
> + * @stage2: True if we faulted at stage 2
> + * @s1ptw: True if we faulted at stage 2 while doing a stage 1 page-table walk
> + */
> +typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
> +struct ARMMMUFaultInfo {
> +    target_ulong s2addr;
> +    bool stage2;
> +    bool s1ptw;

I guess the compiler packs the bools down pretty well but why not just
encode the faulting stage in a single variable? Perhaps I'm
misunderstanding the potential combinations here.

> +};
> +
>  /* Do a page table walk and add page to TLB if possible */
>  bool arm_tlb_fill(CPUState *cpu, vaddr address, int rw, int mmu_idx,
> -                  uint32_t *fsr);
> +                  uint32_t *fsr, ARMMMUFaultInfo *fi);
>  
>  #endif
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 1425a1d..7ff3c61 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -83,8 +83,9 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
>  {
>      bool ret;
>      uint32_t fsr = 0;
> +    struct ARMMMUFaultInfo fi = {0};
>  
> -    ret = arm_tlb_fill(cs, addr, is_write, mmu_idx, &fsr);
> +    ret = arm_tlb_fill(cs, addr, is_write, mmu_idx, &fsr, &fi);
>      if (unlikely(ret)) {
>          ARMCPU *cpu = ARM_CPU(cs);
>          CPUARMState *env = &cpu->env;

-- 
Alex Bennée