[PATCH 0/2 V2] arm: hypercall continuations

[PATCH 0/2 V2] arm: hypercall continuations

[Ian Campbell]

This is v2 of "arm: implement hypercall continuations". I've split the
change in handling of hvc with an immediate which is not the Xen tag
into another patch so it can be considered separately from the unrelated
continuation stuff.

Ian.

[PATCH 1/2] 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).

Multicall variant is untested, arms 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.

We cannot clobber all argument registers in a debug build anymore
because continuations expect them to be preserved. Add nr_args field to the
hypercall dispatch array and use it to only clobber the unused hypercall
arguments.

While debugging this I noticed that hypercall dispatch will happily run off the
end of the hypercall dispatch array, add a suitable bounds check.

Signed-off-by: Ian Campbell <Ian.Campbell@citrix.com>
---
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         |   59 +++++++++++++++++++----------
 xen/include/asm-arm/bitops.h |    9 ++++
 4 files changed, 135 insertions(+), 21 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 f6e6807..52ab943 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,17 +469,22 @@ 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 )
     {
         printk("%s %d: received an alien hypercall iss=%lx\n", __func__ ,
                 __LINE__ , iss);
         regs->r0 = -EINVAL;
+    }
+
+    if ( regs->r12 > ARRAY_SIZE(arm_hypercall_table) )
+    {
+        regs->r0 = -ENOSYS;
         return;
     }
 
-    call = arm_hypercall_table[regs->r12];
+    call = arm_hypercall_table[regs->r12].fn;
     if ( call == NULL )
     {
         regs->r0 = -ENOSYS;
@@ -482,8 +494,15 @@ 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 non-argument registers */
+    switch ( arm_hypercall_table[regs->r12].nr_args ) {
+    case 1: regs->r1 = 0xDEADBEEF;
+    case 2: regs->r2 = 0xDEADBEEF;
+    case 3: regs->r3 = 0xDEADBEEF;
+    case 4: regs->r4 = 0xDEADBEEF;
+            break;
+    default: BUG();
+    }
 #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

[PATCH 2/2] 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.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 52ab943..50b62c0 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -472,11 +472,7 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
     arm_hypercall_fn_t call = NULL;
 
     if ( iss != XEN_HYPERCALL_TAG )
-    {
-        printk("%s %d: received an alien hypercall iss=%lx\n", __func__ ,
-                __LINE__ , iss);
-        regs->r0 = -EINVAL;
-    }
+        domain_crash_synchronous();
 
     if ( regs->r12 > ARRAY_SIZE(arm_hypercall_table) )
     {
-- 
1.7.9.1

Re: [PATCH 1/2] arm: implement hypercall continuations

[Stefano Stabellini]

On Mon, 23 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).
> 
> Multicall variant is untested, arms 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.
> 
> We cannot clobber all argument registers in a debug build anymore
> because continuations expect them to be preserved. Add nr_args field to the
> hypercall dispatch array and use it to only clobber the unused hypercall
> arguments.
> 
> While debugging this I noticed that hypercall dispatch will happily run off the
> end of the hypercall dispatch array, add a suitable bounds check.
> 
> Signed-off-by: Ian Campbell <Ian.Campbell@citrix.com>

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

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

[Stefano Stabellini]

On Mon, 23 Jul 2012, Ian Campbell wrote:
> At best these guests are confused/broken and at worse they are malicious.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/traps.c |    6 +-----
>  1 files changed, 1 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 52ab943..50b62c0 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -472,11 +472,7 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
>      arm_hypercall_fn_t call = NULL;
>  
>      if ( iss != XEN_HYPERCALL_TAG )
> -    {
> -        printk("%s %d: received an alien hypercall iss=%lx\n", __func__ ,
> -                __LINE__ , iss);
> -        regs->r0 = -EINVAL;
> -    }
> +        domain_crash_synchronous();
>  
>      if ( regs->r12 > ARRAY_SIZE(arm_hypercall_table) )
>      {

Are we sure that this is actually necessary?
I think that the old behaviour of returning -EINVAL is sufficient.

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

[Ian Campbell]

On Tue, 2012-07-24 at 11:00 +0100, Stefano Stabellini wrote:
> On Mon, 23 Jul 2012, Ian Campbell wrote:
> > At best these guests are confused/broken and at worse they are malicious.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/traps.c |    6 +-----
> >  1 files changed, 1 insertions(+), 5 deletions(-)
> > 
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 52ab943..50b62c0 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -472,11 +472,7 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
> >      arm_hypercall_fn_t call = NULL;
> >  
> >      if ( iss != XEN_HYPERCALL_TAG )
> > -    {
> > -        printk("%s %d: received an alien hypercall iss=%lx\n", __func__ ,
> > -                __LINE__ , iss);
> > -        regs->r0 = -EINVAL;
> > -    }
> > +        domain_crash_synchronous();
> >  
> >      if ( regs->r12 > ARRAY_SIZE(arm_hypercall_table) )
> >      {
> 
> Are we sure that this is actually necessary?
> I think that the old behaviour of returning -EINVAL is sufficient.

We have no idea what semantics a guest using a tag other than ours is
expecting from the hypercall. We don't even know if they are expecting
negative errno style errors at all.

Worst case you've just answered the question "how much disk shall I
erase" with 0xffffffea (or whatever EINVAL is).

Even if they were expecting -errno we don't know that their idea of
EINVAL is the same as ours (the actual numbers are not standard, only
the names). (and in any case ENOSYS would be the correct one to use).

Ian.

Re: [PATCH 1/2] arm: implement hypercall continuations

[Ian Campbell]

On Mon, 2012-07-23 at 14:32 +0100, Ian Campbell wrote:
> @@ -482,8 +494,15 @@ 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 non-argument registers */
> +    switch ( arm_hypercall_table[regs->r12].nr_args ) {
> +    case 1: regs->r1 = 0xDEADBEEF;
> +    case 2: regs->r2 = 0xDEADBEEF;
> +    case 3: regs->r3 = 0xDEADBEEF;
> +    case 4: regs->r4 = 0xDEADBEEF;
> +            break;
> +    default: BUG();
> +    }
>  #endif
>  } 

This actually inadvertently changes the behaviour to only clobber the
arguments which the particular hypercall does not uses, which is
contrary to the comment in arch-arm.h, which says hypercalls always
clobber all of those registers.

However having spoken to Stefano IRL we think that actually better to
only clobber the ones which are actually used (unless PC has changed, so
continations work). i.e. to change the ABI such that:

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 6e0fe47..a0593e2 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -42,8 +42,9 @@
  *
  * 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 the argument registers, 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.
  *
  */

I'll do this and make the corresponding implementation change and respin
this change on top.

Re: [PATCH 1/2] arm: implement hypercall continuations

[Stefano Stabellini]

On Wed, 25 Jul 2012, Ian Campbell wrote:
> On Mon, 2012-07-23 at 14:32 +0100, Ian Campbell wrote:
> > @@ -482,8 +494,15 @@ 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 non-argument registers */
> > +    switch ( arm_hypercall_table[regs->r12].nr_args ) {
> > +    case 1: regs->r1 = 0xDEADBEEF;
> > +    case 2: regs->r2 = 0xDEADBEEF;
> > +    case 3: regs->r3 = 0xDEADBEEF;
> > +    case 4: regs->r4 = 0xDEADBEEF;
> > +            break;
> > +    default: BUG();
> > +    }
> >  #endif
> >  } 
> 
> This actually inadvertently changes the behaviour to only clobber the
> arguments which the particular hypercall does not uses, which is
> contrary to the comment in arch-arm.h, which says hypercalls always
> clobber all of those registers.
> 
> However having spoken to Stefano IRL we think that actually better to
> only clobber the ones which are actually used (unless PC has changed, so
> continations work). i.e. to change the ABI such that:

I confirm :)


> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 6e0fe47..a0593e2 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -42,8 +42,9 @@
>   *
>   * 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 the argument registers, 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.
>   *
>   */
> 
> I'll do this and make the corresponding implementation change and respin
> this change on top.
> 
>