| Message ID | 1443911939-2825-6-git-send-email-edgar.iglesias@gmail.com |
|---|---|
| State | New |
| Headers | show |
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;
On Wed, Oct 07, 2015 at 05:24:27PM +0100, Alex Bennée wrote: > > 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/? Thanks, I've fixed this > > > > > 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
On Wed, Oct 07, 2015 at 05:24:27PM +0100, Alex Bennée wrote: > > 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 you mean using bitfields? e.g: struct ARMMMUFaultInfo { target_ulong s2addr; unsigned int stage2 : 1; unsigned int s1ptw : 1; } If so, I guess we could. If others agree, I can change to that. We could also maybe structure things differently. I didn't consider the encodings very much here to be honest... Or do you mean why we need these at all? We need it because the places in the code where we figure out that a fault occurs don't have paths to propagate details about the error back to the callers that later need to generate the exception syndromes. The s1ptw field is additional detail to a stage-2 fault describing that the stage-2 fault happened while doing a page-table walk for a stage-1 translation. s2addr is similar additional info to a stage-2 fault, passing the fault address so we can report it via the hypervisor IPA fault address reg. Best regards, Edgar
On 8 October 2015 at 21:06, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > On Wed, Oct 07, 2015 at 05:24:27PM +0100, Alex Bennée wrote: >> >> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: >> >> > +/** >> > + * 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 you mean using bitfields? > e.g: > > struct ARMMMUFaultInfo { > target_ulong s2addr; > unsigned int stage2 : 1; > unsigned int s1ptw : 1; > } > If so, I guess we could. If others agree, I can change to that. > We could also maybe structure things differently. I didn't consider > the encodings very much here to be honest... I'm not a great fan of bitfields personally; I think a pair of bool fields expresses things more clearly. -- PMM
Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: > On Wed, Oct 07, 2015 at 05:24:27PM +0100, Alex Bennée wrote: >> >> 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 you mean using bitfields? > e.g: > > struct ARMMMUFaultInfo { > target_ulong s2addr; > unsigned int stage2 : 1; > unsigned int s1ptw : 1; > } > If so, I guess we could. If others agree, I can change to that. > We could also maybe structure things differently. I didn't consider > the encodings very much here to be honest... No I didn't. > > Or do you mean why we need these at all? > We need it because the places in the code where we figure out that > a fault occurs don't have paths to propagate details about the error > back to the callers that later need to generate the exception syndromes. > > The s1ptw field is additional detail to a stage-2 fault describing that > the stage-2 fault happened while doing a page-table walk for a stage-1 > translation. s2addr is similar additional info to a stage-2 fault, > passing the fault address so we can report it via the hypervisor IPA > fault address reg. My concern was making the meaning of the various variables intuitive. From what I can tell this info is only relevant to stage 2 faults which may occur while walking stage 1 and stage 2 page tables? Will we ever see a point where s2addr = NULL, stage2 = false and s1ptw = false? I'll come back to this once I've gone over the later patches to see if it makes more sense. > > Best regards, > Edgar
On Wed, Oct 14, 2015 at 02:00:17PM +0100, Alex Bennée wrote: > > Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: > > > On Wed, Oct 07, 2015 at 05:24:27PM +0100, Alex Bennée wrote: > >> > >> 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 you mean using bitfields? > > e.g: > > > > struct ARMMMUFaultInfo { > > target_ulong s2addr; > > unsigned int stage2 : 1; > > unsigned int s1ptw : 1; > > } > > If so, I guess we could. If others agree, I can change to that. > > We could also maybe structure things differently. I didn't consider > > the encodings very much here to be honest... > > No I didn't. > > > > > Or do you mean why we need these at all? > > We need it because the places in the code where we figure out that > > a fault occurs don't have paths to propagate details about the error > > back to the callers that later need to generate the exception syndromes. > > > > The s1ptw field is additional detail to a stage-2 fault describing that > > the stage-2 fault happened while doing a page-table walk for a stage-1 > > translation. s2addr is similar additional info to a stage-2 fault, > > passing the fault address so we can report it via the hypervisor IPA > > fault address reg. > > My concern was making the meaning of the various variables intuitive. > From what I can tell this info is only relevant to stage 2 faults which > may occur while walking stage 1 and stage 2 page tables? Will we ever > see a point where s2addr = NULL, stage2 = false and s1ptw = false? A pure stage1 fault would render that. I guess we could throw in more stuff in here (e.g fsr) but I'm also trying to keep this series small to avoid too much change from creeping in. I'll be sending a v4 tonight BTW. Cheers, Edgar
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; +}; + /* 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;