On Thu, Jul 26, 2012 at 04:33:43PM +0100, Stefano Stabellini wrote:

• Basic hypervisor.h and interface.h definitions.
• Skelethon enlighten.c, set xen_start_info to an empty struct.

Skeleton

• Do not limit xen_initial_domain to PV guests.

Better wording: Make xen_initial_domain dependent on the SIF_PRIVILIGED_BIT.

Which reminds me - what about PV guests that do PCI passthrough. Aren't they "more" priviligied than normal PV guests? Or not really?

The new code only compiles when CONFIG_XEN is set, that is going to be added to arch/arm/Kconfig in a later patch.

s/later patch/<name of patch>/

Signed-off-by: Stefano Stabellini stefano.stabellini@eu.citrix.com

arch/arm/Makefile | 1 + arch/arm/include/asm/hypervisor.h | 6 +++ arch/arm/include/asm/xen/hypervisor.h | 19 ++++++++++ arch/arm/include/asm/xen/interface.h | 64 +++++++++++++++++++++++++++++++++ arch/arm/xen/Makefile | 1 + arch/arm/xen/enlighten.c | 35 ++++++++++++++++++ include/xen/xen.h | 2 +- 7 files changed, 127 insertions(+), 1 deletions(-) create mode 100644 arch/arm/include/asm/hypervisor.h create mode 100644 arch/arm/include/asm/xen/hypervisor.h create mode 100644 arch/arm/include/asm/xen/interface.h create mode 100644 arch/arm/xen/Makefile create mode 100644 arch/arm/xen/enlighten.c

diff --git a/arch/arm/Makefile b/arch/arm/Makefile index 0298b00..70aaa82 100644 --- a/arch/arm/Makefile +++ b/arch/arm/Makefile @@ -246,6 +246,7 @@ endif core-$(CONFIG_FPE_NWFPE) += arch/arm/nwfpe/ core-$(CONFIG_FPE_FASTFPE) += $(FASTFPE_OBJ) core-$(CONFIG_VFP) += arch/arm/vfp/ +core-$(CONFIG_XEN) += arch/arm/xen/ # If we have a machine-specific directory, then include it in the build. core-y += arch/arm/kernel/ arch/arm/mm/ arch/arm/common/ diff --git a/arch/arm/include/asm/hypervisor.h b/arch/arm/include/asm/hypervisor.h new file mode 100644 index 0000000..b90d9e5 --- /dev/null +++ b/arch/arm/include/asm/hypervisor.h @@ -0,0 +1,6 @@ +#ifndef _ASM_ARM_HYPERVISOR_H +#define _ASM_ARM_HYPERVISOR_H

+#include <asm/xen/hypervisor.h>

+#endif diff --git a/arch/arm/include/asm/xen/hypervisor.h b/arch/arm/include/asm/xen/hypervisor.h new file mode 100644 index 0000000..d7ab99a --- /dev/null +++ b/arch/arm/include/asm/xen/hypervisor.h @@ -0,0 +1,19 @@ +#ifndef _ASM_ARM_XEN_HYPERVISOR_H +#define _ASM_ARM_XEN_HYPERVISOR_H

+extern struct shared_info *HYPERVISOR_shared_info; +extern struct start_info *xen_start_info;

+/* Lazy mode for batching updates / context switch */ +enum paravirt_lazy_mode {

• PARAVIRT_LAZY_NONE,
• PARAVIRT_LAZY_MMU,
• PARAVIRT_LAZY_CPU,

+};

+static inline enum paravirt_lazy_mode paravirt_get_lazy_mode(void) +{

• return PARAVIRT_LAZY_NONE;

+}

+#endif /* _ASM_ARM_XEN_HYPERVISOR_H */ diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h new file mode 100644 index 0000000..6c3ab59 --- /dev/null +++ b/arch/arm/include/asm/xen/interface.h @@ -0,0 +1,64 @@ +/******************************************************************************

• • Guest OS interface to ARM Xen.
• • Stefano Stabellini stefano.stabellini@eu.citrix.com, Citrix, 2011

2012

• */

+#ifndef _ASM_ARM_XEN_INTERFACE_H +#define _ASM_ARM_XEN_INTERFACE_H

+#include <linux/types.h>

+#define __DEFINE_GUEST_HANDLE(name, type) \

• typedef type * __guest_handle_ ## name

+#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

+#define set_xen_guest_handle(hnd, val) \

• do { \

• if (sizeof(hnd) == 8)			\
  

• 	*(uint64_t *)&(hnd) = 0;	\
  

• (hnd) = val;				\
  

• } while (0)

+#ifndef __ASSEMBLY__ +/* Guest handles for primitive C types. */ +__DEFINE_GUEST_HANDLE(uchar, unsigned char); +__DEFINE_GUEST_HANDLE(uint, unsigned int); +__DEFINE_GUEST_HANDLE(ulong, unsigned long); +DEFINE_GUEST_HANDLE(char); +DEFINE_GUEST_HANDLE(int); +DEFINE_GUEST_HANDLE(long); +DEFINE_GUEST_HANDLE(void); +DEFINE_GUEST_HANDLE(uint64_t); +DEFINE_GUEST_HANDLE(uint32_t);

+/* Maximum number of virtual CPUs in multi-processor guests. */ +#define MAX_VIRT_CPUS 1

+struct arch_vcpu_info { }; +struct arch_shared_info { };

+/* XXX: Move pvclock definitions some place arch independent */ +struct pvclock_vcpu_time_info {

• u32 version;
• u32 pad0;
• u64 tsc_timestamp;
• u64 system_time;
• u32 tsc_to_system_mul;
• s8 tsc_shift;
• u8 flags;
• u8 pad[2];

+} __attribute__((__packed__)); /* 32 bytes */

+struct pvclock_wall_clock {

• u32 version;
• u32 sec;
• u32 nsec;

+} __attribute__((__packed__));

That is weird. It is 4+4+4 = 12 bytes? Don't you want it to be 16 bytes?

+#endif

+#endif /* _ASM_ARM_XEN_INTERFACE_H */ diff --git a/arch/arm/xen/Makefile b/arch/arm/xen/Makefile new file mode 100644 index 0000000..0bad594 --- /dev/null +++ b/arch/arm/xen/Makefile @@ -0,0 +1 @@ +obj-y := enlighten.o diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c new file mode 100644 index 0000000..d27c2a6 --- /dev/null +++ b/arch/arm/xen/enlighten.c @@ -0,0 +1,35 @@ +#include <xen/xen.h> +#include <xen/interface/xen.h> +#include <xen/interface/memory.h> +#include <xen/platform_pci.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 = &_xen_start_info; +EXPORT_SYMBOL_GPL(xen_start_info);

+enum xen_domain_type xen_domain_type = XEN_NATIVE; +EXPORT_SYMBOL_GPL(xen_domain_type);

+struct shared_info xen_dummy_shared_info; +struct shared_info *HYPERVISOR_shared_info = (void *)&xen_dummy_shared_info;

+DEFINE_PER_CPU(struct vcpu_info *, xen_vcpu);

+/* XXX: to be removed */

In this patch series later on? Or just when you try to collapse the x86 and arm variant together?

+__read_mostly int xen_have_vector_callback; +EXPORT_SYMBOL_GPL(xen_have_vector_callback);

+int xen_platform_pci_unplug = XEN_UNPLUG_ALL; +EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);

+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); 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

---

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

On 07/26/2012 10:33 AM, Stefano Stabellini wrote:

• Basic hypervisor.h and interface.h definitions.
• Skelethon enlighten.c, set xen_start_info to an empty struct.
• Do not limit xen_initial_domain to PV guests.

The new code only compiles when CONFIG_XEN is set, that is going to be added to arch/arm/Kconfig in a later patch.

Signed-off-by: Stefano Stabellini stefano.stabellini@eu.citrix.com

arch/arm/Makefile | 1 + arch/arm/include/asm/hypervisor.h | 6 +++ arch/arm/include/asm/xen/hypervisor.h | 19 ++++++++++ arch/arm/include/asm/xen/interface.h | 64 +++++++++++++++++++++++++++++++++

These headers don't seem particularly ARM specific. Could they be moved to asm-generic or include/linux?

Rob

arch/arm/xen/Makefile | 1 + arch/arm/xen/enlighten.c | 35 ++++++++++++++++++ include/xen/xen.h | 2 +- 7 files changed, 127 insertions(+), 1 deletions(-) create mode 100644 arch/arm/include/asm/hypervisor.h create mode 100644 arch/arm/include/asm/xen/hypervisor.h create mode 100644 arch/arm/include/asm/xen/interface.h create mode 100644 arch/arm/xen/Makefile create mode 100644 arch/arm/xen/enlighten.c

diff --git a/arch/arm/Makefile b/arch/arm/Makefile index 0298b00..70aaa82 100644 --- a/arch/arm/Makefile +++ b/arch/arm/Makefile @@ -246,6 +246,7 @@ endif core-$(CONFIG_FPE_NWFPE) += arch/arm/nwfpe/ core-$(CONFIG_FPE_FASTFPE) += $(FASTFPE_OBJ) core-$(CONFIG_VFP) += arch/arm/vfp/ +core-$(CONFIG_XEN) += arch/arm/xen/ # If we have a machine-specific directory, then include it in the build. core-y += arch/arm/kernel/ arch/arm/mm/ arch/arm/common/ diff --git a/arch/arm/include/asm/hypervisor.h b/arch/arm/include/asm/hypervisor.h new file mode 100644 index 0000000..b90d9e5 --- /dev/null +++ b/arch/arm/include/asm/hypervisor.h @@ -0,0 +1,6 @@ +#ifndef _ASM_ARM_HYPERVISOR_H +#define _ASM_ARM_HYPERVISOR_H

+#include <asm/xen/hypervisor.h>

+#endif diff --git a/arch/arm/include/asm/xen/hypervisor.h b/arch/arm/include/asm/xen/hypervisor.h new file mode 100644 index 0000000..d7ab99a --- /dev/null +++ b/arch/arm/include/asm/xen/hypervisor.h @@ -0,0 +1,19 @@ +#ifndef _ASM_ARM_XEN_HYPERVISOR_H +#define _ASM_ARM_XEN_HYPERVISOR_H

+extern struct shared_info *HYPERVISOR_shared_info; +extern struct start_info *xen_start_info;

+/* Lazy mode for batching updates / context switch */ +enum paravirt_lazy_mode {

• PARAVIRT_LAZY_NONE,
• PARAVIRT_LAZY_MMU,
• PARAVIRT_LAZY_CPU,

+};

+static inline enum paravirt_lazy_mode paravirt_get_lazy_mode(void) +{

• return PARAVIRT_LAZY_NONE;

+}

+#endif /* _ASM_ARM_XEN_HYPERVISOR_H */ diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h new file mode 100644 index 0000000..6c3ab59 --- /dev/null +++ b/arch/arm/include/asm/xen/interface.h @@ -0,0 +1,64 @@ +/******************************************************************************

• • Guest OS interface to ARM Xen.
• • Stefano Stabellini stefano.stabellini@eu.citrix.com, Citrix, 2011
• */

+#ifndef _ASM_ARM_XEN_INTERFACE_H +#define _ASM_ARM_XEN_INTERFACE_H

+#include <linux/types.h>

+#define __DEFINE_GUEST_HANDLE(name, type) \

• typedef type * __guest_handle_ ## name

+#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

+#define set_xen_guest_handle(hnd, val) \

• do { \

• if (sizeof(hnd) == 8)			\
  

• 	*(uint64_t *)&(hnd) = 0;	\
  

• (hnd) = val;				\
  

• } while (0)

+#ifndef __ASSEMBLY__ +/* Guest handles for primitive C types. */ +__DEFINE_GUEST_HANDLE(uchar, unsigned char); +__DEFINE_GUEST_HANDLE(uint, unsigned int); +__DEFINE_GUEST_HANDLE(ulong, unsigned long); +DEFINE_GUEST_HANDLE(char); +DEFINE_GUEST_HANDLE(int); +DEFINE_GUEST_HANDLE(long); +DEFINE_GUEST_HANDLE(void); +DEFINE_GUEST_HANDLE(uint64_t); +DEFINE_GUEST_HANDLE(uint32_t);

+/* Maximum number of virtual CPUs in multi-processor guests. */ +#define MAX_VIRT_CPUS 1

+struct arch_vcpu_info { }; +struct arch_shared_info { };

+/* XXX: Move pvclock definitions some place arch independent */ +struct pvclock_vcpu_time_info {

• u32 version;
• u32 pad0;
• u64 tsc_timestamp;
• u64 system_time;
• u32 tsc_to_system_mul;
• s8 tsc_shift;
• u8 flags;
• u8 pad[2];

+} __attribute__((__packed__)); /* 32 bytes */

+struct pvclock_wall_clock {

• u32 version;
• u32 sec;
• u32 nsec;

+} __attribute__((__packed__)); +#endif

+#endif /* _ASM_ARM_XEN_INTERFACE_H */ diff --git a/arch/arm/xen/Makefile b/arch/arm/xen/Makefile new file mode 100644 index 0000000..0bad594 --- /dev/null +++ b/arch/arm/xen/Makefile @@ -0,0 +1 @@ +obj-y := enlighten.o diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c new file mode 100644 index 0000000..d27c2a6 --- /dev/null +++ b/arch/arm/xen/enlighten.c @@ -0,0 +1,35 @@ +#include <xen/xen.h> +#include <xen/interface/xen.h> +#include <xen/interface/memory.h> +#include <xen/platform_pci.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 = &_xen_start_info; +EXPORT_SYMBOL_GPL(xen_start_info);

+enum xen_domain_type xen_domain_type = XEN_NATIVE; +EXPORT_SYMBOL_GPL(xen_domain_type);

+struct shared_info xen_dummy_shared_info; +struct shared_info *HYPERVISOR_shared_info = (void *)&xen_dummy_shared_info;

+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);

+int xen_platform_pci_unplug = XEN_UNPLUG_ALL; +EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);

+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); 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)

On Thu, 2012-07-26 at 17:33 +0100, Konrad Rzeszutek Wilk wrote:

On Thu, Jul 26, 2012 at 04:33:44PM +0100, Stefano Stabellini wrote:

...

Use r12 to pass the hypercall number to the hypervisor.

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@eu.citrix.com

arch/arm/include/asm/xen/hypercall.h | 50 ++++++++++++++++++++++++++ arch/arm/xen/Makefile | 2 +- arch/arm/xen/hypercall.S | 65 ++++++++++++++++++++++++++++++++++ 3 files changed, 116 insertions(+), 1 deletions(-) create mode 100644 arch/arm/include/asm/xen/hypercall.h create mode 100644 arch/arm/xen/hypercall.S

diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h new file mode 100644 index 0000000..4ac0624 --- /dev/null +++ b/arch/arm/include/asm/xen/hypercall.h @@ -0,0 +1,50 @@ +/******************************************************************************

• • hypercall.h
• • Linux-specific hypervisor handling.
• • Stefano Stabellini stefano.stabellini@eu.citrix.com, Citrix, 2012
• • 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.
• */

+#ifndef _ASM_ARM_XEN_HYPERCALL_H +#define _ASM_ARM_XEN_HYPERCALL_H

+#include <xen/interface/xen.h>

+long privcmd_call(unsigned call, unsigned long a1,

• unsigned long a2, unsigned long a3,
  

• unsigned long a4, unsigned long a5);
  

+int HYPERVISOR_xen_version(int cmd, void *arg); +int HYPERVISOR_console_io(int cmd, int count, char *str); +int HYPERVISOR_grant_table_op(unsigned int cmd, void *uop, unsigned int count); +int HYPERVISOR_sched_op(int cmd, void *arg); +int HYPERVISOR_event_channel_op(int cmd, void *arg); +unsigned long HYPERVISOR_hvm_op(int op, void *arg); +int HYPERVISOR_memory_op(unsigned int cmd, void *arg); +int HYPERVISOR_physdev_op(int cmd, void *arg);

+#endif /* _ASM_ARM_XEN_HYPERCALL_H */ diff --git a/arch/arm/xen/Makefile b/arch/arm/xen/Makefile index 0bad594..b9d6acc 100644 --- a/arch/arm/xen/Makefile +++ b/arch/arm/xen/Makefile @@ -1 +1 @@ -obj-y := enlighten.o +obj-y := enlighten.o hypercall.o diff --git a/arch/arm/xen/hypercall.S b/arch/arm/xen/hypercall.S new file mode 100644 index 0000000..038cc5b --- /dev/null +++ b/arch/arm/xen/hypercall.S @@ -0,0 +1,65 @@ +/******************************************************************************

• • hypercall.S
• • Xen hypercall wrappers
• • The Xen hypercall calling convention is very similar to the ARM
• • procedure calling convention: the first paramter is passed in r0, the
• • second in r1, the third in r2 and the third in r3. Considering that

I think you meant 'and the fourth in r3'.

So where does the similarity end? Just in that we use r12?

The standard ARM function calling convention is arguments 1-4 on r0-r3 and arguments 5+ on the stack. r12 is a scratch register which can be clobbered by the *linker* on subroutine call (r12 is also called "ip" the intra-procedure call scratch register).

The hypervisor doesn't want to be accessing hypercall arguments off the guest stack, for obvious reasons, so we use r4 for the fifth argument (and if we even implemented 6 argument hypercalls we'd use r5, etc). There is no equivalent to the hypercall number in the procedure calling convention so we picked r12 because it is up and out of the way and is otherwise a scratch register. Obviously that you must not make a procedure call between setting the hypercall number in r12 and calling the hvc instruction.

...

• • Xen hypercalls have 5 arguments at most, the fifth paramter is passed
• • in r4, differently from the procedure calling convention of using the

...

• • stack for that case.
• • The hypercall number is passed in r12.
• • The return value is in r0.
• • The hvc ISS is required to be 0xEA1, that is the Xen specific ARM
• • hypercall tag.
• • Stefano Stabellini stefano.stabellini@eu.citrix.com, Citrix, 2012
• */

+#include <linux/linkage.h> +#include <asm/assembler.h> +#include <xen/interface/xen.h>

+/* HVC 0xEA1 */ +#ifdef CONFIG_THUMB2_KERNEL +#define xen_hvc .word 0xf7e08ea1 +#else +#define xen_hvc .word 0xe140ea71 +#endif

+/* We need to save and restore r4, because Xen clobbers it. */

Hmm, the comment says r4, but right below I see r12?

The ARM procedure calling convention allows a subroutine to clobber r1..r3 (r0 is the return value) but not r4 which must be preserved. But the hypervisor ABI clobbers all argument registers so the caller has to specially preserve r4 in this context whenever there is a 5 argument hypercall.

I presume that none of the hypercalls defined below have 5 arguments and therefore we don't need to preserve r4 except in the generic privcmd_call function.

To be honest I prefer the style which we use on x86 which is to define hypercall{0,1,2,3,4,5} macros and to wrap those with the specific names using inline functions.

I find the x86 way more self documenting, and being in C prevents errors around the number of arguments. It also allows for better in-lining and exposes to gcc the actual clobbers, which might allow it to avoid saving r4 on the stack at all etc.

Should this comment be by 'privcmd_call'?

When we add a 5 argument hypercall I suppose we'll see the required push/pop of r4 added to this macro too.

...

+#define HYPERCALL(hypercall) \ +ENTRY(HYPERVISOR_##hypercall) \

• mov r12, #__HYPERVISOR_##hypercall; \
• xen_hvc; \
• mov pc, lr; \

+ENDPROC(HYPERVISOR_##hypercall)

•            .text
  

+HYPERCALL(xen_version); +HYPERCALL(console_io); +HYPERCALL(grant_table_op); +HYPERCALL(sched_op); +HYPERCALL(event_channel_op); +HYPERCALL(hvm_op); +HYPERCALL(memory_op); +HYPERCALL(physdev_op);

+ENTRY(privcmd_call)

• stmdb sp!, {r4}
• mov r12, r0
• mov r0, r1
• mov r1, r2
• mov r2, r3
• ldr r3, [sp, #8]
• ldr r4, [sp, #4]
• xen_hvc
• pop {r4}

Why not ldmdb for symmetry?

...

• mov pc, lr

+ENDPROC(privcmd_call);

1.7.2.5

---

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

On Fri, 2012-07-27 at 14:02 +0100, Stefano Stabellini wrote:

On Fri, 27 Jul 2012, Ian Campbell wrote:

...

On Thu, 2012-07-26 at 17:33 +0100, Konrad Rzeszutek Wilk wrote:

...

On Thu, Jul 26, 2012 at 04:33:44PM +0100, Stefano Stabellini wrote:

...

Use r12 to pass the hypercall number to the hypervisor.

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@eu.citrix.com

arch/arm/include/asm/xen/hypercall.h | 50 ++++++++++++++++++++++++++ arch/arm/xen/Makefile | 2 +- arch/arm/xen/hypercall.S | 65 ++++++++++++++++++++++++++++++++++ 3 files changed, 116 insertions(+), 1 deletions(-) create mode 100644 arch/arm/include/asm/xen/hypercall.h create mode 100644 arch/arm/xen/hypercall.S

diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h new file mode 100644 index 0000000..4ac0624 --- /dev/null +++ b/arch/arm/include/asm/xen/hypercall.h @@ -0,0 +1,50 @@ +/******************************************************************************

• • hypercall.h
• • Linux-specific hypervisor handling.
• • Stefano Stabellini stefano.stabellini@eu.citrix.com, Citrix, 2012
• • 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.
• */

+#ifndef _ASM_ARM_XEN_HYPERCALL_H +#define _ASM_ARM_XEN_HYPERCALL_H

+#include <xen/interface/xen.h>

+long privcmd_call(unsigned call, unsigned long a1,

• unsigned long a2, unsigned long a3,
  

• unsigned long a4, unsigned long a5);
  

+int HYPERVISOR_xen_version(int cmd, void *arg); +int HYPERVISOR_console_io(int cmd, int count, char *str); +int HYPERVISOR_grant_table_op(unsigned int cmd, void *uop, unsigned int count); +int HYPERVISOR_sched_op(int cmd, void *arg); +int HYPERVISOR_event_channel_op(int cmd, void *arg); +unsigned long HYPERVISOR_hvm_op(int op, void *arg); +int HYPERVISOR_memory_op(unsigned int cmd, void *arg); +int HYPERVISOR_physdev_op(int cmd, void *arg);

+#endif /* _ASM_ARM_XEN_HYPERCALL_H */ diff --git a/arch/arm/xen/Makefile b/arch/arm/xen/Makefile index 0bad594..b9d6acc 100644 --- a/arch/arm/xen/Makefile +++ b/arch/arm/xen/Makefile @@ -1 +1 @@ -obj-y := enlighten.o +obj-y := enlighten.o hypercall.o diff --git a/arch/arm/xen/hypercall.S b/arch/arm/xen/hypercall.S new file mode 100644 index 0000000..038cc5b --- /dev/null +++ b/arch/arm/xen/hypercall.S @@ -0,0 +1,65 @@ +/******************************************************************************

• • hypercall.S
• • Xen hypercall wrappers
• • The Xen hypercall calling convention is very similar to the ARM
• • procedure calling convention: the first paramter is passed in r0, the
• • second in r1, the third in r2 and the third in r3. Considering that

I think you meant 'and the fourth in r3'.

So where does the similarity end? Just in that we use r12?

The standard ARM function calling convention is arguments 1-4 on r0-r3 and arguments 5+ on the stack. r12 is a scratch register which can be clobbered by the *linker* on subroutine call (r12 is also called "ip" the intra-procedure call scratch register).

The hypervisor doesn't want to be accessing hypercall arguments off the guest stack, for obvious reasons, so we use r4 for the fifth argument (and if we even implemented 6 argument hypercalls we'd use r5, etc). There is no equivalent to the hypercall number in the procedure calling convention so we picked r12 because it is up and out of the way and is otherwise a scratch register. Obviously that you must not make a procedure call between setting the hypercall number in r12 and calling the hvc instruction.

...

...

• • Xen hypercalls have 5 arguments at most, the fifth paramter is passed
• • in r4, differently from the procedure calling convention of using the

...

• • stack for that case.
• • The hypercall number is passed in r12.
• • The return value is in r0.
• • The hvc ISS is required to be 0xEA1, that is the Xen specific ARM
• • hypercall tag.
• • Stefano Stabellini stefano.stabellini@eu.citrix.com, Citrix, 2012
• */

+#include <linux/linkage.h> +#include <asm/assembler.h> +#include <xen/interface/xen.h>

+/* HVC 0xEA1 */ +#ifdef CONFIG_THUMB2_KERNEL +#define xen_hvc .word 0xf7e08ea1 +#else +#define xen_hvc .word 0xe140ea71 +#endif

+/* We need to save and restore r4, because Xen clobbers it. */

Hmm, the comment says r4, but right below I see r12?

The ARM procedure calling convention allows a subroutine to clobber r1..r3 (r0 is the return value) but not r4 which must be preserved. But the hypervisor ABI clobbers all argument registers so the caller has to specially preserve r4 in this context whenever there is a 5 argument hypercall.

I presume that none of the hypercalls defined below have 5 arguments and therefore we don't need to preserve r4 except in the generic privcmd_call function.

To be honest I prefer the style which we use on x86 which is to define hypercall{0,1,2,3,4,5} macros and to wrap those with the specific names using inline functions.

I find the x86 way more self documenting, and being in C prevents errors around the number of arguments. It also allows for better in-lining and exposes to gcc the actual clobbers, which might allow it to avoid saving r4 on the stack at all etc.

Considering that we cannot do the same thing that we do on x86 (see this thread http://marc.info/?l=linux-kernel&m=133052035426427&w=2),

I'd forgotten all about this arm-gcc braindamage.

I decided to go for the assembly implementation because it is much shorter and easier to understand (for me at least, being just 3 lines of code in the generic case and just one macro) and this way we can exploit the code generated by gcc to put the arguments in the right registers.

Also I like the fact that it is the same strategy used by libc to issue syscalls.

Fair enough.

As you can see it results in 3 lines of code for all the hypercalls except the ones that might take more than 4 arguments, that right now is just privcmd.

...

...

Should this comment be by 'privcmd_call'?

When we add a 5 argument hypercall I suppose we'll see the required push/pop of r4 added to this macro too.

For performance and simplicity I would add a second macro that push/pop r4, only required for hypercalls with more than 4 arguments.

For clarity / documentation purposes it might actually be worthwhile to define all of HYPERCALL{0,1,2,3,4} even if the {0,1,2,3} cases are all just: #define HYPERCALL0(x) HYPERCALL_SIMPLE(x)

...

...

...

+#define HYPERCALL(hypercall) \ +ENTRY(HYPERVISOR_##hypercall) \

• mov r12, #__HYPERVISOR_##hypercall; \
• xen_hvc; \
• mov pc, lr; \

+ENDPROC(HYPERVISOR_##hypercall)

•            .text
  

+HYPERCALL(xen_version); +HYPERCALL(console_io); +HYPERCALL(grant_table_op); +HYPERCALL(sched_op); +HYPERCALL(event_channel_op); +HYPERCALL(hvm_op); +HYPERCALL(memory_op); +HYPERCALL(physdev_op);

+ENTRY(privcmd_call)

• stmdb sp!, {r4}
• mov r12, r0
• mov r0, r1
• mov r1, r2
• mov r2, r3
• ldr r3, [sp, #8]
• ldr r4, [sp, #4]
• xen_hvc
• pop {r4}

Why not ldmdb for symmetry?

Yep, I can do that.

On Thu, Jul 26, 2012 at 04:33:45PM +0100, Stefano Stabellini wrote:

ARM Xen guests always use paging in hardware, like PV on HVM guests in the X86 world.

Nice, so no dealing with the P2M at all in the guest?

Signed-off-by: Stefano Stabellini stefano.stabellini@eu.citrix.com

arch/arm/include/asm/xen/page.h | 77 +++++++++++++++++++++++++++++++++++++++ 1 files changed, 77 insertions(+), 0 deletions(-) create mode 100644 arch/arm/include/asm/xen/page.h

diff --git a/arch/arm/include/asm/xen/page.h b/arch/arm/include/asm/xen/page.h new file mode 100644 index 0000000..6cfe9a1 --- /dev/null +++ b/arch/arm/include/asm/xen/page.h @@ -0,0 +1,77 @@ +#ifndef _ASM_ARM_XEN_PAGE_H +#define _ASM_ARM_XEN_PAGE_H

+#include <asm/page.h> +#include <linux/pfn.h> +#include <linux/types.h> +#include <asm/pgtable.h> +#include <xen/interface/grant_table.h>

I don't if it makes such a difference, but putting the headers in sorted order is sometimes nicer than just randomly.. But that might be just me liking an orderly world nowadays :-)

+#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

+/* 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 */

So why is it 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;

+}

+static inline bool set_phys_to_machine(unsigned long pfn, unsigned long mfn) +{

• BUG();
• return false;

+}

+#endif /* _ASM_ARM_XEN_PAGE_H */

1.7.2.5

---

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

On Thu, 2012-07-26 at 17:37 +0100, Konrad Rzeszutek Wilk wrote:

On Thu, Jul 26, 2012 at 04:33:46PM +0100, Stefano Stabellini wrote:

...

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

So why can't the code be changed to use that? Is it that the _set_bit, _clear_bit, etc are not available with !CONFIG_SMP?

_set_bit etc are not SMP safe if !CONFIG_SMP. But under Xen you might be communicating with a completely external entity who might be on another CPU (e.g. two uniprocessor guests communicating via event channels and grant tables). So we need a variant of the bit ops which are SMP safe even on a UP kernel.

The users are common code and the sync_foo vs foo distinction matters on some platforms (e.g. x86 where a UP kernel would omit the LOCK prefix for the normal ones).

...

Signed-off-by: Stefano Stabellini stefano.stabellini@eu.citrix.com

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..d975092903 --- /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

---

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

On Thu, Jul 26, 2012 at 04:33:47PM +0100, Stefano Stabellini wrote:

Signed-off-by: Stefano Stabellini stefano.stabellini@eu.citrix.com

arch/arm/xen/Makefile | 2 +- arch/arm/xen/grant-table.c | 53 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 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 b9d6acc..4384103 100644 --- a/arch/arm/xen/Makefile +++ b/arch/arm/xen/Makefile @@ -1 +1 @@ -obj-y := enlighten.o hypercall.o +obj-y := enlighten.o hypercall.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..0a4ee80 --- /dev/null +++ b/arch/arm/xen/grant-table.c @@ -0,0 +1,53 @@ +/******************************************************************************

• • 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,
  

• 	   void **__shared)
  

+{

• return -1;

-ENOSYS

+}

+void arch_gnttab_unmap(void *shared, unsigned long nr_gframes) +{

• return;

+}

+int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,

• 	   unsigned long max_nr_gframes,
  

• 	   grant_status_t **__shared)
  

+{

• return -1;

Same here -ENOSYS

+}

1.7.2.5

On Fri, 27 Jul 2012, Jan Beulich wrote:

...

...

...

On 26.07.12 at 17:33, Stefano Stabellini stefano.stabellini@eu.citrix.com wrote:

--- a/include/xen/interface/xen.h +++ b/include/xen/interface/xen.h @@ -10,7 +10,10 @@ #define __XEN_PUBLIC_XEN_H__ #include <asm/xen/interface.h> +#include <linux/types.h> +#ifdef CONFIG_X86 #include <asm/pvclock-abi.h> +#endif

Rather than hacking around this, why not clean it up: asm/pvclock-abi.h clearly isn't intended to be included here (from the perspective of the origin of xen/interface/xen.h, at least), nor is linux/types.h.

Or if it is really needed to deviate from the original header in this respect, then clearly the inclusion ought to not be arch specific or be moved to an arch specific header.

I think you are right: linux/types.h is not actually needed and pvclock-abi.h should probably be included by asm/xen/interface.h.

On Thu, 2012-07-26 at 16:33 +0100, Stefano Stabellini wrote:

Check for a "/xen" node in the device tree, if it is present set xen_domain_type to XEN_HVM_DOMAIN and continue initialization.

Map the real shared info page using XENMEM_add_to_physmap with XENMAPSPACE_shared_info.

Signed-off-by: Stefano Stabellini stefano.stabellini@eu.citrix.com

arch/arm/xen/enlighten.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index d27c2a6..8c923af 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -5,6 +5,9 @@ #include <asm/xen/hypervisor.h> #include <asm/xen/hypercall.h> #include <linux/module.h> +#include <linux/of.h> +#include <linux/of_irq.h> +#include <linux/of_address.h> struct start_info _xen_start_info; struct start_info *xen_start_info = &_xen_start_info; @@ -33,3 +36,56 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma, return -ENOSYS; } EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);

+/*

• • == Xen Device Tree format ==
• • • /xen node;
• • • compatible "arm,xen";
• • • one interrupt for Xen event notifications;
• • • one memory region to map the grant_table.
• */

+static int __init xen_guest_init(void) +{

• int cpu;
• struct xen_add_to_physmap xatp;
• static struct shared_info *shared_info_page = 0;
• struct device_node *node;
• node = of_find_compatible_node(NULL, NULL, "arm,xen");
• if (!node) {

• pr_info("No Xen support\n");
  

• return 0;
  

• }

This should either only print in the success case (to avoid spamming everyone) or we need a little bit of infrastructure like on x86 so that we print exactly one of: "Booting natively on bearmetal" "Booting paravirtualised on %s", hypervisor->name

• xen_domain_type = XEN_HVM_DOMAIN;
• if (!shared_info_page)

• shared_info_page = (struct shared_info *)
  

• 	get_zeroed_page(GFP_KERNEL);
  

• if (!shared_info_page) {

• pr_err("not enough memory");
  

• return -ENOMEM;
  

• }
• 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];
  

On ARM the shared info contains exactly 1 CPU (the boot CPU). The guest is required to use VCPUOP_register_vcpu_info to place vcpu info for secondary CPUs as they are brought up.

Ian.

On Fri, 2012-07-27 at 15:48 +0100, Stefano Stabellini wrote:

On Fri, 27 Jul 2012, Ian Campbell wrote:

...

On Thu, 2012-07-26 at 16:33 +0100, Stefano Stabellini wrote:

...

Check for a "/xen" node in the device tree, if it is present set xen_domain_type to XEN_HVM_DOMAIN and continue initialization.

Map the real shared info page using XENMEM_add_to_physmap with XENMAPSPACE_shared_info.

Signed-off-by: Stefano Stabellini stefano.stabellini@eu.citrix.com

arch/arm/xen/enlighten.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index d27c2a6..8c923af 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -5,6 +5,9 @@ #include <asm/xen/hypervisor.h> #include <asm/xen/hypercall.h> #include <linux/module.h> +#include <linux/of.h> +#include <linux/of_irq.h> +#include <linux/of_address.h> struct start_info _xen_start_info; struct start_info *xen_start_info = &_xen_start_info; @@ -33,3 +36,56 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma, return -ENOSYS; } EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);

+/*

• • == Xen Device Tree format ==
• • • /xen node;
• • • compatible "arm,xen";
• • • one interrupt for Xen event notifications;
• • • one memory region to map the grant_table.
• */

+static int __init xen_guest_init(void) +{

• int cpu;
• struct xen_add_to_physmap xatp;
• static struct shared_info *shared_info_page = 0;
• struct device_node *node;
• node = of_find_compatible_node(NULL, NULL, "arm,xen");
• if (!node) {

• pr_info("No Xen support\n");
  

• return 0;
  

• }

This should either only print in the success case (to avoid spamming everyone) or we need a little bit of infrastructure like on x86 so that we print exactly one of: "Booting natively on bearmetal" "Booting paravirtualised on %s", hypervisor->name

This function is only going to be called once (actually it might be called twice with the change introduced by "xen/arm: Introduce xen_guest_init").

Once (or twice), per boot, per ARM system running Linux in the world...

Ian.

On Wed, 1 Aug 2012, Konrad Rzeszutek Wilk wrote:

On Thu, Jul 26, 2012 at 04:33:49PM +0100, Stefano Stabellini wrote:

...

Check for a "/xen" node in the device tree, if it is present set xen_domain_type to XEN_HVM_DOMAIN and continue initialization.

Map the real shared info page using XENMEM_add_to_physmap with XENMAPSPACE_shared_info.

Signed-off-by: Stefano Stabellini stefano.stabellini@eu.citrix.com

arch/arm/xen/enlighten.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index d27c2a6..8c923af 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -5,6 +5,9 @@ #include <asm/xen/hypervisor.h> #include <asm/xen/hypercall.h> #include <linux/module.h> +#include <linux/of.h> +#include <linux/of_irq.h> +#include <linux/of_address.h> struct start_info _xen_start_info; struct start_info *xen_start_info = &_xen_start_info; @@ -33,3 +36,56 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma, return -ENOSYS; } EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);

+/*

• • == Xen Device Tree format ==
• • • /xen node;
• • • compatible "arm,xen";
• • • one interrupt for Xen event notifications;
• • • one memory region to map the grant_table.
• */

+static int __init xen_guest_init(void) +{

• int cpu;
• struct xen_add_to_physmap xatp;
• static struct shared_info *shared_info_page = 0;
• struct device_node *node;
• node = of_find_compatible_node(NULL, NULL, "arm,xen");
• if (!node) {

• pr_info("No Xen support\n");
  

I don't think the pr_info is appropiate here?

Yes, you are right. In fact I had already turned it into a pr_debug.

...

• return 0;
  

Should this be -ENODEV?

Considering that xen_guest_init is called by a core_initcall, I didn't want to return an error just because Xen is not present on the platform.

...

• }
• xen_domain_type = XEN_HVM_DOMAIN;
• if (!shared_info_page)

• shared_info_page = (struct shared_info *)
  

• 	get_zeroed_page(GFP_KERNEL);
  

• if (!shared_info_page) {

• pr_err("not enough memory");
  

\n

OK

...

• return -ENOMEM;
  

• }
• 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];
  

• }
• return 0;

This above looks stringly similar to the x86 one. Could it be abstracted away to share the same code? Or is that something that ought to be done later on when there is more meat on the bone?

Actually I had to remove these three lines because on ARM we are going to have just one vcpu_info struct in the shared_info page and then rely on VCPUOP_register_vcpu_info.

On Thu, Jul 26, 2012 at 04:33:50PM +0100, Stefano Stabellini wrote:

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.

How come?

Bring back xen_pfn_t and let each architecture define xen_pfn_t as they see fit.

I am OK with this as long as your include a comment in both of the interface.h saying why this is needed. I am curious why 'unsinged long' won't work? Is it b/c on ARM you always want a 64-bit type?

Signed-off-by: Stefano Stabellini stefano.stabellini@eu.citrix.com

arch/arm/include/asm/xen/interface.h | 2 ++ arch/ia64/include/asm/xen/interface.h | 2 +- arch/x86/include/asm/xen/interface.h | 2 ++ 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, 15 insertions(+), 13 deletions(-)

diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h index 6c3ab59..76b1ebe 100644 --- a/arch/arm/include/asm/xen/interface.h +++ b/arch/arm/include/asm/xen/interface.h @@ -25,6 +25,7 @@ } while (0) #ifndef __ASSEMBLY__ +typedef uint64_t xen_pfn_t; /* Guest handles for primitive C types. */ __DEFINE_GUEST_HANDLE(uchar, unsigned char); __DEFINE_GUEST_HANDLE(uint, unsigned int); @@ -35,6 +36,7 @@ DEFINE_GUEST_HANDLE(long); DEFINE_GUEST_HANDLE(void); DEFINE_GUEST_HANDLE(uint64_t); DEFINE_GUEST_HANDLE(uint32_t); +DEFINE_GUEST_HANDLE(xen_pfn_t); /* Maximum number of virtual CPUs in multi-processor guests. */ #define MAX_VIRT_CPUS 1 diff --git a/arch/ia64/include/asm/xen/interface.h b/arch/ia64/include/asm/xen/interface.h index 09d5f7f..9efa068 100644 --- a/arch/ia64/include/asm/xen/interface.h +++ b/arch/ia64/include/asm/xen/interface.h @@ -67,6 +67,7 @@ #define set_xen_guest_handle(hnd, val) do { (hnd).p = val; } while (0) #ifndef __ASSEMBLY__ +typedef unsigned long xen_pfn_t; /* Guest handles for primitive C types. */ __DEFINE_GUEST_HANDLE(uchar, unsigned char); __DEFINE_GUEST_HANDLE(uint, unsigned int); @@ -79,7 +80,6 @@ DEFINE_GUEST_HANDLE(void); DEFINE_GUEST_HANDLE(uint64_t); DEFINE_GUEST_HANDLE(uint32_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 cbf0c9d..24c1b07 100644 --- a/arch/x86/include/asm/xen/interface.h +++ b/arch/x86/include/asm/xen/interface.h @@ -47,6 +47,7 @@ #endif #ifndef __ASSEMBLY__ +typedef unsigned long xen_pfn_t; /* Guest handles for primitive C types. */ __DEFINE_GUEST_HANDLE(uchar, unsigned char); __DEFINE_GUEST_HANDLE(uint, unsigned int); @@ -57,6 +58,7 @@ DEFINE_GUEST_HANDLE(long); DEFINE_GUEST_HANDLE(void); DEFINE_GUEST_HANDLE(uint64_t); DEFINE_GUEST_HANDLE(uint32_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 a17d844..7da811b 100644 --- a/include/xen/interface/grant_table.h +++ b/include/xen/interface/grant_table.h @@ -338,7 +338,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. */

@@ -375,7 +375,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 486653f..0bea470 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 4f29f33..d59a991 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

On Wed, 1 Aug 2012, Konrad Rzeszutek Wilk wrote:

On Thu, Jul 26, 2012 at 04:33:51PM +0100, Stefano Stabellini wrote:

...

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.

Signed-off-by: Stefano Stabellini stefano.stabellini@eu.citrix.com

drivers/xen/xenbus/xenbus_comms.c | 2 +- drivers/xen/xenbus/xenbus_probe.c | 27 +++++++++++++++++---------- drivers/xen/xenbus/xenbus_xs.c | 1 + 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c index 52fe7ad..c5aa55c 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 b793723..3ae47c2 100644 --- a/drivers/xen/xenbus/xenbus_probe.c +++ b/drivers/xen/xenbus/xenbus_probe.c @@ -729,16 +729,23 @@ static int __init xenbus_init(void) xenbus_ring_ops_init(); 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);
  

• }
  

This, and along with the Hybrid PV dom0 (not yet posted, but it was doing similar manipulation here) is getting more and more like a rat-mess.

Any chance we can just abstract the three different XenStore access ways and just have something like this:

enum { USE_UNKNOWN USE_HVM, USE_PV, USE_LOCAL USE_ALREADY_INIT }; int usage = USE_UNKNOWN; if (xen_pv_domain()) usage = USE_PV; if (xen_hvm_domain()) usage = USE_HVM; if (xen_initial_domain()) usage = USE_LOCAL;

if (xen_start_info->store_evtchn) usage = USE_ALREADY_INIT; .. other overwrites..

switch (usage) { .. blah blah. }

I'll give it a try.

...

...

On 26.07.12 at 17:33, Stefano Stabellini stefano.stabellini@eu.citrix.com wrote:

--- a/drivers/xen/Makefile +++ b/drivers/xen/Makefile @@ -1,11 +1,15 @@ -obj-y += grant-table.o features.o events.o manage.o balloon.o +ifneq ($(CONFIG_ARM),y) +obj-y += manage.o balloon.o

While I assume that this part (and the cpu_hotplug one below) is temporary, ...

+obj-$(CONFIG_XEN_DOM0) += pci.o acpi.o

... at least this one should imo be solved with a proper long term mechanism, i.e. the usual var-$(CONFIG_...) approach, i.e.

dom0-$(CONFIG_PCI) := pci.o dom0-$(CONFIG_ACPI) := acpi.o obj-$(CONFIG_XEN_DOM0) += $(dom0-y)

Jan

+obj-$(CONFIG_HOTPLUG_CPU) += cpu_hotplug.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) obj-$(CONFIG_BLOCK) += biomerge.o -obj-$(CONFIG_HOTPLUG_CPU) += cpu_hotplug.o obj-$(CONFIG_XEN_XENCOMM) += xencomm.o obj-$(CONFIG_XEN_BALLOON) += xen-balloon.o obj-$(CONFIG_XEN_SELFBALLOONING) += xen-selfballoon.o @@ -17,7 +21,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 acpi.o obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback/ obj-$(CONFIG_XEN_PRIVCMD) += xen-privcmd.o obj-$(CONFIG_XEN_ACPI_PROCESSOR) += xen-acpi-processor.o -- 1.7.2.5

---

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

On Thu, Jul 26, 2012 at 04:33:54PM +0100, Stefano Stabellini wrote:

We used to rely on a core_initcall to initialize Xen on ARM, however core_initcalls are actually called after early consoles are initialized. That means that hvc_xen.c is going to be initialized before Xen.

Given the lack of a better alternative, just call a new Xen initialization function (xen_guest_init) from xen_cons_init.

xen_guest_init has to be arch independent, so write both an ARM and an x86 implementation. The x86 implementation is currently empty because we can be sure that xen_hvm_guest_init is called early enough.

Should the arm version then not be anymore on the core_initcall then?

Signed-off-by: Stefano Stabellini stefano.stabellini@eu.citrix.com

arch/arm/xen/enlighten.c | 7 ++++++- arch/x86/xen/enlighten.c | 8 ++++++++ drivers/tty/hvc/hvc_xen.c | 7 ++++++- include/xen/xen.h | 2 ++ 4 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index 8c923af..dc68074 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -44,7 +44,7 @@ EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);

• • one interrupt for Xen event notifications;
• • one memory region to map the grant_table.

*/ -static int __init xen_guest_init(void) +int __init xen_guest_init(void) { int cpu; struct xen_add_to_physmap xatp; @@ -58,6 +58,10 @@ static int __init xen_guest_init(void) } xen_domain_type = XEN_HVM_DOMAIN;

• /* already setup */
• if (shared_info_page != 0 && HYPERVISOR_shared_info == shared_info_page)

• return 0;
  

• if (!shared_info_page) shared_info_page = (struct shared_info *) get_zeroed_page(GFP_KERNEL);

@@ -88,4 +92,5 @@ static int __init xen_guest_init(void) } return 0; } +EXPORT_SYMBOL_GPL(xen_guest_init);

Why the export symbols?

core_initcall(xen_guest_init); diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c index ff962d4..6131d43 100644 --- a/arch/x86/xen/enlighten.c +++ b/arch/x86/xen/enlighten.c @@ -1567,4 +1567,12 @@ const struct hypervisor_x86 x86_hyper_xen_hvm __refconst = { .init_platform = xen_hvm_guest_init, }; EXPORT_SYMBOL(x86_hyper_xen_hvm);

+int __init xen_guest_init(void) +{

• /* do nothing: rely on x86_hyper_xen_hvm for the initialization */
• return 0;

+} +EXPORT_SYMBOL_GPL(xen_guest_init); #endif diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c index dc07f56..3c04fb8 100644 --- a/drivers/tty/hvc/hvc_xen.c +++ b/drivers/tty/hvc/hvc_xen.c @@ -577,6 +577,12 @@ static void __exit xen_hvc_fini(void) static int xen_cons_init(void) { const struct hv_ops *ops;

• int r;
• /* retrieve xen infos */
• r = xen_guest_init();
• if (r < 0)

• return r;
  

if (!xen_domain()) return 0; @@ -584,7 +590,6 @@ static int xen_cons_init(void) if (xen_initial_domain()) ops = &dom0_hvc_ops; else {

• int r;
  

  ops = &domU_hvc_ops;

if (xen_hvm_domain()) diff --git a/include/xen/xen.h b/include/xen/xen.h index 2c0d3a5..792a4d2 100644 --- a/include/xen/xen.h +++ b/include/xen/xen.h @@ -9,8 +9,10 @@ enum xen_domain_type { #ifdef CONFIG_XEN extern enum xen_domain_type xen_domain_type; +int xen_guest_init(void); #else #define xen_domain_type XEN_NATIVE +static inline int xen_guest_init(void) { return 0; } #endif

#define xen_domain() (xen_domain_type != XEN_NATIVE)

1.7.2.5

On Thu, 2012-07-26 at 16:33 +0100, Stefano Stabellini wrote:

Use Xen features to figure out if we are privileged.

XENFEAT_dom0 was introduced by 23735 in xen-unstable.hg.

Signed-off-by: Stefano Stabellini stefano.stabellini@eu.citrix.com

arch/arm/xen/enlighten.c | 7 +++++++ include/xen/interface/features.h | 3 +++ 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index dc68074..2e013cf 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -2,6 +2,7 @@ #include <xen/interface/xen.h> #include <xen/interface/memory.h> #include <xen/platform_pci.h> +#include <xen/features.h> #include <asm/xen/hypervisor.h> #include <asm/xen/hypercall.h> #include <linux/module.h> @@ -58,6 +59,12 @@ int __init xen_guest_init(void) } xen_domain_type = XEN_HVM_DOMAIN;

• xen_setup_features();
• if (xen_feature(XENFEAT_dom0))

• xen_start_info->flags |= SIF_INITDOMAIN|SIF_PRIVILEGED;
  

• else

• xen_start_info->flags &= ~(SIF_INITDOMAIN|SIF_PRIVILEGED);
  

What happens here on platforms prior to hypervisor changeset 23735?

• /* already setup */ if (shared_info_page != 0 && HYPERVISOR_shared_info == shared_info_page) return 0;

diff --git a/include/xen/interface/features.h b/include/xen/interface/features.h index b6ca39a..131a6cc 100644 --- a/include/xen/interface/features.h +++ b/include/xen/interface/features.h @@ -50,6 +50,9 @@ /* x86: pirq can be used by HVM guests */ #define XENFEAT_hvm_pirqs 10 +/* operation as Dom0 is supported */ +#define XENFEAT_dom0 11

#define XENFEAT_NR_SUBMAPS 1 #endif /* __XEN_PUBLIC_FEATURES_H__ */

On Fri, 2012-07-27 at 15:25 +0100, Stefano Stabellini wrote:

On Fri, 27 Jul 2012, Ian Campbell wrote:

...

On Thu, 2012-07-26 at 16:33 +0100, Stefano Stabellini wrote:

...

Use Xen features to figure out if we are privileged.

XENFEAT_dom0 was introduced by 23735 in xen-unstable.hg.

Signed-off-by: Stefano Stabellini stefano.stabellini@eu.citrix.com

arch/arm/xen/enlighten.c | 7 +++++++ include/xen/interface/features.h | 3 +++ 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index dc68074..2e013cf 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -2,6 +2,7 @@ #include <xen/interface/xen.h> #include <xen/interface/memory.h> #include <xen/platform_pci.h> +#include <xen/features.h> #include <asm/xen/hypervisor.h> #include <asm/xen/hypercall.h> #include <linux/module.h> @@ -58,6 +59,12 @@ int __init xen_guest_init(void) } xen_domain_type = XEN_HVM_DOMAIN;

• xen_setup_features();
• if (xen_feature(XENFEAT_dom0))

• xen_start_info->flags |= SIF_INITDOMAIN|SIF_PRIVILEGED;
  

• else

• xen_start_info->flags &= ~(SIF_INITDOMAIN|SIF_PRIVILEGED);
  

What happens here on platforms prior to hypervisor changeset 23735?

It wouldn't work. Considering that we are certainly not going to backport ARM support to Xen 4.1, and that both ARM and XENFEAT_dom0 will be present in Xen 4.2, do we really need to support the Xen unstable changesets between ARM was introduced and XENFEAT_dom0 appeared?

Sorry, I missed the "arm" in the path.

Ian.

On Wed, 1 Aug 2012, Konrad Rzeszutek Wilk wrote:

On Fri, Jul 27, 2012 at 03:33:50PM +0100, Ian Campbell wrote:

...

On Fri, 2012-07-27 at 15:25 +0100, Stefano Stabellini wrote:

...

On Fri, 27 Jul 2012, Ian Campbell wrote:

...

On Thu, 2012-07-26 at 16:33 +0100, Stefano Stabellini wrote:

...

Use Xen features to figure out if we are privileged.

XENFEAT_dom0 was introduced by 23735 in xen-unstable.hg.

Signed-off-by: Stefano Stabellini stefano.stabellini@eu.citrix.com

arch/arm/xen/enlighten.c | 7 +++++++ include/xen/interface/features.h | 3 +++ 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index dc68074..2e013cf 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -2,6 +2,7 @@ #include <xen/interface/xen.h> #include <xen/interface/memory.h> #include <xen/platform_pci.h> +#include <xen/features.h> #include <asm/xen/hypervisor.h> #include <asm/xen/hypercall.h> #include <linux/module.h> @@ -58,6 +59,12 @@ int __init xen_guest_init(void) } xen_domain_type = XEN_HVM_DOMAIN;

• xen_setup_features();
• if (xen_feature(XENFEAT_dom0))

• xen_start_info->flags |= SIF_INITDOMAIN|SIF_PRIVILEGED;
  

• else

• xen_start_info->flags &= ~(SIF_INITDOMAIN|SIF_PRIVILEGED);
  

What happens here on platforms prior to hypervisor changeset 23735?

It wouldn't work. Considering that we are certainly not going to backport ARM support to Xen 4.1, and that both ARM and XENFEAT_dom0 will be present in Xen 4.2, do we really need to support the Xen unstable changesets between ARM was introduced and XENFEAT_dom0 appeared?

So should it just panic and say "AAAAAAH"?

I could panic if I find out that XENFEAT_dom0 is unimplemented but actually I only get to know if it is available...

On Thu, Jul 26, 2012 at 04:33:56PM +0100, Stefano Stabellini wrote:

Initialize the grant table mapping at the address specified at index 0 in the DT under the /xen node.

Is it always index 0? If so, should it have a #define for the other index values?

After the grant table is initialized, call xenbus_probe (if not dom0).

Signed-off-by: Stefano Stabellini stefano.stabellini@eu.citrix.com

arch/arm/xen/enlighten.c | 13 +++++++++++++ drivers/xen/grant-table.c | 2 +- 2 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index 2e013cf..854af1e 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -1,8 +1,12 @@ #include <xen/xen.h> #include <xen/interface/xen.h> #include <xen/interface/memory.h> +#include <xen/interface/hvm/params.h> #include <xen/platform_pci.h> #include <xen/features.h> +#include <xen/grant_table.h> +#include <xen/hvm.h> +#include <xen/xenbus.h> #include <asm/xen/hypervisor.h> #include <asm/xen/hypercall.h> #include <linux/module.h> @@ -51,12 +55,16 @@ int __init xen_guest_init(void) struct xen_add_to_physmap xatp; static struct shared_info *shared_info_page = 0; struct device_node *node;

• struct resource res;

node = of_find_compatible_node(NULL, NULL, "arm,xen"); if (!node) { pr_info("No Xen support\n"); return 0; }

• if (of_address_to_resource(node, 0, &res))

• return -EINVAL;
  

• xen_hvm_resume_frames = res.start >> PAGE_SHIFT; xen_domain_type = XEN_HVM_DOMAIN;

xen_setup_features(); @@ -97,6 +105,11 @@ int __init xen_guest_init(void) per_cpu(xen_vcpu, cpu) = &HYPERVISOR_shared_info->vcpu_info[cpu]; }

• gnttab_init();
• if (!xen_initial_domain())

• xenbus_probe(NULL);
  

• return 0;

} EXPORT_SYMBOL_GPL(xen_guest_init); diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c index 1d0d95e..fd2137a 100644 --- a/drivers/xen/grant-table.c +++ b/drivers/xen/grant-table.c @@ -62,7 +62,7 @@ static grant_ref_t **gnttab_list; static unsigned int nr_grant_frames; -static unsigned int boot_max_nr_grant_frames; +static unsigned int boot_max_nr_grant_frames = 1;

Is this going to impact x86 version?

static int gnttab_free_count; static grant_ref_t gnttab_free_head; static DEFINE_SPINLOCK(gnttab_list_lock); -- 1.7.2.5

On Wed, 1 Aug 2012, Konrad Rzeszutek Wilk wrote:

On Thu, Jul 26, 2012 at 04:33:57PM +0100, Stefano Stabellini wrote:

...

Compile events.c on ARM. Parse, map and enable the IRQ to get event notifications from the device tree (node "/xen").

On ARM Linux irqs are not enabled by default:

• call enable_percpu_irq for xen_events_irq (drivers are supposed

to call enable_irq after request_irq);

• reset the IRQ_NOAUTOEN and IRQ_NOREQUEST flags that are enabled by

default on ARM. If IRQ_NOAUTOEN is set, __setup_irq doesn't call irq_startup, that is responsible for calling irq_unmask at startup time. As a result event channels remain masked.

Signed-off-by: Stefano Stabellini stefano.stabellini@eu.citrix.com

arch/arm/xen/enlighten.c | 33 +++++++++++++++++++++++++++++++++ arch/x86/xen/enlighten.c | 1 + arch/x86/xen/irq.c | 1 + arch/x86/xen/xen-ops.h | 1 - drivers/xen/events.c | 18 +++++++++++++++--- include/xen/events.h | 2 ++ 6 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index 854af1e..60d6d36 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -7,8 +7,11 @@ #include <xen/grant_table.h> #include <xen/hvm.h> #include <xen/xenbus.h> +#include <xen/events.h> #include <asm/xen/hypervisor.h> #include <asm/xen/hypercall.h> +#include <linux/interrupt.h> +#include <linux/irqreturn.h> #include <linux/module.h> #include <linux/of.h> #include <linux/of_irq.h> @@ -33,6 +36,8 @@ EXPORT_SYMBOL_GPL(xen_have_vector_callback); int xen_platform_pci_unplug = XEN_UNPLUG_ALL; EXPORT_SYMBOL_GPL(xen_platform_pci_unplug); +static __read_mostly int xen_events_irq = -1;

int xen_remap_domain_mfn_range(struct vm_area_struct *vma, unsigned long addr, unsigned long mfn, int nr, @@ -65,6 +70,9 @@ int __init xen_guest_init(void) if (of_address_to_resource(node, 0, &res)) return -EINVAL; xen_hvm_resume_frames = res.start >> PAGE_SHIFT;

• xen_events_irq = irq_of_parse_and_map(node, 0);
• pr_info("Xen support found, events_irq=%d gnttab_frame_pfn=%lx\n",

• 	xen_events_irq, xen_hvm_resume_frames);
  

  xen_domain_type = XEN_HVM_DOMAIN;

xen_setup_features(); @@ -114,3 +122,28 @@ int __init xen_guest_init(void) } EXPORT_SYMBOL_GPL(xen_guest_init); core_initcall(xen_guest_init);

+static irqreturn_t xen_arm_callback(int irq, void *arg) +{

• xen_hvm_evtchn_do_upcall();
• return 0;

Um, IRQ_HANDLED?

Yep

...

+}

+static int __init xen_init_events(void) +{

• if (!xen_domain() || xen_events_irq < 0)

• return -ENODEV;
  

• xen_init_IRQ();
• if (request_percpu_irq(xen_events_irq, xen_arm_callback,

• 	"events", xen_vcpu)) {
  

• pr_err("Error requesting IRQ %d\n", xen_events_irq);
  

• return -EINVAL;
  

• }
• enable_percpu_irq(xen_events_irq, 0);
• return 0;

+} +postcore_initcall(xen_init_events); diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c index 6131d43..5a30502 100644 --- a/arch/x86/xen/enlighten.c +++ b/arch/x86/xen/enlighten.c @@ -33,6 +33,7 @@ #include <linux/memblock.h> #include <xen/xen.h> +#include <xen/events.h> #include <xen/interface/xen.h> #include <xen/interface/version.h> #include <xen/interface/physdev.h> diff --git a/arch/x86/xen/irq.c b/arch/x86/xen/irq.c index 1573376..01a4dc0 100644 --- a/arch/x86/xen/irq.c +++ b/arch/x86/xen/irq.c @@ -5,6 +5,7 @@ #include <xen/interface/xen.h> #include <xen/interface/sched.h> #include <xen/interface/vcpu.h> +#include <xen/events.h> #include <asm/xen/hypercall.h> #include <asm/xen/hypervisor.h> diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h index 202d4c1..2368295 100644 --- a/arch/x86/xen/xen-ops.h +++ b/arch/x86/xen/xen-ops.h @@ -35,7 +35,6 @@ void xen_set_pat(u64); char * __init xen_memory_setup(void); void __init xen_arch_setup(void); -void __init xen_init_IRQ(void); void xen_enable_sysenter(void); void xen_enable_syscall(void); void xen_vcpu_restore(void); diff --git a/drivers/xen/events.c b/drivers/xen/events.c index 7da65d3..9b506b2 100644 --- a/drivers/xen/events.c +++ b/drivers/xen/events.c @@ -31,14 +31,16 @@ #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/page.h> #include <asm/xen/pci.h> +#endif +#include <asm/sync_bitops.h> #include <asm/xen/hypercall.h> #include <asm/xen/hypervisor.h> @@ -50,6 +52,9 @@ #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> +#include <asm/hw_irq.h> /*

• This lock protects updates to the following mapping and reference-count

@@ -834,6 +839,7 @@ int bind_evtchn_to_irq(unsigned int evtchn) struct irq_info *info = info_for_irq(irq); WARN_ON(info == NULL || info->type != IRQT_EVTCHN); }

• irq_clear_status_flags(irq, IRQ_NOREQUEST|IRQ_NOAUTOEN);

I feel that this should be its own commit by itself. I am not certain of the implication of this on x86 and I think it deserves some explanation.

OK. It shouldn't have any effects on x86, considering that both IRQ_NOREQUEST and IRQ_NOAUTOEN are not set there.

...

out: mutex_unlock(&irq_mapping_update_lock); @@ -1377,7 +1383,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

Doesn't exist? Or is that it does not need it?

It does not exist.

On Fri, 27 Jul 2012, Jan Beulich wrote:

...

...

...

On 26.07.12 at 17:33, Stefano Stabellini stefano.stabellini@eu.citrix.com wrote:

In order for privcmd mmap to work correctly, xen_remap_domain_mfn_range needs to be implemented for HVM guests. If it is not, mmap is going to fail later on.

Somehow, for me at least, this description doesn't connect to the actual change.

We can remove the "return -ENOSYS" from privcmd_mmap but the actual mmap is still not going to work unless xen_remap_domain_mfn_range is implemented correctly. The x86 implementation of xen_remap_domain_mfn_range is PV only so it is not going to work for HVM or auto_translated_physmap guests. As a result mmap_batch_fn is going to fail.

...

Signed-off-by: Stefano Stabellini stefano.stabellini@eu.citrix.com

drivers/xen/privcmd.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c index ccee0f1..85226cb 100644 --- a/drivers/xen/privcmd.c +++ b/drivers/xen/privcmd.c @@ -380,10 +380,6 @@ static struct vm_operations_struct privcmd_vm_ops = { static int privcmd_mmap(struct file *file, struct vm_area_struct *vma) {

• /* Unsupported for auto-translate guests. */
• if (xen_feature(XENFEAT_auto_translated_physmap))

• return -ENOSYS;
  

Is this safe on x86?

It is safe in the sense that is not going to crash dom0 or the hypervisor, but it is not going to work.

Actually in order for it to be safe we need this additional change:

diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c index 3a73785..885a223 100644 --- a/arch/x86/xen/mmu.c +++ b/arch/x86/xen/mmu.c @@ -2310,6 +2310,9 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma, unsigned long range; int err = 0;

+ if (xen_feature(XENFEAT_auto_translated_physmap)) + return -EINVAL; + prot = __pgprot(pgprot_val(prot) | _PAGE_IOMAP);

BUG_ON(!((vma->vm_flags & (VM_PFNMAP | VM_RESERVED | VM_IO)) ==

On Wed, 1 Aug 2012, Konrad Rzeszutek Wilk wrote:

On Thu, Jul 26, 2012 at 04:34:00PM +0100, Stefano Stabellini wrote:

...

Signed-off-by: Stefano Stabellini stefano.stabellini@eu.citrix.com

drivers/block/xen-blkback/blkback.c | 1 + include/xen/interface/io/protocols.h | 3 +++ 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c index 73f196c..63dd5b9 100644 --- a/drivers/block/xen-blkback/blkback.c +++ b/drivers/block/xen-blkback/blkback.c @@ -42,6 +42,7 @@ #include <xen/events.h> #include <xen/page.h> +#include <xen/xen.h> #include <asm/xen/hypervisor.h> #include <asm/xen/hypercall.h> #include "common.h" diff --git a/include/xen/interface/io/protocols.h b/include/xen/interface/io/protocols.h index 01fc8ae..0eafaf2 100644 --- a/include/xen/interface/io/protocols.h +++ b/include/xen/interface/io/protocols.h @@ -5,6 +5,7 @@ #define XEN_IO_PROTO_ABI_X86_64 "x86_64-abi" #define XEN_IO_PROTO_ABI_IA64 "ia64-abi" #define XEN_IO_PROTO_ABI_POWERPC64 "powerpc64-abi" +#define XEN_IO_PROTO_ABI_ARM "arm-abi"

So one that has all of the 32/64 issues worked out? Nice.

Yes, that is the idea, but it needs another patch to actually achieve the goal :)

...

#if defined(__i386__) # define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_X86_32 @@ -14,6 +15,8 @@ # define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_IA64 #elif defined(__powerpc64__) # define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_POWERPC64 +#elif defined(__arm__) +# define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_ARM #else # error arch fixup needed here

#endif

1.7.2.5