Re: [Xen-devel] [RFC 01/22] xen/arm: do_trap_instr_abort_guest: Move the IPA computation out of the switch

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 28 Jul 2016, Julien Grall wrote:
> A follow-up patch will add more case to the switch that will require the
> IPA. So move the computation out of the switch.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> ---
>  xen/arch/arm/traps.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 683bcb2..46e0663 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2403,35 +2403,35 @@ static void do_trap_instr_abort_guest(struct 
> cpu_user_regs *regs,
>      int rc;
>      register_t gva = READ_SYSREG(FAR_EL2);
>      uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK;
> +    paddr_t gpa;
> +
> +    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();
> +
> +        rc = gva_to_ipa(gva, &gpa, GV2M_READ);
> +        if ( rc == -EFAULT )
> +            return; /* Try again */

The issue with this is that now for any cases that don't require a gpa
if gva_to_ipa fails we wrongly return -EFAULT.

I suggest having two switches or falling through from the first case to
the second.


> +    }
>  
>      switch ( fsc )
>      {
>      case FSC_FLT_PERM:
>      {
> -        paddr_t gpa;
>          const struct npfec npfec = {
>              .insn_fetch = 1,
>              .gla_valid = 1,
>              .kind = hsr.iabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
>          };
>  
> -        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();
> -
> -            rc = gva_to_ipa(gva, &gpa, GV2M_READ);
> -            if ( rc == -EFAULT )
> -                return; /* Try again */
> -        }
> -
>          rc = p2m_mem_access_check(gpa, gva, npfec);
>  
>          /* Trap was triggered by mem_access, work here is done */
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel