| Message ID | 1469717505-8026-7-git-send-email-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Stefano, On 23/08/16 02:05, Stefano Stabellini wrote: > On Thu, 28 Jul 2016, Julien Grall wrote: >> A data/instruction abort may have occurred if another CPU was playing >> with the stage-2 page table when following the break-before-make >> sequence (see D4.7.1 in ARM DDI 0487A.j). Rather than injecting directly >> the fault to the guest, we need to check whether the mapping exists. If >> it exists, return to the guest to replay the instruction. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> --- >> xen/arch/arm/traps.c | 40 ++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 38 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c >> index b46284c..da56cc0 100644 >> --- a/xen/arch/arm/traps.c >> +++ b/xen/arch/arm/traps.c >> @@ -2404,6 +2404,7 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs, >> register_t gva = READ_SYSREG(FAR_EL2); >> uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK; >> paddr_t gpa; >> + mfn_t mfn; >> >> if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) ) >> gpa = get_faulting_ipa(gva); >> @@ -2417,6 +2418,11 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs, >> */ >> flush_tlb_local(); >> >> + /* >> + * We may not be able to translate because someone is >> + * playing with the Stage-2 page table of the domain. >> + * Return to the guest. >> + */ >> rc = gva_to_ipa(gva, &gpa, GV2M_READ); >> if ( rc == -EFAULT ) >> return; /* Try again */ >> @@ -2437,8 +2443,17 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs, >> /* Trap was triggered by mem_access, work here is done */ >> if ( !rc ) >> return; >> + break; >> } >> - break; >> + case FSC_FLT_TRANS: > > Please add brackets under the case statement for code style This is not part of the coding style. We only use the brackets when local variable is defined for a specific case. See vgic-{v2,v3}.c for instance. Note that I am a bit surprised you complain here about the missing brackets but you did not on commit def4273 "xen/arm: traps: MMIO should only be emulated for fault translation" at the beginning of august. > > >> + /* >> + * The PT walk may have failed because someone was playing >> + * with the Stage-2 page table. Walk the Stage-2 PT to check >> + * if the entry exists. If it's the case, return to the guest >> + */ >> + mfn = p2m_lookup(current->domain, _gfn(paddr_to_pfn(gpa)), NULL); >> + if ( !mfn_eq(mfn, INVALID_MFN) ) >> + return; > > Just checking, but isn't it possible to get a genuine translation abort > with an mfn != invalid_mfn? A translation fault means the entry is not present in the page table at the time the processor did the page table walk. This may happen because the hypervisor is modifying the stage-2 page table on another processor (for instance with the break-before-make sequence). p2m_lookup will do a software page walk to get entry. As the function is using a read lock, it will wait until the other processor finishes to update the page tables before doing the lookup. So if the mfn returned is valid, then we know that the translation fault is spurious and should return to the guest to retry the execution of the faulting instruction. > > >> } >> >> inject_iabt_exception(regs, gva, hsr.len); >> @@ -2455,7 +2470,7 @@ static bool_t try_handle_mmio(struct cpu_user_regs *regs, >> return 0; >> >> /* All the instructions used on emulated MMIO region should be valid */ >> - if ( !dabt.valid ) >> + if ( !info->dabt.valid ) >> return 0; > > Spurious change? Yes. I will drop it. Regards,
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index b46284c..da56cc0 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -2404,6 +2404,7 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs, register_t gva = READ_SYSREG(FAR_EL2); uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK; paddr_t gpa; + mfn_t mfn; if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) ) gpa = get_faulting_ipa(gva); @@ -2417,6 +2418,11 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs, */ flush_tlb_local(); + /* + * We may not be able to translate because someone is + * playing with the Stage-2 page table of the domain. + * Return to the guest. + */ rc = gva_to_ipa(gva, &gpa, GV2M_READ); if ( rc == -EFAULT ) return; /* Try again */ @@ -2437,8 +2443,17 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs, /* Trap was triggered by mem_access, work here is done */ if ( !rc ) return; + break; } - break; + case FSC_FLT_TRANS: + /* + * The PT walk may have failed because someone was playing + * with the Stage-2 page table. Walk the Stage-2 PT to check + * if the entry exists. If it's the case, return to the guest + */ + mfn = p2m_lookup(current->domain, _gfn(paddr_to_pfn(gpa)), NULL); + if ( !mfn_eq(mfn, INVALID_MFN) ) + return; } inject_iabt_exception(regs, gva, hsr.len); @@ -2455,7 +2470,7 @@ static bool_t try_handle_mmio(struct cpu_user_regs *regs, return 0; /* All the instructions used on emulated MMIO region should be valid */ - if ( !dabt.valid ) + if ( !info->dabt.valid ) return 0; /* @@ -2483,6 +2498,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs, int rc; mmio_info_t info; uint8_t fsc = hsr.dabt.dfsc & ~FSC_LL_MASK; + mfn_t mfn; info.dabt = dabt; #ifdef CONFIG_ARM_32 @@ -2496,6 +2512,11 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs, else { rc = gva_to_ipa(info.gva, &info.gpa, GV2M_READ); + /* + * We may not be able to translate because someone is + * playing with the Stage-2 page table of the domain. + * Return to the guest. + */ if ( rc == -EFAULT ) return; /* Try again */ } @@ -2519,11 +2540,26 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs, break; } case FSC_FLT_TRANS: + /* + * Attempt first to emulate the MMIO has the data abort will + * likely happen an emulated region. + */ if ( try_handle_mmio(regs, &info) ) { advance_pc(regs, hsr); return; } + + /* + * The PT walk may have failed because someone was playing + * with the Stage-2 page table. Walk the Stage-2 PT to check + * if the entry exists. If it's the case, return to the guest + */ + mfn = p2m_lookup(current->domain, _gfn(paddr_to_pfn(info.gpa)), NULL); + if ( !mfn_eq(mfn, INVALID_MFN) ) + return; + + break; default: gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n", hsr.bits, dabt.dfsc);
A data/instruction abort may have occurred if another CPU was playing with the stage-2 page table when following the break-before-make sequence (see D4.7.1 in ARM DDI 0487A.j). Rather than injecting directly the fault to the guest, we need to check whether the mapping exists. If it exists, return to the guest to replay the instruction. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/traps.c | 40 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-)