| Message ID | 20180202101444.3510-2-julien.grall@arm.com |
|---|---|
| State | Accepted |
| Commit | 38d815cae108b70f35c27e3a6cdf5700a4d00f34 |
| Headers | show |
| Series | xen/arm: Inject an exception to the guest rather than crashing it | expand |
Hi, On 02/02/18 10:14, Julien Grall wrote: > At the moment, try_handle_mmio() will do check on the HSR and bail out > if one check fail. This means that another method will be tried to > handle the fault even for bad access on emulated region. While this > should not be an issue, this is not future proof. > > Move the checks of try_handle_mmio() in handle_mmio() after we identified > the fault to target an emulated MMIO. While this does not fix the potential > fall-through, a follow-up patch will do by distinguish the potential error. > > Note that the handle_mmio() was renamed to try_handle_mmio() and the > prototype adapted. Why is that? I think the prefix "try_" only makes sense if you have a non-try version as well. To some degree most functions "try" something, when they check for and return errors. I think the return type makes it clear what the semantics are. So personally I would prefer just "handle_mmio" as the function name. But this only a nit, definitely not worth a respin on its own. > While merging the 2 functions, remove the check whether the fault is > stage-2 abort on stage-1 translation walk because the instruction > syndrome will always be invalid (see B3-1433 in DDI 0406C.c and > D10-2460 in DDI 0487C.a). Yes, that looks correct to me. > Signed-off-by: Julien Grall <julien.grall@arm.com> Reviewed-by: Andre Przywara <andre.przywara@arm.com> Cheers, Andre. > --- > Changes in v2: > - Patch added > --- > xen/arch/arm/io.c | 43 ++++++++++++++++++++++++++++++++++++++----- > xen/arch/arm/traps.c | 41 ----------------------------------------- > xen/include/asm-arm/mmio.h | 4 +++- > 3 files changed, 41 insertions(+), 47 deletions(-) > > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c > index c748d8f5bf..c3e9239ffe 100644 > --- a/xen/arch/arm/io.c > +++ b/xen/arch/arm/io.c > @@ -20,9 +20,12 @@ > #include <xen/spinlock.h> > #include <xen/sched.h> > #include <xen/sort.h> > +#include <asm/cpuerrata.h> > #include <asm/current.h> > #include <asm/mmio.h> > > +#include "decode.h" > + > static int handle_read(const struct mmio_handler *handler, struct vcpu *v, > mmio_info_t *info) > { > @@ -100,19 +103,49 @@ static const struct mmio_handler *find_mmio_handler(struct domain *d, > return handler; > } > > -int handle_mmio(mmio_info_t *info) > +int 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; > + const struct hsr_dabt dabt = hsr.dabt; > + mmio_info_t info = { > + .gpa = gpa, > + .dabt = dabt > + }; > + > + ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL); > > - handler = find_mmio_handler(v->domain, info->gpa); > + handler = find_mmio_handler(v->domain, info.gpa); > if ( !handler ) > return 0; > > - if ( info->dabt.write ) > - return handle_write(handler, v, info); > + /* All the instructions used on emulated MMIO region should be valid */ > + if ( !dabt.valid ) > + return 0; > + > + /* > + * Erratum 766422: Thumb store translation fault to Hypervisor may > + * not have correct HSR Rt value. > + */ > + if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) && > + dabt.write ) > + { > + int rc; > + > + rc = decode_instruction(regs, &info.dabt); > + if ( rc ) > + { > + gprintk(XENLOG_DEBUG, "Unable to decode instruction\n"); > + return 0; > + } > + } > + > + if ( info.dabt.write ) > + return handle_write(handler, v, &info); > else > - return handle_read(handler, v, info); > + return handle_read(handler, v, &info); > } > > void register_mmio_handler(struct domain *d, > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index c8534d6cff..2f8d790bb3 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -51,8 +51,6 @@ > #include <asm/vgic.h> > #include <asm/vtimer.h> > > -#include "decode.h" > - > /* The base of the stack must always be double-word aligned, which means > * that both the kernel half of struct cpu_user_regs (which is pushed in > * entry.S) and struct cpu_info (which lives at the bottom of a Xen > @@ -1864,45 +1862,6 @@ 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) > -{ > - const struct hsr_dabt dabt = hsr.dabt; > - mmio_info_t info = { > - .gpa = gpa, > - .dabt = dabt > - }; > - int rc; > - > - ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL); > - > - /* stage-1 page table should never live in an emulated MMIO region */ > - if ( dabt.s1ptw ) > - return false; > - > - /* All the instructions used on emulated MMIO region should be valid */ > - if ( !dabt.valid ) > - return false; > - > - /* > - * Erratum 766422: Thumb store translation fault to Hypervisor may > - * not have correct HSR Rt value. > - */ > - if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) && > - dabt.write ) > - { > - rc = decode_instruction(regs, &info.dabt); > - if ( rc ) > - { > - gprintk(XENLOG_DEBUG, "Unable to decode instruction\n"); > - return false; > - } > - } > - > - return !!handle_mmio(&info); > -} > - > /* > * When using ACPI, most of the MMIO regions will be mapped on-demand > * in stage-2 page tables for the hardware domain because Xen is not > diff --git a/xen/include/asm-arm/mmio.h b/xen/include/asm-arm/mmio.h > index 37e2b7a707..c941073257 100644 > --- a/xen/include/asm-arm/mmio.h > +++ b/xen/include/asm-arm/mmio.h > @@ -56,7 +56,9 @@ struct vmmio { > struct mmio_handler *handlers; > }; > > -extern int handle_mmio(mmio_info_t *info); > +int 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); >
On 02/02/18 14:34, Andre Przywara wrote: > Hi, Hi, > On 02/02/18 10:14, Julien Grall wrote: >> At the moment, try_handle_mmio() will do check on the HSR and bail out >> if one check fail. This means that another method will be tried to >> handle the fault even for bad access on emulated region. While this >> should not be an issue, this is not future proof. >> >> Move the checks of try_handle_mmio() in handle_mmio() after we identified >> the fault to target an emulated MMIO. While this does not fix the potential >> fall-through, a follow-up patch will do by distinguish the potential error. >> >> Note that the handle_mmio() was renamed to try_handle_mmio() and the >> prototype adapted. > > Why is that? I think the prefix "try_" only makes sense if you have a > non-try version as well. To some degree most functions "try" something, > when they check for and return errors. > I think the return type makes it clear what the semantics are. > So personally I would prefer just "handle_mmio" as the function name. Because we have another function called try_map_mmio() just below, so I wanted to keep similar. Also, I think "try_" makes sense here because if you don't succeed, you will fallback to another method. Most of the times, this is not the case of other functions. > > But this only a nit, definitely not worth a respin on its own. > >> While merging the 2 functions, remove the check whether the fault is >> stage-2 abort on stage-1 translation walk because the instruction >> syndrome will always be invalid (see B3-1433 in DDI 0406C.c and >> D10-2460 in DDI 0487C.a). > > Yes, that looks correct to me. > >> Signed-off-by: Julien Grall <julien.grall@arm.com> > > Reviewed-by: Andre Przywara <andre.przywara@arm.com> Thanks! Cheers,
Hi, On 02/02/18 14:47, Julien Grall wrote: > > > On 02/02/18 14:34, Andre Przywara wrote: >> Hi, > > Hi, > >> On 02/02/18 10:14, Julien Grall wrote: >>> At the moment, try_handle_mmio() will do check on the HSR and bail out >>> if one check fail. This means that another method will be tried to >>> handle the fault even for bad access on emulated region. While this >>> should not be an issue, this is not future proof. >>> >>> Move the checks of try_handle_mmio() in handle_mmio() after we >>> identified >>> the fault to target an emulated MMIO. While this does not fix the >>> potential >>> fall-through, a follow-up patch will do by distinguish the potential >>> error. >>> >>> Note that the handle_mmio() was renamed to try_handle_mmio() and the >>> prototype adapted. >> >> Why is that? I think the prefix "try_" only makes sense if you have a >> non-try version as well. To some degree most functions "try" something, >> when they check for and return errors. >> I think the return type makes it clear what the semantics are. >> So personally I would prefer just "handle_mmio" as the function name. > Because we have another function called try_map_mmio() just below, so I > wanted to keep similar. > > Also, I think "try_" makes sense here because if you don't succeed, you > will fallback to another method. Most of the times, this is not the case > of other functions. Yeah, fair enough. Fine for me, then. Cheers, Andre. >> But this only a nit, definitely not worth a respin on its own. >> >>> While merging the 2 functions, remove the check whether the fault is >>> stage-2 abort on stage-1 translation walk because the instruction >>> syndrome will always be invalid (see B3-1433 in DDI 0406C.c and >>> D10-2460 in DDI 0487C.a). >> >> Yes, that looks correct to me. >> >>> Signed-off-by: Julien Grall <julien.grall@arm.com> >> >> Reviewed-by: Andre Przywara <andre.przywara@arm.com> > > Thanks! > > Cheers, >
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c index c748d8f5bf..c3e9239ffe 100644 --- a/xen/arch/arm/io.c +++ b/xen/arch/arm/io.c @@ -20,9 +20,12 @@ #include <xen/spinlock.h> #include <xen/sched.h> #include <xen/sort.h> +#include <asm/cpuerrata.h> #include <asm/current.h> #include <asm/mmio.h> +#include "decode.h" + static int handle_read(const struct mmio_handler *handler, struct vcpu *v, mmio_info_t *info) { @@ -100,19 +103,49 @@ static const struct mmio_handler *find_mmio_handler(struct domain *d, return handler; } -int handle_mmio(mmio_info_t *info) +int 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; + const struct hsr_dabt dabt = hsr.dabt; + mmio_info_t info = { + .gpa = gpa, + .dabt = dabt + }; + + ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL); - handler = find_mmio_handler(v->domain, info->gpa); + handler = find_mmio_handler(v->domain, info.gpa); if ( !handler ) return 0; - if ( info->dabt.write ) - return handle_write(handler, v, info); + /* All the instructions used on emulated MMIO region should be valid */ + if ( !dabt.valid ) + return 0; + + /* + * Erratum 766422: Thumb store translation fault to Hypervisor may + * not have correct HSR Rt value. + */ + if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) && + dabt.write ) + { + int rc; + + rc = decode_instruction(regs, &info.dabt); + if ( rc ) + { + gprintk(XENLOG_DEBUG, "Unable to decode instruction\n"); + return 0; + } + } + + if ( info.dabt.write ) + return handle_write(handler, v, &info); else - return handle_read(handler, v, info); + return handle_read(handler, v, &info); } void register_mmio_handler(struct domain *d, diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index c8534d6cff..2f8d790bb3 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -51,8 +51,6 @@ #include <asm/vgic.h> #include <asm/vtimer.h> -#include "decode.h" - /* The base of the stack must always be double-word aligned, which means * that both the kernel half of struct cpu_user_regs (which is pushed in * entry.S) and struct cpu_info (which lives at the bottom of a Xen @@ -1864,45 +1862,6 @@ 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) -{ - const struct hsr_dabt dabt = hsr.dabt; - mmio_info_t info = { - .gpa = gpa, - .dabt = dabt - }; - int rc; - - ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL); - - /* stage-1 page table should never live in an emulated MMIO region */ - if ( dabt.s1ptw ) - return false; - - /* All the instructions used on emulated MMIO region should be valid */ - if ( !dabt.valid ) - return false; - - /* - * Erratum 766422: Thumb store translation fault to Hypervisor may - * not have correct HSR Rt value. - */ - if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) && - dabt.write ) - { - rc = decode_instruction(regs, &info.dabt); - if ( rc ) - { - gprintk(XENLOG_DEBUG, "Unable to decode instruction\n"); - return false; - } - } - - return !!handle_mmio(&info); -} - /* * When using ACPI, most of the MMIO regions will be mapped on-demand * in stage-2 page tables for the hardware domain because Xen is not diff --git a/xen/include/asm-arm/mmio.h b/xen/include/asm-arm/mmio.h index 37e2b7a707..c941073257 100644 --- a/xen/include/asm-arm/mmio.h +++ b/xen/include/asm-arm/mmio.h @@ -56,7 +56,9 @@ struct vmmio { struct mmio_handler *handlers; }; -extern int handle_mmio(mmio_info_t *info); +int 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);
At the moment, try_handle_mmio() will do check on the HSR and bail out if one check fail. This means that another method will be tried to handle the fault even for bad access on emulated region. While this should not be an issue, this is not future proof. Move the checks of try_handle_mmio() in handle_mmio() after we identified the fault to target an emulated MMIO. While this does not fix the potential fall-through, a follow-up patch will do by distinguish the potential error. Note that the handle_mmio() was renamed to try_handle_mmio() and the prototype adapted. While merging the 2 functions, remove the check whether the fault is stage-2 abort on stage-1 translation walk because the instruction syndrome will always be invalid (see B3-1433 in DDI 0406C.c and D10-2460 in DDI 0487C.a). Signed-off-by: Julien Grall <julien.grall@arm.com> --- Changes in v2: - Patch added --- xen/arch/arm/io.c | 43 ++++++++++++++++++++++++++++++++++++++----- xen/arch/arm/traps.c | 41 ----------------------------------------- xen/include/asm-arm/mmio.h | 4 +++- 3 files changed, 41 insertions(+), 47 deletions(-)