[PATCH xen v2] xen: arm: fully implement multicall interface.

[PATCH xen v2] xen: arm: fully implement multicall interface.

[Ian Campbell]

I'm not sure what I was smoking at the time of 5d74ad1a082e "xen: arm:
implement do_multicall_call for both 32 and 64-bit" but it is obviously
insufficient since it doesn't actually wire up the hypercall.

Before doing so we need to make the usual adjustments for ARM and turn the
unsigned longs into xen_ulong_t. There is no difference in the resulting
structure for x86.

There are knock on changes to the trace interface, but again they are nops on
x86.

For 32-bit ARM guests we require that the arguments which they pass to a
hypercall via a multicall do not use the upper bits of xen_ulong_t and kill
them if they violate this. This should ensure that no ABI surprises can be
silently lurking when running on a 32-bit hypervisor waiting to pounce when the
same kernel is run on a 64-bit hypervisor. Killing the guest is harsh but it
will be far easier to relax the restriction if it turns out to cause problems
than to tighten it up if we were lax to begin with.

In the interests of clarity and always using explicitly sized types change the
unsigned int in the hypercall arguments to a uint32_t. There is no actual
change here on any platform.

We should consider backporting this to 4.4.1 in case a guest decides they want
to use a multicall in common code e.g. I suggested such a thing while
reviewing a netback change recently.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: keir@xen.org
Cc: jbeulich@suse.com
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
This depends on "xen: make sure that likely and unlikely convert the expression
to a boolean"

v2: - update compat version of __trace_multicall_call too
    - update xen.h on requirements when sizeof(xen_ulong_t) > sizeof(a register)
    - kill 32-bit guests which do not follow those requirements. After the
      conversation on v1 I decided that starting out harsh and relaxing if it
      becomes a problem was easier than discovering a mistake later.
---
 xen/arch/arm/traps.c          |   23 +++++++++++++++++++++--
 xen/common/compat/multicall.c |    2 +-
 xen/common/multicall.c        |    4 ++--
 xen/common/trace.c            |    2 +-
 xen/include/public/xen.h      |   10 ++++++----
 xen/include/xen/trace.h       |    2 +-
 6 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index a7edc4e..062c4b5 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1012,6 +1012,7 @@ static arm_hypercall_t arm_hypercall_table[] = {
     HYPERCALL(sysctl, 2),
     HYPERCALL(hvm_op, 2),
     HYPERCALL(grant_table_op, 3),
+    HYPERCALL(multicall, 2),
     HYPERCALL_ARM(vcpu_op, 3),
 };
 
@@ -1159,6 +1160,21 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
 #endif
 }
 
+static void check_multicall_32bit_clean(struct multicall_entry *multi)
+{
+    int i;
+
+    for ( i = 0; i < arm_hypercall_table[multi->op].nr_args; i++ )
+    {
+        if ( unlikely(multi->args[i] & 0xffffffff00000000ULL) )
+        {
+            printk("%pv: multicall argument %d is not 32-bit clean %"PRIx64"\n",
+                   current, i, multi->args[i]);
+            domain_crash_synchronous();
+        }
+    }
+}
+
 void do_multicall_call(struct multicall_entry *multi)
 {
     arm_hypercall_fn_t call = NULL;
@@ -1176,9 +1192,12 @@ void do_multicall_call(struct multicall_entry *multi)
         return;
     }
 
+    if ( is_32bit_domain(current->domain) )
+        check_multicall_32bit_clean(multi);
+
     multi->result = call(multi->args[0], multi->args[1],
-                        multi->args[2], multi->args[3],
-                        multi->args[4]);
+                         multi->args[2], multi->args[3],
+                         multi->args[4]);
 }
 
 /*
diff --git a/xen/common/compat/multicall.c b/xen/common/compat/multicall.c
index 95c047a..2af8aef 100644
--- a/xen/common/compat/multicall.c
+++ b/xen/common/compat/multicall.c
@@ -29,7 +29,7 @@ DEFINE_XEN_GUEST_HANDLE(multicall_entry_compat_t);
 
 static void __trace_multicall_call(multicall_entry_t *call)
 {
-    unsigned long args[6];
+    xen_ulong_t args[6];
     int i;
 
     for ( i = 0; i < ARRAY_SIZE(args); i++ )
diff --git a/xen/common/multicall.c b/xen/common/multicall.c
index e66c798..fa9d910 100644
--- a/xen/common/multicall.c
+++ b/xen/common/multicall.c
@@ -35,10 +35,10 @@ static void trace_multicall_call(multicall_entry_t *call)
 
 ret_t
 do_multicall(
-    XEN_GUEST_HANDLE_PARAM(multicall_entry_t) call_list, unsigned int nr_calls)
+    XEN_GUEST_HANDLE_PARAM(multicall_entry_t) call_list, uint32_t nr_calls)
 {
     struct mc_state *mcs = &current->mc_state;
-    unsigned int     i;
+    uint32_t         i;
     int              rc = 0;
 
     if ( unlikely(__test_and_set_bit(_MCSF_in_multicall, &mcs->flags)) )
diff --git a/xen/common/trace.c b/xen/common/trace.c
index 1814165..f651cf3 100644
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -817,7 +817,7 @@ unlock:
 }
 
 void __trace_hypercall(uint32_t event, unsigned long op,
-                       const unsigned long *args)
+                       const xen_ulong_t *args)
 {
     struct __packed {
         uint32_t op;
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 8c5697e..a6a2092 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -541,13 +541,15 @@ DEFINE_XEN_GUEST_HANDLE(mmu_update_t);
 /*
  * ` enum neg_errnoval
  * ` HYPERVISOR_multicall(multicall_entry_t call_list[],
- * `                      unsigned int nr_calls);
+ * `                      uint32_t nr_calls);
  *
- * NB. The fields are natural register size for this architecture.
+ * NB. The fields are logically the natural register size for this
+ * architecture. In cases where xen_ulong_t is larger than this then
+ * any unused bits in the upper portion must be zero.
  */
 struct multicall_entry {
-    unsigned long op, result;
-    unsigned long args[6];
+    xen_ulong_t op, result;
+    xen_ulong_t args[6];
 };
 typedef struct multicall_entry multicall_entry_t;
 DEFINE_XEN_GUEST_HANDLE(multicall_entry_t);
diff --git a/xen/include/xen/trace.h b/xen/include/xen/trace.h
index 3b8a7b3..12966ea 100644
--- a/xen/include/xen/trace.h
+++ b/xen/include/xen/trace.h
@@ -45,7 +45,7 @@ static inline void trace_var(u32 event, int cycles, int extra,
 }
 
 void __trace_hypercall(uint32_t event, unsigned long op,
-                       const unsigned long *args);
+                       const xen_ulong_t *args);
 
 /* Convenience macros for calling the trace function. */
 #define TRACE_0D(_e)                            \
-- 
1.7.10.4

[PATCH linux v2] arm: xen: implement multicall hypercall support.

[Ian Campbell]

As part of this make the usual change to xen_ulong_t in place of unsigned long.
This change has no impact on x86.

The Linux definition of struct multicall_entry.result differs from the Xen
definition, I think for good reasons, and used a long rather than an unsigned
long. Therefore introduce a xen_long_t, which is a long on x86 architectures
and a signed 64-bit integer on ARM.

Use uint32_t nr_calls on x86 for consistency with the ARM definition.

Build tested on amd64 and i386 builds. Runtime tested on ARM.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: David Vrabel <david.vrabel@citrix.com>
---
v2: - Typo in commit message.
    - Update x86 prototype for HYPERCALL_multicall for consistency (nr_calls
      is a uint32_t).

Tested on ARM with a stupid patch:
	diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
	index b96723e..61c6b94 100644
	--- a/arch/arm/xen/enlighten.c
	+++ b/arch/arm/xen/enlighten.c
	@@ -339,6 +339,36 @@ static int __init xen_pm_init(void)
	 }
	 late_initcall(xen_pm_init);

	+static int __init multicall_test(void)
	+{
	+	struct multicall_entry call[2];
	+	const char *str0 = "This is the first debug string\n";
	+	const char *str1 = "This is the second debug string\n";
	+	int ret;
	+
	+	call[0] = (struct multicall_entry) {
	+		.op = __HYPERVISOR_console_io,
	+		.args[0] = CONSOLEIO_write,
	+		.args[1] = strlen(str0),
	+		.args[2] = (uintptr_t)str0
	+	};
	+	call[1] = (struct multicall_entry) {
	+		.op = __HYPERVISOR_console_io,
	+		.args[0] = CONSOLEIO_write,
	+		.args[1] = strlen(str1),
	+		.args[2] = (uintptr_t)str1
	+	};
	+
	+	ret = HYPERVISOR_multicall(call, ARRAY_SIZE(call));
	+	printk("MULTICALL returned %d\n", ret);
	+	if (ret == 0) {
	+		printk("call[0].result = %"PRI_xen_long"\n", call[0].result);
	+		printk("call[1].result = %"PRI_xen_long"\n", call[1].result);
	+	}
	+	return 0;
	+}
	+late_initcall(multicall_test);
	+
	 /* In the hypervisor.S file. */
	 EXPORT_SYMBOL_GPL(HYPERVISOR_event_channel_op);
	 EXPORT_SYMBOL_GPL(HYPERVISOR_grant_table_op);

fixup
---
 arch/arm/include/asm/xen/hypercall.h |    6 +-----
 arch/arm/include/asm/xen/interface.h |    2 ++
 arch/arm/xen/hypercall.S             |    1 +
 arch/x86/include/asm/xen/hypercall.h |    2 +-
 arch/x86/include/asm/xen/interface.h |    3 +++
 include/xen/interface/xen.h          |    6 +++---
 6 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h
index 7704e28..7658150 100644
--- a/arch/arm/include/asm/xen/hypercall.h
+++ b/arch/arm/include/asm/xen/hypercall.h
@@ -48,6 +48,7 @@ int HYPERVISOR_memory_op(unsigned int cmd, void *arg);
 int HYPERVISOR_physdev_op(int cmd, void *arg);
 int HYPERVISOR_vcpu_op(int cmd, int vcpuid, void *extra_args);
 int HYPERVISOR_tmem_op(void *arg);
+int HYPERVISOR_multicall(struct multicall_entry *calls, uint32_t nr);
 
 static inline void
 MULTI_update_va_mapping(struct multicall_entry *mcl, unsigned long va,
@@ -63,9 +64,4 @@ MULTI_mmu_update(struct multicall_entry *mcl, struct mmu_update *req,
 	BUG();
 }
 
-static inline int
-HYPERVISOR_multicall(void *call_list, int nr_calls)
-{
-	BUG();
-}
 #endif /* _ASM_ARM_XEN_HYPERCALL_H */
diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h
index 1151188..5006600 100644
--- a/arch/arm/include/asm/xen/interface.h
+++ b/arch/arm/include/asm/xen/interface.h
@@ -40,6 +40,8 @@ typedef uint64_t xen_pfn_t;
 #define PRI_xen_pfn "llx"
 typedef uint64_t xen_ulong_t;
 #define PRI_xen_ulong "llx"
+typedef int64_t xen_long_t;
+#define PRI_xen_long "llx"
 /* Guest handles for primitive C types. */
 __DEFINE_GUEST_HANDLE(uchar, unsigned char);
 __DEFINE_GUEST_HANDLE(uint,  unsigned int);
diff --git a/arch/arm/xen/hypercall.S b/arch/arm/xen/hypercall.S
index d1cf7b7..44e3a5f 100644
--- a/arch/arm/xen/hypercall.S
+++ b/arch/arm/xen/hypercall.S
@@ -89,6 +89,7 @@ HYPERCALL2(memory_op);
 HYPERCALL2(physdev_op);
 HYPERCALL3(vcpu_op);
 HYPERCALL1(tmem_op);
+HYPERCALL2(multicall);
 
 ENTRY(privcmd_call)
 	stmdb sp!, {r4}
diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
index e709884..ca08a27 100644
--- a/arch/x86/include/asm/xen/hypercall.h
+++ b/arch/x86/include/asm/xen/hypercall.h
@@ -343,7 +343,7 @@ HYPERVISOR_memory_op(unsigned int cmd, void *arg)
 }
 
 static inline int
-HYPERVISOR_multicall(void *call_list, int nr_calls)
+HYPERVISOR_multicall(void *call_list, uint32_t nr_calls)
 {
 	return _hypercall2(int, multicall, call_list, nr_calls);
 }
diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h
index fd9cb76..3400dba 100644
--- a/arch/x86/include/asm/xen/interface.h
+++ b/arch/x86/include/asm/xen/interface.h
@@ -54,6 +54,9 @@ typedef unsigned long xen_pfn_t;
 #define PRI_xen_pfn "lx"
 typedef unsigned long xen_ulong_t;
 #define PRI_xen_ulong "lx"
+typedef long xen_long_t;
+#define PRI_xen_long "lx"
+
 /* Guest handles for primitive C types. */
 __DEFINE_GUEST_HANDLE(uchar, unsigned char);
 __DEFINE_GUEST_HANDLE(uint,  unsigned int);
diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
index 0cd5ca3..de08213 100644
--- a/include/xen/interface/xen.h
+++ b/include/xen/interface/xen.h
@@ -275,9 +275,9 @@ DEFINE_GUEST_HANDLE_STRUCT(mmu_update);
  * NB. The fields are natural register size for this architecture.
  */
 struct multicall_entry {
-    unsigned long op;
-    long result;
-    unsigned long args[6];
+    xen_ulong_t op;
+    xen_long_t result;
+    xen_ulong_t args[6];
 };
 DEFINE_GUEST_HANDLE_STRUCT(multicall_entry);
 
-- 
1.7.10.4

Re: [PATCH linux v2] arm: xen: implement multicall hypercall support.

[David Vrabel]

On 07/04/14 13:48, Ian Campbell wrote:
> --- a/arch/x86/include/asm/xen/interface.h
> +++ b/arch/x86/include/asm/xen/interface.h
> @@ -54,6 +54,9 @@ typedef unsigned long xen_pfn_t;
>  #define PRI_xen_pfn "lx"
>  typedef unsigned long xen_ulong_t;
>  #define PRI_xen_ulong "lx"
> +typedef long xen_long_t;
> +#define PRI_xen_long "lx"

Do you really want signed values to be printed as unsigned?

David

Re: [PATCH linux v2] arm: xen: implement multicall hypercall support.

[Ian Campbell]

On Mon, 2014-04-07 at 13:51 +0100, David Vrabel wrote:
> On 07/04/14 13:48, Ian Campbell wrote:
> > --- a/arch/x86/include/asm/xen/interface.h
> > +++ b/arch/x86/include/asm/xen/interface.h
> > @@ -54,6 +54,9 @@ typedef unsigned long xen_pfn_t;
> >  #define PRI_xen_pfn "lx"
> >  typedef unsigned long xen_ulong_t;
> >  #define PRI_xen_ulong "lx"
> > +typedef long xen_long_t;
> > +#define PRI_xen_long "lx"
> 
> Do you really want signed values to be printed as unsigned?

There's no signed hex format code (is there?) so it was a choice between
signed decimal or unsigned hex, I went with the latter for consistency
with PRI_xen_ulong.

In the absence of a PRIx64 vs PRId64 type distinction there's probably
no universally correct answer.

Ian.

Re: [PATCH linux v2] arm: xen: implement multicall hypercall support.

[David Vrabel]

On 07/04/14 13:56, Ian Campbell wrote:
> On Mon, 2014-04-07 at 13:51 +0100, David Vrabel wrote:
>> On 07/04/14 13:48, Ian Campbell wrote:
>>> --- a/arch/x86/include/asm/xen/interface.h
>>> +++ b/arch/x86/include/asm/xen/interface.h
>>> @@ -54,6 +54,9 @@ typedef unsigned long xen_pfn_t;
>>>  #define PRI_xen_pfn "lx"
>>>  typedef unsigned long xen_ulong_t;
>>>  #define PRI_xen_ulong "lx"
>>> +typedef long xen_long_t;
>>> +#define PRI_xen_long "lx"
>>
>> Do you really want signed values to be printed as unsigned?
> 
> There's no signed hex format code (is there?) so it was a choice between
> signed decimal or unsigned hex, I went with the latter for consistency
> with PRI_xen_ulong.
> 
> In the absence of a PRIx64 vs PRId64 type distinction there's probably
> no universally correct answer.

Ok.

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David

Re: [PATCH xen v2] xen: arm: fully implement multicall interface.

[Jan Beulich]

>>> On 07.04.14 at 14:48, <ian.campbell@citrix.com> wrote:
> I'm not sure what I was smoking at the time of 5d74ad1a082e "xen: arm:
> implement do_multicall_call for both 32 and 64-bit" but it is obviously
> insufficient since it doesn't actually wire up the hypercall.
> 
> Before doing so we need to make the usual adjustments for ARM and turn the
> unsigned longs into xen_ulong_t. There is no difference in the resulting
> structure for x86.
> 
> There are knock on changes to the trace interface, but again they are nops 
> on
> x86.
> 
> For 32-bit ARM guests we require that the arguments which they pass to a
> hypercall via a multicall do not use the upper bits of xen_ulong_t and kill
> them if they violate this. This should ensure that no ABI surprises can be
> silently lurking when running on a 32-bit hypervisor waiting to pounce when 
> the
> same kernel is run on a 64-bit hypervisor. Killing the guest is harsh but it
> will be far easier to relax the restriction if it turns out to cause 
> problems
> than to tighten it up if we were lax to begin with.
> 
> In the interests of clarity and always using explicitly sized types change 
> the
> unsigned int in the hypercall arguments to a uint32_t. There is no actual
> change here on any platform.
> 
> We should consider backporting this to 4.4.1 in case a guest decides they 
> want
> to use a multicall in common code e.g. I suggested such a thing while
> reviewing a netback change recently.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

on the non-ARM parts, with one comment on the ARM ones below.

> @@ -1159,6 +1160,21 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
>  #endif
>  }
>  
> +static void check_multicall_32bit_clean(struct multicall_entry *multi)
> +{
> +    int i;
> +
> +    for ( i = 0; i < arm_hypercall_table[multi->op].nr_args; i++ )
> +    {
> +        if ( unlikely(multi->args[i] & 0xffffffff00000000ULL) )
> +        {
> +            printk("%pv: multicall argument %d is not 32-bit clean %"PRIx64"\n",
> +                   current, i, multi->args[i]);
> +            domain_crash_synchronous();

On x86 we actually decided quite a long while ago to try to avoid
domain_crash_synchronous() whenever possible. Would
domain_crash(current->domain) not work for you here?

Also I think you want the loop variable to be uint32_t, in line with
your changes elsewhere in this patch.

Jan

Re: [PATCH xen v2] xen: arm: fully implement multicall interface.

[Ian Campbell]

On Mon, 2014-04-07 at 14:27 +0100, Jan Beulich wrote:
> >>> On 07.04.14 at 14:48, <ian.campbell@citrix.com> wrote:
> > I'm not sure what I was smoking at the time of 5d74ad1a082e "xen: arm:
> > implement do_multicall_call for both 32 and 64-bit" but it is obviously
> > insufficient since it doesn't actually wire up the hypercall.
> > 
> > Before doing so we need to make the usual adjustments for ARM and turn the
> > unsigned longs into xen_ulong_t. There is no difference in the resulting
> > structure for x86.
> > 
> > There are knock on changes to the trace interface, but again they are nops 
> > on
> > x86.
> > 
> > For 32-bit ARM guests we require that the arguments which they pass to a
> > hypercall via a multicall do not use the upper bits of xen_ulong_t and kill
> > them if they violate this. This should ensure that no ABI surprises can be
> > silently lurking when running on a 32-bit hypervisor waiting to pounce when 
> > the
> > same kernel is run on a 64-bit hypervisor. Killing the guest is harsh but it
> > will be far easier to relax the restriction if it turns out to cause 
> > problems
> > than to tighten it up if we were lax to begin with.
> > 
> > In the interests of clarity and always using explicitly sized types change 
> > the
> > unsigned int in the hypercall arguments to a uint32_t. There is no actual
> > change here on any platform.
> > 
> > We should consider backporting this to 4.4.1 in case a guest decides they 
> > want
> > to use a multicall in common code e.g. I suggested such a thing while
> > reviewing a netback change recently.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> on the non-ARM parts, with one comment on the ARM ones below.
> 
> > @@ -1159,6 +1160,21 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
> >  #endif
> >  }
> >  
> > +static void check_multicall_32bit_clean(struct multicall_entry *multi)
> > +{
> > +    int i;
> > +
> > +    for ( i = 0; i < arm_hypercall_table[multi->op].nr_args; i++ )
> > +    {
> > +        if ( unlikely(multi->args[i] & 0xffffffff00000000ULL) )
> > +        {
> > +            printk("%pv: multicall argument %d is not 32-bit clean %"PRIx64"\n",
> > +                   current, i, multi->args[i]);
> > +            domain_crash_synchronous();
> 
> On x86 we actually decided quite a long while ago to try to avoid
> domain_crash_synchronous() whenever possible. Would
> domain_crash(current->domain) not work for you here?

I wondered about that and concluded that I didn't really grok the
difference so I went with what I thought was the safer option. I'd be
happy to change if that's considered the right thing to do.

My concern was that domain_crash returns and I wasn't sure what I was
then supposed to do (obviously not actually run the call though) or when
the domain was actually guaranteed to die. In particular I might get
more calls to multicall_call for the rest of the batch. I'm not sure
that matters other than the possibility that skipping one call in the
middle might lead to confusing log spam from the remaining calls.

> Also I think you want the loop variable to be uint32_t, in line with
> your changes elsewhere in this patch.

True.

> 
> Jan
> 

Re: [PATCH xen v2] xen: arm: fully implement multicall interface.

[Jan Beulich]

>>> On 07.04.14 at 15:42, <Ian.Campbell@citrix.com> wrote:
> On Mon, 2014-04-07 at 14:27 +0100, Jan Beulich wrote:
>> >>> On 07.04.14 at 14:48, <ian.campbell@citrix.com> wrote:
>> > +static void check_multicall_32bit_clean(struct multicall_entry *multi)
>> > +{
>> > +    int i;
>> > +
>> > +    for ( i = 0; i < arm_hypercall_table[multi->op].nr_args; i++ )
>> > +    {
>> > +        if ( unlikely(multi->args[i] & 0xffffffff00000000ULL) )
>> > +        {
>> > +            printk("%pv: multicall argument %d is not 32-bit clean %"PRIx64"\n",
>> > +                   current, i, multi->args[i]);
>> > +            domain_crash_synchronous();
>> 
>> On x86 we actually decided quite a long while ago to try to avoid
>> domain_crash_synchronous() whenever possible. Would
>> domain_crash(current->domain) not work for you here?
> 
> I wondered about that and concluded that I didn't really grok the
> difference so I went with what I thought was the safer option. I'd be
> happy to change if that's considered the right thing to do.
> 
> My concern was that domain_crash returns and I wasn't sure what I was
> then supposed to do (obviously not actually run the call though) or when
> the domain was actually guaranteed to die. In particular I might get
> more calls to multicall_call for the rest of the batch. I'm not sure
> that matters other than the possibility that skipping one call in the
> middle might lead to confusing log spam from the remaining calls.

You'd want to return some error indication, avoid anything else to be
done that might confusion on a dead domain (read: abort the entire
multicall), and on the hypercall exit path the vCPU would be taken off
the scheduler, i.e. you run your normal call tree to completion and
you're guaranteed that the vCPU in question won't make it back into
guest context.

Jan

Re: [PATCH xen v2] xen: arm: fully implement multicall interface.

[Ian Campbell]

On Mon, 2014-04-07 at 16:13 +0100, Jan Beulich wrote:
> >>> On 07.04.14 at 15:42, <Ian.Campbell@citrix.com> wrote:
> > On Mon, 2014-04-07 at 14:27 +0100, Jan Beulich wrote:
> >> >>> On 07.04.14 at 14:48, <ian.campbell@citrix.com> wrote:
> >> > +static void check_multicall_32bit_clean(struct multicall_entry *multi)
> >> > +{
> >> > +    int i;
> >> > +
> >> > +    for ( i = 0; i < arm_hypercall_table[multi->op].nr_args; i++ )
> >> > +    {
> >> > +        if ( unlikely(multi->args[i] & 0xffffffff00000000ULL) )
> >> > +        {
> >> > +            printk("%pv: multicall argument %d is not 32-bit clean %"PRIx64"\n",
> >> > +                   current, i, multi->args[i]);
> >> > +            domain_crash_synchronous();
> >> 
> >> On x86 we actually decided quite a long while ago to try to avoid
> >> domain_crash_synchronous() whenever possible.

I meant to ask why this was?

I'm supposing that the for(;;) do_softirqs is not considered very
nice...

>  Would
> >> domain_crash(current->domain) not work for you here?
> > 
> > I wondered about that and concluded that I didn't really grok the
> > difference so I went with what I thought was the safer option. I'd be
> > happy to change if that's considered the right thing to do.
> > 
> > My concern was that domain_crash returns and I wasn't sure what I was
> > then supposed to do (obviously not actually run the call though) or when
> > the domain was actually guaranteed to die. In particular I might get
> > more calls to multicall_call for the rest of the batch. I'm not sure
> > that matters other than the possibility that skipping one call in the
> > middle might lead to confusing log spam from the remaining calls.
> 
> You'd want to return some error indication, avoid anything else to be
> done that might confusion on a dead domain (read: abort the entire
> multicall),

Hrm, that will involve frobbing around with the common do_multicall code
since it currently doesn't consider the possibility of do_multicall_call
failing in a fatal way.

>  and on the hypercall exit path the vCPU would be taken off
> the scheduler, i.e. you run your normal call tree to completion and
> you're guaranteed that the vCPU in question won't make it back into
> guest context.

What about other vcpus? I suppose they get nobbled as and when they
happen to enter the hypervisor?

Ian.

Re: [PATCH xen v2] xen: arm: fully implement multicall interface.

[George Dunlap]

On 04/07/2014 01:48 PM, Ian Campbell wrote:
> I'm not sure what I was smoking at the time of 5d74ad1a082e "xen: arm:
> implement do_multicall_call for both 32 and 64-bit" but it is obviously
> insufficient since it doesn't actually wire up the hypercall.
>
> Before doing so we need to make the usual adjustments for ARM and turn the
> unsigned longs into xen_ulong_t. There is no difference in the resulting
> structure for x86.
>
> There are knock on changes to the trace interface, but again they are nops on
> x86.
>
> For 32-bit ARM guests we require that the arguments which they pass to a
> hypercall via a multicall do not use the upper bits of xen_ulong_t and kill
> them if they violate this. This should ensure that no ABI surprises can be
> silently lurking when running on a 32-bit hypervisor waiting to pounce when the
> same kernel is run on a 64-bit hypervisor. Killing the guest is harsh but it
> will be far easier to relax the restriction if it turns out to cause problems
> than to tighten it up if we were lax to begin with.
>
> In the interests of clarity and always using explicitly sized types change the
> unsigned int in the hypercall arguments to a uint32_t. There is no actual
> change here on any platform.
>
> We should consider backporting this to 4.4.1 in case a guest decides they want
> to use a multicall in common code e.g. I suggested such a thing while
> reviewing a netback change recently.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: keir@xen.org
> Cc: jbeulich@suse.com
> Cc: George Dunlap <george.dunlap@eu.citrix.com>

Trace-related bits:

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

> ---
> This depends on "xen: make sure that likely and unlikely convert the expression
> to a boolean"
>
> v2: - update compat version of __trace_multicall_call too
>      - update xen.h on requirements when sizeof(xen_ulong_t) > sizeof(a register)
>      - kill 32-bit guests which do not follow those requirements. After the
>        conversation on v1 I decided that starting out harsh and relaxing if it
>        becomes a problem was easier than discovering a mistake later.
> ---
>   xen/arch/arm/traps.c          |   23 +++++++++++++++++++++--
>   xen/common/compat/multicall.c |    2 +-
>   xen/common/multicall.c        |    4 ++--
>   xen/common/trace.c            |    2 +-
>   xen/include/public/xen.h      |   10 ++++++----
>   xen/include/xen/trace.h       |    2 +-
>   6 files changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index a7edc4e..062c4b5 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1012,6 +1012,7 @@ static arm_hypercall_t arm_hypercall_table[] = {
>       HYPERCALL(sysctl, 2),
>       HYPERCALL(hvm_op, 2),
>       HYPERCALL(grant_table_op, 3),
> +    HYPERCALL(multicall, 2),
>       HYPERCALL_ARM(vcpu_op, 3),
>   };
>   
> @@ -1159,6 +1160,21 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
>   #endif
>   }
>   
> +static void check_multicall_32bit_clean(struct multicall_entry *multi)
> +{
> +    int i;
> +
> +    for ( i = 0; i < arm_hypercall_table[multi->op].nr_args; i++ )
> +    {
> +        if ( unlikely(multi->args[i] & 0xffffffff00000000ULL) )
> +        {
> +            printk("%pv: multicall argument %d is not 32-bit clean %"PRIx64"\n",
> +                   current, i, multi->args[i]);
> +            domain_crash_synchronous();
> +        }
> +    }
> +}
> +
>   void do_multicall_call(struct multicall_entry *multi)
>   {
>       arm_hypercall_fn_t call = NULL;
> @@ -1176,9 +1192,12 @@ void do_multicall_call(struct multicall_entry *multi)
>           return;
>       }
>   
> +    if ( is_32bit_domain(current->domain) )
> +        check_multicall_32bit_clean(multi);
> +
>       multi->result = call(multi->args[0], multi->args[1],
> -                        multi->args[2], multi->args[3],
> -                        multi->args[4]);
> +                         multi->args[2], multi->args[3],
> +                         multi->args[4]);
>   }
>   
>   /*
> diff --git a/xen/common/compat/multicall.c b/xen/common/compat/multicall.c
> index 95c047a..2af8aef 100644
> --- a/xen/common/compat/multicall.c
> +++ b/xen/common/compat/multicall.c
> @@ -29,7 +29,7 @@ DEFINE_XEN_GUEST_HANDLE(multicall_entry_compat_t);
>   
>   static void __trace_multicall_call(multicall_entry_t *call)
>   {
> -    unsigned long args[6];
> +    xen_ulong_t args[6];
>       int i;
>   
>       for ( i = 0; i < ARRAY_SIZE(args); i++ )
> diff --git a/xen/common/multicall.c b/xen/common/multicall.c
> index e66c798..fa9d910 100644
> --- a/xen/common/multicall.c
> +++ b/xen/common/multicall.c
> @@ -35,10 +35,10 @@ static void trace_multicall_call(multicall_entry_t *call)
>   
>   ret_t
>   do_multicall(
> -    XEN_GUEST_HANDLE_PARAM(multicall_entry_t) call_list, unsigned int nr_calls)
> +    XEN_GUEST_HANDLE_PARAM(multicall_entry_t) call_list, uint32_t nr_calls)
>   {
>       struct mc_state *mcs = &current->mc_state;
> -    unsigned int     i;
> +    uint32_t         i;
>       int              rc = 0;
>   
>       if ( unlikely(__test_and_set_bit(_MCSF_in_multicall, &mcs->flags)) )
> diff --git a/xen/common/trace.c b/xen/common/trace.c
> index 1814165..f651cf3 100644
> --- a/xen/common/trace.c
> +++ b/xen/common/trace.c
> @@ -817,7 +817,7 @@ unlock:
>   }
>   
>   void __trace_hypercall(uint32_t event, unsigned long op,
> -                       const unsigned long *args)
> +                       const xen_ulong_t *args)
>   {
>       struct __packed {
>           uint32_t op;
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index 8c5697e..a6a2092 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -541,13 +541,15 @@ DEFINE_XEN_GUEST_HANDLE(mmu_update_t);
>   /*
>    * ` enum neg_errnoval
>    * ` HYPERVISOR_multicall(multicall_entry_t call_list[],
> - * `                      unsigned int nr_calls);
> + * `                      uint32_t nr_calls);
>    *
> - * NB. The fields are natural register size for this architecture.
> + * NB. The fields are logically the natural register size for this
> + * architecture. In cases where xen_ulong_t is larger than this then
> + * any unused bits in the upper portion must be zero.
>    */
>   struct multicall_entry {
> -    unsigned long op, result;
> -    unsigned long args[6];
> +    xen_ulong_t op, result;
> +    xen_ulong_t args[6];
>   };
>   typedef struct multicall_entry multicall_entry_t;
>   DEFINE_XEN_GUEST_HANDLE(multicall_entry_t);
> diff --git a/xen/include/xen/trace.h b/xen/include/xen/trace.h
> index 3b8a7b3..12966ea 100644
> --- a/xen/include/xen/trace.h
> +++ b/xen/include/xen/trace.h
> @@ -45,7 +45,7 @@ static inline void trace_var(u32 event, int cycles, int extra,
>   }
>   
>   void __trace_hypercall(uint32_t event, unsigned long op,
> -                       const unsigned long *args);
> +                       const xen_ulong_t *args);
>   
>   /* Convenience macros for calling the trace function. */
>   #define TRACE_0D(_e)                            \

Re: [PATCH xen v2] xen: arm: fully implement multicall interface.

[Jan Beulich]

>>> On 07.04.14 at 17:18, <Ian.Campbell@citrix.com> wrote:
> On Mon, 2014-04-07 at 16:13 +0100, Jan Beulich wrote:
>> >> On x86 we actually decided quite a long while ago to try to avoid
>> >> domain_crash_synchronous() whenever possible.
> 
> I meant to ask why this was?
> 
> I'm supposing that the for(;;) do_softirqs is not considered very
> nice...

That plus it misguides you into not writing proper error path code.

>> You'd want to return some error indication, avoid anything else to be
>> done that might confusion on a dead domain (read: abort the entire
>> multicall),
> 
> Hrm, that will involve frobbing around with the common do_multicall code
> since it currently doesn't consider the possibility of do_multicall_call
> failing in a fatal way.

But then again - is there anything wrong with actually carrying
out the multicall (with truncated arguments), resulting in the
domain dying only slightly later?

>>  and on the hypercall exit path the vCPU would be taken off
>> the scheduler, i.e. you run your normal call tree to completion and
>> you're guaranteed that the vCPU in question won't make it back into
>> guest context.
> 
> What about other vcpus? I suppose they get nobbled as and when they
> happen to enter the hypervisor?

Sure - via domain_shutdown() they all get vcpu_pause_nosync()'ed,
i.e. they're being forced into the hypervisor if not already there.

Jan

Re: [PATCH xen v2] xen: arm: fully implement multicall interface.

[Ian Campbell]

On Tue, 2014-04-08 at 08:13 +0100, Jan Beulich wrote:
> >>> On 07.04.14 at 17:18, <Ian.Campbell@citrix.com> wrote:
> > On Mon, 2014-04-07 at 16:13 +0100, Jan Beulich wrote:
> >> >> On x86 we actually decided quite a long while ago to try to avoid
> >> >> domain_crash_synchronous() whenever possible.
> > 
> > I meant to ask why this was?
> > 
> > I'm supposing that the for(;;) do_softirqs is not considered very
> > nice...
> 
> That plus it misguides you into not writing proper error path code.

Which is exactly what it let me get away with :-p

> >> You'd want to return some error indication, avoid anything else to be
> >> done that might confusion on a dead domain (read: abort the entire
> >> multicall),
> > 
> > Hrm, that will involve frobbing around with the common do_multicall code
> > since it currently doesn't consider the possibility of do_multicall_call
> > failing in a fatal way.
> 
> But then again - is there anything wrong with actually carrying
> out the multicall (with truncated arguments), resulting in the
> domain dying only slightly later?

My concern was that this truncation happens naturally when running on a
32-bit hypervisor (since the actual hypercall implementations take
32-bit arguments internally). Meaning that the issue would be hidden
until you move that kernel to a 64-bit hypervisor (with 64-bit hypercall
arguments internally) at which point it mysteriously starts failing
because some previously unnoticed garbage shows up in the top half of
the argument.

On 32-on-64 x86 you avoid this because the multicall_entry_t contains
32-bit arguments and you have a compat layer which extends to 64-bit
when calling the core hypercall implementation.

On ARM we want our structs to be the same on 32- and 64-bit which means
we effectively have some padding -- and I wanted to avoid guests relying
on the contents of that padding being ignored or otherwise setting up an
ABI trap for the future.

> >>  and on the hypercall exit path the vCPU would be taken off
> >> the scheduler, i.e. you run your normal call tree to completion and
> >> you're guaranteed that the vCPU in question won't make it back into
> >> guest context.
> > 
> > What about other vcpus? I suppose they get nobbled as and when they
> > happen to enter the hypervisor?
> 
> Sure - via domain_shutdown() they all get vcpu_pause_nosync()'ed,
> i.e. they're being forced into the hypervisor if not already there.

Not sure how I missed that, thanks!

Ian.

Re: [PATCH xen v2] xen: arm: fully implement multicall interface.

[Jan Beulich]

>>> On 08.04.14 at 13:18, <Ian.Campbell@citrix.com> wrote:
> On Tue, 2014-04-08 at 08:13 +0100, Jan Beulich wrote:
>> But then again - is there anything wrong with actually carrying
>> out the multicall (with truncated arguments), resulting in the
>> domain dying only slightly later?
> 
> My concern was that this truncation happens naturally when running on a
> 32-bit hypervisor (since the actual hypercall implementations take
> 32-bit arguments internally). Meaning that the issue would be hidden
> until you move that kernel to a 64-bit hypervisor (with 64-bit hypercall
> arguments internally) at which point it mysteriously starts failing
> because some previously unnoticed garbage shows up in the top half of
> the argument.

Right - I understand all that. I wasn't suggesting to remove the
domain_crash(), just that by using the non-synchronous variant
you'd get the crash at the end of the multicall (or when it gets
preempted) instead of at the beginning. The effect to the guest
and programmer should be the same - the guest is dead and the
programmer (hopefully) goes looking for the problem.

Jan

Re: [PATCH xen v2] xen: arm: fully implement multicall interface.

[Ian Campbell]

On Tue, 2014-04-08 at 12:29 +0100, Jan Beulich wrote:
> >>> On 08.04.14 at 13:18, <Ian.Campbell@citrix.com> wrote:
> > On Tue, 2014-04-08 at 08:13 +0100, Jan Beulich wrote:
> >> But then again - is there anything wrong with actually carrying
> >> out the multicall (with truncated arguments), resulting in the
> >> domain dying only slightly later?
> > 
> > My concern was that this truncation happens naturally when running on a
> > 32-bit hypervisor (since the actual hypercall implementations take
> > 32-bit arguments internally). Meaning that the issue would be hidden
> > until you move that kernel to a 64-bit hypervisor (with 64-bit hypercall
> > arguments internally) at which point it mysteriously starts failing
> > because some previously unnoticed garbage shows up in the top half of
> > the argument.
> 
> Right - I understand all that. I wasn't suggesting to remove the
> domain_crash(), just that by using the non-synchronous variant
> you'd get the crash at the end of the multicall (or when it gets
> preempted) instead of at the beginning. The effect to the guest
> and programmer should be the same - the guest is dead and the
> programmer (hopefully) goes looking for the problem.

My concern here was that the rest of the multicall might have relied on
the op which we failed and there might be a tonne of log spew which
would obscure the original failure.

On the other hand if that can happen then the guest could deliberately
trigger log spew, which would be bad anyway...

It does seem like it would be reasonably simple to add a success/abort
return value to {do,compat}_multicall_call (probably by making into
static inline on x86) and have do_multicall just abort the rest of the
multicall in that case.

Actually, you mentioned preempt. Does calling domain_crash() cause
hypercall_preempt_check to return true? In that case do_multicall
already does the right thing. That would involve domain_crash either
causing a softirq or an evtchn upcall, which I don't see it doing
though. Perhaps just adding a d->is_shutting_down check alongside the
preempt check in the multicall would do the trick.

Ian.

Re: [PATCH xen v2] xen: arm: fully implement multicall interface.

[Jan Beulich]

>>> On 08.04.14 at 13:39, <Ian.Campbell@citrix.com> wrote:
> Actually, you mentioned preempt. Does calling domain_crash() cause
> hypercall_preempt_check to return true? In that case do_multicall
> already does the right thing. That would involve domain_crash either
> causing a softirq or an evtchn upcall, which I don't see it doing
> though. Perhaps just adding a d->is_shutting_down check alongside the
> preempt check in the multicall would do the trick.

Good point. And yes, a softirq is being raised implicitly:
domain_shutdown() calls vcpu_pause_nosync() for all the domain's
vCPU-s, which - via vcpu_sleep_nosync() - reaches the ->sleep()
callback of the individual schedulers, all of which raise the schedule
softirq if the subject vCPU is the one they're running on.

Jan

Re: [PATCH xen v2] xen: arm: fully implement multicall interface.

[Ian Campbell]

On Tue, 2014-04-08 at 14:08 +0100, Jan Beulich wrote:
> >>> On 08.04.14 at 13:39, <Ian.Campbell@citrix.com> wrote:
> > Actually, you mentioned preempt. Does calling domain_crash() cause
> > hypercall_preempt_check to return true? In that case do_multicall
> > already does the right thing. That would involve domain_crash either
> > causing a softirq or an evtchn upcall, which I don't see it doing
> > though. Perhaps just adding a d->is_shutting_down check alongside the
> > preempt check in the multicall would do the trick.
> 
> Good point. And yes, a softirq is being raised implicitly:
> domain_shutdown() calls vcpu_pause_nosync() for all the domain's
> vCPU-s, which - via vcpu_sleep_nosync() - reaches the ->sleep()
> callback of the individual schedulers, all of which raise the schedule
> softirq if the subject vCPU is the one they're running on.

That's good enough for me, I'll switch to domain_crash+return without
worrying about the rest of the entries messing up the console.

Cheers,
Ian.

Re: [PATCH linux v2] arm: xen: implement multicall hypercall support.

[Stefano Stabellini]

On Mon, 7 Apr 2014, Ian Campbell wrote:
> As part of this make the usual change to xen_ulong_t in place of unsigned long.
> This change has no impact on x86.
> 
> The Linux definition of struct multicall_entry.result differs from the Xen
> definition, I think for good reasons, and used a long rather than an unsigned
> long. Therefore introduce a xen_long_t, which is a long on x86 architectures
> and a signed 64-bit integer on ARM.
> 
> Use uint32_t nr_calls on x86 for consistency with the ARM definition.
> 
> Build tested on amd64 and i386 builds. Runtime tested on ARM.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
>
> diff --git a/arch/arm/xen/hypercall.S b/arch/arm/xen/hypercall.S
> index d1cf7b7..44e3a5f 100644
> --- a/arch/arm/xen/hypercall.S
> +++ b/arch/arm/xen/hypercall.S
> @@ -89,6 +89,7 @@ HYPERCALL2(memory_op);
>  HYPERCALL2(physdev_op);
>  HYPERCALL3(vcpu_op);
>  HYPERCALL1(tmem_op);
> +HYPERCALL2(multicall);
>  
>  ENTRY(privcmd_call)
>  	stmdb sp!, {r4}

This patch is missing the same change in arch/arm64/xen/hypercall.S.

Re: [PATCH xen v2] xen: arm: fully implement multicall interface.

[Ian Campbell]

On Mon, 2014-04-07 at 14:27 +0100, Jan Beulich wrote:
> Also I think you want the loop variable to be uint32_t, in line with
> your changes elsewhere in this patch.

Actually this was a loop over nr_args from the hypercall table, which is
an int, rather than the nr_calls which comes via the hypercall
interface, so in v3 I'll stick with int.

Ian.