Re: [Xen-devel] [RFC 05/22] xen/arm: traps: Move MMIO emulation code in a separate helper

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 28 Jul 2016, Julien Grall wrote:
> Currently, a stage-2 fault translation will likely access an emulated
> region. All the checks are pre-sanitity check for MMIO emulation.
> 
> A follow-up patch will handle a new case that could lead to a stage-2
> translation. To improve the clarity of the code and the changes, the
> current implementation is move in a separate helper.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> ---
>  xen/arch/arm/traps.c | 58 
> ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 33 insertions(+), 25 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 46e0663..b46284c 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2444,6 +2444,38 @@ static void do_trap_instr_abort_guest(struct 
> cpu_user_regs *regs,
>      inject_iabt_exception(regs, gva, hsr.len);
>  }
>  
> +static bool_t try_handle_mmio(struct cpu_user_regs *regs,
> +                              mmio_info_t *info)
> +{
> +    const struct hsr_dabt dabt = info->dabt;
> +    int rc;
> +
> +    /* stage-1 page table should never live in an emulated MMIO region */
> +    if ( dabt.s1ptw )
> +        return 0;
> +
> +    /* All the instructions used on emulated MMIO region should be valid */
> +    if ( !dabt.valid )
> +        return 0;
> +
> +    /*
> +     * Erratum 766422: Thumb store translation fault to Hypervisor may
> +     * not have correct HSR Rt value.
> +     */
> +    if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
> +         dabt.write )
> +    {
> +        rc = decode_instruction(regs, &info->dabt);
> +        if ( rc )
> +        {
> +            gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> +            return 0;
> +        }
> +    }
> +
> +    return !!handle_mmio(info);
> +}
> +
>  static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>                                       const union hsr hsr)
>  {
> @@ -2487,40 +2519,16 @@ static void do_trap_data_abort_guest(struct 
> cpu_user_regs *regs,
>          break;
>      }
>      case FSC_FLT_TRANS:
> -        if ( dabt.s1ptw )
> -            goto bad_data_abort;
> -
> -        /* XXX: Decode the instruction if ISS is not valid */
> -        if ( !dabt.valid )
> -            goto bad_data_abort;
> -
> -        /*
> -         * Erratum 766422: Thumb store translation fault to Hypervisor may
> -         * not have correct HSR Rt value.
> -         */
> -        if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
> -             dabt.write )
> -        {
> -            rc = decode_instruction(regs, &info.dabt);
> -            if ( rc )
> -            {
> -                gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> -                goto bad_data_abort;
> -            }
> -        }
> -
> -        if ( handle_mmio(&info) )
> +        if ( try_handle_mmio(regs, &info) )
>          {
>              advance_pc(regs, hsr);
>              return;
>          }
> -        break;

I would keep this break, because we don't want to print the warning
below in all error cases, such as !dabt.valid.

Aside from the break removal:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


>      default:
>          gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n",
>                  hsr.bits, dabt.dfsc);
>      }
>  
> -bad_data_abort:
>      gdprintk(XENLOG_DEBUG, "HSR=0x%x pc=%#"PRIregister" gva=%#"PRIvaddr
>               " gpa=%#"PRIpaddr"\n", hsr.bits, regs->pc, info.gva, info.gpa);
>      inject_dabt_exception(regs, info.gva, hsr.len);
> -- 
> 1.9.1
> 

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