Hi,

On 02/02/18 10:14, Julien Grall wrote:
> At the moment, try_handle_mmio() will do check on the HSR and bail out
> if one check fail. This means that another method will be tried to
> handle the fault even for bad access on emulated region. While this
> should not be an issue, this is not future proof.
> 
> Move the checks of try_handle_mmio() in handle_mmio() after we identified
> the fault to target an emulated MMIO. While this does not fix the potential
> fall-through, a follow-up patch will do by distinguish the potential error.
> 
> Note that the handle_mmio() was renamed to try_handle_mmio() and the
> prototype adapted.

Why is that? I think the prefix "try_" only makes sense if you have a
non-try version as well. To some degree most functions "try" something,
when they check for and return errors.
I think the return type makes it clear what the semantics are.
So personally I would prefer just "handle_mmio" as the function name.

But this only a nit, definitely not worth a respin on its own.

> While merging the 2 functions, remove the check whether the fault is
> stage-2 abort on stage-1 translation walk because the instruction
> syndrome will always be invalid (see B3-1433 in DDI 0406C.c and
> D10-2460 in DDI 0487C.a).

Yes, that looks correct to me.

> Signed-off-by: Julien Grall <julien.gr...@arm.com>

Reviewed-by: Andre Przywara <andre.przyw...@arm.com>

Cheers,
Andre.

> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/io.c          | 43 ++++++++++++++++++++++++++++++++++++++-----
>  xen/arch/arm/traps.c       | 41 -----------------------------------------
>  xen/include/asm-arm/mmio.h |  4 +++-
>  3 files changed, 41 insertions(+), 47 deletions(-)
> 
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index c748d8f5bf..c3e9239ffe 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -20,9 +20,12 @@
>  #include <xen/spinlock.h>
>  #include <xen/sched.h>
>  #include <xen/sort.h>
> +#include <asm/cpuerrata.h>
>  #include <asm/current.h>
>  #include <asm/mmio.h>
>  
> +#include "decode.h"
> +
>  static int handle_read(const struct mmio_handler *handler, struct vcpu *v,
>                         mmio_info_t *info)
>  {
> @@ -100,19 +103,49 @@ static const struct mmio_handler 
> *find_mmio_handler(struct domain *d,
>      return handler;
>  }
>  
> -int handle_mmio(mmio_info_t *info)
> +int try_handle_mmio(struct cpu_user_regs *regs,
> +                    const union hsr hsr,
> +                    paddr_t gpa)
>  {
>      struct vcpu *v = current;
>      const struct mmio_handler *handler = NULL;
> +    const struct hsr_dabt dabt = hsr.dabt;
> +    mmio_info_t info = {
> +        .gpa = gpa,
> +        .dabt = dabt
> +    };
> +
> +    ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
>  
> -    handler = find_mmio_handler(v->domain, info->gpa);
> +    handler = find_mmio_handler(v->domain, info.gpa);
>      if ( !handler )
>          return 0;
>  
> -    if ( info->dabt.write )
> -        return handle_write(handler, v, info);
> +    /* 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 )
> +    {
> +        int rc;
> +
> +        rc = decode_instruction(regs, &info.dabt);
> +        if ( rc )
> +        {
> +            gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> +            return 0;
> +        }
> +    }
> +
> +    if ( info.dabt.write )
> +        return handle_write(handler, v, &info);
>      else
> -        return handle_read(handler, v, info);
> +        return handle_read(handler, v, &info);
>  }
>  
>  void register_mmio_handler(struct domain *d,
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index c8534d6cff..2f8d790bb3 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -51,8 +51,6 @@
>  #include <asm/vgic.h>
>  #include <asm/vtimer.h>
>  
> -#include "decode.h"
> -
>  /* The base of the stack must always be double-word aligned, which means
>   * that both the kernel half of struct cpu_user_regs (which is pushed in
>   * entry.S) and struct cpu_info (which lives at the bottom of a Xen
> @@ -1864,45 +1862,6 @@ static inline bool hpfar_is_valid(bool s1ptw, uint8_t 
> fsc)
>      return s1ptw || (fsc == FSC_FLT_TRANS && !check_workaround_834220());
>  }
>  
> -static bool try_handle_mmio(struct cpu_user_regs *regs,
> -                            const union hsr hsr,
> -                            paddr_t gpa)
> -{
> -    const struct hsr_dabt dabt = hsr.dabt;
> -    mmio_info_t info = {
> -        .gpa = gpa,
> -        .dabt = dabt
> -    };
> -    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;
> -
> -    /* All the instructions used on emulated MMIO region should be valid */
> -    if ( !dabt.valid )
> -        return false;
> -
> -    /*
> -     * 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 false;
> -        }
> -    }
> -
> -    return !!handle_mmio(&info);
> -}
> -
>  /*
>   * When using ACPI, most of the MMIO regions will be mapped on-demand
>   * in stage-2 page tables for the hardware domain because Xen is not
> diff --git a/xen/include/asm-arm/mmio.h b/xen/include/asm-arm/mmio.h
> index 37e2b7a707..c941073257 100644
> --- a/xen/include/asm-arm/mmio.h
> +++ b/xen/include/asm-arm/mmio.h
> @@ -56,7 +56,9 @@ struct vmmio {
>      struct mmio_handler *handlers;
>  };
>  
> -extern int handle_mmio(mmio_info_t *info);
> +int try_handle_mmio(struct cpu_user_regs *regs,
> +                    const union hsr hsr,
> +                    paddr_t gpa);
>  void register_mmio_handler(struct domain *d,
>                             const struct mmio_handler_ops *ops,
>                             paddr_t addr, paddr_t size, void *priv);
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Reply via email to