* [PATCH v3 1/4] xen/arm: traps: Merge try_handle_mmio() and handle_mmio()
2018-02-02 10:14 [PATCH v3 0/4] xen/arm: Inject an exception to the guest rather than crashing it Julien Grall
@ 2018-02-02 10:14 ` Julien Grall
2018-02-02 14:34 ` Andre Przywara
2018-02-02 10:14 ` [PATCH v3 2/4] xen/arm: io: Distinguish unhandled IO from aborted one Julien Grall
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Julien Grall @ 2018-02-02 10:14 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, Julien Grall, sstabellini, andre.przywara
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(-)
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);
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 1/4] xen/arm: traps: Merge try_handle_mmio() and handle_mmio()
2018-02-02 10:14 ` [PATCH v3 1/4] xen/arm: traps: Merge try_handle_mmio() and handle_mmio() Julien Grall
@ 2018-02-02 14:34 ` Andre Przywara
2018-02-02 14:47 ` Julien Grall
0 siblings, 1 reply; 16+ messages in thread
From: Andre Przywara @ 2018-02-02 14:34 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: andrew.cooper3, sstabellini
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);
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 1/4] xen/arm: traps: Merge try_handle_mmio() and handle_mmio()
2018-02-02 14:34 ` Andre Przywara
@ 2018-02-02 14:47 ` Julien Grall
2018-02-02 15:15 ` Andre Przywara
0 siblings, 1 reply; 16+ messages in thread
From: Julien Grall @ 2018-02-02 14:47 UTC (permalink / raw)
To: Andre Przywara, xen-devel; +Cc: andrew.cooper3, sstabellini
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,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/4] xen/arm: traps: Merge try_handle_mmio() and handle_mmio()
2018-02-02 14:47 ` Julien Grall
@ 2018-02-02 15:15 ` Andre Przywara
0 siblings, 0 replies; 16+ messages in thread
From: Andre Przywara @ 2018-02-02 15:15 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: andrew.cooper3, sstabellini
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,
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 2/4] xen/arm: io: Distinguish unhandled IO from aborted one
2018-02-02 10:14 [PATCH v3 0/4] xen/arm: Inject an exception to the guest rather than crashing it Julien Grall
2018-02-02 10:14 ` [PATCH v3 1/4] xen/arm: traps: Merge try_handle_mmio() and handle_mmio() Julien Grall
@ 2018-02-02 10:14 ` Julien Grall
2018-02-02 14:34 ` Andre Przywara
2018-02-02 10:14 ` [PATCH v3 3/4] xen/arm: Don't crash domain on bad MMIO emulation Julien Grall
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Julien Grall @ 2018-02-02 10:14 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, Julien Grall, sstabellini, andre.przywara
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 avoid 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 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>
---
Changes in v2:
- Always return IO_ABORT when the check failed because we know
it was targeted emulated IO.
- Fix typoes
---
xen/arch/arm/io.c | 32 ++++++++++++++++++--------------
xen/arch/arm/traps.c | 18 +++++++++++++++---
xen/include/asm-arm/mmio.h | 13 ++++++++++---
3 files changed, 43 insertions(+), 20 deletions(-)
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index c3e9239ffe..1f4cb8f37d 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -26,8 +26,9 @@
#include "decode.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();
@@ -40,7 +41,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.
@@ -60,17 +61,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 */
@@ -103,9 +107,9 @@ static const struct mmio_handler *find_mmio_handler(struct domain *d,
return handler;
}
-int try_handle_mmio(struct cpu_user_regs *regs,
- const union hsr hsr,
- paddr_t gpa)
+enum io_state 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;
@@ -119,11 +123,11 @@ int try_handle_mmio(struct cpu_user_regs *regs,
handler = find_mmio_handler(v->domain, info.gpa);
if ( !handler )
- return 0;
+ return IO_UNHANDLED;
/* All the instructions used on emulated MMIO region should be valid */
if ( !dabt.valid )
- return 0;
+ return IO_ABORT;
/*
* Erratum 766422: Thumb store translation fault to Hypervisor may
@@ -138,7 +142,7 @@ int try_handle_mmio(struct cpu_user_regs *regs,
if ( rc )
{
gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
- return 0;
+ return IO_ABORT;
}
}
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 2f8d790bb3..1e85f99ec1 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1964,10 +1964,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:
+ /* IO unhandled, try another way to handle it. */
+ break;
+ }
}
/*
@@ -1988,6 +1999,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 c941073257..c8dadb5006 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 led 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,9 +63,9 @@ struct vmmio {
struct mmio_handler *handlers;
};
-int try_handle_mmio(struct cpu_user_regs *regs,
- const union hsr hsr,
- paddr_t gpa);
+enum io_state 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);
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 2/4] xen/arm: io: Distinguish unhandled IO from aborted one
2018-02-02 10:14 ` [PATCH v3 2/4] xen/arm: io: Distinguish unhandled IO from aborted one Julien Grall
@ 2018-02-02 14:34 ` Andre Przywara
2018-02-02 14:48 ` Julien Grall
0 siblings, 1 reply; 16+ messages in thread
From: Andre Przywara @ 2018-02-02 14:34 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: andrew.cooper3, sstabellini
Hi,
On 02/02/18 10:14, 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 avoid 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 in an abort
> * IO_HANDLED: The IO was handled
> * IO_UNHANDLED: The IO was unhandled
I like that very much!
>
> 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>
>
> ---
> Changes in v2:
> - Always return IO_ABORT when the check failed because we know
> it was targeted emulated IO.
> - Fix typoes
> ---
> xen/arch/arm/io.c | 32 ++++++++++++++++++--------------
> xen/arch/arm/traps.c | 18 +++++++++++++++---
> xen/include/asm-arm/mmio.h | 13 ++++++++++---
> 3 files changed, 43 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index c3e9239ffe..1f4cb8f37d 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -26,8 +26,9 @@
>
> #include "decode.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();
> @@ -40,7 +41,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.
> @@ -60,17 +61,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;
Why the brackets?
Otherwise:
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre.
> }
>
> /* This function assumes that mmio regions are not overlapped */
> @@ -103,9 +107,9 @@ static const struct mmio_handler *find_mmio_handler(struct domain *d,
> return handler;
> }
>
> -int try_handle_mmio(struct cpu_user_regs *regs,
> - const union hsr hsr,
> - paddr_t gpa)
> +enum io_state 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;
> @@ -119,11 +123,11 @@ int try_handle_mmio(struct cpu_user_regs *regs,
>
> handler = find_mmio_handler(v->domain, info.gpa);
> if ( !handler )
> - return 0;
> + return IO_UNHANDLED;
>
> /* All the instructions used on emulated MMIO region should be valid */
> if ( !dabt.valid )
> - return 0;
> + return IO_ABORT;
>
> /*
> * Erratum 766422: Thumb store translation fault to Hypervisor may
> @@ -138,7 +142,7 @@ int try_handle_mmio(struct cpu_user_regs *regs,
> if ( rc )
> {
> gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> - return 0;
> + return IO_ABORT;
> }
> }
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 2f8d790bb3..1e85f99ec1 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1964,10 +1964,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:
> + /* IO unhandled, try another way to handle it. */
> + break;
> + }
> }
>
> /*
> @@ -1988,6 +1999,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 c941073257..c8dadb5006 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 led 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,9 +63,9 @@ struct vmmio {
> struct mmio_handler *handlers;
> };
>
> -int try_handle_mmio(struct cpu_user_regs *regs,
> - const union hsr hsr,
> - paddr_t gpa);
> +enum io_state 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 2/4] xen/arm: io: Distinguish unhandled IO from aborted one
2018-02-02 14:34 ` Andre Przywara
@ 2018-02-02 14:48 ` Julien Grall
0 siblings, 0 replies; 16+ messages in thread
From: Julien Grall @ 2018-02-02 14:48 UTC (permalink / raw)
To: Andre Przywara, xen-devel; +Cc: andrew.cooper3, sstabellini
On 02/02/18 14:34, Andre Przywara wrote:
> Hi,
Hi,
> On 02/02/18 10:14, 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 avoid 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 in an abort
>> * IO_HANDLED: The IO was handled
>> * IO_UNHANDLED: The IO was unhandled
>
> I like that very much!
>
>>
>> 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>
>>
>> ---
>> Changes in v2:
>> - Always return IO_ABORT when the check failed because we know
>> it was targeted emulated IO.
>> - Fix typoes
>> ---
>> xen/arch/arm/io.c | 32 ++++++++++++++++++--------------
>> xen/arch/arm/traps.c | 18 +++++++++++++++---
>> xen/include/asm-arm/mmio.h | 13 ++++++++++---
>> 3 files changed, 43 insertions(+), 20 deletions(-)
>>
>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>> index c3e9239ffe..1f4cb8f37d 100644
>> --- a/xen/arch/arm/io.c
>> +++ b/xen/arch/arm/io.c
>> @@ -26,8 +26,9 @@
>>
>> #include "decode.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();
>> @@ -40,7 +41,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.
>> @@ -60,17 +61,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;
>
> Why the brackets?
I was not feeling lazy that day ;). I will drop them.
>
> Otherwise:
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 3/4] xen/arm: Don't crash domain on bad MMIO emulation
2018-02-02 10:14 [PATCH v3 0/4] xen/arm: Inject an exception to the guest rather than crashing it Julien Grall
2018-02-02 10:14 ` [PATCH v3 1/4] xen/arm: traps: Merge try_handle_mmio() and handle_mmio() Julien Grall
2018-02-02 10:14 ` [PATCH v3 2/4] xen/arm: io: Distinguish unhandled IO from aborted one Julien Grall
@ 2018-02-02 10:14 ` Julien Grall
2018-02-02 14:35 ` Andre Przywara
2018-02-02 10:14 ` [PATCH v3 4/4] xen/arm: Don't crash the domain on invalid HVC immediate Julien Grall
2018-02-02 22:48 ` [PATCH v3 0/4] xen/arm: Inject an exception to the guest rather than crashing it Stefano Stabellini
4 siblings, 1 reply; 16+ messages in thread
From: Julien Grall @ 2018-02-02 10:14 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, Julien Grall, sstabellini, andre.przywara
Now the MMIO emulation is able to distinguish unhandled IO from aborted
one, there are no need to crash the domain when the region is access
with a bad width.
Instead let Xen inject a data abort to the guest and decide what to do.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
Changes in v2
- Add Stefano's reviewed-by
---
xen/arch/arm/vgic-v2.c | 2 --
xen/arch/arm/vgic-v3-its.c | 3 ---
xen/arch/arm/vgic-v3.c | 8 --------
xen/arch/arm/vpl011.c | 2 --
4 files changed, 15 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 2bdb25261a..646d1f3d12 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -348,7 +348,6 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
bad_width:
printk(XENLOG_G_ERR "%pv: vGICD: bad read width %d r%d offset %#08x\n",
v, dabt.size, dabt.reg, gicd_reg);
- domain_crash_synchronous();
return 0;
read_as_zero_32:
@@ -613,7 +612,6 @@ bad_width:
printk(XENLOG_G_ERR
"%pv: vGICD: bad write width %d r%d=%"PRIregister" offset %#08x\n",
v, dabt.size, dabt.reg, r, gicd_reg);
- domain_crash_synchronous();
return 0;
write_ignore_32:
diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index d8fa44258d..32061c6b03 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -1136,7 +1136,6 @@ read_reserved:
bad_width:
printk(XENLOG_G_ERR "vGITS: bad read width %d r%d offset %#04lx\n",
info->dabt.size, info->dabt.reg, (unsigned long)info->gpa & 0xffff);
- domain_crash_synchronous();
return 0;
}
@@ -1446,8 +1445,6 @@ bad_width:
printk(XENLOG_G_ERR "vGITS: bad write width %d r%d offset %#08lx\n",
info->dabt.size, info->dabt.reg, (unsigned long)info->gpa & 0xffff);
- domain_crash_synchronous();
-
return 0;
}
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index af16dfd005..2ad8a6be62 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -328,7 +328,6 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
bad_width:
printk(XENLOG_G_ERR "%pv vGICR: bad read width %d r%d offset %#08x\n",
v, dabt.size, dabt.reg, gicr_reg);
- domain_crash_synchronous();
return 0;
read_as_zero_64:
@@ -648,7 +647,6 @@ bad_width:
printk(XENLOG_G_ERR
"%pv: vGICR: bad write width %d r%d=%"PRIregister" offset %#08x\n",
v, dabt.size, dabt.reg, r, gicr_reg);
- domain_crash_synchronous();
return 0;
write_ignore_64:
@@ -760,7 +758,6 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
bad_width:
printk(XENLOG_G_ERR "%pv: %s: bad read width %d r%d offset %#08x\n",
v, name, dabt.size, dabt.reg, reg);
- domain_crash_synchronous();
return 0;
read_as_zero:
@@ -876,7 +873,6 @@ bad_width:
printk(XENLOG_G_ERR
"%pv: %s: bad write width %d r%d=%"PRIregister" offset %#08x\n",
v, name, dabt.size, dabt.reg, r, reg);
- domain_crash_synchronous();
return 0;
write_ignore_32:
@@ -937,7 +933,6 @@ static int vgic_v3_rdistr_sgi_mmio_read(struct vcpu *v, mmio_info_t *info,
bad_width:
printk(XENLOG_G_ERR "%pv: vGICR: SGI: bad read width %d r%d offset %#08x\n",
v, dabt.size, dabt.reg, gicr_reg);
- domain_crash_synchronous();
return 0;
read_as_zero_32:
@@ -1017,7 +1012,6 @@ bad_width:
printk(XENLOG_G_ERR
"%pv: vGICR: SGI: bad write width %d r%d=%"PRIregister" offset %#08x\n",
v, dabt.size, dabt.reg, r, gicr_reg);
- domain_crash_synchronous();
return 0;
write_ignore_32:
@@ -1268,7 +1262,6 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
bad_width:
printk(XENLOG_G_ERR "%pv: vGICD: bad read width %d r%d offset %#08x\n",
v, dabt.size, dabt.reg, gicd_reg);
- domain_crash_synchronous();
return 0;
read_as_zero_32:
@@ -1456,7 +1449,6 @@ bad_width:
printk(XENLOG_G_ERR
"%pv: vGICD: bad write width %d r%d=%"PRIregister" offset %#08x\n",
v, dabt.size, dabt.reg, r, gicd_reg);
- domain_crash_synchronous();
return 0;
write_ignore_32:
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index 725b2e03ad..7788c2fc32 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -296,7 +296,6 @@ static int vpl011_mmio_read(struct vcpu *v,
bad_width:
gprintk(XENLOG_ERR, "vpl011: bad read width %d r%d offset %#08x\n",
dabt.size, dabt.reg, vpl011_reg);
- domain_crash_synchronous();
return 0;
}
@@ -366,7 +365,6 @@ write_ignore:
bad_width:
gprintk(XENLOG_ERR, "vpl011: bad write width %d r%d offset %#08x\n",
dabt.size, dabt.reg, vpl011_reg);
- domain_crash_synchronous();
return 0;
}
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 3/4] xen/arm: Don't crash domain on bad MMIO emulation
2018-02-02 10:14 ` [PATCH v3 3/4] xen/arm: Don't crash domain on bad MMIO emulation Julien Grall
@ 2018-02-02 14:35 ` Andre Przywara
0 siblings, 0 replies; 16+ messages in thread
From: Andre Przywara @ 2018-02-02 14:35 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: andrew.cooper3, sstabellini
Hi,
On 02/02/18 10:14, Julien Grall wrote:
> Now the MMIO emulation is able to distinguish unhandled IO from aborted
> one, there are no need to crash the domain when the region is access
> with a bad width.
>
> Instead let Xen inject a data abort to the guest and decide what to do.
Very nice!
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre.
> ---
> Changes in v2
> - Add Stefano's reviewed-by
> ---
> xen/arch/arm/vgic-v2.c | 2 --
> xen/arch/arm/vgic-v3-its.c | 3 ---
> xen/arch/arm/vgic-v3.c | 8 --------
> xen/arch/arm/vpl011.c | 2 --
> 4 files changed, 15 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 2bdb25261a..646d1f3d12 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -348,7 +348,6 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
> bad_width:
> printk(XENLOG_G_ERR "%pv: vGICD: bad read width %d r%d offset %#08x\n",
> v, dabt.size, dabt.reg, gicd_reg);
> - domain_crash_synchronous();
> return 0;
>
> read_as_zero_32:
> @@ -613,7 +612,6 @@ bad_width:
> printk(XENLOG_G_ERR
> "%pv: vGICD: bad write width %d r%d=%"PRIregister" offset %#08x\n",
> v, dabt.size, dabt.reg, r, gicd_reg);
> - domain_crash_synchronous();
> return 0;
>
> write_ignore_32:
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index d8fa44258d..32061c6b03 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -1136,7 +1136,6 @@ read_reserved:
> bad_width:
> printk(XENLOG_G_ERR "vGITS: bad read width %d r%d offset %#04lx\n",
> info->dabt.size, info->dabt.reg, (unsigned long)info->gpa & 0xffff);
> - domain_crash_synchronous();
>
> return 0;
> }
> @@ -1446,8 +1445,6 @@ bad_width:
> printk(XENLOG_G_ERR "vGITS: bad write width %d r%d offset %#08lx\n",
> info->dabt.size, info->dabt.reg, (unsigned long)info->gpa & 0xffff);
>
> - domain_crash_synchronous();
> -
> return 0;
> }
>
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index af16dfd005..2ad8a6be62 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -328,7 +328,6 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
> bad_width:
> printk(XENLOG_G_ERR "%pv vGICR: bad read width %d r%d offset %#08x\n",
> v, dabt.size, dabt.reg, gicr_reg);
> - domain_crash_synchronous();
> return 0;
>
> read_as_zero_64:
> @@ -648,7 +647,6 @@ bad_width:
> printk(XENLOG_G_ERR
> "%pv: vGICR: bad write width %d r%d=%"PRIregister" offset %#08x\n",
> v, dabt.size, dabt.reg, r, gicr_reg);
> - domain_crash_synchronous();
> return 0;
>
> write_ignore_64:
> @@ -760,7 +758,6 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
> bad_width:
> printk(XENLOG_G_ERR "%pv: %s: bad read width %d r%d offset %#08x\n",
> v, name, dabt.size, dabt.reg, reg);
> - domain_crash_synchronous();
> return 0;
>
> read_as_zero:
> @@ -876,7 +873,6 @@ bad_width:
> printk(XENLOG_G_ERR
> "%pv: %s: bad write width %d r%d=%"PRIregister" offset %#08x\n",
> v, name, dabt.size, dabt.reg, r, reg);
> - domain_crash_synchronous();
> return 0;
>
> write_ignore_32:
> @@ -937,7 +933,6 @@ static int vgic_v3_rdistr_sgi_mmio_read(struct vcpu *v, mmio_info_t *info,
> bad_width:
> printk(XENLOG_G_ERR "%pv: vGICR: SGI: bad read width %d r%d offset %#08x\n",
> v, dabt.size, dabt.reg, gicr_reg);
> - domain_crash_synchronous();
> return 0;
>
> read_as_zero_32:
> @@ -1017,7 +1012,6 @@ bad_width:
> printk(XENLOG_G_ERR
> "%pv: vGICR: SGI: bad write width %d r%d=%"PRIregister" offset %#08x\n",
> v, dabt.size, dabt.reg, r, gicr_reg);
> - domain_crash_synchronous();
> return 0;
>
> write_ignore_32:
> @@ -1268,7 +1262,6 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
> bad_width:
> printk(XENLOG_G_ERR "%pv: vGICD: bad read width %d r%d offset %#08x\n",
> v, dabt.size, dabt.reg, gicd_reg);
> - domain_crash_synchronous();
> return 0;
>
> read_as_zero_32:
> @@ -1456,7 +1449,6 @@ bad_width:
> printk(XENLOG_G_ERR
> "%pv: vGICD: bad write width %d r%d=%"PRIregister" offset %#08x\n",
> v, dabt.size, dabt.reg, r, gicd_reg);
> - domain_crash_synchronous();
> return 0;
>
> write_ignore_32:
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index 725b2e03ad..7788c2fc32 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -296,7 +296,6 @@ static int vpl011_mmio_read(struct vcpu *v,
> bad_width:
> gprintk(XENLOG_ERR, "vpl011: bad read width %d r%d offset %#08x\n",
> dabt.size, dabt.reg, vpl011_reg);
> - domain_crash_synchronous();
> return 0;
>
> }
> @@ -366,7 +365,6 @@ write_ignore:
> bad_width:
> gprintk(XENLOG_ERR, "vpl011: bad write width %d r%d offset %#08x\n",
> dabt.size, dabt.reg, vpl011_reg);
> - domain_crash_synchronous();
> return 0;
>
> }
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 4/4] xen/arm: Don't crash the domain on invalid HVC immediate
2018-02-02 10:14 [PATCH v3 0/4] xen/arm: Inject an exception to the guest rather than crashing it Julien Grall
` (2 preceding siblings ...)
2018-02-02 10:14 ` [PATCH v3 3/4] xen/arm: Don't crash domain on bad MMIO emulation Julien Grall
@ 2018-02-02 10:14 ` Julien Grall
2018-02-02 14:37 ` Andre Przywara
2018-02-02 22:48 ` [PATCH v3 0/4] xen/arm: Inject an exception to the guest rather than crashing it Stefano Stabellini
4 siblings, 1 reply; 16+ messages in thread
From: Julien Grall @ 2018-02-02 10:14 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, Julien Grall, sstabellini, andre.przywara
domain_crash_synchronous() should only be used when something went wrong
in Xen. It is better to inject to the guest as it will be in a better
position to provide helpful information (stack trace...).
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
We potentially want to return -1 instead. This would make Xen more
future-proof if we decide to implement the other HVC immediate.
Changes in v2:
- Add Stefano's reviewed-by
---
xen/arch/arm/traps.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 1e85f99ec1..1cba7e584d 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1471,14 +1471,17 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
#endif
static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
- unsigned long iss)
+ const union hsr hsr)
{
arm_hypercall_fn_t call = NULL;
BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) );
- if ( iss != XEN_HYPERCALL_TAG )
- domain_crash_synchronous();
+ if ( hsr.iss != XEN_HYPERCALL_TAG )
+ {
+ gprintk(XENLOG_WARNING, "Invalid HVC imm 0x%x\n", hsr.iss);
+ return inject_undef_exception(regs, hsr);
+ }
if ( *nr >= ARRAY_SIZE(arm_hypercall_table) )
{
@@ -2109,7 +2112,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
if ( hsr.iss == 0 )
return do_trap_hvc_smccc(regs);
nr = regs->r12;
- do_trap_hypercall(regs, &nr, hsr.iss);
+ do_trap_hypercall(regs, &nr, hsr);
regs->r12 = (uint32_t)nr;
break;
}
@@ -2123,7 +2126,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
#endif
if ( hsr.iss == 0 )
return do_trap_hvc_smccc(regs);
- do_trap_hypercall(regs, ®s->x16, hsr.iss);
+ do_trap_hypercall(regs, ®s->x16, hsr);
break;
case HSR_EC_SMC64:
/*
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 4/4] xen/arm: Don't crash the domain on invalid HVC immediate
2018-02-02 10:14 ` [PATCH v3 4/4] xen/arm: Don't crash the domain on invalid HVC immediate Julien Grall
@ 2018-02-02 14:37 ` Andre Przywara
2018-02-02 14:59 ` Julien Grall
0 siblings, 1 reply; 16+ messages in thread
From: Andre Przywara @ 2018-02-02 14:37 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: andrew.cooper3, sstabellini
Hi,
On 02/02/18 10:14, Julien Grall wrote:
> domain_crash_synchronous() should only be used when something went wrong
> in Xen. It is better to inject to the guest as it will be in a better
> position to provide helpful information (stack trace...).
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>
> ---
>
> We potentially want to return -1 instead. This would make Xen more
> future-proof if we decide to implement the other HVC immediate.
>
> Changes in v2:
> - Add Stefano's reviewed-by
> ---
> xen/arch/arm/traps.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 1e85f99ec1..1cba7e584d 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1471,14 +1471,17 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
> #endif
>
> static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
> - unsigned long iss)
> + const union hsr hsr)
> {
> arm_hypercall_fn_t call = NULL;
>
> BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) );
>
> - if ( iss != XEN_HYPERCALL_TAG )
> - domain_crash_synchronous();
> + if ( hsr.iss != XEN_HYPERCALL_TAG )
> + {
> + gprintk(XENLOG_WARNING, "Invalid HVC imm 0x%x\n", hsr.iss);
> + return inject_undef_exception(regs, hsr);
Why is this UNDEF?
UNDEF is the exception used when either HVC is disabled or if it's
called from EL0.
I couldn't find anything that talks about how non-implemented immediates
should be handled, I guess they would just be ignored?
Do you have any sources that recommend UNDEF?
The rest looks fine.
Cheers,
Andre.
> + }
>
> if ( *nr >= ARRAY_SIZE(arm_hypercall_table) )
> {
> @@ -2109,7 +2112,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
> if ( hsr.iss == 0 )
> return do_trap_hvc_smccc(regs);
> nr = regs->r12;
> - do_trap_hypercall(regs, &nr, hsr.iss);
> + do_trap_hypercall(regs, &nr, hsr);
> regs->r12 = (uint32_t)nr;
> break;
> }
> @@ -2123,7 +2126,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
> #endif
> if ( hsr.iss == 0 )
> return do_trap_hvc_smccc(regs);
> - do_trap_hypercall(regs, ®s->x16, hsr.iss);
> + do_trap_hypercall(regs, ®s->x16, hsr);
> break;
> case HSR_EC_SMC64:
> /*
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 4/4] xen/arm: Don't crash the domain on invalid HVC immediate
2018-02-02 14:37 ` Andre Przywara
@ 2018-02-02 14:59 ` Julien Grall
0 siblings, 0 replies; 16+ messages in thread
From: Julien Grall @ 2018-02-02 14:59 UTC (permalink / raw)
To: Andre Przywara, xen-devel; +Cc: andrew.cooper3, sstabellini
On 02/02/18 14:37, Andre Przywara wrote:
> Hi,
Hi,
> On 02/02/18 10:14, Julien Grall wrote:
>> domain_crash_synchronous() should only be used when something went wrong
>> in Xen. It is better to inject to the guest as it will be in a better
>> position to provide helpful information (stack trace...).
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>> ---
>>
>> We potentially want to return -1 instead. This would make Xen more
>> future-proof if we decide to implement the other HVC immediate.
>>
>> Changes in v2:
>> - Add Stefano's reviewed-by
>> ---
>> xen/arch/arm/traps.c | 13 ++++++++-----
>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 1e85f99ec1..1cba7e584d 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1471,14 +1471,17 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>> #endif
>>
>> static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
>> - unsigned long iss)
>> + const union hsr hsr)
>> {
>> arm_hypercall_fn_t call = NULL;
>>
>> BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) );
>>
>> - if ( iss != XEN_HYPERCALL_TAG )
>> - domain_crash_synchronous();
>> + if ( hsr.iss != XEN_HYPERCALL_TAG )
>> + {
>> + gprintk(XENLOG_WARNING, "Invalid HVC imm 0x%x\n", hsr.iss);
>> + return inject_undef_exception(regs, hsr);
>
> Why is this UNDEF?
> UNDEF is the exception used when either HVC is disabled or if it's
> called from EL0.
> I couldn't find anything that talks about how non-implemented immediates
> should be handled, I guess they would just be ignored?
> Do you have any sources that recommend UNDEF?
Per the SMCCC, all nonzero immediate for HVC are designated for use by
the hypervisors. So I take as we can implement the way we want.
We have few solutions here:
1) Ignore. This means that a guest may wrongly call an HVC and will
never notice it until implemented. This would lead to some trouble with
introduce new ABI.
2) Return a value. We could return -1 (or -ENOSYS). But that would need
to be defined.
3) Undefined. The guest should not be allow to touch it, so it makes sense.
1# is not a solution because catching them wrong usage on old Xen would
be a nightmare.
2# might be doable, thought I am not sure we want to commit on a return
value now. But old Xen would see crash the domain.
So I choose #3 which still crash the domain but by injecting an undefined.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/4] xen/arm: Inject an exception to the guest rather than crashing it
2018-02-02 10:14 [PATCH v3 0/4] xen/arm: Inject an exception to the guest rather than crashing it Julien Grall
` (3 preceding siblings ...)
2018-02-02 10:14 ` [PATCH v3 4/4] xen/arm: Don't crash the domain on invalid HVC immediate Julien Grall
@ 2018-02-02 22:48 ` Stefano Stabellini
2018-02-02 23:10 ` Julien Grall
4 siblings, 1 reply; 16+ messages in thread
From: Stefano Stabellini @ 2018-02-02 22:48 UTC (permalink / raw)
To: Julien Grall; +Cc: andrew.cooper3, sstabellini, andre.przywara, xen-devel
Committed, thanks
On Fri, 2 Feb 2018, Julien Grall wrote:
> Hi all,
>
> This small series replaces all call to domain_crash_synchronous by injecting
> an exception to the guest.
>
> This will result to a nicer trace from the guest (no need to manually walk
> the stack) and give a chance to the guest to give a bit more information on
> what it was doing.
>
> Cheers,
>
> Julien Grall (4):
> xen/arm: traps: Merge try_handle_mmio() and handle_mmio()
> xen/arm: io: Distinguish unhandled IO from aborted one
> xen/arm: Don't crash domain on bad MMIO emulation
> xen/arm: Don't crash the domain on invalid HVC immediate
>
> xen/arch/arm/io.c | 65 ++++++++++++++++++++++++++++++++---------
> xen/arch/arm/traps.c | 72 +++++++++++++++-------------------------------
> xen/arch/arm/vgic-v2.c | 2 --
> xen/arch/arm/vgic-v3-its.c | 3 --
> xen/arch/arm/vgic-v3.c | 8 ------
> xen/arch/arm/vpl011.c | 2 --
> xen/include/asm-arm/mmio.h | 11 ++++++-
> 7 files changed, 84 insertions(+), 79 deletions(-)
>
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 0/4] xen/arm: Inject an exception to the guest rather than crashing it
2018-02-02 22:48 ` [PATCH v3 0/4] xen/arm: Inject an exception to the guest rather than crashing it Stefano Stabellini
@ 2018-02-02 23:10 ` Julien Grall
2018-02-05 9:54 ` Andre Przywara
0 siblings, 1 reply; 16+ messages in thread
From: Julien Grall @ 2018-02-02 23:10 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: andrew.cooper3, nd, andre.przywara, xen-devel
On 02/02/2018 22:48, Stefano Stabellini wrote:
> Committed, thanks
I know you acked/reviewed all the patches, but it would have been nice
to wait/give more feedback regarding Andre's valid point on patch #4.
Cheers,
> On Fri, 2 Feb 2018, Julien Grall wrote:
>> Hi all,
>>
>> This small series replaces all call to domain_crash_synchronous by injecting
>> an exception to the guest.
>>
>> This will result to a nicer trace from the guest (no need to manually walk
>> the stack) and give a chance to the guest to give a bit more information on
>> what it was doing.
>>
>> Cheers,
>>
>> Julien Grall (4):
>> xen/arm: traps: Merge try_handle_mmio() and handle_mmio()
>> xen/arm: io: Distinguish unhandled IO from aborted one
>> xen/arm: Don't crash domain on bad MMIO emulation
>> xen/arm: Don't crash the domain on invalid HVC immediate
>>
>> xen/arch/arm/io.c | 65 ++++++++++++++++++++++++++++++++---------
>> xen/arch/arm/traps.c | 72 +++++++++++++++-------------------------------
>> xen/arch/arm/vgic-v2.c | 2 --
>> xen/arch/arm/vgic-v3-its.c | 3 --
>> xen/arch/arm/vgic-v3.c | 8 ------
>> xen/arch/arm/vpl011.c | 2 --
>> xen/include/asm-arm/mmio.h | 11 ++++++-
>> 7 files changed, 84 insertions(+), 79 deletions(-)
>>
>> --
>> 2.11.0
>>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/4] xen/arm: Inject an exception to the guest rather than crashing it
2018-02-02 23:10 ` Julien Grall
@ 2018-02-05 9:54 ` Andre Przywara
0 siblings, 0 replies; 16+ messages in thread
From: Andre Przywara @ 2018-02-05 9:54 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini; +Cc: andrew.cooper3, xen-devel
Hi,
On 02/02/18 23:10, Julien Grall wrote:
>
>
> On 02/02/2018 22:48, Stefano Stabellini wrote:
>> Committed, thanks
>
> I know you acked/reviewed all the patches, but it would have been nice
> to wait/give more feedback regarding Andre's valid point on patch #4.
I think that's fine. I didn't have time to answer on Julien's reply, but
I think given the options he presented injecting an UNDEF is the best
way to handle those thing. At least I couldn't find any architectural
reasoning that would deny that and I don't have a better idea.
Cheers,
Andre.
>> On Fri, 2 Feb 2018, Julien Grall wrote:
>>> Hi all,
>>>
>>> This small series replaces all call to domain_crash_synchronous by
>>> injecting
>>> an exception to the guest.
>>>
>>> This will result to a nicer trace from the guest (no need to manually
>>> walk
>>> the stack) and give a chance to the guest to give a bit more
>>> information on
>>> what it was doing.
>>>
>>> Cheers,
>>>
>>> Julien Grall (4):
>>> xen/arm: traps: Merge try_handle_mmio() and handle_mmio()
>>> xen/arm: io: Distinguish unhandled IO from aborted one
>>> xen/arm: Don't crash domain on bad MMIO emulation
>>> xen/arm: Don't crash the domain on invalid HVC immediate
>>>
>>> xen/arch/arm/io.c | 65
>>> ++++++++++++++++++++++++++++++++---------
>>> xen/arch/arm/traps.c | 72
>>> +++++++++++++++-------------------------------
>>> xen/arch/arm/vgic-v2.c | 2 --
>>> xen/arch/arm/vgic-v3-its.c | 3 --
>>> xen/arch/arm/vgic-v3.c | 8 ------
>>> xen/arch/arm/vpl011.c | 2 --
>>> xen/include/asm-arm/mmio.h | 11 ++++++-
>>> 7 files changed, 84 insertions(+), 79 deletions(-)
>>>
>>> --
>>> 2.11.0
>>>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread