Re: [Xen-devel] [PATCH v2 1/3] xen/arm: io: Distinguish unhandled IO from aborted one

[Julien Grall <julien.grall@xxxxxxxxxx>]

(sorry for the formatting)

On 1 Feb 2018 19:05, "Stefano Stabellini" <sstabellini@xxxxxxxxxx> wrote:

On Thu, 1 Feb 2018, 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
>
> 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;
>  }
>
>  /* 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;

Did you forget to send a patch? Or is there a prerequisite for this
series? This patch doesn't apply.

Yes, I messed up my git format-patch command line.

The series is meant to have 4 patches.

Sorry for that. I will resend it tomorrow.

Cheers,

>      /* 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);
> --
> 2.11.0
>

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

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