|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-next 16/16] xen/arm: traps: Merge do_trap_instr_abort_guest and do_trap_data_abort_guest
On Thu, 23 Nov 2017, Julien Grall wrote:
> The two helpers do_trap_instr_abort_guest and do_trap_data_abort_guest
> are used trap stage-2 abort. While the former is only handling prefetch
> abort and the latter data abort, they are very similarly and does not
> warrant to have separate helpers.
>
> For instance, merging the both will make easier to maintain stage-2 abort
> handling. So consolidate the two helpers in a new helper
> do_trap_stage2_abort.
>
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> ---
> xen/arch/arm/traps.c | 133
> ++++++++++++++++-----------------------------------
> 1 file changed, 41 insertions(+), 92 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index a68e01b457..b83a2d9244 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1862,79 +1862,6 @@ static inline bool hpfar_is_valid(bool s1ptw, uint8_t
> fsc)
> return s1ptw || (fsc == FSC_FLT_TRANS && !check_workaround_834220());
> }
>
> -static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
> - const union hsr hsr)
> -{
> - int rc;
> - register_t gva;
> - uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK;
> - paddr_t gpa;
> - mfn_t mfn;
> -
> - gva = get_hfar(false /* is_data */);
> -
> - /*
> - * If this bit has been set, it means that this instruction abort is
> caused
> - * by a guest external abort. We can handle this instruction abort as
> guest
> - * SError.
> - */
> - if ( hsr.iabt.eat )
> - return __do_trap_serror(regs, true);
> -
> -
> - if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
> - gpa = get_faulting_ipa(gva);
> - else
> - {
> - /*
> - * 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_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 */
> - }
> -
> - switch ( fsc )
> - {
> - case FSC_FLT_PERM:
> - {
> - const struct npfec npfec = {
> - .insn_fetch = 1,
> - .gla_valid = 1,
> - .kind = hsr.iabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
> - };
> -
> - p2m_mem_access_check(gpa, gva, npfec);
> - /*
> - * The only way to get here right now is because of mem_access,
> - * thus reinjecting the exception to the guest is never required.
> - */
> - return;
> - }
> - 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 = gfn_to_mfn(current->domain, _gfn(paddr_to_pfn(gpa)));
> - if ( !mfn_eq(mfn, INVALID_MFN) )
> - return;
> - }
> -
> - inject_iabt_exception(regs, gva, hsr.len);
> -}
> -
> static bool try_handle_mmio(struct cpu_user_regs *regs,
> const union hsr hsr,
> paddr_t gpa)
> @@ -1946,6 +1873,8 @@ static bool try_handle_mmio(struct cpu_user_regs *regs,
> };
> int rc;
>
> + ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
> +
> /* stage-1 page table should never live in an emulated MMIO region */
> if ( dabt.s1ptw )
> return false;
> @@ -2001,29 +1930,43 @@ static bool try_map_mmio(gfn_t gfn)
> return !map_regions_p2mt(d, gfn, 1, mfn, p2m_mmio_direct_c);
> }
>
> -static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
> - const union hsr hsr)
> +static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
> + const union hsr hsr)
> {
> - const struct hsr_dabt dabt = hsr.dabt;
> + /*
> + * The encoding of hsr_iabt is a subset of hsr_dabt. So use
> + * hsr_dabt to represent an abort fault.
> + */
> + const struct hsr_xabt xabt = hsr.xabt;
> int rc;
> vaddr_t gva;
> paddr_t gpa;
> - uint8_t fsc = hsr.dabt.dfsc & ~FSC_LL_MASK;
> + uint8_t fsc = xabt.fsc & ~FSC_LL_MASK;
> mfn_t mfn;
> + bool is_data = (hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
>
> /*
> - * If this bit has been set, it means that this data abort is caused
> - * by a guest external abort. We treat this data abort as guest SError.
> + * If this bit has been set, it means that this stage-2 abort is caused
> + * by a guest external abort. We treat this stage-2 abort as guest
> SError.
> */
> - if ( dabt.eat )
> + if ( xabt.eat )
> return __do_trap_serror(regs, true);
>
> - gva = get_hfar(true /* is_data */);
> + gva = get_hfar(is_data);
>
> - if ( hpfar_is_valid(dabt.s1ptw, fsc) )
> + if ( hpfar_is_valid(xabt.s1ptw, fsc) )
> gpa = get_faulting_ipa(gva);
> else
> {
> + /*
> + * 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.
> + */
> + if ( !is_data )
> + flush_tlb_local();
> +
> rc = gva_to_ipa(gva, &gpa, GV2M_READ);
> /*
> * We may not be able to translate because someone is
> @@ -2039,10 +1982,11 @@ static void do_trap_data_abort_guest(struct
> cpu_user_regs *regs,
> case FSC_FLT_PERM:
> {
> const struct npfec npfec = {
> - .read_access = !dabt.write,
> - .write_access = dabt.write,
> + .insn_fetch = !is_data,
> + .read_access = is_data && !hsr.dabt.write,
> + .write_access = is_data && !hsr.dabt.write,
Shouldn't this be:
.write_access = is_data && hsr.dabt.write,
> .gla_valid = 1,
> - .kind = dabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
> + .kind = xabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
> };
>
> p2m_mem_access_check(gpa, gva, npfec);
> @@ -2056,8 +2000,10 @@ static void do_trap_data_abort_guest(struct
> cpu_user_regs *regs,
> /*
> * Attempt first to emulate the MMIO as the data abort will
> * likely happen in an emulated region.
> + *
> + * Note that emulated region cannot be executed
> */
> - if ( try_handle_mmio(regs, hsr, gpa) )
> + if ( is_data && try_handle_mmio(regs, hsr, gpa) )
> {
> advance_pc(regs, hsr);
> return;
> @@ -2072,18 +2018,21 @@ static void do_trap_data_abort_guest(struct
> cpu_user_regs *regs,
> if ( !mfn_eq(mfn, INVALID_MFN) )
> return;
>
> - if ( try_map_mmio(gaddr_to_gfn(gpa)) )
> + if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
> return;
>
> break;
> default:
> - gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n",
> - hsr.bits, dabt.dfsc);
> + gprintk(XENLOG_WARNING, "Unsupported FSC: HSR=%#x DFSC=%#x\n",
> + hsr.bits, xabt.fsc);
> }
>
> gdprintk(XENLOG_DEBUG, "HSR=0x%x pc=%#"PRIregister" gva=%#"PRIvaddr
> " gpa=%#"PRIpaddr"\n", hsr.bits, regs->pc, gva, gpa);
> - inject_dabt_exception(regs, gva, hsr.len);
> + if ( is_data )
> + inject_dabt_exception(regs, gva, hsr.len);
> + else
> + inject_iabt_exception(regs, gva, hsr.len);
> }
>
> static void enter_hypervisor_head(struct cpu_user_regs *regs)
> @@ -2216,11 +2165,11 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>
> case HSR_EC_INSTR_ABORT_LOWER_EL:
> perfc_incr(trap_iabt);
> - do_trap_instr_abort_guest(regs, hsr);
> + do_trap_stage2_abort_guest(regs, hsr);
> break;
> case HSR_EC_DATA_ABORT_LOWER_EL:
> perfc_incr(trap_dabt);
> - do_trap_data_abort_guest(regs, hsr);
> + do_trap_stage2_abort_guest(regs, hsr);
> break;
>
> default:
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |