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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 30/01/18 18:14, Stefano Stabellini wrote:
> On Tue, 30 Jan 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 avodi to try another way and directly inject a guest data abort.
>           ^ avoid
>
>
> > Introduce a tri-state return to distinguish the following state:
> >      * IO_ABORT: The IO was handled but resulted to an abort
>                                                    ^ 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>
> > ---
> >   xen/arch/arm/io.c          | 24 ++++++++++++++----------
> >   xen/arch/arm/traps.c       | 34 +++++++++++++++++++++++-----------
> >   xen/include/asm-arm/mmio.h |  9 ++++++++-
> >   3 files changed, 45 insertions(+), 22 deletions(-)
> >
> > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> > index c748d8f5bf..a74ec21b86 100644
> > --- a/xen/arch/arm/io.c
> > +++ b/xen/arch/arm/io.c
> > @@ -23,8 +23,9 @@
> >   #include <asm/current.h>
> >   #include <asm/mmio.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();
> > @@ -37,7 +38,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.
> > @@ -57,17 +58,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 */
> > @@ -100,14 +104,14 @@ static const struct mmio_handler 
> > *find_mmio_handler(struct domain *d,
> >       return handler;
> >   }
> >   
> > -int handle_mmio(mmio_info_t *info)
> > +enum io_state handle_mmio(mmio_info_t *info)
> >   {
> >       struct vcpu *v = current;
> >       const struct mmio_handler *handler = NULL;
> >   
> >       handler = find_mmio_handler(v->domain, info->gpa);
> >       if ( !handler )
> > -        return 0;
> > +        return IO_UNHANDLED;
> >   
> >       if ( info->dabt.write )
> >           return handle_write(handler, v, info);
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index c8534d6cff..843adf4959 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1864,10 +1864,10 @@ 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)
> > -{
> > +static enum io_state 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,
> > @@ -1879,11 +1879,11 @@ static bool try_handle_mmio(struct cpu_user_regs *regs,
> >   
> >       /* stage-1 page table should never live in an emulated MMIO region */
> >       if ( dabt.s1ptw )
> > -        return false;
> > +        return IO_UNHANDLED;
> >   
> >       /* All the instructions used on emulated MMIO region should be valid */
> >       if ( !dabt.valid )
> > -        return false;
> > +        return IO_UNHANDLED;
> >   
> >       /*
> >        * Erratum 766422: Thumb store translation fault to Hypervisor may
> > @@ -1896,11 +1896,11 @@ static bool try_handle_mmio(struct cpu_user_regs *regs,
> >           if ( rc )
> >           {
> >               gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> > -            return false;
> > +            return IO_ABORT;
> >           }
> >       }
> Why do the first two error checks result in IO_UNHANDLED, while the
> third result in IO_ABORT? Specifically in relation to pagetable walk
> failures due to someone else changing stage-2 entry simultaneously (see
> comment toward the end of do_trap_stage2_abort_guest)?
Good question. Somehow I considered the first two as part of looking up 
the proper handler and not the device itself.
But I guess that at this stage we know that IO was targeting an emulated 
region. So we can return IO_ABORT.
On a side note, it looks like the check dabt.s1ptw is unnecessary 
because a stage 2 abort on stage 1 translation table lookup will not 
return a valid instruction syndrome (see B3-1433 in DDI 0406C.c and 
D10-2460 in DDI 0487C.a).
Cheers,

--
Julien Grall

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