Edgar E. Iglesias <edgar.igles...@gmail.com> writes: > On Wed, Oct 07, 2015 at 05:24:27PM +0100, Alex Bennée wrote: >> >> Edgar E. Iglesias <edgar.igles...@gmail.com> writes: >> >> > From: "Edgar E. Iglesias" <edgar.igles...@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.igles...@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 -- Alex Bennée