| Message ID | 20180130161433.6910-2-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm: Inject an exception to the guest rather than crashing it | expand |
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@arm.com> > --- > 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)? > - return !!handle_mmio(&info); > + return handle_mmio(&info); > } > > /* > @@ -2005,10 +2005,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: > + /* Nothing to do */ > + break; > + } > } > > /* > @@ -2029,6 +2040,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 37e2b7a707..baaecf6931 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 lead to an abort. */ ^ led > + 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,7 +63,7 @@ struct vmmio { > struct mmio_handler *handlers; > }; > > -extern int handle_mmio(mmio_info_t *info); > +extern enum io_state handle_mmio(mmio_info_t *info); > void register_mmio_handler(struct domain *d, > const struct mmio_handler_ops *ops, > paddr_t addr, paddr_t size, void *priv); > -- > 2.11.0 >
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@arm.com> >> --- >> 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,
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@arm.com> > > > --- > > > 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
Hi Stefano, On 30/01/18 19:09, Stefano Stabellini wrote: > On Tue, 30 Jan 2018, Julien Grall wrote: >>>> 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 Actually, I have said something completely wrong yesterday. Must have been too tired :/. At the time you call try_handle_mmio, you still don't know whether the fault was because of accessing an emulated MMIO region. You will only be sure when find_mmio_handler() has returned a non-NULL pointer (see handle_mmio()). So returning IO_UNHANDLED here is correct as you want to try another method to handle the fault. However, it also means that even bad access to emulated region will result to fallback on another method. While this should not be issue, I don't think this is future proof (I am mostly worry on the ACPI case where MMIO are mapped on-demand). So I will send a patch to fold try_handle_mmio() into handle_mmio(). > > >> 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 I will do that in a patch that fold try_handle_mmio() in handle_mmio(). Cheers,
On Wed, 31 Jan 2018, Julien Grall wrote: > Hi Stefano, > > On 30/01/18 19:09, Stefano Stabellini wrote: > > On Tue, 30 Jan 2018, Julien Grall wrote: > > > > > 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 > > Actually, I have said something completely wrong yesterday. Must have been too > tired :/. > > At the time you call try_handle_mmio, you still don't know whether the fault > was because of accessing an emulated MMIO region. You will only be sure when > find_mmio_handler() has returned a non-NULL pointer (see handle_mmio()). > > So returning IO_UNHANDLED here is correct as you want to try another method to > handle the fault. > > However, it also means that even bad access to emulated region will result to > fallback on another method. While this should not be issue, I don't think this > is future proof (I am mostly worry on the ACPI case where MMIO are mapped > on-demand). > > So I will send a patch to fold try_handle_mmio() into handle_mmio(). All right > > > > > > > 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 > > I will do that in a patch that fold try_handle_mmio() in handle_mmio(). > > Cheers, > > -- > Julien Grall >
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; } } - return !!handle_mmio(&info); + return handle_mmio(&info); } /* @@ -2005,10 +2005,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: + /* Nothing to do */ + break; + } } /* @@ -2029,6 +2040,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 37e2b7a707..baaecf6931 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 lead 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,7 +63,7 @@ struct vmmio { struct mmio_handler *handlers; }; -extern int handle_mmio(mmio_info_t *info); +extern enum io_state handle_mmio(mmio_info_t *info); void register_mmio_handler(struct domain *d, const struct mmio_handler_ops *ops, paddr_t addr, paddr_t size, void *priv);
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. Introduce a tri-state return to distinguish the following state: * IO_ABORT: The IO was handled but resulted to 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@arm.com> --- 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(-)