| Message ID | 20171123183210.12045-17-julien.grall@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Stage-2 handling cleanup | expand |
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@linaro.org> > --- > 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 >
Hi Stefano, On 7 December 2017 at 22:43, Stefano Stabellini <sstabellini@kernel.org> wrote: > On Thu, 23 Nov 2017, Julien Grall wrote: >> @@ -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, > Hmmm, yes it should. I will resend the series minus the one you would merge. Cheers,
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, .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:
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@linaro.org> --- xen/arch/arm/traps.c | 133 ++++++++++++++++----------------------------------- 1 file changed, 41 insertions(+), 92 deletions(-)