[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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.