Xen project Mailing List

On Fri, Mar 27, 2015 at 3:52 PM, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:

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 <tklengyel@xxxxxxxxxxxxx>
> Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Reviewed-by:Â Julien Grall <julien.grall@xxxxxxxxxx>
> ---
> 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 <asm/psci.h>
>Â #include <asm/mmio.h>
>Â #include <asm/cpufeature.h>
> +#include <asm/flushtlb.h>
>
>Â #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?

Depending on the fault cause, we might. For permission faults, HPFAR is only valid during s1ptw.. Given that the only check we do is for permission faults and that's the only thing that cares about the API anyway, we can put this entire block into the switch itself once the fault check is already determined to be a permission fault.

> +Â Â {
> +Â Â Â Â /*
> +Â Â Â Â Â* 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 */

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