From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH V14 5/7] xen/arm: Instruction prefetch abort (X) mem_access event handling Date: Fri, 27 Mar 2015 15:52:49 +0000 Message-ID: <1427471569.13935.177.camel@citrix.com> References: <1427407531-31694-1-git-send-email-tklengyel@sec.in.tum.de> <1427407531-31694-6-git-send-email-tklengyel@sec.in.tum.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1427407531-31694-6-git-send-email-tklengyel@sec.in.tum.de> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas K Lengyel Cc: wei.liu2@citrix.com, stefano.stabellini@eu.citrix.com, ian.jackson@eu.citrix.com, julien.grall@linaro.org, tim@xen.org, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, jbeulich@suse.com, keir@xen.org List-Id: xen-devel@lists.xenproject.org On Thu, 2015-03-26 at 23:05 +0100, Tamas K Lengyel wrote: > Add missing structure definition for iabt and update the trap handling > mechanism to only inject the exception if the mem_access checker > decides to do so. > > Signed-off-by: Tamas K Lengyel > Acked-by: Ian Campbell > Reviewed-by: Julien Grall > --- > v14: - Query HPFAR_EL2 when valid in iabt handler > - Flush TLB before doing GVA->IPA translation in iabt handler > v10: - Minor comment fix for describing s1ptw. > v8: - Revert to arch specific p2m_mem_access_check. > - Retire iabt_fsc enum and use FSC_FLT instead. > - Complete the struct definition of hsr_iabt. > v7: - Use the new common mem_access_check. > v6: - Make npfec a const. > v4: - Don't mark instruction fetch violation as read violation. > - Use new struct npfec to pass violation info. > v2: - Add definition for instruction abort instruction fetch status codes > (enum iabt_ifsc) and only call p2m_mem_access_check for traps triggered > for permission violations. > --- > xen/arch/arm/traps.c | 46 +++++++++++++++++++++++++++++++++++++++-- > xen/include/asm-arm/processor.h | 11 ++++++++++ > 2 files changed, 55 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index e02c848..be90c9e 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -40,6 +40,7 @@ > #include > #include > #include > +#include > > #include "decode.h" > #include "vtimer.h" > @@ -1961,8 +1962,49 @@ done: > static void do_trap_instr_abort_guest(struct cpu_user_regs *regs, > union hsr hsr) > { > - register_t addr = READ_SYSREG(FAR_EL2); > - inject_iabt_exception(regs, addr, hsr.len); > + struct hsr_iabt iabt = hsr.iabt; > + int rc; > + paddr_t gpa; > + register_t gva = READ_SYSREG(FAR_EL2); > + > + if ( iabt.s1ptw ) > + gpa = READ_SYSREG(HPFAR_EL2); > + else Can you not avoid the else case entirely by extending the if to cover the other cases where HPFAR is explicitly valid? I can't be bothered to go look right now but IIRC it included at least stage 2 access permissions related failures, which would cover more xenaccess scenarios, no? > + { > + /* > + * Flush the TLB to make sure the DTLB is clear before > + * doing GVA->IPA translation. If we got here because of > + * an entry only present in the ITLB, this translation may > + * still be inaccurate. > + */ > + flush_tlb_domain(current->domain); > + > + rc = gva_to_ipa(gva, &gpa, GV2M_READ); > + if ( rc == -EFAULT ) > + goto bad_insn_abort; > + } > + > + switch ( iabt.ifsc & 0x3f ) > + { > + case FSC_FLT_PERM ... FSC_FLT_PERM + 3: > + { > + const struct npfec npfec = { > + .insn_fetch = 1, > + .gla_valid = 1, > + .kind = iabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla > + }; > + > + rc = p2m_mem_access_check(gpa, gva, npfec); > + > + /* Trap was triggered by mem_access, work here is done */ > + if ( !rc ) > + return; > + } > + break; > + } > + > +bad_insn_abort: > + inject_iabt_exception(regs, gva, hsr.len); > } > > static void do_trap_data_abort_guest(struct cpu_user_regs *regs, > diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h > index cf7ab7c..db07fdd 100644 > --- a/xen/include/asm-arm/processor.h > +++ b/xen/include/asm-arm/processor.h > @@ -438,6 +438,17 @@ union hsr { > } sysreg; /* HSR_EC_SYSREG */ > #endif > > + struct hsr_iabt { > + unsigned long ifsc:6; /* Instruction fault status code */ > + unsigned long res0:1; > + unsigned long s1ptw:1; /* Stage 2 fault during stage 1 translation */ > + unsigned long res1:1; > + unsigned long eat:1; /* External abort type */ > + unsigned long res2:15; > + unsigned long len:1; /* Instruction length */ > + unsigned long ec:6; /* Exception Class */ > + } iabt; /* HSR_EC_INSTR_ABORT_* */ > + > struct hsr_dabt { > unsigned long dfsc:6; /* Data Fault Status Code */ > unsigned long write:1; /* Write / not Read */