[PATCH 0/4] arm: hypercall handling improvements

[PATCH 0/4] arm: hypercall handling improvements

[Ian Campbell]

This contains v3 of my hypercall continuation patch plus the change to
the argument register clobbering semantics as discussed. I also split
the bounds check into its own patch.

[PATCH 1/4] arm: clobber only argument registers

[Ian Campbell]

Previously it was declared that r1..r4 would all be clobbered by all
hypercalls. Instead declare that only actually used hypercall argument
registers are clobbered. This is more inline with generally expected
conventions and allows for more optimal code in the caller in some cases.

This is an ABI change, although an older guest which expects more things to be
clobbered than we do now won't be adversely impacted.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/traps.c          |   56 ++++++++++++++++++++++++++--------------
 xen/include/public/arch-arm.h |    6 +++-
 2 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6e6807..f2c25b5 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -413,24 +413,31 @@ unsigned long do_arch_0(unsigned int cmd, unsigned long long value)
         return 0;
 }
 
-typedef unsigned long arm_hypercall_t(
+typedef unsigned long (*arm_hypercall_fn_t)(
     unsigned int, unsigned int, unsigned int, unsigned int, unsigned int);
 
-#define HYPERCALL(x)                                        \
-    [ __HYPERVISOR_ ## x ] = (arm_hypercall_t *) do_ ## x
-
-static arm_hypercall_t *arm_hypercall_table[] = {
-    HYPERCALL(memory_op),
-    HYPERCALL(domctl),
-    HYPERCALL(arch_0),
-    HYPERCALL(sched_op),
-    HYPERCALL(console_io),
-    HYPERCALL(xen_version),
-    HYPERCALL(event_channel_op),
-    HYPERCALL(memory_op),
-    HYPERCALL(physdev_op),
-    HYPERCALL(sysctl),
-    HYPERCALL(hvm_op),
+typedef struct {
+    arm_hypercall_fn_t fn;
+    int nr_args;
+} arm_hypercall_t;
+
+#define HYPERCALL(_name, _nr_args)                                   \
+    [ __HYPERVISOR_ ## _name ] =  {                                  \
+        .fn = (arm_hypercall_fn_t) &do_ ## _name,                    \
+        .nr_args = _nr_args,                                         \
+    }
+
+static arm_hypercall_t arm_hypercall_table[] = {
+    HYPERCALL(memory_op, 2),
+    HYPERCALL(domctl, 1),
+    HYPERCALL(arch_0, 2),
+    HYPERCALL(sched_op, 2),
+    HYPERCALL(console_io, 3),
+    HYPERCALL(xen_version, 2),
+    HYPERCALL(event_channel_op, 2),
+    HYPERCALL(physdev_op, 2),
+    HYPERCALL(sysctl, 2),
+    HYPERCALL(hvm_op, 2),
 };
 
 static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
@@ -462,7 +469,7 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
 
 static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
 {
-    arm_hypercall_t *call = NULL;
+    arm_hypercall_fn_t call = NULL;
 
     if ( iss != XEN_HYPERCALL_TAG )
     {
@@ -472,7 +479,7 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
         return;
     }
 
-    call = arm_hypercall_table[regs->r12];
+    call = arm_hypercall_table[regs->r12].fn;
     if ( call == NULL )
     {
         regs->r0 = -ENOSYS;
@@ -482,8 +489,17 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
     regs->r0 = call(regs->r0, regs->r1, regs->r2, regs->r3, regs->r4);
 
 #ifndef NDEBUG
-    /* clobber registers */
-    regs->r1 = regs->r2 = regs->r3 = regs->r4 = regs->r12 = 0xDEADBEEF;
+    /* Clobber argument registers */
+    switch ( arm_hypercall_table[regs->r12].nr_args ) {
+    case 5: regs->r4 = 0xDEADBEEF;
+    case 4: regs->r3 = 0xDEADBEEF;
+    case 3: regs->r2 = 0xDEADBEEF;
+    case 2: regs->r1 = 0xDEADBEEF;
+    case 1: /* Don't clobber r0 -- it's the return value */
+        break;
+    default: BUG();
+    }
+    regs->r12 = 0xDEADBEEF;
 #endif
 }
 
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 6e0fe47..14ad0ab 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -42,8 +42,10 @@
  *
  * The return value is in r0.
  *
- * The hypercall will always clobber r0, r1, r2, r3, r4 and r12,
- * regardless of how many arguments the particular hypercall takes.
+ * The hypercall will clobber r12 and the argument registers used by
+ * that hypercall (except r0 which is the return value) i.e. a 2
+ * argument hypercall will clobber r1 and a 4 argument hypercall will
+ * clobber r1, r2 and r3.
  *
  */
 
-- 
1.7.9.1

Re: [PATCH 1/4] arm: clobber only argument registers

[Stefano Stabellini]

On Wed, 25 Jul 2012, Ian Campbell wrote:
> Previously it was declared that r1..r4 would all be clobbered by all
> hypercalls. Instead declare that only actually used hypercall argument
> registers are clobbered. This is more inline with generally expected
> conventions and allows for more optimal code in the caller in some cases.
> 
> This is an ABI change, although an older guest which expects more things to be
> clobbered than we do now won't be adversely impacted.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

[PATCH 2/4] arm: add bounds check on hypercall array

[Ian Campbell]

Otherwise a guest can cause us to run off the end of the array.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/traps.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f2c25b5..6201d38 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -479,6 +479,12 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
         return;
     }
 
+    if ( regs->r12 > ARRAY_SIZE(arm_hypercall_table) )
+    {
+        regs->r0 = -ENOSYS;
+        return;
+    }
+
     call = arm_hypercall_table[regs->r12].fn;
     if ( call == NULL )
     {
-- 
1.7.9.1

Re: [PATCH 2/4] arm: add bounds check on hypercall array

[Stefano Stabellini]

On Wed, 25 Jul 2012, Ian Campbell wrote:
> Otherwise a guest can cause us to run off the end of the array.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/traps.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index f2c25b5..6201d38 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -479,6 +479,12 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
>          return;
>      }
>  
> +    if ( regs->r12 > ARRAY_SIZE(arm_hypercall_table) )
> +    {
> +        regs->r0 = -ENOSYS;
> +        return;
> +    }

shouldn't this be >=?

>      call = arm_hypercall_table[regs->r12].fn;
>      if ( call == NULL )
>      {
> -- 
> 1.7.9.1
> 

Re: [PATCH 2/4] arm: add bounds check on hypercall array

[Ian Campbell]

On Wed, 2012-07-25 at 16:16 +0100, Stefano Stabellini wrote:
> On Wed, 25 Jul 2012, Ian Campbell wrote:
> > Otherwise a guest can cause us to run off the end of the array.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/traps.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index f2c25b5..6201d38 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -479,6 +479,12 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
> >          return;
> >      }
> >  
> > +    if ( regs->r12 > ARRAY_SIZE(arm_hypercall_table) )
> > +    {
> > +        regs->r0 = -ENOSYS;
> > +        return;
> > +    }
> 
> shouldn't this be >=?

Er, yes, I suspect so.

Rather than repost how about you ack with the proviso that I fix it as I
commit?

> 
> >      call = arm_hypercall_table[regs->r12].fn;
> >      if ( call == NULL )
> >      {
> > -- 
> > 1.7.9.1
> > 

Re: [PATCH 2/4] arm: add bounds check on hypercall array

[Stefano Stabellini]

On Wed, 25 Jul 2012, Ian Campbell wrote:
> On Wed, 2012-07-25 at 16:16 +0100, Stefano Stabellini wrote:
> > On Wed, 25 Jul 2012, Ian Campbell wrote:
> > > Otherwise a guest can cause us to run off the end of the array.
> > > 
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > ---
> > >  xen/arch/arm/traps.c |    6 ++++++
> > >  1 files changed, 6 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > > index f2c25b5..6201d38 100644
> > > --- a/xen/arch/arm/traps.c
> > > +++ b/xen/arch/arm/traps.c
> > > @@ -479,6 +479,12 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
> > >          return;
> > >      }
> > >  
> > > +    if ( regs->r12 > ARRAY_SIZE(arm_hypercall_table) )
> > > +    {
> > > +        regs->r0 = -ENOSYS;
> > > +        return;
> > > +    }
> > 
> > shouldn't this be >=?
> 
> Er, yes, I suspect so.
> 
> Rather than repost how about you ack with the proviso that I fix it as I
> commit?

Yep, fine by me

[PATCH 3/4] arm: implement hypercall continuations

[Ian Campbell]

Largely cribbed from x86, register names differ and the return value is r0 ==
the first argument rather than the hypercall number (which is r12).

We must only clobber the hypercall arguments if PC has not been changed since
continuations rely on them.

Multicall variant is untested, On ARM do_multicall_call is currently a BUG() so
we obviously don't use that yet. I have left a BUG in the hypercall
continuation path too since it will need validation once multicalls are
implemented.

Since the multicall state is local we do not need a globally atomic
{test,set}_bit. However we do need to be atomic WRT interrupts so can't just
use the naive RMW version. Stick with the global atomic implementation for now
but keep the __ as documentaion of the intention.

Signed-off-by: Ian Campbell <Ian.Campbell@citrix.com>
---
v3: split out array bounds check. change to argument clobbering separated out
    into its own patch earlier in the series.
v2: dropped change to handling of hvc with immediate != XEN_HYPERCALL_TAG
---
 xen/arch/arm/domain.c        |   87 ++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/dummy.S         |    1 -
 xen/arch/arm/traps.c         |   29 +++++++++-----
 xen/include/asm-arm/bitops.h |    9 ++++
 4 files changed, 115 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index f61568b..ee58d68 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -5,6 +5,7 @@
 #include <xen/softirq.h>
 #include <xen/wait.h>
 #include <xen/errno.h>
+#include <xen/bitops.h>
 
 #include <asm/current.h>
 #include <asm/regs.h>
@@ -224,6 +225,92 @@ void sync_vcpu_execstate(struct vcpu *v)
     /* Nothing to do -- no lazy switching */
 }
 
+#define next_arg(fmt, args) ({                                              \
+    unsigned long __arg;                                                    \
+    switch ( *(fmt)++ )                                                     \
+    {                                                                       \
+    case 'i': __arg = (unsigned long)va_arg(args, unsigned int);  break;    \
+    case 'l': __arg = (unsigned long)va_arg(args, unsigned long); break;    \
+    case 'h': __arg = (unsigned long)va_arg(args, void *);        break;    \
+    default:  __arg = 0; BUG();                                             \
+    }                                                                       \
+    __arg;                                                                  \
+})
+
+void hypercall_cancel_continuation(void)
+{
+    struct cpu_user_regs *regs = guest_cpu_user_regs();
+    struct mc_state *mcs = &current->mc_state;
+
+    if ( test_bit(_MCSF_in_multicall, &mcs->flags) )
+    {
+        __clear_bit(_MCSF_call_preempted, &mcs->flags);
+    }
+    else
+    {
+        regs->pc += 4; /* undo re-execute 'hvc #XEN_HYPERCALL_TAG' */
+    }
+}
+
+unsigned long hypercall_create_continuation(
+    unsigned int op, const char *format, ...)
+{
+    struct mc_state *mcs = &current->mc_state;
+    struct cpu_user_regs *regs;
+    const char *p = format;
+    unsigned long arg, rc;
+    unsigned int i;
+    va_list args;
+
+    /* All hypercalls take at least one argument */
+    BUG_ON( !p || *p == '\0' );
+
+    va_start(args, format);
+
+    if ( test_bit(_MCSF_in_multicall, &mcs->flags) )
+    {
+        BUG(); /* XXX multicalls not implemented yet. */
+
+        __set_bit(_MCSF_call_preempted, &mcs->flags);
+
+        for ( i = 0; *p != '\0'; i++ )
+            mcs->call.args[i] = next_arg(p, args);
+
+        /* Return value gets written back to mcs->call.result */
+        rc = mcs->call.result;
+    }
+    else
+    {
+        regs      = guest_cpu_user_regs();
+        regs->r12 = op;
+
+        /* Ensure the hypercall trap instruction is re-executed. */
+        regs->pc -= 4;  /* re-execute 'hvc #XEN_HYPERCALL_TAG' */
+
+        for ( i = 0; *p != '\0'; i++ )
+        {
+            arg = next_arg(p, args);
+
+            switch ( i )
+            {
+            case 0: regs->r0 = arg; break;
+            case 1: regs->r1 = arg; break;
+            case 2: regs->r2 = arg; break;
+            case 3: regs->r3 = arg; break;
+            case 4: regs->r4 = arg; break;
+            case 5: regs->r5 = arg; break;
+            }
+        }
+
+        /* Return value gets written back to r0 */
+        rc = regs->r0;
+    }
+
+    va_end(args);
+
+    return rc;
+}
+
 void startup_cpu_idle_loop(void)
 {
     struct vcpu *v = current;
diff --git a/xen/arch/arm/dummy.S b/xen/arch/arm/dummy.S
index cab9522..5406077 100644
--- a/xen/arch/arm/dummy.S
+++ b/xen/arch/arm/dummy.S
@@ -46,7 +46,6 @@ DUMMY(domain_relinquish_resources);
 DUMMY(domain_set_time_offset);
 DUMMY(dom_cow);
 DUMMY(gmfn_to_mfn);
-DUMMY(hypercall_create_continuation);
 DUMMY(send_timer_event);
 DUMMY(share_xen_page_with_privileged_guests);
 DUMMY(wallclock_time);
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 6201d38..7b12832 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -470,6 +470,9 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
 static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
 {
     arm_hypercall_fn_t call = NULL;
+#ifndef NDEBUG
+    uint32_t orig_pc = regs->pc;
+#endif
 
     if ( iss != XEN_HYPERCALL_TAG )
     {
@@ -495,17 +498,23 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
     regs->r0 = call(regs->r0, regs->r1, regs->r2, regs->r3, regs->r4);
 
 #ifndef NDEBUG
-    /* Clobber argument registers */
-    switch ( arm_hypercall_table[regs->r12].nr_args ) {
-    case 5: regs->r4 = 0xDEADBEEF;
-    case 4: regs->r3 = 0xDEADBEEF;
-    case 3: regs->r2 = 0xDEADBEEF;
-    case 2: regs->r1 = 0xDEADBEEF;
-    case 1: /* Don't clobber r0 -- it's the return value */
-        break;
-    default: BUG();
+    /*
+     * Clobber argument registers only if pc is unchanged, otherwise
+     * this is a hypercall continuation.
+     */
+    if ( orig_pc == regs->pc )
+    {
+        switch ( arm_hypercall_table[regs->r12].nr_args ) {
+        case 5: regs->r4 = 0xDEADBEEF;
+        case 4: regs->r3 = 0xDEADBEEF;
+        case 3: regs->r2 = 0xDEADBEEF;
+        case 2: regs->r1 = 0xDEADBEEF;
+        case 1: /* Don't clobber r0 -- it's the return value */
+            break;
+        default: BUG();
+        }
+        regs->r12 = 0xDEADBEEF;
     }
-    regs->r12 = 0xDEADBEEF;
 #endif
 }
 
diff --git a/xen/include/asm-arm/bitops.h b/xen/include/asm-arm/bitops.h
index e5c1781..87de5db 100644
--- a/xen/include/asm-arm/bitops.h
+++ b/xen/include/asm-arm/bitops.h
@@ -23,6 +23,15 @@ extern int _test_and_change_bit(int nr, volatile void * p);
 #define test_and_clear_bit(n,p)   _test_and_clear_bit(n,p)
 #define test_and_change_bit(n,p)  _test_and_change_bit(n,p)
 
+/*
+ * Non-atomic bit manipulation.
+ *
+ * Implemented using atomics to be interrupt safe. Could alternatively
+ * implement with local interrupt masking.
+ */
+#define __set_bit(n,p)            _set_bit(n,p)
+#define __clear_bit(n,p)          _clear_bit(n,p)
+
 #define BIT(nr)                 (1UL << (nr))
 #define BIT_MASK(nr)            (1UL << ((nr) % BITS_PER_LONG))
 #define BIT_WORD(nr)            ((nr) / BITS_PER_LONG)
-- 
1.7.9.1

Re: [PATCH 3/4] arm: implement hypercall continuations

[Stefano Stabellini]

On Wed, 25 Jul 2012, Ian Campbell wrote:
> Largely cribbed from x86, register names differ and the return value is r0 ==
> the first argument rather than the hypercall number (which is r12).
> 
> We must only clobber the hypercall arguments if PC has not been changed since
> continuations rely on them.
> 
> Multicall variant is untested, On ARM do_multicall_call is currently a BUG() so
> we obviously don't use that yet. I have left a BUG in the hypercall
> continuation path too since it will need validation once multicalls are
> implemented.
> 
> Since the multicall state is local we do not need a globally atomic
> {test,set}_bit. However we do need to be atomic WRT interrupts so can't just
> use the naive RMW version. Stick with the global atomic implementation for now
> but keep the __ as documentaion of the intention.
> 
> Signed-off-by: Ian Campbell <Ian.Campbell@citrix.com>

looks good

[PATCH 4/4] arm: kill a guest which uses hvc with an immediate operand != XEN_HYPERCALL_TAG

[Ian Campbell]

At best these guests are confused/broken and at worse they are malicious. In
any case we don't know that they are expecting to handle a -errno style error.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/traps.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 7b12832..c2f3135 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -475,12 +475,7 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
 #endif
 
     if ( iss != XEN_HYPERCALL_TAG )
-    {
-        printk("%s %d: received an alien hypercall iss=%lx\n", __func__ ,
-                __LINE__ , iss);
-        regs->r0 = -EINVAL;
-        return;
-    }
+        domain_crash_synchronous();
 
     if ( regs->r12 > ARRAY_SIZE(arm_hypercall_table) )
     {
-- 
1.7.9.1

Re: [PATCH 0/4] arm: hypercall handling improvements

[Ian Campbell]

On Wed, 2012-07-25 at 14:57 +0100, Ian Campbell wrote:
> This contains v3 of my hypercall continuation patch plus the change to
> the argument register clobbering semantics as discussed. I also split
> the bounds check into its own patch.

Applied 1,2 & 3 with Stefano's ACK.

> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel