|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/3] xen/arm: io: Distinguish unhandled IO from aborted one
On Tue, 30 Jan 2018, Julien Grall wrote:
> 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.
That is what I thought as well
> 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).
in that case, go ahead and remove it as part of this patch, mention it
in the commit message
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |