Re: [Xen-devel] [PATCH v2 for 4.5] arm32: fix build after 063188f4b3

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 13 Oct 2014, Julien Grall wrote:
> "xen: arm: Add support for the Exynos secure firmware" introduced code
> assuming that exynos_smc() would get called with arguments in certain
> registers. While the "noinline" attribute guarantees the function to
> not get inlined, it does not guarantee that all arguments arrive in the
> assumed registers: gcc's interprocedural analysis can result in clone
> functions to be created where some of the incoming arguments (commonly
> when they have constant values) get replaced by putting in place the
> respective values inside the clone.
> 
> Xen contains in multiple place of this SMC function: consolidate the function
> in a single place and write it in assembly.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> ---
> This is a fix for Xen 4.5 to compile the hypervisor with GCC 4.9.1, used by
> Fedora & co.
> 
> Jan: I kept your Signed-off-by for the commit message.
> 
> Changes in v2:
>     - Write the SMC call in assembly
>     - Consolidate the code in a single place
> ---
>  xen/arch/arm/arm32/Makefile      |    1 +
>  xen/arch/arm/arm32/smc.S         |   19 +++++++++++++++++++
>  xen/arch/arm/arm64/Makefile      |    1 +
>  xen/arch/arm/arm64/smc.S         |   19 +++++++++++++++++++
>  xen/arch/arm/platforms/exynos5.c |   15 +--------------
>  xen/arch/arm/platforms/seattle.c |   12 ++----------
>  xen/arch/arm/psci.c              |   29 ++---------------------------
>  xen/include/asm-arm/processor.h  |    2 ++
>  8 files changed, 47 insertions(+), 51 deletions(-)
>  create mode 100644 xen/arch/arm/arm32/smc.S
>  create mode 100644 xen/arch/arm/arm64/smc.S
> 
> diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
> index df0e7de..51b64f9 100644
> --- a/xen/arch/arm/arm32/Makefile
> +++ b/xen/arch/arm/arm32/Makefile
> @@ -8,5 +8,6 @@ obj-y += domain.o
>  obj-y += vfp.o
>  obj-y += smpboot.o
>  obj-y += domctl.o
> +obj-y += smc.o
>  
>  obj-$(EARLY_PRINTK) += debug.o
> diff --git a/xen/arch/arm/arm32/smc.S b/xen/arch/arm/arm32/smc.S
> new file mode 100644
> index 0000000..ea1dba5
> --- /dev/null
> +++ b/xen/arch/arm/arm32/smc.S
> @@ -0,0 +1,19 @@
> +/*
> + * xen/arch/arm/arm32/smc.S
> + *
> + * Wrapper for Secure Monitors Calls
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +GLOBAL(do_smc)
> +        smc   #0
> +        mov   pc, lr
> diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
> index c7243f5..4debaf4 100644
> --- a/xen/arch/arm/arm64/Makefile
> +++ b/xen/arch/arm/arm64/Makefile
> @@ -1,6 +1,7 @@
>  subdir-y += lib
>  
>  obj-y += entry.o
> +obj-y += smc.o
>  
>  obj-y += traps.o
>  obj-y += domain.o
> diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S
> new file mode 100644
> index 0000000..dfe3f02
> --- /dev/null
> +++ b/xen/arch/arm/arm64/smc.S
> @@ -0,0 +1,19 @@
> +/*
> + * xen/arch/arm/arm64/smc.S
> + *
> + * Wrapper for Secure Monitors Calls
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +GLOBAL(do_smc)
> +        smc   #0
> +        ret

[...]

> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index e719c26..5b7385c 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -614,6 +614,8 @@ void vcpu_regs_hyp_to_user(const struct vcpu *vcpu,
>  void vcpu_regs_user_to_hyp(struct vcpu *vcpu,
>                             const struct vcpu_guest_core_regs *regs);
>  
> +int do_smc(register_t function_id, ...);

I am not sure whether this is safe: the smc calling convention on arm64
doesn't promise to save the x0-x17 registers. The smc calling convention
on arm32 only promises to save r4-r15. That means that after issuing an
smc call with just two arguments, you could still find r3 to be changed
afterwards.
I think you'll have to manually save/restore all the registers outside
the safety guarantees of the smc protocol.
See:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0028a/index.html

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