[PATCH 0/3] xen/arm: Inject an exception to the guest rather than crashing it

[PATCH 0/3] xen/arm: Inject an exception to the guest rather than crashing it

[Julien Grall]

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 (3):
  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          | 24 +++++++++++++----------
 xen/arch/arm/traps.c       | 47 ++++++++++++++++++++++++++++++----------------
 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 |  9 ++++++++-
 7 files changed, 53 insertions(+), 42 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/3] xen/arm: io: Distinguish unhandled IO from aborted one

[Julien Grall]

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(-)

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);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/3] xen/arm: Don't crash domain on bad MMIO emulation

[Julien Grall]

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>
---
 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

[PATCH 3/3] xen/arm: Don't crash the domain on invalid HVC immediate

[Julien Grall]

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 better
position to provide helpful information (stack trace...).

Signed-off-by: Julien Grall <julien.grall@arm.com>

---

    We potentially want to return -1 instead. This would make Xen more
    future-proof if we decide to implement the other HVC immediate.
---
 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 843adf4959..18da45dff3 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1473,14 +1473,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) )
     {
@@ -2150,7 +2153,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;
     }
@@ -2164,7 +2167,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, &regs->x16, hsr.iss);
+        do_trap_hypercall(regs, &regs->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

Re: [PATCH 0/3] xen/arm: Inject an exception to the guest rather than crashing it

[Andrew Cooper]

On 30/01/18 16:14, 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 (3):
>   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

Thanks.

I don't feel qualified to review these, but some notes.

Patch 1.  s/avodi/avoid/ in the commit message

Patches 2 and 3.  You probably want to convert the printks to
gdprintk()s, otherwise guests can choke up the ratelimited log.  Doing
so will also mean that the vcpu will be identified consistently, which
it isn't currently.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 0/3] xen/arm: Inject an exception to the guest rather than crashing it

[Julien Grall]

On 30/01/18 16:38, Andrew Cooper wrote:
> On 30/01/18 16:14, 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 (3):
>>    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
> 
> Thanks.
> 
> I don't feel qualified to review these, but some notes.
> 
> Patch 1.  s/avodi/avoid/ in the commit message
> 
> Patches 2 and 3.  You probably want to convert the printks to
> gdprintk()s, otherwise guests can choke up the ratelimited log.  Doing
> so will also mean that the vcpu will be identified consistently, which
> it isn't currently.
We didn't use g*printk because it would be more confusing to print the 
current vCPU in some cases (e.g when accessing the re-distributor of 
another vCPU) or does not matter (e.g for ITS).

The problem with the debug version is those information are actually 
quite useful in non-debug build. We found quite a few issues thanks to them.

I think it would make more sense for Xen to provide per-guest 
ratelimited than hiding those messages in non-debug build.

> 
> ~Andrew
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] xen/arm: io: Distinguish unhandled IO from aborted one

[Stefano Stabellini]

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
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] xen/arm: Don't crash domain on bad MMIO emulation

[Stefano Stabellini]

On Tue, 30 Jan 2018, 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.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Nice!

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

> ---
>  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

Re: [PATCH 3/3] xen/arm: Don't crash the domain on invalid HVC immediate

[Stefano Stabellini]

On Tue, 30 Jan 2018, 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 better
                                                              ^ a better


> position to provide helpful information (stack trace...).
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> 
>     We potentially want to return -1 instead. This would make Xen more
>     future-proof if we decide to implement the other HVC immediate.
> ---
>  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 843adf4959..18da45dff3 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1473,14 +1473,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);

It looks a bit weird, given that inject_undef_exception doesn't return
anything. This is just code style so anyway:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> +    }
>  
>      if ( *nr >= ARRAY_SIZE(arm_hypercall_table) )
>      {
> @@ -2150,7 +2153,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;
>      }
> @@ -2164,7 +2167,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, &regs->x16, hsr.iss);
> +        do_trap_hypercall(regs, &regs->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

Re: [PATCH 3/3] xen/arm: Don't crash the domain on invalid HVC immediate

[Julien Grall]

On 30/01/18 18:24, Stefano Stabellini wrote:
> On Tue, 30 Jan 2018, 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 better
>                                                                ^ a better
> 
> 
>> position to provide helpful information (stack trace...).
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>
>>      We potentially want to return -1 instead. This would make Xen more
>>      future-proof if we decide to implement the other HVC immediate.
>> ---
>>   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 843adf4959..18da45dff3 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1473,14 +1473,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);
> 
> It looks a bit weird, given that inject_undef_exception doesn't return
> anything. This is just code style so anyway:

I followed coding style in other part of the emulation (see do_sysreg 
for instance). I am happy to change that though.

Cheers,

> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 
>> +    }
>>   
>>       if ( *nr >= ARRAY_SIZE(arm_hypercall_table) )
>>       {
>> @@ -2150,7 +2153,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;
>>       }
>> @@ -2164,7 +2167,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, &regs->x16, hsr.iss);
>> +        do_trap_hypercall(regs, &regs->x16, hsr);
>>           break;
>>       case HSR_EC_SMC64:
>>           /*
>> -- 
>> 2.11.0
>>

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 0/3] xen/arm: Inject an exception to the guest rather than crashing it

[Andrew Cooper]

On 30/01/18 17:00, Julien Grall wrote:
>
>
> On 30/01/18 16:38, Andrew Cooper wrote:
>> On 30/01/18 16:14, 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 (3):
>>>    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
>>
>> Thanks.
>>
>> I don't feel qualified to review these, but some notes.
>>
>> Patch 1.  s/avodi/avoid/ in the commit message
>>
>> Patches 2 and 3.  You probably want to convert the printks to
>> gdprintk()s, otherwise guests can choke up the ratelimited log.  Doing
>> so will also mean that the vcpu will be identified consistently, which
>> it isn't currently.
> We didn't use g*printk because it would be more confusing to print the
> current vCPU in some cases (e.g when accessing the re-distributor of
> another vCPU) or does not matter (e.g for ITS).

In the former case, you'd want to print both current, and the target
vcpu.  The latter still matters what current is if something goes wrong.

We have plenty of similar cases in x86, but at the point you are
printing an diagnostic message, ignoring current is almost always the
wrong think to do.

>
> The problem with the debug version is those information are actually
> quite useful in non-debug build. We found quite a few issues thanks to
> them.
>
> I think it would make more sense for Xen to provide per-guest
> ratelimited than hiding those messages in non-debug build.

Per guest is quite a lot more complicated than global, and would still
require a global limit to prevent a concerted attack from multiple
guests to avoid DoSing the system.

Debug vs unilateral is your prerogative as a maintainer, but as you've
said yourself, the are used for debugging purposes, which proves my point.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] xen/arm: io: Distinguish unhandled IO from aborted one

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 0/3] xen/arm: Inject an exception to the guest rather than crashing it

[Julien Grall]

Hi,

On 30/01/18 18:29, Andrew Cooper wrote:
> On 30/01/18 17:00, Julien Grall wrote:
>>
>>
>> On 30/01/18 16:38, Andrew Cooper wrote:
>>> On 30/01/18 16:14, 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 (3):
>>>>     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
>>>
>>> Thanks.
>>>
>>> I don't feel qualified to review these, but some notes.
>>>
>>> Patch 1.  s/avodi/avoid/ in the commit message
>>>
>>> Patches 2 and 3.  You probably want to convert the printks to
>>> gdprintk()s, otherwise guests can choke up the ratelimited log.  Doing
>>> so will also mean that the vcpu will be identified consistently, which
>>> it isn't currently.
>> We didn't use g*printk because it would be more confusing to print the
>> current vCPU in some cases (e.g when accessing the re-distributor of
>> another vCPU) or does not matter (e.g for ITS).
> 
> In the former case, you'd want to print both current, and the target
> vcpu.  The latter still matters what current is if something goes wrong.
> 
> We have plenty of similar cases in x86, but at the point you are
> printing an diagnostic message, ignoring current is almost always the
> wrong think to do.

I will look at it on another series.

> 
>>
>> The problem with the debug version is those information are actually
>> quite useful in non-debug build. We found quite a few issues thanks to
>> them.
>>
>> I think it would make more sense for Xen to provide per-guest
>> ratelimited than hiding those messages in non-debug build.
> 
> Per guest is quite a lot more complicated than global, and would still
> require a global limit to prevent a concerted attack from multiple
> guests to avoid DoSing the system.
> 
> Debug vs unilateral is your prerogative as a maintainer, but as you've
> said yourself, the are used for debugging purposes, which proves my point.

So on x86, you always request the user to reproduce it with debug build 
enable?

Stefano, what's your opinion here?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] xen/arm: io: Distinguish unhandled IO from aborted one

[Stefano Stabellini]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 0/3] xen/arm: Inject an exception to the guest rather than crashing it

[Andrew Cooper]

On 30/01/18 18:46, Julien Grall wrote:
> Hi,
>
> On 30/01/18 18:29, Andrew Cooper wrote:
>> On 30/01/18 17:00, Julien Grall wrote:
>>>
>>>
>>> On 30/01/18 16:38, Andrew Cooper wrote:
>>>> On 30/01/18 16:14, 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 (3):
>>>>>     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
>>>>
>>>> Thanks.
>>>>
>>>> I don't feel qualified to review these, but some notes.
>>>>
>>>> Patch 1.  s/avodi/avoid/ in the commit message
>>>>
>>>> Patches 2 and 3.  You probably want to convert the printks to
>>>> gdprintk()s, otherwise guests can choke up the ratelimited log.  Doing
>>>> so will also mean that the vcpu will be identified consistently, which
>>>> it isn't currently.
>>> We didn't use g*printk because it would be more confusing to print the
>>> current vCPU in some cases (e.g when accessing the re-distributor of
>>> another vCPU) or does not matter (e.g for ITS).
>>
>> In the former case, you'd want to print both current, and the target
>> vcpu.  The latter still matters what current is if something goes wrong.
>>
>> We have plenty of similar cases in x86, but at the point you are
>> printing an diagnostic message, ignoring current is almost always the
>> wrong think to do.
>
> I will look at it on another series.

Fair enough.

>
>>
>>>
>>> The problem with the debug version is those information are actually
>>> quite useful in non-debug build. We found quite a few issues thanks to
>>> them.
>>>
>>> I think it would make more sense for Xen to provide per-guest
>>> ratelimited than hiding those messages in non-debug build.
>>
>> Per guest is quite a lot more complicated than global, and would still
>> require a global limit to prevent a concerted attack from multiple
>> guests to avoid DoSing the system.
>>
>> Debug vs unilateral is your prerogative as a maintainer, but as you've
>> said yourself, the are used for debugging purposes, which proves my
>> point.
>
> So on x86, you always request the user to reproduce it with debug
> build enable?

That is very specific to what goes wrong, but no, we generally don't
have release-build messages for "the guest screwed up and we hit it
expected way for doing so".  We do (well - are trying to, which is how I
found this domain_crash_sync issue in the first place) get consistent at
printing useful release-build messages for abnormal termination of the
guest.

In terms of making it easier to debug, XenServer always ships with a
release and debug hypervisor built from the same source so customers can
trivially switch into the debug version and collect logs if we think
that is a useful thing to do, but this is only used in a fraction of
cases in the first place.

Anything more complicated is definitely going to require an experienced
human to investigate, at which point they can do whatever they want.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 0/3] xen/arm: Inject an exception to the guest rather than crashing it

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3265 bytes --]

On Tue, 30 Jan 2018, Julien Grall wrote:
> Hi,
> 
> On 30/01/18 18:29, Andrew Cooper wrote:
> > On 30/01/18 17:00, Julien Grall wrote:
> > > 
> > > 
> > > On 30/01/18 16:38, Andrew Cooper wrote:
> > > > On 30/01/18 16:14, 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 (3):
> > > > >     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
> > > > 
> > > > Thanks.
> > > > 
> > > > I don't feel qualified to review these, but some notes.
> > > > 
> > > > Patch 1.  s/avodi/avoid/ in the commit message
> > > > 
> > > > Patches 2 and 3.  You probably want to convert the printks to
> > > > gdprintk()s, otherwise guests can choke up the ratelimited log.  Doing
> > > > so will also mean that the vcpu will be identified consistently, which
> > > > it isn't currently.
> > > We didn't use g*printk because it would be more confusing to print the
> > > current vCPU in some cases (e.g when accessing the re-distributor of
> > > another vCPU) or does not matter (e.g for ITS).
> > 
> > In the former case, you'd want to print both current, and the target
> > vcpu.  The latter still matters what current is if something goes wrong.
> > 
> > We have plenty of similar cases in x86, but at the point you are
> > printing an diagnostic message, ignoring current is almost always the
> > wrong think to do.
> 
> I will look at it on another series.
> 
> > 
> > > 
> > > The problem with the debug version is those information are actually
> > > quite useful in non-debug build. We found quite a few issues thanks to
> > > them.
> > > 
> > > I think it would make more sense for Xen to provide per-guest
> > > ratelimited than hiding those messages in non-debug build.
> > 
> > Per guest is quite a lot more complicated than global, and would still
> > require a global limit to prevent a concerted attack from multiple
> > guests to avoid DoSing the system.
> > 
> > Debug vs unilateral is your prerogative as a maintainer, but as you've
> > said yourself, the are used for debugging purposes, which proves my point.
> 
> So on x86, you always request the user to reproduce it with debug build
> enable?
> 
> Stefano, what's your opinion here?

I think it would be great to have per-guest rate limiting.

On ARM, it would be impossible to ask users to repro with debug enabled,
given that many deployments are embedded (set-top boxes, etc).

I think it is OK to keep the XENLOG_DEBUG, they should be filtered out
by loglvl. But we need to be careful about the others. We might want to
convert the XENLOG_WARNING in traps.c to XENLOG_DEBUG: they are warning
about guests misbehaving, but from the hypervisor point of view, it's not
a problem, guests can shoot themselves if they want to; it's OK.

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 0/3] xen/arm: Inject an exception to the guest rather than crashing it

[Andrew Cooper]

On 30/01/18 19:21, Stefano Stabellini wrote:
> On Tue, 30 Jan 2018, Julien Grall wrote:
>> Hi,
>>
>> On 30/01/18 18:29, Andrew Cooper wrote:
>>> On 30/01/18 17:00, Julien Grall wrote:
>>>>
>>>> On 30/01/18 16:38, Andrew Cooper wrote:
>>>>> On 30/01/18 16:14, 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 (3):
>>>>>>     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
>>>>> Thanks.
>>>>>
>>>>> I don't feel qualified to review these, but some notes.
>>>>>
>>>>> Patch 1.  s/avodi/avoid/ in the commit message
>>>>>
>>>>> Patches 2 and 3.  You probably want to convert the printks to
>>>>> gdprintk()s, otherwise guests can choke up the ratelimited log.  Doing
>>>>> so will also mean that the vcpu will be identified consistently, which
>>>>> it isn't currently.
>>>> We didn't use g*printk because it would be more confusing to print the
>>>> current vCPU in some cases (e.g when accessing the re-distributor of
>>>> another vCPU) or does not matter (e.g for ITS).
>>> In the former case, you'd want to print both current, and the target
>>> vcpu.  The latter still matters what current is if something goes wrong.
>>>
>>> We have plenty of similar cases in x86, but at the point you are
>>> printing an diagnostic message, ignoring current is almost always the
>>> wrong think to do.
>> I will look at it on another series.
>>
>>>> The problem with the debug version is those information are actually
>>>> quite useful in non-debug build. We found quite a few issues thanks to
>>>> them.
>>>>
>>>> I think it would make more sense for Xen to provide per-guest
>>>> ratelimited than hiding those messages in non-debug build.
>>> Per guest is quite a lot more complicated than global, and would still
>>> require a global limit to prevent a concerted attack from multiple
>>> guests to avoid DoSing the system.
>>>
>>> Debug vs unilateral is your prerogative as a maintainer, but as you've
>>> said yourself, the are used for debugging purposes, which proves my point.
>> So on x86, you always request the user to reproduce it with debug build
>> enable?
>>
>> Stefano, what's your opinion here?
> I think it would be great to have per-guest rate limiting.
>
> On ARM, it would be impossible to ask users to repro with debug enabled,
> given that many deployments are embedded (set-top boxes, etc).
>
> I think it is OK to keep the XENLOG_DEBUG, they should be filtered out
> by loglvl. But we need to be careful about the others. We might want to
> convert the XENLOG_WARNING in traps.c to XENLOG_DEBUG: they are warning
> about guests misbehaving, but from the hypervisor point of view, it's not
> a problem, guests can shoot themselves if they want to; it's OK.

This is a valid argument, but to do it consistently, you'll want to
provide an arch-specific version of gdprintk() so that the common code
printk()s don't evaporate into /dev/null.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 0/3] xen/arm: Inject an exception to the guest rather than crashing it

[Julien Grall]

Hi Stefano,

On 30/01/18 19:21, Stefano Stabellini wrote:
> On Tue, 30 Jan 2018, Julien Grall wrote:
>> Hi,
>>
>> On 30/01/18 18:29, Andrew Cooper wrote:
>>> On 30/01/18 17:00, Julien Grall wrote:
>>>>
>>>>
>>>> On 30/01/18 16:38, Andrew Cooper wrote:
>>>>> On 30/01/18 16:14, 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 (3):
>>>>>>      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
>>>>>
>>>>> Thanks.
>>>>>
>>>>> I don't feel qualified to review these, but some notes.
>>>>>
>>>>> Patch 1.  s/avodi/avoid/ in the commit message
>>>>>
>>>>> Patches 2 and 3.  You probably want to convert the printks to
>>>>> gdprintk()s, otherwise guests can choke up the ratelimited log.  Doing
>>>>> so will also mean that the vcpu will be identified consistently, which
>>>>> it isn't currently.
>>>> We didn't use g*printk because it would be more confusing to print the
>>>> current vCPU in some cases (e.g when accessing the re-distributor of
>>>> another vCPU) or does not matter (e.g for ITS).
>>>
>>> In the former case, you'd want to print both current, and the target
>>> vcpu.  The latter still matters what current is if something goes wrong.
>>>
>>> We have plenty of similar cases in x86, but at the point you are
>>> printing an diagnostic message, ignoring current is almost always the
>>> wrong think to do.
>>
>> I will look at it on another series.
>>
>>>
>>>>
>>>> The problem with the debug version is those information are actually
>>>> quite useful in non-debug build. We found quite a few issues thanks to
>>>> them.
>>>>
>>>> I think it would make more sense for Xen to provide per-guest
>>>> ratelimited than hiding those messages in non-debug build.
>>>
>>> Per guest is quite a lot more complicated than global, and would still
>>> require a global limit to prevent a concerted attack from multiple
>>> guests to avoid DoSing the system.
>>>
>>> Debug vs unilateral is your prerogative as a maintainer, but as you've
>>> said yourself, the are used for debugging purposes, which proves my point.
>>
>> So on x86, you always request the user to reproduce it with debug build
>> enable?
>>
>> Stefano, what's your opinion here?
> 
> I think it would be great to have per-guest rate limiting.
> 
> On ARM, it would be impossible to ask users to repro with debug enabled,
> given that many deployments are embedded (set-top boxes, etc).
> 
> I think it is OK to keep the XENLOG_DEBUG, they should be filtered out
> by loglvl. But we need to be careful about the others. We might want to
> convert the XENLOG_WARNING in traps.c to XENLOG_DEBUG: they are warning
> about guests misbehaving, but from the hypervisor point of view, it's not
> a problem, guests can shoot themselves if they want to; it's OK.

Not really. Some of the XENLOG_WARNING may happen because Xen does not 
handle a trap (see the one on do_trap_guest_sync). That has nothing to 
do with a guest misbehaving and could happen if Xen boots on newer hardware.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] xen/arm: io: Distinguish unhandled IO from aborted one

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] xen/arm: io: Distinguish unhandled IO from aborted one

[Stefano Stabellini]

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
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel