|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 1/4] xen/arm: traps: Merge try_handle_mmio() and handle_mmio()
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@xxxxxxx>
Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>
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);
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |