Re: [Xen-devel] [PATCH v3 2/4] xen/arm: io: Distinguish unhandled IO from aborted one

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 02/02/18 10:14, Julien Grall wrote:
> Currently, Xen is considering that an IO could either be handled or
> unhandled. When unhandled, the stage-2 abort function will try another
> way to resolve the abort.
> 
> However, the MMIO emulation may return unhandled when the address
> belongs to an emulated range but was not correct. In that case, Xen
> should avoid to try another way and directly inject a guest data abort.
> 
> Introduce a tri-state return to distinguish the following state:
>     * IO_ABORT: The IO was handled but resulted in an abort
>     * IO_HANDLED: The IO was handled
>     * IO_UNHANDLED: The IO was unhandled

I like that very much!

> 
> For now, it is considered that an IO belonging to an emulated range
> could either be handled or inject an abort. This could be revisit in the
> future if overlapped region exist (or we want to try another way to
> resolve the abort).
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> 
> ---
>     Changes in v2:
>         - Always return IO_ABORT when the check failed because we know
>         it was targeted emulated IO.
>         - Fix typoes
> ---
>  xen/arch/arm/io.c          | 32 ++++++++++++++++++--------------
>  xen/arch/arm/traps.c       | 18 +++++++++++++++---
>  xen/include/asm-arm/mmio.h | 13 ++++++++++---
>  3 files changed, 43 insertions(+), 20 deletions(-)
> 
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index c3e9239ffe..1f4cb8f37d 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -26,8 +26,9 @@
>  
>  #include "decode.h"
>  
> -static int handle_read(const struct mmio_handler *handler, struct vcpu *v,
> -                       mmio_info_t *info)
> +static enum io_state handle_read(const struct mmio_handler *handler,
> +                                 struct vcpu *v,
> +                                 mmio_info_t *info)
>  {
>      const struct hsr_dabt dabt = info->dabt;
>      struct cpu_user_regs *regs = guest_cpu_user_regs();
> @@ -40,7 +41,7 @@ static int handle_read(const struct mmio_handler *handler, 
> struct vcpu *v,
>      uint8_t size = (1 << dabt.size) * 8;
>  
>      if ( !handler->ops->read(v, info, &r, handler->priv) )
> -        return 0;
> +        return IO_ABORT;
>  
>      /*
>       * Sign extend if required.
> @@ -60,17 +61,20 @@ static int handle_read(const struct mmio_handler 
> *handler, struct vcpu *v,
>  
>      set_user_reg(regs, dabt.reg, r);
>  
> -    return 1;
> +    return IO_HANDLED;
>  }
>  
> -static int handle_write(const struct mmio_handler *handler, struct vcpu *v,
> -                        mmio_info_t *info)
> +static enum io_state handle_write(const struct mmio_handler *handler,
> +                                  struct vcpu *v,
> +                                  mmio_info_t *info)
>  {
>      const struct hsr_dabt dabt = info->dabt;
>      struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    int ret;
>  
> -    return handler->ops->write(v, info, get_user_reg(regs, dabt.reg),
> -                               handler->priv);
> +    ret = handler->ops->write(v, info, get_user_reg(regs, dabt.reg),
> +                              handler->priv);
> +    return ( ret ) ? IO_HANDLED : IO_ABORT;

Why the brackets?

Otherwise:

Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>

Cheers,
Andre.

>  }
>  
>  /* This function assumes that mmio regions are not overlapped */
> @@ -103,9 +107,9 @@ static const struct mmio_handler 
> *find_mmio_handler(struct domain *d,
>      return handler;
>  }
>  
> -int try_handle_mmio(struct cpu_user_regs *regs,
> -                    const union hsr hsr,
> -                    paddr_t gpa)
> +enum io_state 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;
> @@ -119,11 +123,11 @@ int try_handle_mmio(struct cpu_user_regs *regs,
>  
>      handler = find_mmio_handler(v->domain, info.gpa);
>      if ( !handler )
> -        return 0;
> +        return IO_UNHANDLED;
>  
>      /* All the instructions used on emulated MMIO region should be valid */
>      if ( !dabt.valid )
> -        return 0;
> +        return IO_ABORT;
>  
>      /*
>       * Erratum 766422: Thumb store translation fault to Hypervisor may
> @@ -138,7 +142,7 @@ int try_handle_mmio(struct cpu_user_regs *regs,
>          if ( rc )
>          {
>              gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> -            return 0;
> +            return IO_ABORT;
>          }
>      }
>  
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 2f8d790bb3..1e85f99ec1 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1964,10 +1964,21 @@ static void do_trap_stage2_abort_guest(struct 
> cpu_user_regs *regs,
>           *
>           * Note that emulated region cannot be executed
>           */
> -        if ( is_data && try_handle_mmio(regs, hsr, gpa) )
> +        if ( is_data )
>          {
> -            advance_pc(regs, hsr);
> -            return;
> +            enum io_state state = try_handle_mmio(regs, hsr, gpa);
> +
> +            switch ( state )
> +            {
> +            case IO_ABORT:
> +                goto inject_abt;
> +            case IO_HANDLED:
> +                advance_pc(regs, hsr);
> +                return;
> +            case IO_UNHANDLED:
> +                /* IO unhandled, try another way to handle it. */
> +                break;
> +            }
>          }
>  
>          /*
> @@ -1988,6 +1999,7 @@ static void do_trap_stage2_abort_guest(struct 
> cpu_user_regs *regs,
>                  hsr.bits, xabt.fsc);
>      }
>  
> +inject_abt:
>      gdprintk(XENLOG_DEBUG, "HSR=0x%x pc=%#"PRIregister" gva=%#"PRIvaddr
>               " gpa=%#"PRIpaddr"\n", hsr.bits, regs->pc, gva, gpa);
>      if ( is_data )
> diff --git a/xen/include/asm-arm/mmio.h b/xen/include/asm-arm/mmio.h
> index c941073257..c8dadb5006 100644
> --- a/xen/include/asm-arm/mmio.h
> +++ b/xen/include/asm-arm/mmio.h
> @@ -32,6 +32,13 @@ typedef struct
>      paddr_t gpa;
>  } mmio_info_t;
>  
> +enum io_state
> +{
> +    IO_ABORT,       /* The IO was handled by the helper and led to an abort. 
> */
> +    IO_HANDLED,     /* The IO was successfully handled by the helper. */
> +    IO_UNHANDLED,   /* The IO was not handled by the helper. */
> +};
> +
>  typedef int (*mmio_read_t)(struct vcpu *v, mmio_info_t *info,
>                             register_t *r, void *priv);
>  typedef int (*mmio_write_t)(struct vcpu *v, mmio_info_t *info,
> @@ -56,9 +63,9 @@ struct vmmio {
>      struct mmio_handler *handlers;
>  };
>  
> -int try_handle_mmio(struct cpu_user_regs *regs,
> -                    const union hsr hsr,
> -                    paddr_t gpa);
> +enum io_state 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel