Xen devel - Feb 2012 - [PATCH-WIP 00/13] xen/arm: receive Xen events and initialize xenbus

Hi all,
this patch series is part of the work in progress support for Xen on
ARMv7 with virtualization extensions in Linux.

It is obviously NOT ready to be accepted upstream but implements
enough support to allow Linux Dom0 to receive event channel
notifications and initialize xenbus.
With this series applied and the corresponding Xen patch series
(http://marc.info/?l=xen-devel&m=133001696312879) is possible to boot
Linux as Dom0 on Xen on a Versatile Express Cortex A15 emulator and
issue basic xl commands, like "xl list" and "xl uptime".
"xl create" is still not working though but it is the next on the list
:)


Working on this series it became obvious that passing the hypercall
number as IMM parameter to HVC is not flexible enough because we don''t
always know the hypercall number at compile time.
As a result I changed the hypercall.h header file to use r12 to pass the
hypercall number instead. r12 was chosen because it is defined as
"intra-procedure call scratch register" so it seems the most
appropriate.

I have CC''ed the KVM list on the first patch because following previous
discussions hypercall.h might become a common header file to issue
hypercalls on different hypervisors on ARM. I haven''t disentangled the
Xen specific bits from the generic ones yet, however it should be
straightforward.

I am looking forward to hearing your opinions, especially on the
hypercall calling convention.


The patch series is available here:

git://xenbits.xen.org/people/sstabellini/linux-pvhvm.git xenarmv7-1

It is based on the vexpress-dt branch of
git://xenbits.xen.org/people/dvrabel/linux.git, that we are currently
using as development tree for Linux on Xen on Cortex A15.  See
http://wiki.xen.org/wiki/Xen_ARMv7_with_Virtualization_Extensions.


The list of patches with diffstat follows:

Stefano Stabellini (13):
      xen/arm: use r12 to pass the hypercall number to the hypervisor
      xen/arm: introduce privcmp, physdev_op and memory_op hypercalls.
      xen/arm: mmu.h and page.h related definitions
      xen/arm: sync_bitops
      xen/arm: empty implementation of grant_table arch specific functions
      xen/arm: missing includes
      xen/arm: receive xen events on arm
      xen/arm: fix arm xen guest handle definitions
      xen/arm: shared_info and start_info
      xen/arm: empty implementation of xen_remap_domain_mfn_range
      xen/arm: Introduce xen_pfn_t for pfn and mfn types
      xen/arm: compile and run xenbus
      xen/arm: compile grant-table features events and xenbus, do not compile
pci

 arch/arm/Kconfig                           |    4 +
 arch/arm/include/asm/sync_bitops.h         |   17 ++++
 arch/arm/include/asm/xen/events.h          |    9 ++
 arch/arm/include/asm/xen/grant_table.h     |    2 +
 arch/arm/include/asm/xen/hypercall.h       |  111 ++++++++++++++++++----------
 arch/arm/include/asm/xen/interface.h       |   12 +--
 arch/arm/include/asm/xen/mmu.h             |   61 +++++++++++++++
 arch/arm/include/asm/xen/page.h            |   14 +++-
 arch/arm/xen/Makefile                      |    2 +-
 arch/arm/xen/enlighten.c                   |   71 ++++++++++++++++--
 arch/arm/xen/grant-table.c                 |   47 ++++++++++++
 arch/ia64/include/asm/xen/interface.h      |    3 +-
 arch/x86/include/asm/xen/interface.h       |    3 +
 drivers/xen/Makefile                       |    7 +-
 drivers/xen/events.c                       |   36 +++++++++-
 drivers/xen/grant-table.c                  |    2 +
 drivers/xen/xenbus/xenbus_client.c         |    1 +
 drivers/xen/xenbus/xenbus_comms.c          |    2 +-
 drivers/xen/xenbus/xenbus_probe.c          |   26 ++++---
 drivers/xen/xenbus/xenbus_probe_frontend.c |    1 +
 drivers/xen/xenbus/xenbus_xs.c             |    3 +-
 drivers/xen/xenfs/xenstored.c              |    1 +
 include/xen/interface/grant_table.h        |    4 +-
 include/xen/interface/memory.h             |    6 +-
 include/xen/interface/platform.h           |    4 +-
 include/xen/interface/xen.h                |    6 +-
 include/xen/privcmd.h                      |    3 +-
 include/xen/xen.h                          |    2 +-
 28 files changed, 371 insertions(+), 89 deletions(-)


Cheers,

Stefano

We need a register to pass the hypercall number because we might not
know it at compile time and HVC only takes an immediate argument.

Among the available registers r12 seems to be the best choice because it
is defined as "intra-procedure call scratch register".

Use the ISS to pass an hypervisor specific tag.

Signed-off-by: Stefano Stabellini
<stefano.stabellini-mvvWK6WmYclDPfheJLI6IQ@public.gmane.org>
CC: kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 arch/arm/include/asm/xen/hypercall.h |   87 +++++++++++++++++++---------------
 1 files changed, 48 insertions(+), 39 deletions(-)

diff --git a/arch/arm/include/asm/xen/hypercall.h
b/arch/arm/include/asm/xen/hypercall.h
index 404e63f0..04eba1c 100644
--- a/arch/arm/include/asm/xen/hypercall.h
+++ b/arch/arm/include/asm/xen/hypercall.h
@@ -33,13 +33,17 @@
 #ifndef _ASM_ARM_XEN_HYPERCALL_H
 #define _ASM_ARM_XEN_HYPERCALL_H
 
-#define __HVC_IMM(name)	"( " #name " & 0xf) + "	  \
-			    "((" #name " << 4) & 0xfff00)"
+#include <xen/interface/xen.h>
+#include <asm/errno.h>
 
-#define ____HYPERCALL(name) ".word 0xe1400070 + " __HVC_IMM(name)
-#define __HYPERCALL(name) ____HYPERCALL(__HYPERVISOR_##name)
+#define XEN_HYPERCALL_TAG  "0XEA1"
+
+#define __HVC_IMM(tag)	"( " tag " & 0xf) + "	  \
+			    "((" tag " << 4) & 0xfff00)"
+#define __HYPERCALL ".word 0xe1400070 + "
__HVC_IMM(XEN_HYPERCALL_TAG)
 
 #define __HYPERCALL_RETREG	"r0"
+#define __HYPERCALL_NUMBER	"r12"
 #define __HYPERCALL_ARG1REG	"r0"
 #define __HYPERCALL_ARG2REG	"r1"
 #define __HYPERCALL_ARG3REG	"r2"
@@ -48,30 +52,32 @@
 
 #define __HYPERCALL_DECLS						\
 	register unsigned long __res  asm(__HYPERCALL_RETREG);		\
+	register unsigned long __num  asm(__HYPERCALL_NUMBER) = __num; \
 	register unsigned long __arg1 asm(__HYPERCALL_ARG1REG) = __arg1; \
 	register unsigned long __arg2 asm(__HYPERCALL_ARG2REG) = __arg2; \
 	register unsigned long __arg3 asm(__HYPERCALL_ARG3REG) = __arg3; \
 	register unsigned long __arg4 asm(__HYPERCALL_ARG4REG) = __arg4; \
 	register unsigned long __arg5 asm(__HYPERCALL_ARG5REG) = __arg5;
 
-#define __HYPERCALL_0PARAM	"=r" (__res)
+#define __HYPERCALL_0PARAM	"=r" (__res), "+r" (__num)
 #define __HYPERCALL_1PARAM	__HYPERCALL_0PARAM, "+r" (__arg1)
 #define __HYPERCALL_2PARAM	__HYPERCALL_1PARAM, "+r" (__arg2)
 #define __HYPERCALL_3PARAM	__HYPERCALL_2PARAM, "+r" (__arg3)
 #define __HYPERCALL_4PARAM	__HYPERCALL_3PARAM, "+r" (__arg4)
 #define __HYPERCALL_5PARAM	__HYPERCALL_4PARAM, "+r" (__arg5)
 
-#define __HYPERCALL_0ARG()
-#define __HYPERCALL_1ARG(a1)						\
-	__HYPERCALL_0ARG()		__arg1 = (unsigned long)(a1);
-#define __HYPERCALL_2ARG(a1,a2)						\
-	__HYPERCALL_1ARG(a1)		__arg2 = (unsigned long)(a2);
-#define __HYPERCALL_3ARG(a1,a2,a3)					\
-	__HYPERCALL_2ARG(a1,a2)		__arg3 = (unsigned long)(a3);
-#define __HYPERCALL_4ARG(a1,a2,a3,a4)					\
-	__HYPERCALL_3ARG(a1,a2,a3)	__arg4 = (unsigned long)(a4);
-#define __HYPERCALL_5ARG(a1,a2,a3,a4,a5)				\
-	__HYPERCALL_4ARG(a1,a2,a3,a4)	__arg5 = (unsigned long)(a5);
+#define __HYPERCALL_0ARG(hypercall)						\
+	__num = (unsigned long)hypercall;
+#define __HYPERCALL_1ARG(hypercall,a1)						\
+	__HYPERCALL_0ARG(hypercall)		__arg1 = (unsigned long)(a1);
+#define __HYPERCALL_2ARG(hypercall,a1,a2)						\
+	__HYPERCALL_1ARG(hypercall,a1)		__arg2 = (unsigned long)(a2);
+#define __HYPERCALL_3ARG(hypercall,a1,a2,a3)					\
+	__HYPERCALL_2ARG(hypercall,a1,a2)		__arg3 = (unsigned long)(a3);
+#define __HYPERCALL_4ARG(hypercall,a1,a2,a3,a4)					\
+	__HYPERCALL_3ARG(hypercall,a1,a2,a3)	__arg4 = (unsigned long)(a4);
+#define __HYPERCALL_5ARG(hypercall,a1,a2,a3,a4,a5)				\
+	__HYPERCALL_4ARG(hypercall,a1,a2,a3,a4)	__arg5 = (unsigned long)(a5);
 
 #define __HYPERCALL_CLOBBER5	"memory"
 #define __HYPERCALL_CLOBBER4	__HYPERCALL_CLOBBER5, __HYPERCALL_ARG5REG
@@ -80,102 +86,105 @@
 #define __HYPERCALL_CLOBBER1	__HYPERCALL_CLOBBER2, __HYPERCALL_ARG2REG
 #define __HYPERCALL_CLOBBER0	__HYPERCALL_CLOBBER1, __HYPERCALL_ARG1REG
 
-#define _hypercall0(type, name)						\
+#define _hypercall0(type, hypercall)						\
 ({									\
 	__HYPERCALL_DECLS;						\
-	__HYPERCALL_0ARG();						\
-	asm volatile (__HYPERCALL(name)					\
+	__HYPERCALL_0ARG(hypercall);						\
+	asm volatile (__HYPERCALL					\
 		      : __HYPERCALL_0PARAM				\
 		      : 						\
 		      : __HYPERCALL_CLOBBER0);				\
 	(type)__res;							\
 })
 
-#define _hypercall1(type, name, a1)					\
+#define _hypercall1(type, hypercall, a1)					\
 ({									\
 	__HYPERCALL_DECLS;						\
-	__HYPERCALL_1ARG(a1);						\
-	asm volatile (__HYPERCALL(name)					\
+	__HYPERCALL_1ARG(hypercall, a1);						\
+	asm volatile (__HYPERCALL					\
 		      : __HYPERCALL_1PARAM				\
 		      : 						\
 		      : __HYPERCALL_CLOBBER1);				\
 	(type)__res;							\
 })
 
-#define _hypercall2(type, name, a1, a2)					\
+#define _hypercall2(type, hypercall, a1, a2)					\
 ({									\
 	__HYPERCALL_DECLS;						\
-	__HYPERCALL_2ARG(a1, a2);					\
-	asm volatile (__HYPERCALL(name)					\
+	__HYPERCALL_2ARG(hypercall, a1, a2);					\
+	asm volatile (__HYPERCALL					\
 		      : __HYPERCALL_2PARAM				\
 		      : 						\
 		      : __HYPERCALL_CLOBBER2);				\
 	(type)__res;							\
 })
 
-#define _hypercall3(type, name, a1, a2, a3)				\
+#define _hypercall3(type, hypercall, a1, a2, a3)				\
 ({									\
 	__HYPERCALL_DECLS;						\
-	__HYPERCALL_3ARG(a1, a2, a3);					\
-	asm volatile (__HYPERCALL(name)					\
+	__HYPERCALL_3ARG(hypercall, a1, a2, a3);					\
+	asm volatile (__HYPERCALL					\
 		      : __HYPERCALL_3PARAM				\
 		      : 						\
 		      : __HYPERCALL_CLOBBER3);				\
 	(type)__res;							\
 })
 
-#define _hypercall4(type, name, a1, a2, a3, a4)				\
+#define _hypercall4(type, hypercall, a1, a2, a3, a4)				\
 ({									\
 	__HYPERCALL_DECLS;						\
-	__HYPERCALL_4ARG(a1, a2, a3, a4);				\
-	asm volatile (__HYPERCALL(name)					\
+	__HYPERCALL_4ARG(hypercall, a1, a2, a3, a4);				\
+	asm volatile (__HYPERCALL					\
 		      : __HYPERCALL_4PARAM				\
 		      : 						\
 		      : __HYPERCALL_CLOBBER4);				\
 	(type)__res;							\
 })
 
-#define _hypercall5(type, name, a1, a2, a3, a4, a5)			\
+#define _hypercall5(type, hypercall, a1, a2, a3, a4, a5)			\
 ({									\
 	__HYPERCALL_DECLS;						\
-	__HYPERCALL_5ARG(a1, a2, a3, a4, a5);				\
-	asm volatile (__HYPERCALL(name)					\
+	__HYPERCALL_5ARG(hypercall, a1, a2, a3, a4, a5);				\
+	asm volatile (__HYPERCALL					\
 		      : __HYPERCALL_5PARAM				\
 		      : 						\
 		      : __HYPERCALL_CLOBBER5);				\
 	(type)__res;							\
 })
 
+#define HYPERCALL(name) \
+	(__HYPERVISOR_##name)
+
 /* -- Hypercall definitions go below -- */
 
 static inline int
 HYPERVISOR_xen_version(int cmd, void *arg)
 {
-	return _hypercall2(int, xen_version, cmd, arg);
+	return _hypercall2(int, HYPERCALL(xen_version), cmd, arg);
 }
 
 static inline int
 HYPERVISOR_console_io(int cmd, int count, char *str)
 {
-	return _hypercall3(int, console_io, cmd, count, str);
+	return _hypercall3(int, HYPERCALL(console_io), cmd, count, str);
 }
 
 static inline int
 HYPERVISOR_grant_table_op(unsigned int cmd, void *uop, unsigned int count)
 {
-	return _hypercall3(int, grant_table_op, cmd, uop, count);
+	return _hypercall3(int, HYPERCALL(grant_table_op), cmd, uop, count);
 }
 
 static inline int
 HYPERVISOR_sched_op(int cmd, void *arg)
 {
-	return _hypercall2(int, sched_op, cmd, arg);
+	return _hypercall2(int, HYPERCALL(sched_op), cmd, arg);
 }
 
 static inline int
 HYPERVISOR_event_channel_op(int cmd, void *arg)
 {
-	return _hypercall2(int, event_channel_op, cmd, arg);
+	return _hypercall2(int, HYPERCALL(event_channel_op), cmd, arg);
 }
 
 #endif /* _ASM_ARM_XEN_HYPERCALL_H */
-- 
1.7.2.5

Signed-off-by: Stefano Stabellini
<stefano.stabellini-mvvWK6WmYclDPfheJLI6IQ@public.gmane.org>
---
 arch/arm/include/asm/xen/hypercall.h |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/xen/hypercall.h
b/arch/arm/include/asm/xen/hypercall.h
index 04eba1c..5abba48 100644
--- a/arch/arm/include/asm/xen/hypercall.h
+++ b/arch/arm/include/asm/xen/hypercall.h
@@ -187,4 +187,28 @@ HYPERVISOR_event_channel_op(int cmd, void *arg)
 	return _hypercall2(int, HYPERCALL(event_channel_op), cmd, arg);
 }
 
+static inline unsigned long HYPERVISOR_hvm_op(int op, void *arg)
+{
+       return -ENOSYS;
+}
+
+static inline int
+HYPERVISOR_memory_op(unsigned int cmd, void *arg)
+{
+	return _hypercall2(int, HYPERCALL(memory_op), cmd, arg);
+}
+
+static inline int HYPERVISOR_physdev_op(int cmd, void *arg)
+{
+	return _hypercall2(int, HYPERCALL(physdev_op), cmd, arg);
+}
+
+static inline long privcmd_call(unsigned call,
+		unsigned long a1, unsigned long a2,
+		unsigned long a3, unsigned long a4,
+		unsigned long a5)
+{
+	return _hypercall5(long, call, a1, a2, a3, a4, a5);
+}
+
 #endif /* _ASM_ARM_XEN_HYPERCALL_H */
-- 
1.7.2.5

Signed-off-by: Stefano Stabellini
<stefano.stabellini-mvvWK6WmYclDPfheJLI6IQ@public.gmane.org>
---
 arch/arm/include/asm/xen/mmu.h  |   61 +++++++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/xen/page.h |   14 +++++++--
 2 files changed, 72 insertions(+), 3 deletions(-)
 create mode 100644 arch/arm/include/asm/xen/mmu.h

diff --git a/arch/arm/include/asm/xen/mmu.h b/arch/arm/include/asm/xen/mmu.h
new file mode 100644
index 0000000..23e9962
--- /dev/null
+++ b/arch/arm/include/asm/xen/mmu.h
@@ -0,0 +1,61 @@
+#ifndef _ASM_ARM_XEN_MMU_H
+#define _ASM_ARM_XEN_MMU_H
+
+#include <asm/page.h>
+#include <linux/types.h>
+#include <xen/interface/grant_table.h>
+
+/* Xen machine address */
+typedef struct xmaddr {
+	phys_addr_t maddr;
+} xmaddr_t;
+
+/* Xen pseudo-physical address */
+typedef struct xpaddr {
+	phys_addr_t paddr;
+} xpaddr_t;
+
+#define XMADDR(x)	((xmaddr_t) { .maddr = (x) })
+#define XPADDR(x)	((xpaddr_t) { .paddr = (x) })
+
+static inline xmaddr_t phys_to_machine(xpaddr_t phys)
+{
+	unsigned offset = phys.paddr & ~PAGE_MASK;
+	return XMADDR(PFN_PHYS(pfn_to_mfn(PFN_DOWN(phys.paddr))) | offset);
+}
+
+static inline xpaddr_t machine_to_phys(xmaddr_t machine)
+{
+	unsigned offset = machine.maddr & ~PAGE_MASK;
+	return XPADDR(PFN_PHYS(mfn_to_pfn(PFN_DOWN(machine.maddr))) | offset);
+}
+/* VIRT <-> MACHINE conversion */
+#define virt_to_machine(v)	(phys_to_machine(XPADDR(__pa(v))))
+#define virt_to_pfn(v)          (PFN_DOWN(__pa(v)))
+#define virt_to_mfn(v)		(pfn_to_mfn(virt_to_pfn(v)))
+#define mfn_to_virt(m)		(__va(mfn_to_pfn(m) << PAGE_SHIFT))
+
+static inline xmaddr_t arbitrary_virt_to_machine(void *vaddr)
+{
+	/* XXX: assuming it is mapped in the kernel 1:1 */
+	return virt_to_machine(vaddr);
+}
+
+/* XXX: this shouldn''t be here */
+static inline pte_t *lookup_address(unsigned long address, unsigned int *level)
+{
+	BUG();
+	return NULL;
+}
+
+static inline int m2p_add_override(unsigned long mfn, struct page *page,
+		struct gnttab_map_grant_ref *kmap_op)
+{
+	return 0;
+}
+
+static inline int m2p_remove_override(struct page *page, bool clear_pte)
+{
+	return 0;
+}
+#endif
diff --git a/arch/arm/include/asm/xen/page.h b/arch/arm/include/asm/xen/page.h
index 17bfb55..5ee3dbe 100644
--- a/arch/arm/include/asm/xen/page.h
+++ b/arch/arm/include/asm/xen/page.h
@@ -1,8 +1,16 @@
 #ifndef _ASM_ARM_XEN_PAGE_H
 #define _ASM_ARM_XEN_PAGE_H
 
-#define mfn_to_virt(m)		(~0)
-#define mfn_to_pfn(m)		(~0)
-#define pfn_to_mfn(m)		(~0)
+#include <asm/page.h>
+#include <asm/pgtable.h>
+#include <linux/types.h>
+
+#define pfn_to_mfn(pfn)			(pfn)
+#define phys_to_machine_mapping_valid	(1)
+#define mfn_to_pfn(mfn)			(mfn)
+#define mfn_to_virt(m)			(__va(mfn_to_pfn(m) << PAGE_SHIFT))
+
+#define pte_mfn	    pte_pfn
+#define mfn_pte	    pfn_pte
 
 #endif /* _ASM_ARM_XEN_PAGE_H */
-- 
1.7.2.5

sync_bitops functions are equivalent to the SMP implementation of the
original functions, independently from CONFIG_SMP being defined.

Signed-off-by: Stefano Stabellini
<stefano.stabellini-mvvWK6WmYclDPfheJLI6IQ@public.gmane.org>
---
 arch/arm/include/asm/sync_bitops.h |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/include/asm/sync_bitops.h

diff --git a/arch/arm/include/asm/sync_bitops.h
b/arch/arm/include/asm/sync_bitops.h
new file mode 100644
index 0000000..2b51456
--- /dev/null
+++ b/arch/arm/include/asm/sync_bitops.h
@@ -0,0 +1,17 @@
+#ifndef __ASM_SYNC_BITOPS_H__
+#define __ASM_SYNC_BITOPS_H__
+
+#include <asm/bitops.h>
+#include <asm/system.h>
+
+#define sync_set_bit(nr,p)		_set_bit(nr,p)
+#define sync_clear_bit(nr,p)		_clear_bit(nr,p)
+#define sync_change_bit(nr,p)		_change_bit(nr,p)
+#define sync_test_and_set_bit(nr,p)	_test_and_set_bit(nr,p)
+#define sync_test_and_clear_bit(nr,p)	_test_and_clear_bit(nr,p)
+#define sync_test_and_change_bit(nr,p)	_test_and_change_bit(nr,p)
+#define sync_test_bit(nr, addr)		test_bit(nr, addr)
+#define sync_cmpxchg			cmpxchg
+
+
+#endif
-- 
1.7.2.5

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/arm/xen/Makefile      |    2 +-
 arch/arm/xen/grant-table.c |   47 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/xen/grant-table.c

diff --git a/arch/arm/xen/Makefile b/arch/arm/xen/Makefile
index 0bad594..563f22a 100644
--- a/arch/arm/xen/Makefile
+++ b/arch/arm/xen/Makefile
@@ -1 +1 @@
-obj-y		:= enlighten.o
+obj-y		:= enlighten.o grant-table.o
diff --git a/arch/arm/xen/grant-table.c b/arch/arm/xen/grant-table.c
new file mode 100644
index 0000000..b82a799
--- /dev/null
+++ b/arch/arm/xen/grant-table.c
@@ -0,0 +1,47 @@
+/******************************************************************************
+ * grant_table.c
+ * ARM specific part
+ *
+ * Granting foreign access to our memory reservation.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation; or, when distributed
+ * separately from the Linux kernel or incorporated into other
+ * software packages, subject to the following license:
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this source file (the "Software"), to deal in the Software
without
+ * restriction, including without limitation the rights to use, copy, modify,
+ * merge, publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include <xen/interface/xen.h>
+#include <xen/page.h>
+#include <xen/grant_table.h>
+
+int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
+			   unsigned long max_nr_gframes,
+			   struct grant_entry **__shared)
+{
+	return -1;
+}
+
+void arch_gnttab_unmap_shared(struct grant_entry *shared,
+			      unsigned long nr_gframes)
+{
+	return;
+}
-- 
1.7.2.5

Signed-off-by: Stefano Stabellini
<stefano.stabellini-mvvWK6WmYclDPfheJLI6IQ@public.gmane.org>
---
 arch/arm/include/asm/xen/grant_table.h     |    2 ++
 drivers/xen/grant-table.c                  |    2 ++
 drivers/xen/xenbus/xenbus_client.c         |    1 +
 drivers/xen/xenbus/xenbus_probe_frontend.c |    1 +
 drivers/xen/xenfs/xenstored.c              |    1 +
 include/xen/privcmd.h                      |    1 +
 6 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/xen/grant_table.h
b/arch/arm/include/asm/xen/grant_table.h
index 4e3f7b2..43c0d4b 100644
--- a/arch/arm/include/asm/xen/grant_table.h
+++ b/arch/arm/include/asm/xen/grant_table.h
@@ -1,6 +1,8 @@
 #ifndef _ASM_ARM_XEN_GRANT_TABLE_H
 #define _ASM_ARM_XEN_GRANT_TABLE_H
 
+#include <asm/xen/mmu.h>
+
 #define xen_alloc_vm_area(size)	alloc_vm_area(size)
 #define xen_free_vm_area(area)	free_vm_area(area)
 
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index bf1c094..de77304 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -45,6 +45,8 @@
 #include <xen/grant_table.h>
 #include <xen/interface/memory.h>
 #include <asm/xen/hypercall.h>
+#include <asm/xen/mmu.h>
+#include <asm/xen/interface.h>
 
 #include <asm/pgtable.h>
 #include <asm/sync_bitops.h>
diff --git a/drivers/xen/xenbus/xenbus_client.c
b/drivers/xen/xenbus/xenbus_client.c
index 1906125..65088964 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -36,6 +36,7 @@
 #include <linux/export.h>
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/page.h>
+#include <asm/xen/grant_table.h>
 #include <xen/interface/xen.h>
 #include <xen/interface/event_channel.h>
 #include <xen/events.h>
diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c
b/drivers/xen/xenbus/xenbus_probe_frontend.c
index 2f73195..e8d1798 100644
--- a/drivers/xen/xenbus/xenbus_probe_frontend.c
+++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
@@ -21,6 +21,7 @@
 #include <xen/xenbus.h>
 #include <xen/events.h>
 #include <xen/page.h>
+#include <xen/xen.h>
 
 #include <xen/platform_pci.h>
 
diff --git a/drivers/xen/xenfs/xenstored.c b/drivers/xen/xenfs/xenstored.c
index fef20db..ddf2585 100644
--- a/drivers/xen/xenfs/xenstored.c
+++ b/drivers/xen/xenfs/xenstored.c
@@ -4,6 +4,7 @@
 #include <linux/fs.h>
 
 #include <xen/page.h>
+#include <asm/xen/mmu.h>
 
 #include "xenfs.h"
 #include "../xenbus/xenbus_comms.h"
diff --git a/include/xen/privcmd.h b/include/xen/privcmd.h
index 17857fb..4d58881 100644
--- a/include/xen/privcmd.h
+++ b/include/xen/privcmd.h
@@ -35,6 +35,7 @@
 
 #include <linux/types.h>
 #include <linux/compiler.h>
+#include <xen/interface/xen.h>
 
 typedef unsigned long xen_pfn_t;
 
-- 
1.7.2.5

Compile events.c and use IRQ 32 to receive events notifications.

Signed-off-by: Stefano Stabellini
<stefano.stabellini-mvvWK6WmYclDPfheJLI6IQ@public.gmane.org>
---
 arch/arm/include/asm/xen/events.h |    9 +++++++++
 drivers/xen/events.c              |   36 +++++++++++++++++++++++++++++++++++-
 2 files changed, 44 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/xen/events.h
b/arch/arm/include/asm/xen/events.h
index efa7c61..94b4e90 100644
--- a/arch/arm/include/asm/xen/events.h
+++ b/arch/arm/include/asm/xen/events.h
@@ -1,9 +1,18 @@
 #ifndef _ASM_ARM_XEN_EVENTS_H
 #define _ASM_ARM_XEN_EVENTS_H
 
+#include <asm/ptrace.h>
+
 enum ipi_vector {
+	XEN_PLACEHOLDER_VECTOR,
+
 	/* Xen IPIs go here */
 	XEN_NR_IPIS,
 };
 
+static inline int xen_irqs_disabled(struct pt_regs *regs)
+{
+	return raw_irqs_disabled_flags(regs->ARM_cpsr);
+}
+
 #endif /* _ASM_ARM_XEN_EVENTS_H */
diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 6e075cd..18139ee 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -31,13 +31,15 @@
 #include <linux/irqnr.h>
 #include <linux/pci.h>
 
+#ifdef CONFIG_X86
 #include <asm/desc.h>
 #include <asm/ptrace.h>
 #include <asm/irq.h>
 #include <asm/idle.h>
 #include <asm/io_apic.h>
-#include <asm/sync_bitops.h>
 #include <asm/xen/pci.h>
+#endif
+#include <asm/sync_bitops.h>
 #include <asm/xen/hypercall.h>
 #include <asm/xen/hypervisor.h>
 
@@ -49,6 +51,8 @@
 #include <xen/interface/event_channel.h>
 #include <xen/interface/hvm/hvm_op.h>
 #include <xen/interface/hvm/params.h>
+#include <xen/interface/physdev.h>
+#include <xen/interface/sched.h>
 
 /*
  * This lock protects updates to the following mapping and reference-count
@@ -801,10 +805,12 @@ EXPORT_SYMBOL_GPL(xen_pirq_from_irq);
 int bind_evtchn_to_irq(unsigned int evtchn)
 {
 	int irq;
+	struct irq_desc *desc;
 
 	mutex_lock(&irq_mapping_update_lock);
 
 	irq = evtchn_to_irq[evtchn];
+	irq_clear_status_flags(irq, IRQ_NOREQUEST);
 
 	if (irq == -1) {
 		irq = xen_allocate_irq_dynamic();
@@ -813,6 +819,8 @@ int bind_evtchn_to_irq(unsigned int evtchn)
 
 		irq_set_chip_and_handler_name(irq, &xen_dynamic_chip,
 					      handle_edge_irq, "event");
+		desc = irq_to_desc(irq);
+		irq_clear_status_flags(irq, IRQ_NOREQUEST);
 
 		xen_irq_info_evtchn_init(irq, evtchn);
 	}
@@ -1282,7 +1290,9 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
 {
 	struct pt_regs *old_regs = set_irq_regs(regs);
 
+#ifdef CONFIG_X86
 	exit_idle();
+#endif
 	irq_enter();
 
 	__xen_evtchn_do_upcall();
@@ -1707,6 +1717,7 @@ void __init xen_init_IRQ(void)
 	for (i = 0; i < NR_EVENT_CHANNELS; i++)
 		mask_evtchn(i);
 
+#ifdef CONFIG_X86
 	if (xen_hvm_domain()) {
 		xen_callback_vector();
 		native_init_IRQ();
@@ -1718,4 +1729,27 @@ void __init xen_init_IRQ(void)
 		if (xen_initial_domain())
 			pci_xen_initial_domain();
 	}
+#endif
 }
+#ifdef CONFIG_ARM
+#define IRQ_EVTCHN_CALLBACK 63
+irqreturn_t xen_arm_callback(int irq, void *arg)
+{
+	__xen_evtchn_do_upcall();
+	return 0;
+}
+
+int __init xen_init_IRQ_arm(void)
+{
+	int rc;
+	xen_init_IRQ();
+	rc = request_irq(IRQ_EVTCHN_CALLBACK, xen_arm_callback,
+			IRQF_DISABLED | IRQF_NOBALANCING | IRQF_TRIGGER_RISING,
+			"events", "events");	
+	if (rc) {
+		printk(KERN_ERR "Error requesting IRQ %d\n", IRQ_EVTCHN_CALLBACK);
+	}
+	return rc;
+}
+core_initcall(xen_init_IRQ_arm);
+#endif
-- 
1.7.2.5

__XEN__ is never defined in Linux: remove non-relevant functions and
macros

Signed-off-by: Stefano Stabellini
<stefano.stabellini-mvvWK6WmYclDPfheJLI6IQ@public.gmane.org>
---
 arch/arm/include/asm/xen/interface.h |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/arch/arm/include/asm/xen/interface.h
b/arch/arm/include/asm/xen/interface.h
index 93b0139..2ee39e8 100644
--- a/arch/arm/include/asm/xen/interface.h
+++ b/arch/arm/include/asm/xen/interface.h
@@ -9,27 +9,20 @@
 
 #include <linux/types.h>
 
-#ifdef __XEN__
-#define __DEFINE_GUEST_HANDLE(name, type) \
-    typedef struct { type *p; } __guest_handle_ ## name
-#else
 #define __DEFINE_GUEST_HANDLE(name, type) \
     typedef type * __guest_handle_ ## name
-#endif
 
 #define DEFINE_GUEST_HANDLE_STRUCT(name) \
 	__DEFINE_GUEST_HANDLE(name, struct name)
 #define DEFINE_GUEST_HANDLE(name) __DEFINE_GUEST_HANDLE(name, name)
 #define GUEST_HANDLE(name)        __guest_handle_ ## name
 
-#ifdef __XEN__
 #define set_xen_guest_handle(hnd, val)			\
 	do {						\
 		if (sizeof(hnd) == 8)			\
 			*(uint64_t *)&(hnd) = 0;	\
-		(hnd).p = val;				\
+		(hnd) = val;				\
 	} while (0)
-#endif
 
 #ifndef __ASSEMBLY__
 /* Guest handles for primitive C types. */
-- 
1.7.2.5

Allow xen_hvm_domain''s to be xen_initial_domain.

Set xen_domain_type to XEN_HVM_DOMAIN.

Set xen_start_info to an empty struct, set flags to SIF_INITDOMAIN and
SIF_PRIVILEGED so that we identify as initial domain by default.

Map the real shared info page using XENMEM_add_to_physmap with
XENMAPSPACE_shared_info.

Signed-off-by: Stefano Stabellini
<stefano.stabellini-mvvWK6WmYclDPfheJLI6IQ@public.gmane.org>
---
 arch/arm/xen/enlighten.c |   61 ++++++++++++++++++++++++++++++++++++++++------
 include/xen/xen.h        |    2 +-
 2 files changed, 54 insertions(+), 9 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 39ef68c..d76f3b4e 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -1,24 +1,69 @@
 #include <xen/xen.h>
 #include <xen/interface/xen.h>
+#include <xen/interface/memory.h>
 #include <asm/xen/hypervisor.h>
+#include <asm/xen/hypercall.h>
 #include <linux/module.h>
 
-struct start_info *xen_start_info;
+struct start_info _xen_start_info = { .flags = (SIF_INITDOMAIN|SIF_PRIVILEGED)
};
+struct start_info *xen_start_info = &_xen_start_info;
 EXPORT_SYMBOL_GPL(xen_start_info);
 
-enum xen_domain_type xen_domain_type = XEN_NATIVE;
+enum xen_domain_type xen_domain_type = XEN_HVM_DOMAIN;
 EXPORT_SYMBOL_GPL(xen_domain_type);
 
+struct shared_info xen_dummy_shared_info;
+struct shared_info *HYPERVISOR_shared_info = (void
*)&xen_dummy_shared_info;
 
-/* TODO: remove these functions below and use the real implementation
- * instead
- */
-void rebind_evtchn_irq(int evtchn, int irq)
+DEFINE_PER_CPU(struct vcpu_info *, xen_vcpu);
+
+/* XXX: to be removed */
+__read_mostly int xen_have_vector_callback;
+EXPORT_SYMBOL_GPL(xen_have_vector_callback);
+
+/* XXX: to be removed */
+int xen_platform_pci_unplug;
+EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
+
+void __ref xen_hvm_init_shared_info(void)
 {
+	int cpu;
+	struct xen_add_to_physmap xatp;
+	static struct shared_info *shared_info_page = 0;
+
+	if (!shared_info_page)
+		shared_info_page = (struct shared_info *)
+			get_zeroed_page(GFP_KERNEL);
+	if (!shared_info_page) {
+		printk(KERN_ERR "not enough memory");
+		return;
+	}
+	xatp.domid = DOMID_SELF;
+	xatp.idx = 0;
+	xatp.space = XENMAPSPACE_shared_info;
+	xatp.gpfn = __pa(shared_info_page) >> PAGE_SHIFT;
+	if (HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp))
+		BUG();
+
+	HYPERVISOR_shared_info = (struct shared_info *)shared_info_page;
+
+	/* xen_vcpu is a pointer to the vcpu_info struct in the shared_info
+	 * page, we use it in the event channel upcall and in some pvclock
+	 * related functions. We don''t need the vcpu_info placement
+	 * optimizations because we don''t use any pv_mmu or pv_irq op on
+	 * HVM.
+	 * When xen_hvm_init_shared_info is run at boot time only vcpu 0 is
+	 * online but xen_hvm_init_shared_info is run at resume time too and
+	 * in that case multiple vcpus might be online. */
+	for_each_online_cpu(cpu) {
+		per_cpu(xen_vcpu, cpu) = &HYPERVISOR_shared_info->vcpu_info[cpu];
+	}
 }
 
-int bind_evtchn_to_irq(unsigned int evtchn)
+static int __init xen_hvm_guest_init(void)
 {
+	xen_hvm_init_shared_info();
 	return 0;
 }
-EXPORT_SYMBOL_GPL(bind_evtchn_to_irq);
+
+core_initcall(xen_hvm_guest_init);
diff --git a/include/xen/xen.h b/include/xen/xen.h
index a164024..2c0d3a5 100644
--- a/include/xen/xen.h
+++ b/include/xen/xen.h
@@ -23,7 +23,7 @@ extern enum xen_domain_type xen_domain_type;
 #include <xen/interface/xen.h>
 #include <asm/xen/hypervisor.h>
 
-#define xen_initial_domain()	(xen_pv_domain() && \
+#define xen_initial_domain()	(xen_domain() && \
 				 xen_start_info->flags & SIF_INITDOMAIN)
 #else  /* !CONFIG_XEN_DOM0 */
 #define xen_initial_domain()	(0)
-- 
1.7.2.5

Signed-off-by: Stefano Stabellini
<stefano.stabellini-mvvWK6WmYclDPfheJLI6IQ@public.gmane.org>
---
 arch/arm/xen/enlighten.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index d76f3b4e..986bec3 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -25,6 +25,16 @@ EXPORT_SYMBOL_GPL(xen_have_vector_callback);
 int xen_platform_pci_unplug;
 EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
 
+/* TODO */
+int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
+			       unsigned long addr,
+			       unsigned long mfn, int nr,
+			       pgprot_t prot, unsigned domid)
+{
+	return -ENOSYS;
+}
+EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
+
 void __ref xen_hvm_init_shared_info(void)
 {
 	int cpu;
-- 
1.7.2.5

All the original Xen headers have xen_pfn_t as mfn and pfn type, however
when they have been imported in Linux, xen_pfn_t has been replaced with
unsigned long. That might work for x86 and ia64 but it does not for arm.
Bring back xen_pfn_t and let each architecture define xen_pfn_t as they
see fit.

Signed-off-by: Stefano Stabellini
<stefano.stabellini-mvvWK6WmYclDPfheJLI6IQ@public.gmane.org>
---
 arch/arm/include/asm/xen/interface.h  |    3 +++
 arch/ia64/include/asm/xen/interface.h |    3 ++-
 arch/x86/include/asm/xen/interface.h  |    3 +++
 include/xen/interface/grant_table.h   |    4 ++--
 include/xen/interface/memory.h        |    6 +++---
 include/xen/interface/platform.h      |    4 ++--
 include/xen/interface/xen.h           |    6 +++---
 include/xen/privcmd.h                 |    2 --
 8 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/arch/arm/include/asm/xen/interface.h
b/arch/arm/include/asm/xen/interface.h
index 2ee39e8..65577de 100644
--- a/arch/arm/include/asm/xen/interface.h
+++ b/arch/arm/include/asm/xen/interface.h
@@ -24,6 +24,8 @@
 		(hnd) = val;				\
 	} while (0)
 
+typedef uint64_t xen_pfn_t;
+
 #ifndef __ASSEMBLY__
 /* Guest handles for primitive C types. */
 __DEFINE_GUEST_HANDLE(uchar, unsigned char);
@@ -33,6 +35,7 @@ DEFINE_GUEST_HANDLE(char);
 DEFINE_GUEST_HANDLE(int);
 DEFINE_GUEST_HANDLE(long);
 DEFINE_GUEST_HANDLE(void);
+DEFINE_GUEST_HANDLE(xen_pfn_t);
 #endif
 
 /* Maximum number of virtual CPUs in multi-processor guests. */
diff --git a/arch/ia64/include/asm/xen/interface.h
b/arch/ia64/include/asm/xen/interface.h
index 1d2427d..18904ac 100644
--- a/arch/ia64/include/asm/xen/interface.h
+++ b/arch/ia64/include/asm/xen/interface.h
@@ -66,6 +66,8 @@
 #define GUEST_HANDLE_64(name)		GUEST_HANDLE(name)
 #define set_xen_guest_handle(hnd, val)	do { (hnd).p = val; } while (0)
 
+typedef unsigned long xen_pfn_t;
+
 #ifndef __ASSEMBLY__
 /* Guest handles for primitive C types. */
 __DEFINE_GUEST_HANDLE(uchar, unsigned char);
@@ -78,7 +80,6 @@ DEFINE_GUEST_HANDLE(long);
 DEFINE_GUEST_HANDLE(void);
 DEFINE_GUEST_HANDLE(uint64_t);
 
-typedef unsigned long xen_pfn_t;
 DEFINE_GUEST_HANDLE(xen_pfn_t);
 #define PRI_xen_pfn	"lx"
 #endif
diff --git a/arch/x86/include/asm/xen/interface.h
b/arch/x86/include/asm/xen/interface.h
index a1f2db5..59a37ec 100644
--- a/arch/x86/include/asm/xen/interface.h
+++ b/arch/x86/include/asm/xen/interface.h
@@ -46,6 +46,8 @@
 #endif
 #endif
 
+typedef unsigned long xen_pfn_t;
+
 #ifndef __ASSEMBLY__
 /* Guest handles for primitive C types. */
 __DEFINE_GUEST_HANDLE(uchar, unsigned char);
@@ -56,6 +58,7 @@ DEFINE_GUEST_HANDLE(int);
 DEFINE_GUEST_HANDLE(long);
 DEFINE_GUEST_HANDLE(void);
 DEFINE_GUEST_HANDLE(uint64_t);
+DEFINE_GUEST_HANDLE(xen_pfn_t);
 #endif
 
 #ifndef HYPERVISOR_VIRT_START
diff --git a/include/xen/interface/grant_table.h
b/include/xen/interface/grant_table.h
index 39e5717..1fd3a66 100644
--- a/include/xen/interface/grant_table.h
+++ b/include/xen/interface/grant_table.h
@@ -254,7 +254,7 @@ DEFINE_GUEST_HANDLE_STRUCT(gnttab_dump_table);
 #define GNTTABOP_transfer                4
 struct gnttab_transfer {
     /* IN parameters. */
-    unsigned long mfn;
+    xen_pfn_t mfn;
     domid_t       domid;
     grant_ref_t   ref;
     /* OUT parameters. */
@@ -291,7 +291,7 @@ struct gnttab_copy {
 	struct {
 		union {
 			grant_ref_t ref;
-			unsigned long   gmfn;
+			xen_pfn_t   gmfn;
 		} u;
 		domid_t  domid;
 		uint16_t offset;
diff --git a/include/xen/interface/memory.h b/include/xen/interface/memory.h
index eac3ce1..abbbff0 100644
--- a/include/xen/interface/memory.h
+++ b/include/xen/interface/memory.h
@@ -31,7 +31,7 @@ struct xen_memory_reservation {
      *   OUT: GMFN bases of extents that were allocated
      *   (NB. This command also updates the mach_to_phys translation table)
      */
-    GUEST_HANDLE(ulong) extent_start;
+    GUEST_HANDLE(xen_pfn_t) extent_start;
 
     /* Number of extents, and size/alignment of each (2^extent_order pages). */
     unsigned long  nr_extents;
@@ -130,7 +130,7 @@ struct xen_machphys_mfn_list {
      * any large discontiguities in the machine address space, 2MB gaps in
      * the machphys table will be represented by an MFN base of zero.
      */
-    GUEST_HANDLE(ulong) extent_start;
+    GUEST_HANDLE(xen_pfn_t) extent_start;
 
     /*
      * Number of extents written to the above array. This will be smaller
@@ -172,7 +172,7 @@ struct xen_add_to_physmap {
     unsigned long idx;
 
     /* GPFN where the source mapping page should appear. */
-    unsigned long gpfn;
+    xen_pfn_t gpfn;
 };
 DEFINE_GUEST_HANDLE_STRUCT(xen_add_to_physmap);
 
diff --git a/include/xen/interface/platform.h b/include/xen/interface/platform.h
index c168468..1c172ce 100644
--- a/include/xen/interface/platform.h
+++ b/include/xen/interface/platform.h
@@ -54,7 +54,7 @@ DEFINE_GUEST_HANDLE_STRUCT(xenpf_settime_t);
 #define XENPF_add_memtype         31
 struct xenpf_add_memtype {
 	/* IN variables. */
-	unsigned long mfn;
+	xen_pfn_t mfn;
 	uint64_t nr_mfns;
 	uint32_t type;
 	/* OUT variables. */
@@ -84,7 +84,7 @@ struct xenpf_read_memtype {
 	/* IN variables. */
 	uint32_t reg;
 	/* OUT variables. */
-	unsigned long mfn;
+	xen_pfn_t mfn;
 	uint64_t nr_mfns;
 	uint32_t type;
 };
diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
index 19354db..3edc110 100644
--- a/include/xen/interface/xen.h
+++ b/include/xen/interface/xen.h
@@ -192,7 +192,7 @@ struct mmuext_op {
 	unsigned int cmd;
 	union {
 		/* [UN]PIN_TABLE, NEW_BASEPTR, NEW_USER_BASEPTR */
-		unsigned long mfn;
+		xen_pfn_t mfn;
 		/* INVLPG_LOCAL, INVLPG_ALL, SET_LDT */
 		unsigned long linear_addr;
 	} arg1;
@@ -432,11 +432,11 @@ struct start_info {
 	unsigned long nr_pages;     /* Total pages allocated to this domain.  */
 	unsigned long shared_info;  /* MACHINE address of shared info struct. */
 	uint32_t flags;             /* SIF_xxx flags.                         */
-	unsigned long store_mfn;    /* MACHINE page number of shared page.    */
+	xen_pfn_t store_mfn;        /* MACHINE page number of shared page.    */
 	uint32_t store_evtchn;      /* Event channel for store communication. */
 	union {
 		struct {
-			unsigned long mfn;  /* MACHINE page number of console page.   */
+			xen_pfn_t mfn;      /* MACHINE page number of console page.   */
 			uint32_t  evtchn;   /* Event channel for console page.        */
 		} domU;
 		struct {
diff --git a/include/xen/privcmd.h b/include/xen/privcmd.h
index 4d58881..45c1aa1 100644
--- a/include/xen/privcmd.h
+++ b/include/xen/privcmd.h
@@ -37,8 +37,6 @@
 #include <linux/compiler.h>
 #include <xen/interface/xen.h>
 
-typedef unsigned long xen_pfn_t;
-
 struct privcmd_hypercall {
 	__u64 op;
 	__u64 arg[5];
-- 
1.7.2.5

bind_evtchn_to_irqhandler can legitimately return 0 (irq 0), it is not
an error.

If Linux is running as an HVM domain and is running as Dom0, use
xenstored_local_init to initialize the xenstore page and event channel,
and do not call xs_reset_watches at boot.

Signed-off-by: Stefano Stabellini
<stefano.stabellini-mvvWK6WmYclDPfheJLI6IQ@public.gmane.org>
---
 drivers/xen/xenbus/xenbus_comms.c |    2 +-
 drivers/xen/xenbus/xenbus_probe.c |   26 ++++++++++++++++----------
 drivers/xen/xenbus/xenbus_xs.c    |    3 ++-
 3 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_comms.c
b/drivers/xen/xenbus/xenbus_comms.c
index 2eff7a6..57b8230 100644
--- a/drivers/xen/xenbus/xenbus_comms.c
+++ b/drivers/xen/xenbus/xenbus_comms.c
@@ -224,7 +224,7 @@ int xb_init_comms(void)
 		int err;
 		err = bind_evtchn_to_irqhandler(xen_store_evtchn, wake_waiting,
 						0, "xenbus", &xb_waitq);
-		if (err <= 0) {
+		if (err < 0) {
 			printk(KERN_ERR "XENBUS request irq failed %i\n", err);
 			return err;
 		}
diff --git a/drivers/xen/xenbus/xenbus_probe.c
b/drivers/xen/xenbus/xenbus_probe.c
index 1b178c6..f3d5105 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -731,16 +731,22 @@ static int __init xenbus_init(void)
 		return -ENODEV;
 
 	if (xen_hvm_domain()) {
-		uint64_t v = 0;
-		err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
-		if (err)
-			goto out_error;
-		xen_store_evtchn = (int)v;
-		err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
-		if (err)
-			goto out_error;
-		xen_store_mfn = (unsigned long)v;
-		xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE);
+		if (xen_initial_domain()) {
+			err = xenstored_local_init();
+			xen_store_interface = phys_to_virt(xen_store_mfn << PAGE_SHIFT);
+		} else {
+			uint64_t v = 0;
+			err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
+			if (err)
+				goto out_error;
+			xen_store_evtchn = (int)v;
+			err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
+			if (err)
+				goto out_error;
+			xen_store_mfn = (unsigned long)v;
+			xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT,
+					PAGE_SIZE);
+		}
 	} else {
 		xen_store_evtchn = xen_start_info->store_evtchn;
 		xen_store_mfn = xen_start_info->store_mfn;
diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
index b3b8f2f..edcef19 100644
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -44,6 +44,7 @@
 #include <linux/rwsem.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <asm/xen/hypervisor.h>
 #include <xen/xenbus.h>
 #include <xen/xen.h>
 #include "xenbus_comms.h"
@@ -907,7 +908,7 @@ int xs_init(void)
 		return PTR_ERR(task);
 
 	/* shutdown watches for kexec boot */
-	if (xen_hvm_domain())
+	if (xen_hvm_domain() && !xen_initial_domain())
 		xs_reset_watches();
 
 	return 0;
-- 
1.7.2.5

Also select XEN_DOM0 by default.

Signed-off-by: Stefano Stabellini
<stefano.stabellini-mvvWK6WmYclDPfheJLI6IQ@public.gmane.org>
---
 arch/arm/Kconfig     |    4 ++++
 drivers/xen/Makefile |    7 ++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 57b294c..1a95b35 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -2201,9 +2201,13 @@ config NEON
 	  Say Y to include support code for NEON, the ARMv7 Advanced SIMD
 	  Extension.
 
+config XEN_DOM0
+	def_bool y
+
 config XEN
 	bool "Xen guest support on ARM"
 	depends on ARM
+	select XEN_DOM0
 	help
 	  Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
 
diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
index dab8305..e3542e1 100644
--- a/drivers/xen/Makefile
+++ b/drivers/xen/Makefile
@@ -1,7 +1,9 @@
 ifeq (CONFIG_X86,y)
-obj-y	+= grant-table.o features.o events.o manage.o balloon.o
-obj-y	+= xenbus/
+obj-y	+= manage.o balloon.o
+obj-$(CONFIG_XEN_DOM0)			+= pci.o
 endif
+obj-y	+= grant-table.o features.o events.o
+obj-y	+= xenbus/
 
 nostackp := $(call cc-option, -fno-stack-protector)
 CFLAGS_features.o			:= $(nostackp)
@@ -21,7 +23,6 @@ obj-$(CONFIG_XEN_SYS_HYPERVISOR)	+= sys-hypervisor.o
 obj-$(CONFIG_XEN_PVHVM)			+= platform-pci.o
 obj-$(CONFIG_XEN_TMEM)			+= tmem.o
 obj-$(CONFIG_SWIOTLB_XEN)		+= swiotlb-xen.o
-obj-$(CONFIG_XEN_DOM0)			+= pci.o
 obj-$(CONFIG_XEN_PCIDEV_BACKEND)	+= xen-pciback/
 
 xen-evtchn-y				:= evtchn.o
-- 
1.7.2.5

On 23/02/12 17:48, Stefano Stabellini wrote:
> Compile events.c and use IRQ 32 to receive events notifications.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> +#ifdef CONFIG_ARM
> +#define IRQ_EVTCHN_CALLBACK 63
> +irqreturn_t xen_arm_callback(int irq, void *arg)
> +{
> +	__xen_evtchn_do_upcall();
> +	return 0;
> +}
> +
> +int __init xen_init_IRQ_arm(void)
> +{
> +	int rc;
> +	xen_init_IRQ();
> +	rc = request_irq(IRQ_EVTCHN_CALLBACK, xen_arm_callback,
> +			IRQF_DISABLED | IRQF_NOBALANCING | IRQF_TRIGGER_RISING,
> +			"events", "events");	
> +	if (rc) {
> +		printk(KERN_ERR "Error requesting IRQ %d\n",
IRQ_EVTCHN_CALLBACK);
> +	}
> +	return rc;
> +}
> +core_initcall(xen_init_IRQ_arm);
> +#endif
You should (eventually) have a device tree binding for the event channel
and use a OF (device tree) device driver instead of this core_initcall()
to register the handler etc.

David

On Fri, 24 Feb 2012, David Vrabel wrote:
> On 23/02/12 17:48, Stefano Stabellini wrote:
> > Compile events.c and use IRQ 32 to receive events notifications.
> > 
> > Signed-off-by: Stefano Stabellini
<stefano.stabellini-mvvWK6WmYclDPfheJLI6IQ@public.gmane.org>
> 
> > +#ifdef CONFIG_ARM
> > +#define IRQ_EVTCHN_CALLBACK 63
> > +irqreturn_t xen_arm_callback(int irq, void *arg)
> > +{
> > +	__xen_evtchn_do_upcall();
> > +	return 0;
> > +}
> > +
> > +int __init xen_init_IRQ_arm(void)
> > +{
> > +	int rc;
> > +	xen_init_IRQ();
> > +	rc = request_irq(IRQ_EVTCHN_CALLBACK, xen_arm_callback,
> > +			IRQF_DISABLED | IRQF_NOBALANCING | IRQF_TRIGGER_RISING,
> > +			"events", "events");	
> > +	if (rc) {
> > +		printk(KERN_ERR "Error requesting IRQ %d\n",
IRQ_EVTCHN_CALLBACK);
> > +	}
> > +	return rc;
> > +}
> > +core_initcall(xen_init_IRQ_arm);
> > +#endif
> 
> You should (eventually) have a device tree binding for the event channel
> and use a OF (device tree) device driver instead of this core_initcall()
> to register the handler etc.
 
Yes, that is the idea, once we have better device tree support in Xen.
We should also pass the IRQ number to be used as event injection
mechanism through the device tree.

On Thu, 2012-02-23 at 17:48 +0000, Stefano Stabellini
wrote:
> We need a register to pass the hypercall number because we might not
> know it at compile time and HVC only takes an immediate argument.
> 
> Among the available registers r12 seems to be the best choice because it
> is defined as "intra-procedure call scratch register".
R12 is not accessible from the 16 bit "T1" Thumb encoding of mov
immediate (which can only target r0..r7).

Since we support only ARMv7+ there are "T2" and "T3"
encodings available
which do allow direct mov of an immediate into R12, but are 32 bit Thumb
instructions.

Should we use r7 instead to maximise instruction density for Thumb code?

Ian.

On Thu, Feb 23, 2012 at 05:48:22PM +0000, Stefano Stabellini
wrote:
> We need a register to pass the hypercall number because we might not
> know it at compile time and HVC only takes an immediate argument.
> 
> Among the available registers r12 seems to be the best choice because it
> is defined as "intra-procedure call scratch register".
This would be massively simplified if you didn''t try to inline the HVC.
Does it really need to be inline?
> 
> Use the ISS to pass an hypervisor specific tag.
> 
> Signed-off-by: Stefano Stabellini
<stefano.stabellini-mvvWK6WmYclDPfheJLI6IQ@public.gmane.org>
> CC: kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  arch/arm/include/asm/xen/hypercall.h |   87
+++++++++++++++++++---------------
>  1 files changed, 48 insertions(+), 39 deletions(-)
> 
> diff --git a/arch/arm/include/asm/xen/hypercall.h
b/arch/arm/include/asm/xen/hypercall.h
> index 404e63f0..04eba1c 100644
> --- a/arch/arm/include/asm/xen/hypercall.h
> +++ b/arch/arm/include/asm/xen/hypercall.h
> @@ -33,13 +33,17 @@
>  #ifndef _ASM_ARM_XEN_HYPERCALL_H
>  #define _ASM_ARM_XEN_HYPERCALL_H
>  
> -#define __HVC_IMM(name)	"( " #name " & 0xf) + "	 
\
> -			    "((" #name " << 4) & 0xfff00)"
> +#include <xen/interface/xen.h>
> +#include <asm/errno.h>
>  
> -#define ____HYPERCALL(name) ".word 0xe1400070 + "
__HVC_IMM(name)
> -#define __HYPERCALL(name) ____HYPERCALL(__HYPERVISOR_##name)
> +#define XEN_HYPERCALL_TAG  "0XEA1"
> +
> +#define __HVC_IMM(tag)	"( " tag " & 0xf) + "	  \
> +			    "((" tag " << 4) & 0xfff00)"
> +#define __HYPERCALL ".word 0xe1400070 + "
__HVC_IMM(XEN_HYPERCALL_TAG)
Please, do not do this.  It won''t work in Thumb, where the encodings
are
different.

It is reasonable to expect anyone building Xen to have reasonably new
tools, you you can justifiably use

AFLAGS_thisfile.o := -Wa,-march=armv7-a+virt

in the Makefile and just use the hvc instruction directly.


Of course, this is only practical if the HVC invocation is not inlined.
If we can''t avoid macro-ising HVC, we should do it globally, not
locally
to the Xen code.  That way we at least keep all the horror in one place.

Cheers
---Dave
>  
>  #define __HYPERCALL_RETREG	"r0"
> +#define __HYPERCALL_NUMBER	"r12"
>  #define __HYPERCALL_ARG1REG	"r0"
>  #define __HYPERCALL_ARG2REG	"r1"
>  #define __HYPERCALL_ARG3REG	"r2"
> @@ -48,30 +52,32 @@
>  
>  #define __HYPERCALL_DECLS						\
>  	register unsigned long __res  asm(__HYPERCALL_RETREG);		\
> +	register unsigned long __num  asm(__HYPERCALL_NUMBER) = __num; \
>  	register unsigned long __arg1 asm(__HYPERCALL_ARG1REG) = __arg1; \
>  	register unsigned long __arg2 asm(__HYPERCALL_ARG2REG) = __arg2; \
>  	register unsigned long __arg3 asm(__HYPERCALL_ARG3REG) = __arg3; \
>  	register unsigned long __arg4 asm(__HYPERCALL_ARG4REG) = __arg4; \
>  	register unsigned long __arg5 asm(__HYPERCALL_ARG5REG) = __arg5;
>  
> -#define __HYPERCALL_0PARAM	"=r" (__res)
> +#define __HYPERCALL_0PARAM	"=r" (__res), "+r" (__num)
>  #define __HYPERCALL_1PARAM	__HYPERCALL_0PARAM, "+r" (__arg1)
>  #define __HYPERCALL_2PARAM	__HYPERCALL_1PARAM, "+r" (__arg2)
>  #define __HYPERCALL_3PARAM	__HYPERCALL_2PARAM, "+r" (__arg3)
>  #define __HYPERCALL_4PARAM	__HYPERCALL_3PARAM, "+r" (__arg4)
>  #define __HYPERCALL_5PARAM	__HYPERCALL_4PARAM, "+r" (__arg5)
>  
> -#define __HYPERCALL_0ARG()
> -#define __HYPERCALL_1ARG(a1)						\
> -	__HYPERCALL_0ARG()		__arg1 = (unsigned long)(a1);
> -#define __HYPERCALL_2ARG(a1,a2)						\
> -	__HYPERCALL_1ARG(a1)		__arg2 = (unsigned long)(a2);
> -#define __HYPERCALL_3ARG(a1,a2,a3)					\
> -	__HYPERCALL_2ARG(a1,a2)		__arg3 = (unsigned long)(a3);
> -#define __HYPERCALL_4ARG(a1,a2,a3,a4)					\
> -	__HYPERCALL_3ARG(a1,a2,a3)	__arg4 = (unsigned long)(a4);
> -#define __HYPERCALL_5ARG(a1,a2,a3,a4,a5)				\
> -	__HYPERCALL_4ARG(a1,a2,a3,a4)	__arg5 = (unsigned long)(a5);
> +#define __HYPERCALL_0ARG(hypercall)						\
> +	__num = (unsigned long)hypercall;
> +#define __HYPERCALL_1ARG(hypercall,a1)						\
> +	__HYPERCALL_0ARG(hypercall)		__arg1 = (unsigned long)(a1);
> +#define __HYPERCALL_2ARG(hypercall,a1,a2)						\
> +	__HYPERCALL_1ARG(hypercall,a1)		__arg2 = (unsigned long)(a2);
> +#define __HYPERCALL_3ARG(hypercall,a1,a2,a3)					\
> +	__HYPERCALL_2ARG(hypercall,a1,a2)		__arg3 = (unsigned long)(a3);
> +#define __HYPERCALL_4ARG(hypercall,a1,a2,a3,a4)					\
> +	__HYPERCALL_3ARG(hypercall,a1,a2,a3)	__arg4 = (unsigned long)(a4);
> +#define __HYPERCALL_5ARG(hypercall,a1,a2,a3,a4,a5)				\
> +	__HYPERCALL_4ARG(hypercall,a1,a2,a3,a4)	__arg5 = (unsigned long)(a5);
>  
>  #define __HYPERCALL_CLOBBER5	"memory"
>  #define __HYPERCALL_CLOBBER4	__HYPERCALL_CLOBBER5, __HYPERCALL_ARG5REG
> @@ -80,102 +86,105 @@
>  #define __HYPERCALL_CLOBBER1	__HYPERCALL_CLOBBER2, __HYPERCALL_ARG2REG
>  #define __HYPERCALL_CLOBBER0	__HYPERCALL_CLOBBER1, __HYPERCALL_ARG1REG
>  
> -#define _hypercall0(type, name)						\
> +#define _hypercall0(type, hypercall)						\
>  ({									\
>  	__HYPERCALL_DECLS;						\
> -	__HYPERCALL_0ARG();						\
> -	asm volatile (__HYPERCALL(name)					\
> +	__HYPERCALL_0ARG(hypercall);						\
> +	asm volatile (__HYPERCALL					\
>  		      : __HYPERCALL_0PARAM				\
>  		      : 						\
>  		      : __HYPERCALL_CLOBBER0);				\
>  	(type)__res;							\
>  })
>  
> -#define _hypercall1(type, name, a1)					\
> +#define _hypercall1(type, hypercall, a1)					\
>  ({									\
>  	__HYPERCALL_DECLS;						\
> -	__HYPERCALL_1ARG(a1);						\
> -	asm volatile (__HYPERCALL(name)					\
> +	__HYPERCALL_1ARG(hypercall, a1);						\
> +	asm volatile (__HYPERCALL					\
>  		      : __HYPERCALL_1PARAM				\
>  		      : 						\
>  		      : __HYPERCALL_CLOBBER1);				\
>  	(type)__res;							\
>  })
>  
> -#define _hypercall2(type, name, a1, a2)					\
> +#define _hypercall2(type, hypercall, a1, a2)					\
>  ({									\
>  	__HYPERCALL_DECLS;						\
> -	__HYPERCALL_2ARG(a1, a2);					\
> -	asm volatile (__HYPERCALL(name)					\
> +	__HYPERCALL_2ARG(hypercall, a1, a2);					\
> +	asm volatile (__HYPERCALL					\
>  		      : __HYPERCALL_2PARAM				\
>  		      : 						\
>  		      : __HYPERCALL_CLOBBER2);				\
>  	(type)__res;							\
>  })
>  
> -#define _hypercall3(type, name, a1, a2, a3)				\
> +#define _hypercall3(type, hypercall, a1, a2, a3)				\
>  ({									\
>  	__HYPERCALL_DECLS;						\
> -	__HYPERCALL_3ARG(a1, a2, a3);					\
> -	asm volatile (__HYPERCALL(name)					\
> +	__HYPERCALL_3ARG(hypercall, a1, a2, a3);					\
> +	asm volatile (__HYPERCALL					\
>  		      : __HYPERCALL_3PARAM				\
>  		      : 						\
>  		      : __HYPERCALL_CLOBBER3);				\
>  	(type)__res;							\
>  })
>  
> -#define _hypercall4(type, name, a1, a2, a3, a4)				\
> +#define _hypercall4(type, hypercall, a1, a2, a3, a4)				\
>  ({									\
>  	__HYPERCALL_DECLS;						\
> -	__HYPERCALL_4ARG(a1, a2, a3, a4);				\
> -	asm volatile (__HYPERCALL(name)					\
> +	__HYPERCALL_4ARG(hypercall, a1, a2, a3, a4);				\
> +	asm volatile (__HYPERCALL					\
>  		      : __HYPERCALL_4PARAM				\
>  		      : 						\
>  		      : __HYPERCALL_CLOBBER4);				\
>  	(type)__res;							\
>  })
>  
> -#define _hypercall5(type, name, a1, a2, a3, a4, a5)			\
> +#define _hypercall5(type, hypercall, a1, a2, a3, a4, a5)			\
>  ({									\
>  	__HYPERCALL_DECLS;						\
> -	__HYPERCALL_5ARG(a1, a2, a3, a4, a5);				\
> -	asm volatile (__HYPERCALL(name)					\
> +	__HYPERCALL_5ARG(hypercall, a1, a2, a3, a4, a5);				\
> +	asm volatile (__HYPERCALL					\
>  		      : __HYPERCALL_5PARAM				\
>  		      : 						\
>  		      : __HYPERCALL_CLOBBER5);				\
>  	(type)__res;							\
>  })
>  
> +#define HYPERCALL(name) \
> +	(__HYPERVISOR_##name)
> +
>  /* -- Hypercall definitions go below -- */
>  
>  static inline int
>  HYPERVISOR_xen_version(int cmd, void *arg)
>  {
> -	return _hypercall2(int, xen_version, cmd, arg);
> +	return _hypercall2(int, HYPERCALL(xen_version), cmd, arg);
>  }
>  
>  static inline int
>  HYPERVISOR_console_io(int cmd, int count, char *str)
>  {
> -	return _hypercall3(int, console_io, cmd, count, str);
> +	return _hypercall3(int, HYPERCALL(console_io), cmd, count, str);
>  }
>  
>  static inline int
>  HYPERVISOR_grant_table_op(unsigned int cmd, void *uop, unsigned int count)
>  {
> -	return _hypercall3(int, grant_table_op, cmd, uop, count);
> +	return _hypercall3(int, HYPERCALL(grant_table_op), cmd, uop, count);
>  }
>  
>  static inline int
>  HYPERVISOR_sched_op(int cmd, void *arg)
>  {
> -	return _hypercall2(int, sched_op, cmd, arg);
> +	return _hypercall2(int, HYPERCALL(sched_op), cmd, arg);
>  }
>  
>  static inline int
>  HYPERVISOR_event_channel_op(int cmd, void *arg)
>  {
> -	return _hypercall2(int, event_channel_op, cmd, arg);
> +	return _hypercall2(int, HYPERCALL(event_channel_op), cmd, arg);
>  }
>  
>  #endif /* _ASM_ARM_XEN_HYPERCALL_H */
> -- 
> 1.7.2.5
> 
> 
> _______________________________________________
> linaro-dev mailing list
> linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev

On Mon, Feb 27, 2012 at 04:27:23PM +0000, Ian Campbell
wrote:
> On Thu, 2012-02-23 at 17:48 +0000, Stefano Stabellini wrote:
> > We need a register to pass the hypercall number because we might not
> > know it at compile time and HVC only takes an immediate argument.
> > 
> > Among the available registers r12 seems to be the best choice because
it
> > is defined as "intra-procedure call scratch register".
> 
> R12 is not accessible from the 16 bit "T1" Thumb encoding of mov
> immediate (which can only target r0..r7).
This is untrue.  The important instructions, like MOV Rd, Rn can access
all the regs.  But anyway, there is no such thing as a Thumb-1 kernel,
so we won''t really care.
> Since we support only ARMv7+ there are "T2" and "T3"
encodings available
> which do allow direct mov of an immediate into R12, but are 32 bit Thumb
> instructions.
> 
> Should we use r7 instead to maximise instruction density for Thumb code?
The difference seems trivial when put into context, even if you code a
special Thumb version of the code to maximise density (the Thumb-2 code
which gets built from assembler in the kernel is very suboptimal in
size, but there simply isn''t a high proportion of asm code in the
kernel
anyway.)  I wouldn''t consider the ARM/Thumb differences as an important
factor when deciding on a register.

One argument for _not_ using r12 for this purpose is that it is then
harder to put a generic "HVC" function (analogous to the
"syscall"
syscall) out-of-line, since r12 could get destroyed by the call.  

If you don''t think you will ever care about putting HVC out of line
though, it may not matter.

Cheers
---Dave

On Mon, 2012-02-27 at 18:03 +0000, Dave Martin wrote:
> On Mon, Feb 27, 2012 at 04:27:23PM +0000, Ian Campbell wrote:
> > On Thu, 2012-02-23 at 17:48 +0000, Stefano Stabellini wrote:
> > > We need a register to pass the hypercall number because we might
not
> > > know it at compile time and HVC only takes an immediate argument.
> > > 
> > > Among the available registers r12 seems to be the best choice
because it
> > > is defined as "intra-procedure call scratch register".
> > 
> > R12 is not accessible from the 16 bit "T1" Thumb encoding of
mov
> > immediate (which can only target r0..r7).
> 
> This is untrue.  The important instructions, like MOV Rd, Rn can access
> all the regs.  But anyway, there is no such thing as a Thumb-1 kernel,
> so we won''t really care.
I did say "mov immediate", which is the one which matters when loading
a
constant hypercall number (the common case). AFAIK the "mov Rd, #imm"
T1
encoding cannot access all registers.

The "mov rd,rn" form only helps for syscall(2) like functions, which
are
unusual, at least for Xen, although as Stefano says, they do exist.
> > Since we support only ARMv7+ there are "T2" and
"T3" encodings available
> > which do allow direct mov of an immediate into R12, but are 32 bit
Thumb
> > instructions.
> > 
> > Should we use r7 instead to maximise instruction density for Thumb
code?
> 
> The difference seems trivial when put into context, even if you code a
> special Thumb version of the code to maximise density (the Thumb-2 code
> which gets built from assembler in the kernel is very suboptimal in
> size, but there simply isn''t a high proportion of asm code in the
kernel
> anyway.)  I wouldn''t consider the ARM/Thumb differences as an
important
> factor when deciding on a register.
OK, that''s useful information. thanks.
> One argument for _not_ using r12 for this purpose is that it is then
> harder to put a generic "HVC" function (analogous to the
"syscall"
> syscall) out-of-line, since r12 could get destroyed by the call.
For an out of line syscall(2) wouldn''t the syscall number either be in
a
standard C calling convention argument register or on the stack when the
function was called, since it is just a normal argument at that point?
As you point out it cannot be passed in r12 (and could never be, due to
the clobbering).

The syscall function itself would have to move the arguments and syscall
nr etc around before issuing the syscall.

I think the same is true of a similar hypercall(2)
> If you don''t think you will ever care about putting HVC out of
line
> though, it may not matter.
Ian.

On Mon, 2012-02-27 at 17:53 +0000, Dave Martin wrote:
> On Thu, Feb 23, 2012 at 05:48:22PM +0000, Stefano Stabellini wrote:
> > We need a register to pass the hypercall number because we might not
> > know it at compile time and HVC only takes an immediate argument.
> > 
> > Among the available registers r12 seems to be the best choice because
it
> > is defined as "intra-procedure call scratch register".
> 
> This would be massively simplified if you didn''t try to inline the
HVC.
> Does it really need to be inline?
>
> > +#define __HYPERCALL ".word 0xe1400070 + "
__HVC_IMM(XEN_HYPERCALL_TAG)
> 
> Please, do not do this.  It won''t work in Thumb, where the
encodings are
> different.
> 
> It is reasonable to expect anyone building Xen to have reasonably new
> tools, you you can justifiably use
> 
> AFLAGS_thisfile.o := -Wa,-march=armv7-a+virt
> 
> in the Makefile and just use the hvc instruction directly.
Our aim is for guest kernel binaries not to be specific to Xen -- i.e.
they should be able to run on baremetal and other hypervisors as well.
The differences should only be in the device-tree passed to the kernel.
> Of course, this is only practical if the HVC invocation is not inlined.
I suppose we could make the stub functions out of line, we just copied
what Xen does on x86.

The only thing which springs to mind is that 5 argument hypercalls will
end up pushing the fifth argument to the stack only to pop it back into
r4 for the hypercall and IIRC it also needs to preserve r4 (callee saved
reg) which is going to involve some small amount of code to move stuff
around too.

So by inlining the functions we avoid some thunking because the compiler
would know exactly what was happening at the hypercall site.

We don''t currently have any 6 argument hypercalls but the same would
extend there.
> If we can''t avoid macro-ising HVC, we should do it globally, not
locally
> to the Xen code.  That way we at least keep all the horror in one place.
That sounds like a good idea to me.

Given that Stefano is proposing to make the ISS a (per-hypervisor)
constant we could consider just defining the Thumb and non-Thumb
constants instead of doing all the construction with the __HVC_IMM stuff
-- that would remove a big bit of the macroization.

Ian.

On 27 February 2012 16:27, Ian Campbell
<Ian.Campbell-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
wrote:
> R12 is not accessible from the 16 bit "T1" Thumb encoding of mov
> immediate (which can only target r0..r7).
>
> Since we support only ARMv7+ there are "T2" and "T3"
encodings available
> which do allow direct mov of an immediate into R12, but are 32 bit Thumb
> instructions.
>
> Should we use r7 instead to maximise instruction density for Thumb code?
r7 is (used by gcc as) the Thumb frame pointer; I don''t know if this
makes it worth avoiding in this context.

-- PMM

On Mon, Feb 27, 2012 at 07:48:45PM +0000, Ian Campbell
wrote:
> On Mon, 2012-02-27 at 17:53 +0000, Dave Martin wrote:
> > On Thu, Feb 23, 2012 at 05:48:22PM +0000, Stefano Stabellini wrote:
> > > We need a register to pass the hypercall number because we might
not
> > > know it at compile time and HVC only takes an immediate argument.
> > > 
> > > Among the available registers r12 seems to be the best choice
because it
> > > is defined as "intra-procedure call scratch register".
> > 
> > This would be massively simplified if you didn''t try to
inline the HVC.
> > Does it really need to be inline?
> >
> > > +#define __HYPERCALL ".word 0xe1400070 + "
__HVC_IMM(XEN_HYPERCALL_TAG)
> > 
> > Please, do not do this.  It won''t work in Thumb, where the
encodings are
> > different.
> > 
> > It is reasonable to expect anyone building Xen to have reasonably new
> > tools, you you can justifiably use
> > 
> > AFLAGS_thisfile.o := -Wa,-march=armv7-a+virt
> > 
> > in the Makefile and just use the hvc instruction directly.
> 
> Our aim is for guest kernel binaries not to be specific to Xen -- i.e.
> they should be able to run on baremetal and other hypervisors as well.
> The differences should only be in the device-tree passed to the kernel.
> 
> > Of course, this is only practical if the HVC invocation is not
inlined.
> 
> I suppose we could make the stub functions out of line, we just copied
> what Xen does on x86.
> 
> The only thing which springs to mind is that 5 argument hypercalls will
> end up pushing the fifth argument to the stack only to pop it back into
> r4 for the hypercall and IIRC it also needs to preserve r4 (callee saved
> reg) which is going to involve some small amount of code to move stuff
> around too.
> 
> So by inlining the functions we avoid some thunking because the compiler
> would know exactly what was happening at the hypercall site.
True ...
> 
> We don''t currently have any 6 argument hypercalls but the same
would
> extend there.
> 
> > If we can''t avoid macro-ising HVC, we should do it globally,
not locally
> > to the Xen code.  That way we at least keep all the horror in one
place.
> 
> That sounds like a good idea to me.
> 
> Given that Stefano is proposing to make the ISS a (per-hypervisor)
> constant we could consider just defining the Thumb and non-Thumb
> constants instead of doing all the construction with the __HVC_IMM stuff
> -- that would remove a big bit of the macroization.
It''s not quite as simple as that -- emitting instructions using data
directives is not endianness safe, and even in the cases where .long gives
the right result for ARM, it gives the wrong result for 32-bit Thumb
instructions if the opcode is given in human-readable order.

I was trying to solve the same problem for the kvm guys with some global
macros -- I''m aiming to get a patch posted soon, so I''ll make
sure
you''re on CC.

Cheers
---Dave

On Tue, 2012-02-28 at 09:46 +0000, Dave Martin wrote:
> On Mon, Feb 27, 2012 at 07:48:45PM +0000, Ian Campbell wrote:
> > Given that Stefano is proposing to make the ISS a (per-hypervisor)
> > constant we could consider just defining the Thumb and non-Thumb
> > constants instead of doing all the construction with the __HVC_IMM
stuff
> > -- that would remove a big bit of the macroization.
> 
> It''s not quite as simple as that -- emitting instructions using
data
> directives is not endianness safe, and even in the cases where .long gives
> the right result for ARM, it gives the wrong result for 32-bit Thumb
> instructions if the opcode is given in human-readable order.
Urk, yes,..
> I was trying to solve the same problem for the kvm guys with some global
> macros -- I''m aiming to get a patch posted soon, so I''ll
make sure
> you''re on CC.
Awesome, thanks!

Ian.

On Mon, 2012-02-27 at 21:05 +0000, Peter Maydell wrote:
> On 27 February 2012 16:27, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> > R12 is not accessible from the 16 bit "T1" Thumb encoding of
mov
> > immediate (which can only target r0..r7).
> >
> > Since we support only ARMv7+ there are "T2" and
"T3" encodings available
> > which do allow direct mov of an immediate into R12, but are 32 bit
Thumb
> > instructions.
> >
> > Should we use r7 instead to maximise instruction density for Thumb
code?
> 
> r7 is (used by gcc as) the Thumb frame pointer; I don''t know if
this
> makes it worth avoiding in this context.
I think it does.

It actually sounds as if using r12 is fine here, the impact on code
density should be pretty small -- there aren''t really all that many
call
sites which involve hypercalls.

By way of an example I measured an x86 kernel which should be using more
hypercalls due to pv paging etc and found that 0.014% of the lines in
"objdump -d" contained a call to the hypercall_page. (I know not all
lines of objdump -d output are instructions but it''s a reasonable
approx
IMHO).

So I think using 3 16 bit instructions slots instead of 2 won''t make
much impact in practice.

Thanks,
Ian.

On Mon, Feb 27, 2012 at 07:33:39PM +0000, Ian Campbell
wrote:
> On Mon, 2012-02-27 at 18:03 +0000, Dave Martin wrote:
> > On Mon, Feb 27, 2012 at 04:27:23PM +0000, Ian Campbell wrote:
> > > On Thu, 2012-02-23 at 17:48 +0000, Stefano Stabellini wrote:
> > > > We need a register to pass the hypercall number because we
might not
> > > > know it at compile time and HVC only takes an immediate
argument.
> > > > 
> > > > Among the available registers r12 seems to be the best
choice because it
> > > > is defined as "intra-procedure call scratch
register".
> > > 
> > > R12 is not accessible from the 16 bit "T1" Thumb
encoding of mov
> > > immediate (which can only target r0..r7).
> > 
> > This is untrue.  The important instructions, like MOV Rd, Rn can
access
> > all the regs.  But anyway, there is no such thing as a Thumb-1 kernel,
> > so we won''t really care.
> 
> I did say "mov immediate", which is the one which matters when
loading a
> constant hypercall number (the common case). AFAIK the "mov Rd,
#imm" T1
> encoding cannot access all registers.
> 
> The "mov rd,rn" form only helps for syscall(2) like functions,
which are
> unusual, at least for Xen, although as Stefano says, they do exist.
Apologies -- looks like I misread you here.  I agree, but it''s probably
a minor issue nonetheless.
> 
> > > Since we support only ARMv7+ there are "T2" and
"T3" encodings available
> > > which do allow direct mov of an immediate into R12, but are 32
bit Thumb
> > > instructions.
> > > 
> > > Should we use r7 instead to maximise instruction density for
Thumb code?
> > 
> > The difference seems trivial when put into context, even if you code a
> > special Thumb version of the code to maximise density (the Thumb-2
code
> > which gets built from assembler in the kernel is very suboptimal in
> > size, but there simply isn''t a high proportion of asm code in
the kernel
> > anyway.)  I wouldn''t consider the ARM/Thumb differences as an
important
> > factor when deciding on a register.
> 
> OK, that''s useful information. thanks.
> 
> > One argument for _not_ using r12 for this purpose is that it is then
> > harder to put a generic "HVC" function (analogous to the
"syscall"
> > syscall) out-of-line, since r12 could get destroyed by the call.
> 
> For an out of line syscall(2) wouldn''t the syscall number either
be in a
> standard C calling convention argument register or on the stack when the
> function was called, since it is just a normal argument at that point?
> As you point out it cannot be passed in r12 (and could never be, due to
> the clobbering).
> 
> The syscall function itself would have to move the arguments and syscall
> nr etc around before issuing the syscall.
> 
> I think the same is true of a similar hypercall(2)
> 
> > If you don''t think you will ever care about putting HVC out
of line
> > though, it may not matter.
If you have both inline and out-of-line hypercalls, it''s hard to ensure
that you never have to shuffle the registers in either case.

Shuffling can be reduced but only at the expense of strange argument
ordering in some cases when calling from C -- the complexity is probably
not worth it.  Linux doesn''t bother for its own syscalls.

Note that even in assembler, a branch from one section to a label in
another section may cause r12 to get destroyed, so you will need to be
careful about how you code the hypervisor trap handler.  However, this
is not different from coding exception handlers in general, so I don''t
know that it constitutes a conclusive argument on its own.

My instinctive preference would therefore be for r7 (which also seems to
be good enough for Linux syscalls) -- but it really depends how many
arguments you expect to need to support.

Cheers
---Dave

On Tue, 2012-02-28 at 10:20 +0000, Dave Martin wrote:
> On Mon, Feb 27, 2012 at 07:33:39PM +0000, Ian Campbell wrote:
> > On Mon, 2012-02-27 at 18:03 +0000, Dave Martin wrote:
> > > > Since we support only ARMv7+ there are "T2" and
"T3" encodings available
> > > > which do allow direct mov of an immediate into R12, but are
32 bit Thumb
> > > > instructions.
> > > > 
> > > > Should we use r7 instead to maximise instruction density for
Thumb code?
> > > 
> > > The difference seems trivial when put into context, even if you
code a
> > > special Thumb version of the code to maximise density (the
Thumb-2 code
> > > which gets built from assembler in the kernel is very suboptimal
in
> > > size, but there simply isn''t a high proportion of asm
code in the kernel
> > > anyway.)  I wouldn''t consider the ARM/Thumb differences
as an important
> > > factor when deciding on a register.
> > 
> > OK, that''s useful information. thanks.
> > 
> > > One argument for _not_ using r12 for this purpose is that it is
then
> > > harder to put a generic "HVC" function (analogous to
the "syscall"
> > > syscall) out-of-line, since r12 could get destroyed by the call.
> > 
> > For an out of line syscall(2) wouldn''t the syscall number
either be in a
> > standard C calling convention argument register or on the stack when
the
> > function was called, since it is just a normal argument at that point?
> > As you point out it cannot be passed in r12 (and could never be, due
to
> > the clobbering).
> > 
> > The syscall function itself would have to move the arguments and
syscall
> > nr etc around before issuing the syscall.
> > 
> > I think the same is true of a similar hypercall(2)
> > 
> > > If you don''t think you will ever care about putting HVC
out of line
> > > though, it may not matter.
> 
> If you have both inline and out-of-line hypercalls, it''s hard to
ensure
> that you never have to shuffle the registers in either case.
Agreed.

I think we want to optimise for the inline case since those are the
majority.

The only non-inline case is the special "privcmd ioctl" which is the
mechanism that allows the Xen toolstack to make hypercalls. It''s
somewhat akin to syscall(2). By the time you get to it you will already
have done a system call for the ioctl, pulled the arguments from the
ioctl argument structure etc, plus such hypercalls are not really
performance critical.
> Shuffling can be reduced but only at the expense of strange argument
> ordering in some cases when calling from C -- the complexity is probably
> not worth it.  Linux doesn''t bother for its own syscalls.
> 
> Note that even in assembler, a branch from one section to a label in
> another section may cause r12 to get destroyed, so you will need to be
> careful about how you code the hypervisor trap handler.  However, this
> is not different from coding exception handlers in general, so I
don''t
> know that it constitutes a conclusive argument on its own.
We are happy to arrange that this doesn''t occur on our trap entry
paths,
at least until the guest register state has been saved. Currently the
hypercall dispatcher is in C and gets r12 from the on-stack saved state.
We will likely eventually optimise the hypercall path directly in ASM
and in that case we are happy to take steps to ensure we don''t clobber
r12 before we need it.
> My instinctive preference would therefore be for r7 (which also seems to
> be good enough for Linux syscalls) -- but it really depends how many
> arguments you expect to need to support.
Apparently r7 is the frame pointer for gcc in thumb mode which I think
is a good reason to avoid it.

We currently have some 5 argument hypercalls and there have been
occasional suggestions for interfaces which use 6 -- although none of
them have come to reality.

Ian.

On Tue, 28 Feb 2012, Dave Martin wrote:
> > Given that Stefano is proposing to make the ISS a (per-hypervisor)
> > constant we could consider just defining the Thumb and non-Thumb
> > constants instead of doing all the construction with the __HVC_IMM
stuff
> > -- that would remove a big bit of the macroization.
> 
> It''s not quite as simple as that -- emitting instructions using
data
> directives is not endianness safe, and even in the cases where .long gives
> the right result for ARM, it gives the wrong result for 32-bit Thumb
> instructions if the opcode is given in human-readable order.
> 
> I was trying to solve the same problem for the kvm guys with some global
> macros -- I''m aiming to get a patch posted soon, so I''ll
make sure
> you''re on CC.
 
That would be great, thanks!

On Tue, 28 Feb 2012, Ian Campbell wrote:
> On Tue, 2012-02-28 at 10:20 +0000, Dave Martin wrote:
> > On Mon, Feb 27, 2012 at 07:33:39PM +0000, Ian Campbell wrote:
> > > On Mon, 2012-02-27 at 18:03 +0000, Dave Martin wrote:
> > > > > Since we support only ARMv7+ there are "T2"
and "T3" encodings available
> > > > > which do allow direct mov of an immediate into R12, but
are 32 bit Thumb
> > > > > instructions.
> > > > > 
> > > > > Should we use r7 instead to maximise instruction
density for Thumb code?
> > > > 
> > > > The difference seems trivial when put into context, even if
you code a
> > > > special Thumb version of the code to maximise density (the
Thumb-2 code
> > > > which gets built from assembler in the kernel is very
suboptimal in
> > > > size, but there simply isn''t a high proportion of
asm code in the kernel
> > > > anyway.)  I wouldn''t consider the ARM/Thumb
differences as an important
> > > > factor when deciding on a register.
> > > 
> > > OK, that''s useful information. thanks.
> > > 
> > > > One argument for _not_ using r12 for this purpose is that it
is then
> > > > harder to put a generic "HVC" function (analogous
to the "syscall"
> > > > syscall) out-of-line, since r12 could get destroyed by the
call.
> > > 
> > > For an out of line syscall(2) wouldn''t the syscall
number either be in a
> > > standard C calling convention argument register or on the stack
when the
> > > function was called, since it is just a normal argument at that
point?
> > > As you point out it cannot be passed in r12 (and could never be,
due to
> > > the clobbering).
> > > 
> > > The syscall function itself would have to move the arguments and
syscall
> > > nr etc around before issuing the syscall.
> > > 
> > > I think the same is true of a similar hypercall(2)
> > > 
> > > > If you don''t think you will ever care about putting
HVC out of line
> > > > though, it may not matter.
> > 
> > If you have both inline and out-of-line hypercalls, it''s hard
to ensure
> > that you never have to shuffle the registers in either case.
> 
> Agreed.
> 
> I think we want to optimise for the inline case since those are the
> majority.
They are not just the majority, all of them are static inline at the
moment, even on x86 (where the number of hypercalls is much higher).

So yes, we should optimize for the inline case.

> The only non-inline case is the special "privcmd ioctl" which is
the
> mechanism that allows the Xen toolstack to make hypercalls. It''s
> somewhat akin to syscall(2). By the time you get to it you will already
> have done a system call for the ioctl, pulled the arguments from the
> ioctl argument structure etc, plus such hypercalls are not really
> performance critical.
Even the privcmd hypercall (privcmd_call) is a static inline function,
it is just that at the moment there is only one caller :)

> > Shuffling can be reduced but only at the expense of strange argument
> > ordering in some cases when calling from C -- the complexity is
probably
> > not worth it.  Linux doesn''t bother for its own syscalls.
> > 
> > Note that even in assembler, a branch from one section to a label in
> > another section may cause r12 to get destroyed, so you will need to be
> > careful about how you code the hypervisor trap handler.  However, this
> > is not different from coding exception handlers in general, so I
don''t
> > know that it constitutes a conclusive argument on its own.
> 
> We are happy to arrange that this doesn''t occur on our trap entry
paths,
> at least until the guest register state has been saved. Currently the
> hypercall dispatcher is in C and gets r12 from the on-stack saved state.
> We will likely eventually optimise the hypercall path directly in ASM
> and in that case we are happy to take steps to ensure we don''t
clobber
> r12 before we need it.
Yes, I don''t think this should be an issue.

> > My instinctive preference would therefore be for r7 (which also seems
to
> > be good enough for Linux syscalls) -- but it really depends how many
> > arguments you expect to need to support.
> 
> Apparently r7 is the frame pointer for gcc in thumb mode which I think
> is a good reason to avoid it.
> 
> We currently have some 5 argument hypercalls and there have been
> occasional suggestions for interfaces which use 6 -- although none of
> them have come to reality.
 
I don''t have a very strong opinion on which register we should use, but
I would like to avoid r7 if it is already actively used by gcc.

The fact that r12 can be destroyed so easily is actually a good argument
for using it because it means it is less likely to contain useful data
that needs to be saved/restored by gcc.

On Tue, Feb 28, 2012 at 12:28:29PM +0000, Stefano Stabellini
wrote:
> On Tue, 28 Feb 2012, Ian Campbell wrote:
> > On Tue, 2012-02-28 at 10:20 +0000, Dave Martin wrote:
> > > On Mon, Feb 27, 2012 at 07:33:39PM +0000, Ian Campbell wrote:
> > > > On Mon, 2012-02-27 at 18:03 +0000, Dave Martin wrote:
> > > > > > Since we support only ARMv7+ there are
"T2" and "T3" encodings available
> > > > > > which do allow direct mov of an immediate into
R12, but are 32 bit Thumb
> > > > > > instructions.
> > > > > > 
> > > > > > Should we use r7 instead to maximise instruction
density for Thumb code?
> > > > > 
> > > > > The difference seems trivial when put into context,
even if you code a
> > > > > special Thumb version of the code to maximise density
(the Thumb-2 code
> > > > > which gets built from assembler in the kernel is very
suboptimal in
> > > > > size, but there simply isn''t a high proportion
of asm code in the kernel
> > > > > anyway.)  I wouldn''t consider the ARM/Thumb
differences as an important
> > > > > factor when deciding on a register.
> > > > 
> > > > OK, that''s useful information. thanks.
> > > > 
> > > > > One argument for _not_ using r12 for this purpose is
that it is then
> > > > > harder to put a generic "HVC" function
(analogous to the "syscall"
> > > > > syscall) out-of-line, since r12 could get destroyed by
the call.
> > > > 
> > > > For an out of line syscall(2) wouldn''t the syscall
number either be in a
> > > > standard C calling convention argument register or on the
stack when the
> > > > function was called, since it is just a normal argument at
that point?
> > > > As you point out it cannot be passed in r12 (and could never
be, due to
> > > > the clobbering).
> > > > 
> > > > The syscall function itself would have to move the arguments
and syscall
> > > > nr etc around before issuing the syscall.
> > > > 
> > > > I think the same is true of a similar hypercall(2)
> > > > 
> > > > > If you don''t think you will ever care about
putting HVC out of line
> > > > > though, it may not matter.
> > > 
> > > If you have both inline and out-of-line hypercalls, it''s
hard to ensure
> > > that you never have to shuffle the registers in either case.
> > 
> > Agreed.
> > 
> > I think we want to optimise for the inline case since those are the
> > majority.
> 
> They are not just the majority, all of them are static inline at the
> moment, even on x86 (where the number of hypercalls is much higher).
> 
> So yes, we should optimize for the inline case.
> 
> 
> > The only non-inline case is the special "privcmd ioctl"
which is the
> > mechanism that allows the Xen toolstack to make hypercalls.
It''s
> > somewhat akin to syscall(2). By the time you get to it you will
already
> > have done a system call for the ioctl, pulled the arguments from the
> > ioctl argument structure etc, plus such hypercalls are not really
> > performance critical.
> 
> Even the privcmd hypercall (privcmd_call) is a static inline function,
> it is just that at the moment there is only one caller :)
> 
> 
> > > Shuffling can be reduced but only at the expense of strange
argument
> > > ordering in some cases when calling from C -- the complexity is
probably
> > > not worth it.  Linux doesn''t bother for its own
syscalls.
> > > 
> > > Note that even in assembler, a branch from one section to a label
in
> > > another section may cause r12 to get destroyed, so you will need
to be
> > > careful about how you code the hypervisor trap handler.  However,
this
> > > is not different from coding exception handlers in general, so I
don''t
> > > know that it constitutes a conclusive argument on its own.
> > 
> > We are happy to arrange that this doesn''t occur on our trap
entry paths,
> > at least until the guest register state has been saved. Currently the
> > hypercall dispatcher is in C and gets r12 from the on-stack saved
state.
> > We will likely eventually optimise the hypercall path directly in ASM
> > and in that case we are happy to take steps to ensure we
don''t clobber
> > r12 before we need it.
> 
> Yes, I don''t think this should be an issue.
Fair enough.
> > > My instinctive preference would therefore be for r7 (which also
seems to
> > > be good enough for Linux syscalls) -- but it really depends how
many
> > > arguments you expect to need to support.
> > 
> > Apparently r7 is the frame pointer for gcc in thumb mode which I think
> > is a good reason to avoid it.
> > 
> > We currently have some 5 argument hypercalls and there have been
> > occasional suggestions for interfaces which use 6 -- although none of
> > them have come to reality.
>  
> I don''t have a very strong opinion on which register we should
use, but
> I would like to avoid r7 if it is already actively used by gcc.
But there is no framepointer for Thumb-2 code (?)
> The fact that r12 can be destroyed so easily is actually a good argument
> for using it because it means it is less likely to contain useful data
> that needs to be saved/restored by gcc.
That''s a fair point.

Cheers
---Dave

On Wed, 2012-02-29 at 09:34 +0000, Dave Martin wrote:
> On Tue, Feb 28, 2012 at 12:28:29PM +0000, Stefano Stabellini wrote:
> > I don''t have a very strong opinion on which register we
should use, but
> > I would like to avoid r7 if it is already actively used by gcc.
> 
> But there is no framepointer for Thumb-2 code (?)
Peter Maydell suggested there was:
> r7 is (used by gcc as) the Thumb frame pointer; I don''t know if
this
> makes it worth avoiding in this context.
Sounds like it might be a gcc-ism, possibly a non-default option?

Anyway, I think r12 will be fine for our purposes so the point is rather
moot.

Ian.

On Wed, Feb 29, 2012 at 09:56:02AM +0000, Ian Campbell
wrote:
> On Wed, 2012-02-29 at 09:34 +0000, Dave Martin wrote:
> > On Tue, Feb 28, 2012 at 12:28:29PM +0000, Stefano Stabellini wrote:
> 
> > > I don''t have a very strong opinion on which register we
should use, but
> > > I would like to avoid r7 if it is already actively used by gcc.
> > 
> > But there is no framepointer for Thumb-2 code (?)
> 
> Peter Maydell suggested there was:
> > r7 is (used by gcc as) the Thumb frame pointer; I don''t know
if this
> > makes it worth avoiding in this context.
> 
> Sounds like it might be a gcc-ism, possibly a non-default option?
I seem to remember discussions about some cruft in gcc related to this.
gcc actually barfs at you if you try to allocate r7 to inline asm
without -fomit-frame-pointer.  That use for r7 really relates to the
legacy ABI, so this may be a bug.
> Anyway, I think r12 will be fine for our purposes so the point is rather
> moot.
Yes, it sounds like it.  If that r7 issue is a gcc bug, this would avoid
it.

If you leave the job of putting the right constant into r12 up to gcc,
it should generate reasonable for you without having to code it
explicitly anyway:

	register int hvc_num asm("r12") = 0xDEADBEEF;

	asm volatile (
		"hvc	0"
		:: "r" (hvc_num)
	)

Cheers
---Dave

On Wed, Feb 29, 2012 at 09:56:02AM +0000, Ian Campbell
wrote:
> On Wed, 2012-02-29 at 09:34 +0000, Dave Martin wrote:
> > On Tue, Feb 28, 2012 at 12:28:29PM +0000, Stefano Stabellini wrote:
> 
> > > I don''t have a very strong opinion on which register we
should use, but
> > > I would like to avoid r7 if it is already actively used by gcc.
> > 
> > But there is no framepointer for Thumb-2 code (?)
> 
> Peter Maydell suggested there was:
> > r7 is (used by gcc as) the Thumb frame pointer; I don''t know
if this
> > makes it worth avoiding in this context.
> 
> Sounds like it might be a gcc-ism, possibly a non-default option?
> 
> Anyway, I think r12 will be fine for our purposes so the point is rather
> moot.
Just had a chat with some tools guys -- apparently, when passing register
arguments to gcc inline asms there really isn''t a guarantee that those
variables will be in the expected registers on entry to the inline asm.

If gcc reorders other function calls or other code around the inline asm
(which it can do, except under certain controlled situations), then
intervening code can clobber any registers in general.

Or, to summarise another way, there is no way to control which register
is used to pass something to an inline asm in general (often we get away
with this, and there are a lot of inline asms in the kernel that assume
it works, but the more you inline the more likely you are to get nasty
surprises).  There is no workaroud, except on some architectures where
special asm constraints allow specific individual registers to be
specified for operands (i386 for example).

If you need a specific register, this means that you must set up that
register explicitly inside the asm if you want a guarantee that the
code will work:

	asm volatile (
		"movw	r12, %[hvc_num]\n\t"
		...
		"hvc	#0"
		:: [hvc_num] "i" (NUMBER) : "r12"
	);

Of course, if you need to set up more than about 5 or 6 registers in
this way, the doubled register footprint means that the compiler will
have to start spilling stuff to the stack.


This is the kind of problem which goes away when out-of-lining the
hvc wrapper behind a C function interface, since the ABI then provides
guarantees about how values are mershaled into and out of that code.


Notwithstanding the above, even if we do make theoretically unsound
(but often true) assumptions about inline asms, ARM will be no worse
than other arches in this respect.


Other than serving as a reminder that inline asm is a deep can of
worms, this doesn''t really give us a neat solution...

---Dave

On Wed, 2012-02-29 at 12:58 +0000, Dave Martin wrote:
> On Wed, Feb 29, 2012 at 09:56:02AM +0000, Ian Campbell wrote:
> > On Wed, 2012-02-29 at 09:34 +0000, Dave Martin wrote:
> > > On Tue, Feb 28, 2012 at 12:28:29PM +0000, Stefano Stabellini
wrote:
> > 
> > > > I don''t have a very strong opinion on which
register we should use, but
> > > > I would like to avoid r7 if it is already actively used by
gcc.
> > > 
> > > But there is no framepointer for Thumb-2 code (?)
> > 
> > Peter Maydell suggested there was:
> > > r7 is (used by gcc as) the Thumb frame pointer; I don''t
know if this
> > > makes it worth avoiding in this context.
> > 
> > Sounds like it might be a gcc-ism, possibly a non-default option?
> > 
> > Anyway, I think r12 will be fine for our purposes so the point is
rather
> > moot.
> 
> Just had a chat with some tools guys -- apparently, when passing register
> arguments to gcc inline asms there really isn''t a guarantee that
those
> variables will be in the expected registers on entry to the inline asm.
> 
> If gcc reorders other function calls or other code around the inline asm
> (which it can do, except under certain controlled situations), then
> intervening code can clobber any registers in general.
> 
> Or, to summarise another way, there is no way to control which register
> is used to pass something to an inline asm in general (often we get away
> with this, and there are a lot of inline asms in the kernel that assume
> it works, but the more you inline the more likely you are to get nasty
> surprises).  There is no workaroud, except on some architectures where
> special asm constraints allow specific individual registers to be
> specified for operands (i386 for example).
I had assumed I just couldn''t find the right syntax. Useful to know
that
I couldn''t find it because it doesn''t exist!
> If you need a specific register, this means that you must set up that
> register explicitly inside the asm if you want a guarantee that the
> code will work:
> 
> 	asm volatile (
> 		"movw	r12, %[hvc_num]\n\t"
Is gcc (or gas?) smart enough to optimise this away if it turns out that
%[hvc_num] == r12?
> 		...
> 		"hvc	#0"
> 		:: [hvc_num] "i" (NUMBER) : "r12"
> 	);
> 
> Of course, if you need to set up more than about 5 or 6 registers in
> this way, the doubled register footprint means that the compiler will
> have to start spilling stuff to the stack.
> 
> 
> This is the kind of problem which goes away when out-of-lining the
> hvc wrapper behind a C function interface, since the ABI then provides
> guarantees about how values are mershaled into and out of that code.
I don''t think anything would stop gcc from clobbering an argument
register right on function entry (e..g it might move r0 to r8 and
clobber r0, for whatever reason), so that they are no longer where you
expect them to be when you hit the asm. Unlikely perhaps but no more so
than the other issues you''ve raised?
	
Or did you mean out-of-line as in "written in a .S file" as well as
out
of line?
> Notwithstanding the above, even if we do make theoretically unsound
> (but often true) assumptions about inline asms, ARM will be no worse
> than other arches in this respect.
This is true.
> Other than serving as a reminder that inline asm is a deep can of
> worms, this doesn''t really give us a neat solution...
How are system calls implemented on the userspace side? I confess I
don''t know what the ARM syscall ABI looks like -- is it all registers
or
is some of it on the stack? It sounds like the solution ought to be
pretty similar though.

Ian.

On Wed, 29 Feb 2012, Dave Martin wrote:
> On Wed, Feb 29, 2012 at 09:56:02AM +0000, Ian Campbell wrote:
> > On Wed, 2012-02-29 at 09:34 +0000, Dave Martin wrote:
> > > On Tue, Feb 28, 2012 at 12:28:29PM +0000, Stefano Stabellini
wrote:
> > 
> > > > I don''t have a very strong opinion on which
register we should use, but
> > > > I would like to avoid r7 if it is already actively used by
gcc.
> > > 
> > > But there is no framepointer for Thumb-2 code (?)
> > 
> > Peter Maydell suggested there was:
> > > r7 is (used by gcc as) the Thumb frame pointer; I don''t
know if this
> > > makes it worth avoiding in this context.
> > 
> > Sounds like it might be a gcc-ism, possibly a non-default option?
> > 
> > Anyway, I think r12 will be fine for our purposes so the point is
rather
> > moot.
> 
> Just had a chat with some tools guys -- apparently, when passing register
> arguments to gcc inline asms there really isn''t a guarantee that
those
> variables will be in the expected registers on entry to the inline asm.
> 
> If gcc reorders other function calls or other code around the inline asm
> (which it can do, except under certain controlled situations), then
> intervening code can clobber any registers in general.
> 
> Or, to summarise another way, there is no way to control which register
> is used to pass something to an inline asm in general (often we get away
> with this, and there are a lot of inline asms in the kernel that assume
> it works, but the more you inline the more likely you are to get nasty
> surprises).  There is no workaroud, except on some architectures where
> special asm constraints allow specific individual registers to be
> specified for operands (i386 for example).
> 
> If you need a specific register, this means that you must set up that
> register explicitly inside the asm if you want a guarantee that the
> code will work:
> 
> 	asm volatile (
> 		"movw	r12, %[hvc_num]\n\t"
> 		...
> 		"hvc	#0"
> 		:: [hvc_num] "i" (NUMBER) : "r12"
> 	);
> 
OK, we can arrange the hypercall code to be like that.
Also with your patch series it would be "_hvc" because of the .macro,
right?


> This is the kind of problem which goes away when out-of-lining the
> hvc wrapper behind a C function interface, since the ABI then provides
> guarantees about how values are mershaled into and out of that code.
Do you mean implementing the entire HYPERVISOR_example_op in assembly
and calling it from C?
Because I guess that gcc would still be free to mess with the registers
between the C function entry point and any inline assembly code.

On Wed, Feb 29, 2012 at 02:44:24PM +0000, Ian Campbell
wrote:
> On Wed, 2012-02-29 at 12:58 +0000, Dave Martin wrote:
> > On Wed, Feb 29, 2012 at 09:56:02AM +0000, Ian Campbell wrote:
> > > On Wed, 2012-02-29 at 09:34 +0000, Dave Martin wrote:
> > > > On Tue, Feb 28, 2012 at 12:28:29PM +0000, Stefano Stabellini
wrote:
> > > 
> > > > > I don''t have a very strong opinion on which
register we should use, but
> > > > > I would like to avoid r7 if it is already actively used
by gcc.
> > > > 
> > > > But there is no framepointer for Thumb-2 code (?)
> > > 
> > > Peter Maydell suggested there was:
> > > > r7 is (used by gcc as) the Thumb frame pointer; I
don''t know if this
> > > > makes it worth avoiding in this context.
> > > 
> > > Sounds like it might be a gcc-ism, possibly a non-default option?
> > > 
> > > Anyway, I think r12 will be fine for our purposes so the point is
rather
> > > moot.
> > 
> > Just had a chat with some tools guys -- apparently, when passing
register
> > arguments to gcc inline asms there really isn''t a guarantee
that those
> > variables will be in the expected registers on entry to the inline
asm.
> > 
> > If gcc reorders other function calls or other code around the inline
asm
> > (which it can do, except under certain controlled situations), then
> > intervening code can clobber any registers in general.
> > 
> > Or, to summarise another way, there is no way to control which
register
> > is used to pass something to an inline asm in general (often we get
away
> > with this, and there are a lot of inline asms in the kernel that
assume
> > it works, but the more you inline the more likely you are to get nasty
> > surprises).  There is no workaroud, except on some architectures where
> > special asm constraints allow specific individual registers to be
> > specified for operands (i386 for example).
> 
> I had assumed I just couldn''t find the right syntax. Useful to
know that
> I couldn''t find it because it doesn''t exist!
> 
> > If you need a specific register, this means that you must set up that
> > register explicitly inside the asm if you want a guarantee that the
> > code will work:
> > 
> > 	asm volatile (
> > 		"movw	r12, %[hvc_num]\n\t"
> 
> Is gcc (or gas?) smart enough to optimise this away if it turns out that
> %[hvc_num] == r12?
No, unfortunately.  Except for the information defined by the constraints,
the inline asm block is completely opaque to the compiler (except for
pasting in operands -- which is a string operation done with no knowledge
of what the text means for the assembler).
> 
> > 		...
> > 		"hvc	#0"
> > 		:: [hvc_num] "i" (NUMBER) : "r12"
> > 	);
> > 
> > Of course, if you need to set up more than about 5 or 6 registers in
> > this way, the doubled register footprint means that the compiler will
> > have to start spilling stuff to the stack.
> > 
> > 
> > This is the kind of problem which goes away when out-of-lining the
> > hvc wrapper behind a C function interface, since the ABI then provides
> > guarantees about how values are mershaled into and out of that code.
> 
> I don''t think anything would stop gcc from clobbering an argument
> register right on function entry (e..g it might move r0 to r8 and
> clobber r0, for whatever reason), so that they are no longer where you
> expect them to be when you hit the asm. Unlikely perhaps but no more so
> than the other issues you''ve raised?
> 	
> Or did you mean out-of-line as in "written in a .S file" as well
as out
> of line?
Yes.  Some toolchains have a concept of out-of-line assembler functions
in a .c file, but gcc doesn''t -- the asm is always inline in its
immediate context, even if the containing function won''t be inlined.

However, the compiler would have to be applying pretty creative
optimizations to break cases cases where an inlinable function contains,
say, nothing except for declarations, the asm() and a return statement.

I feel that the kernel implicitly relies on such things working in too
many places for breakage of that assumption to go unnoticed.
> 
> > Notwithstanding the above, even if we do make theoretically unsound
> > (but often true) assumptions about inline asms, ARM will be no worse
> > than other arches in this respect.
> 
> This is true.
> 
> > Other than serving as a reminder that inline asm is a deep can of
> > worms, this doesn''t really give us a neat solution...
> 
> How are system calls implemented on the userspace side? I confess I
> don''t know what the ARM syscall ABI looks like -- is it all
registers or
> is some of it on the stack? It sounds like the solution ought to be
> pretty similar though.
I _believe_ it''s now out of line in most cases.

I''m not sure I totally understand it all, though:

http://www.eglibc.org/cgi-bin/viewvc.cgi/trunk/ports/sysdeps/unix/sysv/linux/arm/eabi/

There is an internal inline syscall wrapper INTERNAL_SYSCALL_RAW(), but
I can''t see where it is used.  For Thumb code it actually just munges
registers around and calls an out-of-line function.

If I grep the disassembly of a recent EABI libc, there appear to be only
207 svc call sites, and most of them look like they are out-of-linux
wrappers, generated from the DO_CALL macro in
ports/sysdeps/unix/sysv/linux/arm/eabi/sysdep.h

That''s based on a hasty reading of the code though... I''m not
very
familiar with the way libc works.  (Disassembling stripped arm binaries
can also be a bit unrelieable.)

It''s also worth nothing that the inline asm sycall macros which used
to exist in userspace <asm/unistd.h> are gone (at least for EABI).

Cheers
---Dave

On Wed, Feb 29, 2012 at 02:52:38PM +0000, Stefano Stabellini
wrote:
> On Wed, 29 Feb 2012, Dave Martin wrote:
> > On Wed, Feb 29, 2012 at 09:56:02AM +0000, Ian Campbell wrote:
> > > On Wed, 2012-02-29 at 09:34 +0000, Dave Martin wrote:
> > > > On Tue, Feb 28, 2012 at 12:28:29PM +0000, Stefano Stabellini
wrote:
> > > 
> > > > > I don''t have a very strong opinion on which
register we should use, but
> > > > > I would like to avoid r7 if it is already actively used
by gcc.
> > > > 
> > > > But there is no framepointer for Thumb-2 code (?)
> > > 
> > > Peter Maydell suggested there was:
> > > > r7 is (used by gcc as) the Thumb frame pointer; I
don''t know if this
> > > > makes it worth avoiding in this context.
> > > 
> > > Sounds like it might be a gcc-ism, possibly a non-default option?
> > > 
> > > Anyway, I think r12 will be fine for our purposes so the point is
rather
> > > moot.
> > 
> > Just had a chat with some tools guys -- apparently, when passing
register
> > arguments to gcc inline asms there really isn''t a guarantee
that those
> > variables will be in the expected registers on entry to the inline
asm.
> > 
> > If gcc reorders other function calls or other code around the inline
asm
> > (which it can do, except under certain controlled situations), then
> > intervening code can clobber any registers in general.
> > 
> > Or, to summarise another way, there is no way to control which
register
> > is used to pass something to an inline asm in general (often we get
away
> > with this, and there are a lot of inline asms in the kernel that
assume
> > it works, but the more you inline the more likely you are to get nasty
> > surprises).  There is no workaroud, except on some architectures where
> > special asm constraints allow specific individual registers to be
> > specified for operands (i386 for example).
> > 
> > If you need a specific register, this means that you must set up that
> > register explicitly inside the asm if you want a guarantee that the
> > code will work:
> > 
> > 	asm volatile (
> > 		"movw	r12, %[hvc_num]\n\t"
> > 		...
> > 		"hvc	#0"
> > 		:: [hvc_num] "i" (NUMBER) : "r12"
> > 	);
> > 
> 
> OK, we can arrange the hypercall code to be like that.
> Also with your patch series it would be "_hvc" because of the
.macro,
> right?
Yes, but I would avoid making too many assumptions about the final form
of that patch -- it looks like there''s significant work to do there,
since I made some unsafe assumptions about how the tools work...

We might end up with a magic #define after all.
> > This is the kind of problem which goes away when out-of-lining the
> > hvc wrapper behind a C function interface, since the ABI then provides
> > guarantees about how values are mershaled into and out of that code.
> 
> Do you mean implementing the entire HYPERVISOR_example_op in assembly
> and calling it from C?
> Because I guess that gcc would still be free to mess with the registers
> between the C function entry point and any inline assembly code.
gcc can arrange for the relevant things to be already in r0-r3 and the
relevant stack slots before branching to a function just as for inline
asm.  The only differences are that the compiler cannot choose which
registers to use, and the branch cannot be optimised away by the compiler
(the CPU may be able to optimise the branch away at runtime of course,
but that''s another story...)

What libc appears to do is wrap each syscall in a separate function.
This means that it''s not necessary to shuffle all the arguments by
one position when invoking the actual syscall.  (The generic "syscall"
function does of course need to shuffle the arguments so as to
displace the syscall number from the first argument to r7 --
but that''s hard to avoid without inlining.)

For example:

00090b50 <shmdt>:
   90b50:       e52d7004        push    {r7}            ; (str r7, [sp, #-4]!)
   90b54:       e59f7010        ldr     r7, [pc, #16]   ; 90b6c
<shmdt+0x1c>
   90b58:       ef000000        svc     0x00000000
   90b5c:       e49d7004        pop     {r7}            ; (ldr r7, [sp], #4)
   90b60:       e3700a01        cmn     r0, #4096       ; 0x1000
...

Syscalls with more than 4 args still need to load the extra ones
from the stack, of course:

00090090 <getsockopt>:
   90090:       e92d0090        push    {r4, r7}
   90094:       e59d4008        ldr     r4, [sp, #8]
   90098:       e59f7010        ldr     r7, [pc, #16]   ; 900b0
<getsockopt+0x20>
   9009c:       ef000000        svc     0x00000000
...


I don''t know whether that makes sense for a hypervisor... it partly
depends on how many different hypercalls there are.

By all means implement it both ways and measure the performance
difference, if possible.

Cheers
---Dave

On Wed, Feb 29, 2012 at 12:58:26PM +0000, Dave Martin
wrote:
> On Wed, Feb 29, 2012 at 09:56:02AM +0000, Ian Campbell wrote:
> > On Wed, 2012-02-29 at 09:34 +0000, Dave Martin wrote:
> > > On Tue, Feb 28, 2012 at 12:28:29PM +0000, Stefano Stabellini
wrote:
> > 
> > > > I don''t have a very strong opinion on which
register we should use, but
> > > > I would like to avoid r7 if it is already actively used by
gcc.
> > > 
> > > But there is no framepointer for Thumb-2 code (?)
> > 
> > Peter Maydell suggested there was:
> > > r7 is (used by gcc as) the Thumb frame pointer; I don''t
know if this
> > > makes it worth avoiding in this context.
> > 
> > Sounds like it might be a gcc-ism, possibly a non-default option?
> > 
> > Anyway, I think r12 will be fine for our purposes so the point is
rather
> > moot.
> 
> Just had a chat with some tools guys -- apparently, when passing register
> arguments to gcc inline asms there really isn''t a guarantee that
those
> variables will be in the expected registers on entry to the inline asm.
The best you can do is:

	register unsigned int foo asm("r7") = value;

	asm("blah %0" : : "r" (foo));

and ensure that your assembly checks that %0 _is_ r7 and produces a build
error if not.  See the __asmeq() macro in asm/system.h to find out how to
do that.

This feature has been missing from ARM GCC for quite a long time - it''s
used extensively on x86 GCC, where they have one register class per
register, so they can do stuff like:

	asm("blah %0" : : "a" (value));

and be guaranteed that %0 will be eax.
> If you need a specific register, this means that you must set up that
> register explicitly inside the asm if you want a guarantee that the
> code will work:
> 
> 	asm volatile (
> 		"movw	r12, %[hvc_num]\n\t"
> 		...
> 		"hvc	#0"
> 		:: [hvc_num] "i" (NUMBER) : "r12"
> 	);
> 
> Of course, if you need to set up more than about 5 or 6 registers in
> this way, the doubled register footprint means that the compiler will
> have to start spilling stuff to the stack.
No it won''t - it will barf instead - think about it.  If
you''re clobbering
r0 - r5, but need to pass in six values in registers, gcc can''t use
r0-r5
for that, so it must use the remaining registers.  It gets rather unhappy
with that, and starts erroring out (iirc ''too many reloads'' or
some similar
error.)  I''ve been there.

If you want to do it that way, your only option is to store them to memory
and pass the address of the block into the assembly, and reload them there.
Which is extremely sucky and inefficient.

Practically, the register variable plus asm() does seem to work, and seems
to work for virtually all gcc versions out there (there have been the odd
buggy version, but those bugs appear to get fixed.)

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Feb 29, 2012 at 02:44:24PM +0000, Ian Campbell
wrote:
> > If you need a specific register, this means that you must set up that
> > register explicitly inside the asm if you want a guarantee that the
> > code will work:
> > 
> > 	asm volatile (
> > 		"movw	r12, %[hvc_num]\n\t"
> 
> Is gcc (or gas?) smart enough to optimise this away if it turns out that
> %[hvc_num] == r12?
No, and it won''t do, because %[hvc_num] is specified in these operands:
> > 		...
> > 		"hvc	#0"
> > 		:: [hvc_num] "i" (NUMBER) : "r12"
to be an integer, not a register.
> How are system calls implemented on the userspace side? I confess I
> don''t know what the ARM syscall ABI looks like -- is it all
registers or
> is some of it on the stack? It sounds like the solution ought to be
> pretty similar though.
All registers.  We have a few which take a pointer to an in memory array,
but those are for some old multiplexed syscalls.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Mar 01, 2012 at 10:10:29AM +0000, Russell King - ARM Linux
wrote:
> On Wed, Feb 29, 2012 at 12:58:26PM +0000, Dave Martin wrote:
> > On Wed, Feb 29, 2012 at 09:56:02AM +0000, Ian Campbell wrote:
> > > On Wed, 2012-02-29 at 09:34 +0000, Dave Martin wrote:
> > > > On Tue, Feb 28, 2012 at 12:28:29PM +0000, Stefano Stabellini
wrote:
> > > 
> > > > > I don''t have a very strong opinion on which
register we should use, but
> > > > > I would like to avoid r7 if it is already actively used
by gcc.
> > > > 
> > > > But there is no framepointer for Thumb-2 code (?)
> > > 
> > > Peter Maydell suggested there was:
> > > > r7 is (used by gcc as) the Thumb frame pointer; I
don''t know if this
> > > > makes it worth avoiding in this context.
> > > 
> > > Sounds like it might be a gcc-ism, possibly a non-default option?
> > > 
> > > Anyway, I think r12 will be fine for our purposes so the point is
rather
> > > moot.
> > 
> > Just had a chat with some tools guys -- apparently, when passing
register
> > arguments to gcc inline asms there really isn''t a guarantee
that those
> > variables will be in the expected registers on entry to the inline
asm.
> 
> The best you can do is:
> 
> 	register unsigned int foo asm("r7") = value;
> 
> 	asm("blah %0" : : "r" (foo));
> 
> and ensure that your assembly checks that %0 _is_ r7 and produces a build
> error if not.  See the __asmeq() macro in asm/system.h to find out how to
> do that.
> 
> This feature has been missing from ARM GCC for quite a long time -
it''s
> used extensively on x86 GCC, where they have one register class per
> register, so they can do stuff like:
> 
> 	asm("blah %0" : : "a" (value));
> 
> and be guaranteed that %0 will be eax.
> 
> > If you need a specific register, this means that you must set up that
> > register explicitly inside the asm if you want a guarantee that the
> > code will work:
> > 
> > 	asm volatile (
> > 		"movw	r12, %[hvc_num]\n\t"
> > 		...
> > 		"hvc	#0"
> > 		:: [hvc_num] "i" (NUMBER) : "r12"
> > 	);
> > 
> > Of course, if you need to set up more than about 5 or 6 registers in
> > this way, the doubled register footprint means that the compiler will
> > have to start spilling stuff to the stack.
> 
> No it won''t - it will barf instead - think about it.  If
you''re clobbering
> r0 - r5, but need to pass in six values in registers, gcc can''t
use r0-r5
> for that, so it must use the remaining registers.  It gets rather unhappy
> with that, and starts erroring out (iirc ''too many
reloads'' or some similar
> error.)  I''ve been there.
You''re right about that -- I didn''t pursue my line of thought
to the end,
there.  I have see the behaviour you describe.
> If you want to do it that way, your only option is to store them to memory
> and pass the address of the block into the assembly, and reload them there.
> Which is extremely sucky and inefficient.
> 
> Practically, the register variable plus asm() does seem to work, and seems
> to work for virtually all gcc versions out there (there have been the odd
> buggy version, but those bugs appear to get fixed.)
That is inconvenient for us, but it''s a not a bug.  The ability for asm
contraints to be able to gcc to put things in specific registers (as with
the gcc "abcd" constraints for i386) would be nice, but as you point
out,
this capability is simply not supported by gcc right now for ARM -- the
compiler guys seem to be pretty opposed to it, so I can''t say I
anticiapte
this being supported in the near future.

So, where there''s a compelling reason to inline these things, we can
use
the existing techniques if we''re alert to the risks.  But in cases
where
there isn''t a compelling reason, aren''t we just inviting
fragility
unnecessarily?

Cheers
---Dave

On Thu, Mar 01, 2012 at 10:27:02AM +0000, Dave Martin
wrote:
> So, where there''s a compelling reason to inline these things, we
can use
> the existing techniques if we''re alert to the risks.  But in cases
where
> there isn''t a compelling reason, aren''t we just inviting
fragility
> unnecessarily?
The practical experience from the kernel suggests that there isn''t a
problem - that''s not to say that future versions of gcc won''t
become
a problem, and that the compiler guys may refuse to fix it.

I think it''s a feature that we should be pressing gcc guys for -
it''s
fairly fundamental to any programming which requires interfaces that
require certain args in certain registers, or receive results in
certain registers.

The options over this are basically:
1. refusing to upgrade to any version of gcc which does not allow
   registers-in-asm
2. doing the store-to-memory reload-in-asm thing
3. hand-coding veneers for every call to marshall the registers

Each of those has its down sides, but I suspect with (1), it may be
possible to have enough people applying pressure to the compiler guys
that they finally see sense on this matter.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 1 Mar 2012, Russell King - ARM Linux wrote:
> On Thu, Mar 01, 2012 at 10:27:02AM +0000, Dave Martin wrote:
> > So, where there''s a compelling reason to inline these things,
we can use
> > the existing techniques if we''re alert to the risks.  But in
cases where
> > there isn''t a compelling reason, aren''t we just
inviting fragility
> > unnecessarily?
> 
> The practical experience from the kernel suggests that there isn''t
a
> problem - that''s not to say that future versions of gcc
won''t become
> a problem, and that the compiler guys may refuse to fix it.
> 
> I think it''s a feature that we should be pressing gcc guys for -
it''s
> fairly fundamental to any programming which requires interfaces that
> require certain args in certain registers, or receive results in
> certain registers.
> 
> The options over this are basically:
> 1. refusing to upgrade to any version of gcc which does not allow
>    registers-in-asm
> 2. doing the store-to-memory reload-in-asm thing
> 3. hand-coding veneers for every call to marshall the registers
> 
> Each of those has its down sides, but I suspect with (1), it may be
> possible to have enough people applying pressure to the compiler guys
> that they finally see sense on this matter.
I tend to have a very practical approach about this sort of issues, so I
am tempted to go with 1) if you agree.

[ coming back from vacation and trying to catch up ]

On Wed, 29 Feb 2012, Dave Martin wrote:
> Just had a chat with some tools guys -- apparently, when passing register
> arguments to gcc inline asms there really isn''t a guarantee that
those
> variables will be in the expected registers on entry to the inline asm.
> 
> If gcc reorders other function calls or other code around the inline asm
> (which it can do, except under certain controlled situations), then
> intervening code can clobber any registers in general.
I''m hearing this argument about once every year or so for the last 8 
years.  I think that the tools people are getting confused between 
themselves as you may get a different interpretation of what gcc should 
do depending to whom you happen to talk to.

I did submit a bug to gcc in 2004 about this:

	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15089

You can see the confusion among gcc developers lurking there.

So let''s quote the relevant gcc documentation:

-> * C Extensions::    GNU extensions to the C language family.
   -> * Explicit Reg Vars::   Defining variables residing in specified
registers.

|GNU C allows you to put a few global variables into specified hardware 
|registers.  You can also specify the register in which an ordinary 
|register variable should be allocated.
|
|   * Global register variables reserve registers throughout the program.
|     This may be useful in programs such as programming language
|     interpreters which have a couple of global variables that are
|     accessed very often.
|
|   * Local register variables in specific registers do not reserve the
|     registers, except at the point where they are used as input or
|     output operands in an `asm'' statement and the `asm''
statement
|     itself is not deleted.  The compiler''s data flow analysis is
|     capable of determining where the specified registers contain live
|     values, and where they are available for other uses.  Stores into
|     local register variables may be deleted when they appear to be
|     dead according to dataflow analysis.  References to local register
|     variables may be deleted or moved or simplified.
|
|     These local variables are sometimes convenient for use with the
|     extended `asm'' feature (*note Extended Asm::), if you want to
|     write one output of the assembler instruction directly into a
|     particular register.  (This will work provided the register you
|     specify fits the constraints specified for that operand in the
|     `asm''.)

      -> * Local Reg Vars::

[...]

| Defining such a register variable does not reserve the register; it 
|remains available for other uses in places where flow control 
|determines the variable''s value is not live.
|
| This option does not guarantee that GCC will generate code that has
|this variable in the register you specify at all times.  You may not
|code an explicit reference to this register in the _assembler
|instruction template_ part of an `asm'' statement and assume it will
|always refer to this variable.  However, using the variable as an
`asm''
|_operand_ guarantees that the specified register is used for the
|operand.

So, to me, the gcc documentation is perfectly clear on this topic.  
there really _is_ a guarantee that those asm marked variables will be in 
the expected registers on entry to the inline asm, given that the 
variable is _also_ listed as an operand to the asm statement.  But only 
in that case.

It is true that gcc may reorder other function calls or other code 
around the inline asm and then intervening code can clobber any 
registers.  Then it is up to gcc to preserve the variable''s content 
elsewhere when its register is used for other purposes, and restore it 
when some inline asm statement is referring to it.

And if gcc does not do this then it is buggy.  Version 3.4.0 of gcc was 
buggy.  No other gcc versions in the last 7 years had such a problem or 
the __asmeq macro in the kernel would have told us.
> Or, to summarise another way, there is no way to control which register
> is used to pass something to an inline asm in general (often we get away
> with this, and there are a lot of inline asms in the kernel that assume
> it works, but the more you inline the more likely you are to get nasty
> surprises). 
This statement is therefore unfounded and wrong.  Please direct the 
tools guy who mislead you to the above gcc documentation.


Nicolas

On Wed, 29 Feb 2012, Ian Campbell wrote:
> On Wed, 2012-02-29 at 12:58 +0000, Dave Martin wrote:
> > On Wed, Feb 29, 2012 at 09:56:02AM +0000, Ian Campbell wrote:
> > > On Wed, 2012-02-29 at 09:34 +0000, Dave Martin wrote:
> > > > On Tue, Feb 28, 2012 at 12:28:29PM +0000, Stefano Stabellini
wrote:
> > > 
> > > > > I don''t have a very strong opinion on which
register we should use, but
> > > > > I would like to avoid r7 if it is already actively used
by gcc.
> > > > 
> > > > But there is no framepointer for Thumb-2 code (?)
> > > 
> > > Peter Maydell suggested there was:
> > > > r7 is (used by gcc as) the Thumb frame pointer; I
don''t know if this
> > > > makes it worth avoiding in this context.
> > > 
> > > Sounds like it might be a gcc-ism, possibly a non-default option?
> > > 
> > > Anyway, I think r12 will be fine for our purposes so the point is
rather
> > > moot.
> > 
> > Just had a chat with some tools guys -- apparently, when passing
register
> > arguments to gcc inline asms there really isn''t a guarantee
that those
> > variables will be in the expected registers on entry to the inline
asm.
> > 
> > If gcc reorders other function calls or other code around the inline
asm
> > (which it can do, except under certain controlled situations), then
> > intervening code can clobber any registers in general.
> > 
> > Or, to summarise another way, there is no way to control which
register
> > is used to pass something to an inline asm in general (often we get
away
> > with this, and there are a lot of inline asms in the kernel that
assume
> > it works, but the more you inline the more likely you are to get nasty
> > surprises).  There is no workaroud, except on some architectures where
> > special asm constraints allow specific individual registers to be
> > specified for operands (i386 for example).
> 
> I had assumed I just couldn''t find the right syntax. Useful to
know that
> I couldn''t find it because it doesn''t exist!
It does exist.  See my previous reply to this thread.

You can find this syntax described in section 6.44.2 "Specifying 
Registers for Local Variables" in gcc v4.6.1 (possibly another section 
number in the documentation for a different gcc version).


Nicolas

On 02/03/12 21:15, Nicolas Pitre wrote:
> [ coming back from vacation and trying to catch up ]
>
> On Wed, 29 Feb 2012, Dave Martin wrote:
>
>> Just had a chat with some tools guys -- apparently, when passing
register
>> arguments to gcc inline asms there really isn''t a guarantee
that those
>> variables will be in the expected registers on entry to the inline asm.
>>
>> If gcc reorders other function calls or other code around the inline
asm
>> (which it can do, except under certain controlled situations), then
>> intervening code can clobber any registers in general.
>
> I''m hearing this argument about once every year or so for the last
8
> years.  I think that the tools people are getting confused between
> themselves as you may get a different interpretation of what gcc should
> do depending to whom you happen to talk to.
>
> I did submit a bug to gcc in 2004 about this:
>
>       http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15089
>
> You can see the confusion among gcc developers lurking there.
>
> So let''s quote the relevant gcc documentation:
>
> -> * C Extensions::    GNU extensions to the C language family.
>    -> * Explicit Reg Vars::   Defining variables residing in specified
registers.
>
> |GNU C allows you to put a few global variables into specified hardware
> |registers.  You can also specify the register in which an ordinary
> |register variable should be allocated.
> |
> |   * Global register variables reserve registers throughout the program.
> |     This may be useful in programs such as programming language
> |     interpreters which have a couple of global variables that are
> |     accessed very often.
> |
> |   * Local register variables in specific registers do not reserve the
> |     registers, except at the point where they are used as input or
> |     output operands in an `asm'' statement and the `asm''
statement
> |     itself is not deleted.  The compiler''s data flow analysis is
> |     capable of determining where the specified registers contain live
> |     values, and where they are available for other uses.  Stores into
> |     local register variables may be deleted when they appear to be
> |     dead according to dataflow analysis.  References to local register
> |     variables may be deleted or moved or simplified.
> |
> |     These local variables are sometimes convenient for use with the
> |     extended `asm'' feature (*note Extended Asm::), if you want
to
> |     write one output of the assembler instruction directly into a
> |     particular register.  (This will work provided the register you
> |     specify fits the constraints specified for that operand in the
> |     `asm''.)
>
>       -> * Local Reg Vars::
>
> [...]
>
> | Defining such a register variable does not reserve the register; it
> |remains available for other uses in places where flow control
> |determines the variable''s value is not live.
> |
> | This option does not guarantee that GCC will generate code that has
> |this variable in the register you specify at all times.  You may not
> |code an explicit reference to this register in the _assembler
> |instruction template_ part of an `asm'' statement and assume it
will
> |always refer to this variable.  However, using the variable as an
`asm''
> |_operand_ guarantees that the specified register is used for the
> |operand.
>
> So, to me, the gcc documentation is perfectly clear on this topic.
> there really _is_ a guarantee that those asm marked variables will be in
> the expected registers on entry to the inline asm, given that the
> variable is _also_ listed as an operand to the asm statement.  But only
> in that case.
>
> It is true that gcc may reorder other function calls or other code
> around the inline asm and then intervening code can clobber any
> registers.  Then it is up to gcc to preserve the variable''s
content
> elsewhere when its register is used for other purposes, and restore it
> when some inline asm statement is referring to it.
>
> And if gcc does not do this then it is buggy.  Version 3.4.0 of gcc was
> buggy.  No other gcc versions in the last 7 years had such a problem or
> the __asmeq macro in the kernel would have told us.
>
>> Or, to summarise another way, there is no way to control which register
>> is used to pass something to an inline asm in general (often we get
away
>> with this, and there are a lot of inline asms in the kernel that assume
>> it works, but the more you inline the more likely you are to get nasty
>> surprises).
>
> This statement is therefore unfounded and wrong.  Please direct the
> tools guy who mislead you to the above gcc documentation.
>
The problem is not really about re-ordering functions but about implicit
functions that come from the source code; for example

int foo (int a, int b)
{
  register int x __asm__("r0") = 33;

  register int c __asm__("r1") = a / b; /* Ooops, clobbers r0 with
division function call.  */

  asm ("svc 0" : : "r" (x));
}

There''s nothing in the specification to say what happens if
there''s a
statement in the code that causes an implicit clobber of your assembly
register.

The best you can do here is to never initialize asm regs until you are
about to use them, but even that won''t help if your register
initialization code needs the help of a function call.

Other common ways of exposing this weakness in the spec are TLS
variables and structure copies...

R.

-- IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium.  Thank you.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Mar 08, 2012 at 09:58:23AM +0000, Richard Earnshaw
wrote:
> On 02/03/12 21:15, Nicolas Pitre wrote:
> > [ coming back from vacation and trying to catch up ]
> >
> > On Wed, 29 Feb 2012, Dave Martin wrote:
> >
> >> Just had a chat with some tools guys -- apparently, when passing
register
> >> arguments to gcc inline asms there really isn''t a
guarantee that those
> >> variables will be in the expected registers on entry to the inline
asm.
> >>
> >> If gcc reorders other function calls or other code around the
inline asm
> >> (which it can do, except under certain controlled situations),
then
> >> intervening code can clobber any registers in general.
> >
> > I''m hearing this argument about once every year or so for the
last 8
> > years.  I think that the tools people are getting confused between
> > themselves as you may get a different interpretation of what gcc
should
> > do depending to whom you happen to talk to.
> >
> > I did submit a bug to gcc in 2004 about this:
> >
> >       http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15089
> >
> > You can see the confusion among gcc developers lurking there.
> >
> > So let''s quote the relevant gcc documentation:
> >
> > -> * C Extensions::    GNU extensions to the C language family.
> >    -> * Explicit Reg Vars::   Defining variables residing in
specified registers.
> >
> > |GNU C allows you to put a few global variables into specified
hardware
> > |registers.  You can also specify the register in which an ordinary
> > |register variable should be allocated.
> > |
> > |   * Global register variables reserve registers throughout the
program.
> > |     This may be useful in programs such as programming language
> > |     interpreters which have a couple of global variables that are
> > |     accessed very often.
> > |
> > |   * Local register variables in specific registers do not reserve
the
> > |     registers, except at the point where they are used as input or
> > |     output operands in an `asm'' statement and the
`asm'' statement
> > |     itself is not deleted.  The compiler''s data flow
analysis is
> > |     capable of determining where the specified registers contain
live
> > |     values, and where they are available for other uses.  Stores
into
> > |     local register variables may be deleted when they appear to be
> > |     dead according to dataflow analysis.  References to local
register
> > |     variables may be deleted or moved or simplified.
> > |
> > |     These local variables are sometimes convenient for use with the
> > |     extended `asm'' feature (*note Extended Asm::), if you
want to
> > |     write one output of the assembler instruction directly into a
> > |     particular register.  (This will work provided the register you
> > |     specify fits the constraints specified for that operand in the
> > |     `asm''.)
> >
> >       -> * Local Reg Vars::
> >
> > [...]
> >
> > | Defining such a register variable does not reserve the register; it
> > |remains available for other uses in places where flow control
> > |determines the variable''s value is not live.
> > |
> > | This option does not guarantee that GCC will generate code that has
> > |this variable in the register you specify at all times.  You may not
> > |code an explicit reference to this register in the _assembler
> > |instruction template_ part of an `asm'' statement and assume
it will
> > |always refer to this variable.  However, using the variable as an
`asm''
> > |_operand_ guarantees that the specified register is used for the
> > |operand.
Hmmm, it''s a while since I saw that documentation, and it had clearly
fallen out of my head when I made my previous statements...
> >
> > So, to me, the gcc documentation is perfectly clear on this topic.
> > there really _is_ a guarantee that those asm marked variables will be
in
> > the expected registers on entry to the inline asm, given that the
> > variable is _also_ listed as an operand to the asm statement.  But
only
> > in that case.
> >
> > It is true that gcc may reorder other function calls or other code
> > around the inline asm and then intervening code can clobber any
> > registers.  Then it is up to gcc to preserve the variable''s
content
> > elsewhere when its register is used for other purposes, and restore it
> > when some inline asm statement is referring to it.
> >
> > And if gcc does not do this then it is buggy.  Version 3.4.0 of gcc
was
> > buggy.  No other gcc versions in the last 7 years had such a problem
or
> > the __asmeq macro in the kernel would have told us.
> >
> >> Or, to summarise another way, there is no way to control which
register
> >> is used to pass something to an inline asm in general (often we
get away
> >> with this, and there are a lot of inline asms in the kernel that
assume
> >> it works, but the more you inline the more likely you are to get
nasty
> >> surprises).
> >
> > This statement is therefore unfounded and wrong.  Please direct the
> > tools guy who mislead you to the above gcc documentation.
> >
> 
> The problem is not really about re-ordering functions but about implicit
> functions that come from the source code; for example
> 
> int foo (int a, int b)
> {
>   register int x __asm__("r0") = 33;
> 
>   register int c __asm__("r1") = a / b; /* Ooops, clobbers r0
with
> division function call.  */
> 
>   asm ("svc 0" : : "r" (x));
> }
|   * Local register variables in specific registers do not reserve the
|     registers, except at the point where they are used as input or
|     output operands in an `asm'' statement and the `asm''
statement
|     itself is not deleted.  The compiler''s data flow analysis is

So, I guess the issue is how to interpret this statement in the context
of the above code:  i.e., what does it mean for a register to be reserved
for a local register variable?

"The above paragraph says that Local register variables [do] reserve the
registers _[at] the point_ where they are used as input or output
operands in an `asm'' statement and the `asm'' statement itself
is not
deleted." (my emphasis)

Under that reading, r0 must be reserved for x on entry to the asm, but
not necessarily at points preceding that.  If the asm sees anything in
r0 except for x, that would be noncompliant with the above paragraph.

Nevertheless, a slightly modified version of the above which does not
allow gcc to optimise the asm away does trigger just the kind of
behaviour you describe:

int foo(int a, int b)
{
	register int x asm("r0") = 33;
	register int c asm("r1") = a / b;

	asm("svc 0" : "+r" (x) : "r" (c));

	return x;
}

 -->

00000000 <foo>:
   0:   e92d4008        push    {r3, lr}
   4:   ebfffffe        bl      0 <__aeabi_idiv>
   8:   e1a01000        mov     r1, r0
   c:   ef000000        svc     0x00000000
  10:   e8bd8008        pop     {r3, pc}


This is doubly weird: x is an I/O to the asm with a "+r" constraint,
so even if the asm("rX") assignments are not guaranteed, then x should
be _somewhere_ on entry to the asm (even if not in r0).  But it is
completely gone.

Is this allowed, or wrong?  I don''t see how this can be rationalised
with the gcc documentation that Nico quoted.

Have I missed something?

Cheers
---Dave
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 8 Mar 2012, Richard Earnshaw wrote:
> On 02/03/12 21:15, Nicolas Pitre wrote:
> > So, to me, the gcc documentation is perfectly clear on this topic.
> > there really _is_ a guarantee that those asm marked variables will be
in
> > the expected registers on entry to the inline asm, given that the
> > variable is _also_ listed as an operand to the asm statement.  But
only
> > in that case.
> >
> > It is true that gcc may reorder other function calls or other code
> > around the inline asm and then intervening code can clobber any
> > registers.  Then it is up to gcc to preserve the variable''s
content
> > elsewhere when its register is used for other purposes, and restore it
> > when some inline asm statement is referring to it.
> >
> > And if gcc does not do this then it is buggy.  Version 3.4.0 of gcc
was
> > buggy.  No other gcc versions in the last 7 years had such a problem
or
> > the __asmeq macro in the kernel would have told us.
> >
> >> Or, to summarise another way, there is no way to control which
register
> >> is used to pass something to an inline asm in general (often we
get away
> >> with this, and there are a lot of inline asms in the kernel that
assume
> >> it works, but the more you inline the more likely you are to get
nasty
> >> surprises).
> >
> > This statement is therefore unfounded and wrong.  Please direct the
> > tools guy who mislead you to the above gcc documentation.
> >
> 
> The problem is not really about re-ordering functions but about implicit
> functions that come from the source code; for example
> 
> int foo (int a, int b)
> {
>   register int x __asm__("r0") = 33;
> 
>   register int c __asm__("r1") = a / b; /* Ooops, clobbers r0
with
> division function call.  */
> 
>   asm ("svc 0" : : "r" (x));
> }
> 
> There''s nothing in the specification to say what happens if
there''s a
> statement in the code that causes an implicit clobber of your assembly
> register.
I''m sure gcc is full of implicit behaviors that are not mentioned in 
the specification.  But as long as the specification is respected, then 
there is no need to mention any unobservable side effects from a program 
flow point of view, right?

Why wouldn''t gcc be able to respect the documented feature by 
preventing live variable from being clobbered and reloading them in 
the specified register at the inline asm entry point, just like it does 
for function calls?

Here''s an example code that shows that, unfortunately, gcc is still 
broken with regards to the documented behavior:

extern int bar(int);
int foo(int y)
{
        register int x __asm__("r1") = 33;
        y += bar(x);
        asm ("@ x should be live in %0 here" : "+r" (x) :
"r" (y));
        y += bar(x);
        asm ("@ x should be live in %0 here" : "+r" (x) :
"r" (y));
        return x;
}

Result is:

foo:
        stmfd   sp!, {r4, lr}
        mov     r4, r0
        mov     r0, #33
        bl      bar
        add     r4, r0, r4
        @ x should be live in r1 here
        mov     r0, r1
        bl      bar
        add     r0, r0, r4
        @ x should be live in r1 here
        mov     r0, r1
        ldmfd   sp!, {r4, lr}
        bx      lr

To me this is clearly a bug if gcc is not able to meet the documented 
expectation.  And the documented expectation is not at all unreasonable.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 08/03/12 17:21, Nicolas Pitre wrote:
> On Thu, 8 Mar 2012, Richard Earnshaw wrote:
>
>> On 02/03/12 21:15, Nicolas Pitre wrote:
>>> So, to me, the gcc documentation is perfectly clear on this topic.
>>> there really _is_ a guarantee that those asm marked variables will
be in
>>> the expected registers on entry to the inline asm, given that the
>>> variable is _also_ listed as an operand to the asm statement.  But
only
>>> in that case.
>>>
>>> It is true that gcc may reorder other function calls or other code
>>> around the inline asm and then intervening code can clobber any
>>> registers.  Then it is up to gcc to preserve the
variable''s content
>>> elsewhere when its register is used for other purposes, and restore
it
>>> when some inline asm statement is referring to it.
>>>
>>> And if gcc does not do this then it is buggy.  Version 3.4.0 of gcc
was
>>> buggy.  No other gcc versions in the last 7 years had such a
problem or
>>> the __asmeq macro in the kernel would have told us.
>>>
>>>> Or, to summarise another way, there is no way to control which
register
>>>> is used to pass something to an inline asm in general (often we
get away
>>>> with this, and there are a lot of inline asms in the kernel
that assume
>>>> it works, but the more you inline the more likely you are to
get nasty
>>>> surprises).
>>>
>>> This statement is therefore unfounded and wrong.  Please direct the
>>> tools guy who mislead you to the above gcc documentation.
>>>
>>
>> The problem is not really about re-ordering functions but about
implicit
>> functions that come from the source code; for example
>>
>> int foo (int a, int b)
>> {
>>   register int x __asm__("r0") = 33;
>>
>>   register int c __asm__("r1") = a / b; /* Ooops, clobbers r0
with
>> division function call.  */
>>
>>   asm ("svc 0" : : "r" (x));
>> }
>>
>> There''s nothing in the specification to say what happens if
there''s a
>> statement in the code that causes an implicit clobber of your assembly
>> register.
>
> I''m sure gcc is full of implicit behaviors that are not mentioned
in
> the specification.  But as long as the specification is respected, then
> there is no need to mention any unobservable side effects from a program
> flow point of view, right?
>
> Why wouldn''t gcc be able to respect the documented feature by
> preventing live variable from being clobbered and reloading them in
> the specified register at the inline asm entry point, just like it does
> for function calls?
>
> Here''s an example code that shows that, unfortunately, gcc is
still
> broken with regards to the documented behavior:
>
> extern int bar(int);
> int foo(int y)
> {
>         register int x __asm__("r1") = 33;
>         y += bar(x);
>         asm ("@ x should be live in %0 here" : "+r" (x)
: "r" (y));
>         y += bar(x);
>         asm ("@ x should be live in %0 here" : "+r" (x)
: "r" (y));
>         return x;
> }
>
> Result is:
>
> foo:
>         stmfd   sp!, {r4, lr}
>         mov     r4, r0
>         mov     r0, #33
>         bl      bar
>         add     r4, r0, r4
>         @ x should be live in r1 here
>         mov     r0, r1
>         bl      bar
>         add     r0, r0, r4
>         @ x should be live in r1 here
>         mov     r0, r1
>         ldmfd   sp!, {r4, lr}
>         bx      lr
>
> To me this is clearly a bug if gcc is not able to meet the documented
> expectation.  And the documented expectation is not at all unreasonable.
>
No, in this case it is presumed that /you/ know that calling bar() will
modify x.  Thus the code is either well defined (if you know what is in
r1 after each call to bar), or undefined (if you can''t say anything
about r1 after each call).

As I said, the problem really comes from compiler generated calls which
are not mentioned explicitly in the source code.

R.


--
Richard Earnshaw             Email:
Richard.Earnshaw-5wv7dgnIgG8@public.gmane.org
Engineering Manager          Phone: +44 1223 400569 (Direct + VoiceMail)
OpenSource Tools             Switchboard: +44 1223 400400
ARM Ltd                      Fax: +44 1223 400410
110 Fulbourn Rd              Web: http://www.arm.com/
Cambridge, UK. CB1 9NJ

-- IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium.  Thank you.

On Thu, Mar 8, 2012 at 6:47 PM, Richard Earnshaw
<Richard.Earnshaw-5wv7dgnIgG8@public.gmane.org>
wrote:
> On 08/03/12 17:21, Nicolas Pitre wrote:
>> On Thu, 8 Mar 2012, Richard Earnshaw wrote:
>>
>>> On 02/03/12 21:15, Nicolas Pitre wrote:
>>>> So, to me, the gcc documentation is perfectly clear on this
topic.
>>>> there really _is_ a guarantee that those asm marked variables
will be in
>>>> the expected registers on entry to the inline asm, given that
the
>>>> variable is _also_ listed as an operand to the asm statement.
 But only
>>>> in that case.
>>>>
>>>> It is true that gcc may reorder other function calls or other
code
>>>> around the inline asm and then intervening code can clobber any
>>>> registers.  Then it is up to gcc to preserve the
variable''s content
>>>> elsewhere when its register is used for other purposes, and
restore it
>>>> when some inline asm statement is referring to it.
>>>>
>>>> And if gcc does not do this then it is buggy.  Version 3.4.0 of
gcc was
>>>> buggy.  No other gcc versions in the last 7 years had such a
problem or
>>>> the __asmeq macro in the kernel would have told us.
>>>>
>>>>> Or, to summarise another way, there is no way to control
which register
>>>>> is used to pass something to an inline asm in general
(often we get away
>>>>> with this, and there are a lot of inline asms in the kernel
that assume
>>>>> it works, but the more you inline the more likely you are
to get nasty
>>>>> surprises).
>>>>
>>>> This statement is therefore unfounded and wrong.  Please direct
the
>>>> tools guy who mislead you to the above gcc documentation.
>>>>
>>>
>>> The problem is not really about re-ordering functions but about
implicit
>>> functions that come from the source code; for example
>>>
>>> int foo (int a, int b)
>>> {
>>>   register int x __asm__("r0") = 33;
>>>
>>>   register int c __asm__("r1") = a / b; /* Ooops,
clobbers r0 with
>>> division function call.  */
>>>
>>>   asm ("svc 0" : : "r" (x));
>>> }
>>>
>>> There''s nothing in the specification to say what happens
if there''s a
>>> statement in the code that causes an implicit clobber of your
assembly
>>> register.
>>
>> I''m sure gcc is full of implicit behaviors that are not
mentioned in
>> the specification.  But as long as the specification is respected, then
>> there is no need to mention any unobservable side effects from a
program
>> flow point of view, right?
>>
>> Why wouldn''t gcc be able to respect the documented feature by
>> preventing live variable from being clobbered and reloading them in
>> the specified register at the inline asm entry point, just like it does
>> for function calls?
>>
>> Here''s an example code that shows that, unfortunately, gcc is
still
>> broken with regards to the documented behavior:
>>
>> extern int bar(int);
>> int foo(int y)
>> {
>>         register int x __asm__("r1") = 33;
>>         y += bar(x);
>>         asm ("@ x should be live in %0 here" : "+r"
(x) : "r" (y));
>>         y += bar(x);
>>         asm ("@ x should be live in %0 here" : "+r"
(x) : "r" (y));
>>         return x;
>> }
>>
>> Result is:
>>
>> foo:
>>         stmfd   sp!, {r4, lr}
>>         mov     r4, r0
>>         mov     r0, #33
>>         bl      bar
>>         add     r4, r0, r4
>>         @ x should be live in r1 here
>>         mov     r0, r1
>>         bl      bar
>>         add     r0, r0, r4
>>         @ x should be live in r1 here
>>         mov     r0, r1
>>         ldmfd   sp!, {r4, lr}
>>         bx      lr
>>
>> To me this is clearly a bug if gcc is not able to meet the documented
>> expectation.  And the documented expectation is not at all
unreasonable.
>>
> No, in this case it is presumed that /you/ know that calling bar() will
> modify x.  Thus the code is either well defined (if you know what is in
> r1 after each call to bar), or undefined (if you can''t say
anything
> about r1 after each call).
It could be argued that since the set of registers involved in the PCS
are well-known, then if the programmer assigns a variable to one of
those registers, then that is a conscious aliasing of the variable
with a global register which can be destroyed at any time as a
consequence of the ABI.  Because there are few guarantees about how
the compiler will or won''t transform the code, this suggessts that
asm("rX") annotations can''t work reliably for r0-r3 or r12
with the
ARM PCS.

Indeed, the GCC docs do in fact have this to say:

    "register int *p1 asm ("r0") = ...;
    register int *p2 asm ("r1") = ...;
    register int *result asm ("r0");
    asm ( [...] );

[...] beware that a register that is call-clobbered by the target ABI
will be overwritten by any function call in the assignment including
library calls for arithmetic operators.  Also a register may be
clobbered when generating some operations, like variable shift, memory
copy or memory move on x86.  Assuming it is a call-clobbered register,
this may happen to `r0'' above by the assignment to `p2''.  Ig
you have
to use such a register, use temporary variables for expressions
between the register assignment and use:

    int t1 = ...;
    register int *p1 asm("r0") = ...;
    register int *p2 asm("r1") = t1;
    register int *result asm("r0");
    asm ( [...] )"

But this is at least somewhat in conflict with "The compiler''s
data
flow analysis is capable of determining where the specified registers
contain live values, and where they are available for other uses."

It also seems to assume -O0 type behaviour where the compiler is doing
a straightforward sequential translation of the code.  Why it is
guaranteed that the assignment to p2 now certainly does not clobber p1
(even as a side effect), what the implied aliasing of result with p1
actually guarantees (or whether the compiler really understands it at
all); or what constraints there are on the compiler reordering or
inserting random extraneous code into the above, I have no idea.  Such
assumptions don''t feel very safe in the presence of optimisation.

In other words, all sorts of undocumented guarantees beyond the C
language are needed for it even to be possible to interpret what the
above code examples should mean in the first place.

The documentation leaves a lot of questions unanswered, but it does at
least suggest that other arches have the same kind of potential
pitfalls that we observed on ARM.


Register variables feel like a red herring though.  We''re only using
those because we can''t do the needful thing and actually desscribe
these constraints in the asm constraints (which would seem to be the
right place).  We specifically don''t care where those values are
except at the boundaries of the asm block itself.

Is there a reason why ARM gcc doesn''t provide the ability to specify
such exact-register constraints, or is this more for historical
reasons?  It is possible?

Cheers
---Dave

On Fri, 9 Mar 2012, Dave Martin wrote:
> Register variables feel like a red herring though.  We''re only
using
> those because we can''t do the needful thing and actually desscribe
> these constraints in the asm constraints (which would seem to be the
> right place).  We specifically don''t care where those values are
> except at the boundaries of the asm block itself.
Absolutely.
> Is there a reason why ARM gcc doesn''t provide the ability to
specify
> such exact-register constraints, or is this more for historical
> reasons?  It is possible?
I don''t know how much things have changed since I last looked at the
gcc
code, but implementing this seemed to be pretty trivial at the time.  
The problem would be to determine a good letter scheme to map to actual 
registers.


Nicolas

On 09/03/12 16:20, Nicolas Pitre wrote:
> On Fri, 9 Mar 2012, Dave Martin wrote:
>
>> Register variables feel like a red herring though.  We''re only
using
>> those because we can''t do the needful thing and actually
desscribe
>> these constraints in the asm constraints (which would seem to be the
>> right place).  We specifically don''t care where those values
are
>> except at the boundaries of the asm block itself.
>
> Absolutely.
>
>> Is there a reason why ARM gcc doesn''t provide the ability to
specify
>> such exact-register constraints, or is this more for historical
>> reasons?  It is possible?
>
> I don''t know how much things have changed since I last looked at
the gcc
> code, but implementing this seemed to be pretty trivial at the time.
> The problem would be to determine a good letter scheme to map to actual
> registers.
>
>
> Nicolas
>
While it is technically possible, it is likely to end up hurting overall
compiler performance as we''ll then have to define the machine as having
small register classes.

--
Richard Earnshaw             Email:
Richard.Earnshaw-5wv7dgnIgG8@public.gmane.org
Engineering Manager          Phone: +44 1223 400569 (Direct + VoiceMail)
OpenSource Tools             Switchboard: +44 1223 400400
ARM Ltd                      Fax: +44 1223 400410
110 Fulbourn Rd              Web: http://www.arm.com/
Cambridge, UK. CB1 9NJ

-- IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium.  Thank you.