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

[Julien Grall <julien.grall@xxxxxxx>]

On 02/02/18 14:34, Andre Przywara wrote:
> Hi,
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?
I was not feeling lazy that day ;). I will drop them.

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

--
Julien Grall

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