Re: [Xen-devel] [PATCH V13 5/7] xen/arm: Instruction prefetch abort (X) mem_event handling

[Tamas K Lengyel <tklengyel@xxxxxxxxxxxxx>]

On Fri, Mar 6, 2015 at 10:24 PM, Tamas K Lengyel <tklengyel@xxxxxxxxxxxxx> 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 <tklengyel@xxxxxxxxxxxxx>
Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
Reviewed-by:Â Julien Grall <julien.grall@xxxxxxxxxx>
---
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      | 31 +++++++++++++++++++++++++++++--
Âxen/include/asm-arm/processor.h | 11 +++++++++++
Â2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f5aa647..0670145 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1961,8 +1961,35 @@ 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);

So I have a question here. The following call to gva_to_ipa will use the MMU to translate the gva as if it was a data-read access. However, we got here because of an instruction fetch access. I did a quick check and (at least some) ARM CPUs have split-TLBs. So technically using gva_to_ipa here could get us an IPA that wasn't the actual address if the guest pagetable has since been updated and the TLBs primed. Should the TLB be flushed here just to be sure we have an accurate translation?

Â

+Â Â rc = gva_to_ipa(gva, &gpa);
+Â Â if ( -EFAULT == rc )
+Â Â Â Â return;
+
+Â Â 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;
+Â Â }
+
+Â Â 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 */
--
2.1.4

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel