[PATCH 2/3] xen: arm: correctly handle continuations for 64-bit guests

[PATCH 2/3] xen: arm: correctly handle continuations for 64-bit guests

[Ian Campbell]

The 64-bit ABI is different to 32-bit:

 - uses x16 as the op register rather than r12.
 - arguments in x0..x5 and not r0..r5. Using rN here potentially
   truncates.
 - return value goes in x0, not r0.

Hypercalls can only be made directly from kernel space, so checking
the domain's size is sufficient.

The update of regs->pc is duplicated in both halves because the 32-bit
case is going to need fixing to handle Thumb mode (next patch).

Spotted due to spurious -EFAULT when destroying a domain, due to the
hypercall's pointer argument being truncated. I'm unclear why I am
only seeing this now.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
I imagine this needs backporting everywhere...

For reference the git diff -b version of this is:
	@@ -357,6 +357,36 @@ unsigned long hypercall_create_continuation(
	     else
	     {
		 regs = guest_cpu_user_regs();
	+
	+#ifdef CONFIG_ARM_64
	+        if ( !is_32bit_domain(current->domain) )
	+        {
	+            regs->x16 = 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->x0 = arg; break;
	+                case 1: regs->x1 = arg; break;
	+                case 2: regs->x2 = arg; break;
	+                case 3: regs->x3 = arg; break;
	+                case 4: regs->x4 = arg; break;
	+                case 5: regs->x5 = arg; break;
	+                }
	+            }
	+
	+            /* Return value gets written back to x0 */
	+            rc = regs->x0;
	+        }
	+        else
	+#endif
	+        {
		     regs->r12 = op;

		     /* Ensure the hypercall trap instruction is re-executed.
	 * */
	@@ -380,6 +410,7 @@ unsigned long hypercall_create_continuation(
             /* Return value gets written back to r0 */
             rc = regs->r0;
         }
	+    }

	     va_end(args);
---
 xen/arch/arm/domain.c |   63 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 47 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 939d8cd..10f13e4 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -356,29 +356,60 @@ unsigned long hypercall_create_continuation(
     }
     else
     {
-        regs      = guest_cpu_user_regs();
-        regs->r12 = op;
+        regs = guest_cpu_user_regs();
 
-        /* Ensure the hypercall trap instruction is re-executed. */
-        regs->pc -= 4;  /* re-execute 'hvc #XEN_HYPERCALL_TAG' */
-
-        for ( i = 0; *p != '\0'; i++ )
+#ifdef CONFIG_ARM_64
+        if ( !is_32bit_domain(current->domain) )
         {
-            arg = next_arg(p, args);
+            regs->x16 = op;
 
-            switch ( i )
+            /* Ensure the hypercall trap instruction is re-executed. */
+            regs->pc -= 4;  /* re-execute 'hvc #XEN_HYPERCALL_TAG' */
+
+            for ( i = 0; *p != '\0'; 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;
+                arg = next_arg(p, args);
+
+                switch ( i )
+                {
+                case 0: regs->x0 = arg; break;
+                case 1: regs->x1 = arg; break;
+                case 2: regs->x2 = arg; break;
+                case 3: regs->x3 = arg; break;
+                case 4: regs->x4 = arg; break;
+                case 5: regs->x5 = arg; break;
+                }
             }
+
+            /* Return value gets written back to x0 */
+            rc = regs->x0;
         }
+        else
+#endif
+        {
+            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;
+            /* Return value gets written back to r0 */
+            rc = regs->r0;
+        }
     }
 
     va_end(args);
-- 
1.7.10.4

Re: [PATCH 2/3] xen: arm: correctly handle continuations for 64-bit guests

[Andrew Cooper]

On 25/03/15 15:34, Ian Campbell wrote:
> The 64-bit ABI is different to 32-bit:
>
>   - uses x16 as the op register rather than r12.
>   - arguments in x0..x5 and not r0..r5. Using rN here potentially
>     truncates.
>   - return value goes in x0, not r0.
>
> Hypercalls can only be made directly from kernel space, so checking
> the domain's size is sufficient.
>
> The update of regs->pc is duplicated in both halves because the 32-bit
> case is going to need fixing to handle Thumb mode (next patch).
>
> Spotted due to spurious -EFAULT when destroying a domain, due to the
> hypercall's pointer argument being truncated. I'm unclear why I am
> only seeing this now.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Almost certainly 15e0aac6fe76be6a710a8e6d3da610d437903266 which changed 
XEN_DOMCTL_destroydomain to use continuations rather than repeated 
hypercalls.

~Andrew

Re: [PATCH 2/3] xen: arm: correctly handle continuations for 64-bit guests

[Ian Campbell]

On Wed, 2015-03-25 at 15:41 +0000, Andrew Cooper wrote:
> On 25/03/15 15:34, Ian Campbell wrote:
> > The 64-bit ABI is different to 32-bit:
> >
> >   - uses x16 as the op register rather than r12.
> >   - arguments in x0..x5 and not r0..r5. Using rN here potentially
> >     truncates.
> >   - return value goes in x0, not r0.
> >
> > Hypercalls can only be made directly from kernel space, so checking
> > the domain's size is sufficient.
> >
> > The update of regs->pc is duplicated in both halves because the 32-bit
> > case is going to need fixing to handle Thumb mode (next patch).
> >
> > Spotted due to spurious -EFAULT when destroying a domain, due to the
> > hypercall's pointer argument being truncated. I'm unclear why I am
> > only seeing this now.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Almost certainly 15e0aac6fe76be6a710a8e6d3da610d437903266 which changed 
> XEN_DOMCTL_destroydomain to use continuations rather than repeated 
> hypercalls.

That sounds like why domaindestroy now exhibits it, but not why it
hasn't been plaguing us on different hypercalls for ever. I suppose we
don't actually hit preempt very often in practice.

Ian.

Re: [PATCH 2/3] xen: arm: correctly handle continuations for 64-bit guests

[Julien Grall]

Hi Ian,

On 25/03/2015 15:34, Ian Campbell wrote:
> The 64-bit ABI is different to 32-bit:
>
>   - uses x16 as the op register rather than r12.
>   - arguments in x0..x5 and not r0..r5. Using rN here potentially
>     truncates.
>   - return value goes in x0, not r0.
>
> Hypercalls can only be made directly from kernel space, so checking
> the domain's size is sufficient.
>
> The update of regs->pc is duplicated in both halves because the 32-bit
> case is going to need fixing to handle Thumb mode (next patch).
>
> Spotted due to spurious -EFAULT when destroying a domain, due to the
> hypercall's pointer argument being truncated. I'm unclear why I am
> only seeing this now.

Good catch!

x16 would still contain the valid operation, because we are (most of the 
time?) continuing on the same hypercall.

So the only issue would be argument truncation. I guess that we don't 
have big value (i.e > 32 bits) to store.

> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> I imagine this needs backporting everywhere...

Agree for Xen 4.4 and Xen 4.5.

Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

Re: [PATCH 2/3] xen: arm: correctly handle continuations for 64-bit guests

[Ian Campbell]

On Wed, 2015-03-25 at 22:39 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 25/03/2015 15:34, Ian Campbell wrote:
> > The 64-bit ABI is different to 32-bit:
> >
> >   - uses x16 as the op register rather than r12.
> >   - arguments in x0..x5 and not r0..r5. Using rN here potentially
> >     truncates.
> >   - return value goes in x0, not r0.
> >
> > Hypercalls can only be made directly from kernel space, so checking
> > the domain's size is sufficient.
> >
> > The update of regs->pc is duplicated in both halves because the 32-bit
> > case is going to need fixing to handle Thumb mode (next patch).
> >
> > Spotted due to spurious -EFAULT when destroying a domain, due to the
> > hypercall's pointer argument being truncated. I'm unclear why I am
> > only seeing this now.
> 
> Good catch!
> 
> x16 would still contain the valid operation, because we are (most of the 
> time?) continuing on the same hypercall.

All the other contination code sets the op back, I guess to handle cases
where it may have been clobbered somewhere along the line. I can't think
of a reason to not do it.

> So the only issue would be argument truncation. I guess that we don't 
> have big value (i.e > 32 bits) to store.

I would hope not, and we'd have been hitting them already since that was
the nature of the bug.

> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> > I imagine this needs backporting everywhere...
> 
> Agree for Xen 4.4 and Xen 4.5.
> 
> Reviewed-by: Julien Grall <julien.grall@linaro.org>
> 
> Regards,
>