From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40605) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZkGtl-000320-GJ for qemu-devel@nongnu.org; Thu, 08 Oct 2015 15:30:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZkGtj-0002lF-RQ for qemu-devel@nongnu.org; Thu, 08 Oct 2015 15:30:41 -0400 Received: from mail-pa0-x232.google.com ([2607:f8b0:400e:c03::232]:33091) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZkGtj-0002kv-CJ for qemu-devel@nongnu.org; Thu, 08 Oct 2015 15:30:39 -0400 Received: by pacex6 with SMTP id ex6so63110858pac.0 for ; Thu, 08 Oct 2015 12:30:38 -0700 (PDT) Date: Thu, 8 Oct 2015 12:25:56 -0700 From: "Edgar E. Iglesias" Message-ID: <20151008192556.GI24839@toto> References: <1443911939-2825-1-git-send-email-edgar.iglesias@gmail.com> <1443911939-2825-6-git-send-email-edgar.iglesias@gmail.com> <874mi2y7d0.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <874mi2y7d0.fsf@linaro.org> Subject: Re: [Qemu-devel] [PATCH v3 5/9] target-arm: Add ARMMMUFaultInfo List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex =?iso-8859-1?Q?Benn=E9e?= Cc: edgar.iglesias@xilinx.com, peter.maydell@linaro.org, qemu-devel@nongnu.org, agraf@suse.de, laurent.desnogues@gmail.com, serge.fdrv@gmail.com On Wed, Oct 07, 2015 at 05:24:27PM +0100, Alex Bennée wrote: > > Edgar E. Iglesias writes: > > > From: "Edgar E. Iglesias" > > > > 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 > > --- > > 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