[PATCH v2 0/6] xen/arm: SMCCC fixup and improvement

[PATCH v2 0/6] xen/arm: SMCCC fixup and improvement

[Julien Grall]

Hi all,

This patch series contains fixup and improvement for the SMCCC subsystem.

Patch #1 - #2 are candidates for backporting.

Cheers,

Julien Grall (3):
  xen/arm: cpufeature: Add helper to check constant caps
  xen/arm: smccc: Add wrapper to automatically select the calling
    convention
  xen/arm: Replace call_smc with arm_smccc_smc

Marc Zyngier (2):
  xen/arm: smccc-1.1: Make return values unsigned long
  xen/arm: smccc-1.1: Handle function result as parameters

Volodymyr Babchuk (1):
  xen/arm: add SMC wrapper that is compatible with SMCCC v1.0

 xen/arch/arm/Makefile            |   1 -
 xen/arch/arm/arm64/Makefile      |   1 +
 xen/arch/arm/arm64/asm-offsets.c |   5 ++
 xen/arch/arm/arm64/smc.S         |  32 +++++++++++++
 xen/arch/arm/platforms/exynos5.c |   3 +-
 xen/arch/arm/platforms/seattle.c |   4 +-
 xen/arch/arm/psci.c              |  41 +++++++++++-----
 xen/arch/arm/smc.S               |  21 --------
 xen/include/asm-arm/cpufeature.h |  15 +++++-
 xen/include/asm-arm/processor.h  |   3 --
 xen/include/asm-arm/smccc.h      | 100 +++++++++++++++++++++++++++++++++------
 11 files changed, 170 insertions(+), 56 deletions(-)
 create mode 100644 xen/arch/arm/arm64/smc.S
 delete mode 100644 xen/arch/arm/smc.S

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 1/6] xen/arm: smccc-1.1: Make return values unsigned long

[Julien Grall]

From: Marc Zyngier <marc.zyngier@arm.com>

An unfortunate consequence of having a strong typing for the input
values to the SMC call is that it also affects the type of the
return values, limiting r0 to 32 bits and r{1,2,3} to whatever
was passed as an input.

Let's turn everything into "unsigned long", which satisfies the
requirements of both architectures, and allows for the full
range of return values.

Reported-by: Stefano Stabellini <stefanos@xilinx.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

---
    Changes in v2:
        - Add Volodymyr reviewed-by
---
 xen/include/asm-arm/smccc.h | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index 74c13f8419..a31d67a1de 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -119,35 +119,35 @@ struct arm_smccc_res {
 
 #define __declare_arg_0(a0, res)                        \
     struct arm_smccc_res    *___res = res;              \
-    register uin32_t        r0 asm("r0") = a0;          \
+    register unsigned long  r0 asm("r0") = (uint32_t)a0;\
     register unsigned long  r1 asm("r1");               \
     register unsigned long  r2 asm("r2");               \
     register unsigned long  r3 asm("r3")
 
 #define __declare_arg_1(a0, a1, res)                    \
     struct arm_smccc_res    *___res = res;              \
-    register uint32_t       r0 asm("r0") = a0;          \
-    register typeof(a1)     r1 asm("r1") = a1;          \
+    register unsigned long  r0 asm("r0") = (uint32_t)a0;\
+    register unsigned long  r1 asm("r1") = a1;          \
     register unsigned long  r2 asm("r2");               \
     register unsigned long  r3 asm("r3")
 
 #define __declare_arg_2(a0, a1, a2, res)                \
     struct arm_smccc_res    *___res = res;				\
-    register u32            r0 asm("r0") = a0;          \
-    register typeof(a1)     r1 asm("r1") = a1;          \
-    register typeof(a2)     r2 asm("r2") = a2;          \
+    register unsigned long  r0 asm("r0") = (uint32_t)a0;\
+    register unsigned long  r1 asm("r1") = a1;          \
+    register unsigned long  r2 asm("r2") = a2;          \
     register unsigned long  r3 asm("r3")
 
 #define __declare_arg_3(a0, a1, a2, a3, res)            \
     struct arm_smccc_res    *___res = res;              \
-    register u32            r0 asm("r0") = a0;          \
-    register typeof(a1)     r1 asm("r1") = a1;          \
-    register typeof(a2)     r2 asm("r2") = a2;          \
-    register typeof(a3)     r3 asm("r3") = a3
+    register unsigned long  r0 asm("r0") = (uint32_t)a0;\
+    register unsigned long  r1 asm("r1") = a1;          \
+    register unsigned long  r2 asm("r2") = a2;          \
+    register unsigned long  r3 asm("r3") = a3
 
 #define __declare_arg_4(a0, a1, a2, a3, a4, res)        \
     __declare_arg_3(a0, a1, a2, a3, res);               \
-    register typeof(a4) r4 asm("r4") = a4
+    register unsigned long r4 asm("r4") = a4
 
 #define __declare_arg_5(a0, a1, a2, a3, a4, a5, res)    \
     __declare_arg_4(a0, a1, a2, a3, a4, res);           \
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 2/6] xen/arm: smccc-1.1: Handle function result as parameters

[Julien Grall]

From: Marc Zyngier <marc.zyngier@arm.com>

If someone has the silly idea to write something along those lines:

	extern u64 foo(void);

	void bar(struct arm_smccc_res *res)
	{
		arm_smccc_1_1_smc(0xbad, foo(), res);
	}

they are in for a surprise, as this gets compiled as:

	0000000000000588 <bar>:
	 588:   a9be7bfd        stp     x29, x30, [sp, #-32]!
	 58c:   910003fd        mov     x29, sp
	 590:   f9000bf3        str     x19, [sp, #16]
	 594:   aa0003f3        mov     x19, x0
	 598:   aa1e03e0        mov     x0, x30
	 59c:   94000000        bl      0 <_mcount>
	 5a0:   94000000        bl      0 <foo>
	 5a4:   aa0003e1        mov     x1, x0
	 5a8:   d4000003        smc     #0x0
	 5ac:   b4000073        cbz     x19, 5b8 <bar+0x30>
	 5b0:   a9000660        stp     x0, x1, [x19]
	 5b4:   a9010e62        stp     x2, x3, [x19, #16]
	 5b8:   f9400bf3        ldr     x19, [sp, #16]
	 5bc:   a8c27bfd        ldp     x29, x30, [sp], #32
	 5c0:   d65f03c0        ret
	 5c4:   d503201f        nop

The call to foo "overwrites" the x0 register for the return value,
and we end up calling the wrong secure service.

A solution is to evaluate all the parameters before assigning
anything to specific registers, leading to the expected result:

	0000000000000588 <bar>:
	 588:   a9be7bfd        stp     x29, x30, [sp, #-32]!
	 58c:   910003fd        mov     x29, sp
	 590:   f9000bf3        str     x19, [sp, #16]
	 594:   aa0003f3        mov     x19, x0
	 598:   aa1e03e0        mov     x0, x30
	 59c:   94000000        bl      0 <_mcount>
	 5a0:   94000000        bl      0 <foo>
	 5a4:   aa0003e1        mov     x1, x0
	 5a8:   d28175a0        mov     x0, #0xbad
	 5ac:   d4000003        smc     #0x0
	 5b0:   b4000073        cbz     x19, 5bc <bar+0x34>
	 5b4:   a9000660        stp     x0, x1, [x19]
	 5b8:   a9010e62        stp     x2, x3, [x19, #16]
	 5bc:   f9400bf3        ldr     x19, [sp, #16]
	 5c0:   a8c27bfd        ldp     x29, x30, [sp], #32
	 5c4:   d65f03c0        ret

Reported-by: Stefano Stabellini <stefanos@xilinx.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

---
    Changes in v2:
        - Add Volodymyr's reviewed-by
---
 xen/include/asm-arm/smccc.h | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index a31d67a1de..648bef28bd 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -125,41 +125,51 @@ struct arm_smccc_res {
     register unsigned long  r3 asm("r3")
 
 #define __declare_arg_1(a0, a1, res)                    \
+    typeof(a1) __a1 = a1;                               \
     struct arm_smccc_res    *___res = res;              \
     register unsigned long  r0 asm("r0") = (uint32_t)a0;\
-    register unsigned long  r1 asm("r1") = a1;          \
+    register unsigned long  r1 asm("r1") = __a1;        \
     register unsigned long  r2 asm("r2");               \
     register unsigned long  r3 asm("r3")
 
 #define __declare_arg_2(a0, a1, a2, res)                \
+    typeof(a1) __a1 = a1;                               \
+    typeof(a2) __a2 = a2;                               \
     struct arm_smccc_res    *___res = res;				\
     register unsigned long  r0 asm("r0") = (uint32_t)a0;\
-    register unsigned long  r1 asm("r1") = a1;          \
-    register unsigned long  r2 asm("r2") = a2;          \
+    register unsigned long  r1 asm("r1") = __a1;        \
+    register unsigned long  r2 asm("r2") = __a2;        \
     register unsigned long  r3 asm("r3")
 
 #define __declare_arg_3(a0, a1, a2, a3, res)            \
+    typeof(a1) __a1 = a1;                               \
+    typeof(a2) __a2 = a2;                               \
+    typeof(a3) __a3 = a3;                               \
     struct arm_smccc_res    *___res = res;              \
     register unsigned long  r0 asm("r0") = (uint32_t)a0;\
-    register unsigned long  r1 asm("r1") = a1;          \
-    register unsigned long  r2 asm("r2") = a2;          \
-    register unsigned long  r3 asm("r3") = a3
+    register unsigned long  r1 asm("r1") = __a1;        \
+    register unsigned long  r2 asm("r2") = __a2;        \
+    register unsigned long  r3 asm("r3") = __a3
 
 #define __declare_arg_4(a0, a1, a2, a3, a4, res)        \
+    typeof(a4) __a4 = a4;                               \
     __declare_arg_3(a0, a1, a2, a3, res);               \
-    register unsigned long r4 asm("r4") = a4
+    register unsigned long r4 asm("r4") = __a4
 
 #define __declare_arg_5(a0, a1, a2, a3, a4, a5, res)    \
+    typeof(a5) __a5 = a5;                               \
     __declare_arg_4(a0, a1, a2, a3, a4, res);           \
-    register typeof(a5) r5 asm("r5") = a5
+    register typeof(a5) r5 asm("r5") = __a5
 
 #define __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res)    \
+    typeof(a6) __a6 = a6;                                   \
     __declare_arg_5(a0, a1, a2, a3, a4, a5, res);           \
-    register typeof(a6) r6 asm("r6") = a6
+    register typeof(a6) r6 asm("r6") = __a6
 
 #define __declare_arg_7(a0, a1, a2, a3, a4, a5, a6, a7, res)    \
+    typeof(a7) __a7 = a7;                                       \
     __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res);           \
-    register typeof(a7) r7 asm("r7") = a7
+    register typeof(a7) r7 asm("r7") = __a7
 
 #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
 #define __declare_args(count, ...)  ___declare_args(count, __VA_ARGS__)
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 3/6] xen/arm: add SMC wrapper that is compatible with SMCCC v1.0

[Julien Grall]

From: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Existing SMC wrapper call_smc() allows only 4 parameters and
returns only one value. This is enough for existing
use in PSCI code, but TEE mediator will need a call that is
fully compatible with ARM SMCCC v1.0.

This patch adds a wrapper for both arm32 and arm64. In the case of
arm32, the wrapper is just an alias to the ARM SMCCC v1.1 as the
convention is the same.

CC: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
[julien: Rework the wrapper to make it closer to SMCC 1.1 wrapper]
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm64/Makefile      |  1 +
 xen/arch/arm/arm64/asm-offsets.c |  5 ++++
 xen/arch/arm/arm64/smc.S         | 32 +++++++++++++++++++++++++
 xen/include/asm-arm/smccc.h      | 51 +++++++++++++++++++++++++++++++++++++++-
 4 files changed, 88 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/arm/arm64/smc.S

diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
index bb5c610b2a..c4f3a28a0d 100644
--- a/xen/arch/arm/arm64/Makefile
+++ b/xen/arch/arm/arm64/Makefile
@@ -8,6 +8,7 @@ obj-y += domain.o
 obj-y += entry.o
 obj-y += insn.o
 obj-$(CONFIG_LIVEPATCH) += livepatch.o
+obj-y += smc.o
 obj-y += smpboot.o
 obj-y += traps.o
 obj-y += vfp.o
diff --git a/xen/arch/arm/arm64/asm-offsets.c b/xen/arch/arm/arm64/asm-offsets.c
index 62833d8c8b..280ddb55bf 100644
--- a/xen/arch/arm/arm64/asm-offsets.c
+++ b/xen/arch/arm/arm64/asm-offsets.c
@@ -10,6 +10,7 @@
 #include <xen/bitops.h>
 #include <public/xen.h>
 #include <asm/current.h>
+#include <asm/smccc.h>
 
 #define DEFINE(_sym, _val)                                                 \
     asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \
@@ -51,6 +52,10 @@ void __dummy__(void)
 
    BLANK();
    OFFSET(INITINFO_stack, struct init_info, stack);
+
+   BLANK();
+   OFFSET(SMCCC_RES_a0, struct arm_smccc_res, a0);
+   OFFSET(SMCCC_RES_a2, struct arm_smccc_res, a2);
 }
 
 /*
diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S
new file mode 100644
index 0000000000..b0752be57e
--- /dev/null
+++ b/xen/arch/arm/arm64/smc.S
@@ -0,0 +1,32 @@
+/*
+ * 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.
+ *
+ * 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.
+ */
+
+#include <asm/asm_defns.h>
+#include <asm/macros.h>
+
+/*
+ * void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
+ *                          register_t a3, register_t a4, register_t a5,
+ *                          register_t a6, register_t a7,
+ *                          struct arm_smccc_res *res)
+ */
+ENTRY(__arm_smccc_1_0_smc)
+        smc     #0
+        ldr     x4, [sp]
+        cbz     x4, 1f          /* No need to store the result */
+        stp     x0, x1, [x4, #SMCCC_RES_a0]
+        stp     x2, x3, [x4, #SMCCC_RES_a2]
+1:
+        ret
diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index 648bef28bd..1ed6cbaa48 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -207,7 +207,56 @@ struct arm_smccc_res {
         *___res = (typeof(*___res)){r0, r1, r2, r3};            \
     } while ( 0 )
 
-#endif
+/*
+ * The calling convention for arm32 is the same for both SMCCC v1.0 and
+ * v1.1.
+ */
+#ifdef CONFIG_ARM_32
+#define arm_smccc_1_0_smc(...) arm_smccc_1_1_smc(__VA_ARGS__)
+#else
+
+void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
+                         register_t a3, register_t a4, register_t a5,
+                         register_t a6, register_t a7,
+                         struct arm_smccc_res *res);
+
+/* Macros to handle variadic parameter for SMCCC v1.0 helper */
+#define __arm_smccc_1_0_smc_7(a0, a1, a2, a3, a4, a5, a6, a7, res)  \
+    __arm_smccc_1_0_smc(a0, a1, a2, a3, a4, a5, a6, a7, res)
+
+#define __arm_smccc_1_0_smc_6(a0, a1, a2, a3, a4, a5, a6, res)  \
+    __arm_smccc_1_0_smc_7(a0, a1, a2, a3, a4, a5, a6, 0, res)
+
+#define __arm_smccc_1_0_smc_5(a0, a1, a2, a3, a4, a5, res)  \
+    __arm_smccc_1_0_smc_6(a0, a1, a2, a3, a4, a5, 0, res)
+
+#define __arm_smccc_1_0_smc_4(a0, a1, a2, a3, a4, res)  \
+    __arm_smccc_1_0_smc_5(a0, a1, a2, a3, a4, 0, res)
+
+#define __arm_smccc_1_0_smc_3(a0, a1, a2, a3, res)  \
+    __arm_smccc_1_0_smc_4(a0, a1, a2, a3, 0, res)
+
+#define __arm_smccc_1_0_smc_2(a0, a1, a2, res)  \
+    __arm_smccc_1_0_smc_3(a0, a1, a2, 0, res)
+
+#define __arm_smccc_1_0_smc_1(a0, a1, res)  \
+    __arm_smccc_1_0_smc_2(a0, a1, 0, res)
+
+#define __arm_smccc_1_0_smc_0(a0, res)  \
+    __arm_smccc_1_0_smc_1(a0, 0, res)
+
+#define ___arm_smccc_1_0_smc_count(count, ...)    \
+    __arm_smccc_1_0_smc_ ## count(__VA_ARGS__)
+
+#define __arm_smccc_1_0_smc_count(count, ...)   \
+    ___arm_smccc_1_0_smc_count(count, __VA_ARGS__)
+
+#define arm_smccc_1_0_smc(...)                                              \
+        __arm_smccc_1_0_smc_count(__count_args(__VA_ARGS__), __VA_ARGS__)
+
+#endif /* CONFIG_ARM_64 */
+
+#endif /* __ASSEMBLY__ */
 
 /*
  * Construct function identifier from call type (fast or standard),
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 4/6] xen/arm: cpufeature: Add helper to check constant caps

[Julien Grall]

Some capababilities are set right during boot and will never change
afterwards. At the moment, the function cpu_have_caps will check whether
the cap is enabled from the memory.

It is possible to avoid the load from the memory by using an
ALTERNATIVE. With that the check is just reduced to 1 instruction.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---

This is the static key for the poor. At some point we might want to
introduce something similar to static key in Xen.

    Changes in v2:
        - Use unlikely
---
 xen/include/asm-arm/cpufeature.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index 3de6b54301..c6cbc2ec84 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -63,6 +63,18 @@ static inline bool cpus_have_cap(unsigned int num)
     return test_bit(num, cpu_hwcaps);
 }
 
+/* System capability check for constant cap */
+#define cpus_have_const_cap(num) ({                 \
+        bool __ret;                                 \
+                                                    \
+        asm volatile (ALTERNATIVE("mov %0, #0",     \
+                                  "mov %0, #1",     \
+                                  num)              \
+                      : "=r" (__ret));              \
+                                                    \
+        unlikely(__ret);                            \
+        })
+
 static inline void cpus_set_cap(unsigned int num)
 {
     if (num >= ARM_NCAPS)
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    Changes in v2:
        - Invert the condition
        - Add missing includes
---
 xen/arch/arm/psci.c              |  4 ++++
 xen/include/asm-arm/cpufeature.h |  3 ++-
 xen/include/asm-arm/smccc.h      | 11 +++++++++++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 3cf5ecf0f3..941eec921b 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -21,6 +21,7 @@
 #include <xen/types.h>
 #include <xen/mm.h>
 #include <xen/smp.h>
+#include <asm/cpufeature.h>
 #include <asm/psci.h>
 #include <asm/acpi.h>
 
@@ -118,6 +119,9 @@ static void __init psci_init_smccc(void)
             smccc_ver = ret;
     }
 
+    if ( smccc_ver >= SMCCC_VERSION(1, 1) )
+        cpus_set_cap(ARM_SMCCC_1_1);
+
     printk(XENLOG_INFO "Using SMC Calling Convention v%u.%u\n",
            SMCCC_VERSION_MAJOR(smccc_ver), SMCCC_VERSION_MINOR(smccc_ver));
 }
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index c6cbc2ec84..2d82264427 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -44,8 +44,9 @@
 #define SKIP_CTXT_SWITCH_SERROR_SYNC 6
 #define ARM_HARDEN_BRANCH_PREDICTOR 7
 #define ARM_SSBD 8
+#define ARM_SMCCC_1_1 9
 
-#define ARM_NCAPS           9
+#define ARM_NCAPS           10
 
 #ifndef __ASSEMBLY__
 
diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index 1ed6cbaa48..126399dd70 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -16,6 +16,9 @@
 #ifndef __ASM_ARM_SMCCC_H__
 #define __ASM_ARM_SMCCC_H__
 
+#include <asm/alternative.h>
+#include <asm/cpufeature.h>
+
 #define SMCCC_VERSION_MAJOR_SHIFT            16
 #define SMCCC_VERSION_MINOR_MASK             \
         ((1U << SMCCC_VERSION_MAJOR_SHIFT) - 1)
@@ -213,6 +216,7 @@ struct arm_smccc_res {
  */
 #ifdef CONFIG_ARM_32
 #define arm_smccc_1_0_smc(...) arm_smccc_1_1_smc(__VA_ARGS__)
+#define arm_smccc_smc(...) arm_smccc_1_1_smc(__VA_ARGS__)
 #else
 
 void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
@@ -254,6 +258,13 @@ void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
 #define arm_smccc_1_0_smc(...)                                              \
         __arm_smccc_1_0_smc_count(__count_args(__VA_ARGS__), __VA_ARGS__)
 
+#define arm_smccc_smc(...)                                      \
+    do {                                                        \
+        if ( cpus_have_const_cap(ARM_SMCCC_1_1) )               \
+            arm_smccc_1_1_smc(__VA_ARGS__);                     \
+        else                                                    \
+            arm_smccc_1_0_smc(__VA_ARGS__);                     \
+    } while ( 0 )
 #endif /* CONFIG_ARM_64 */
 
 #endif /* __ASSEMBLY__ */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 6/6] xen/arm: Replace call_smc with arm_smccc_smc

[Julien Grall]

call_smc is a subset of arm_smccc_smc. Rather than having 2 methods to
do SMCCC call, replace all call to the former by the later.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/Makefile            |  1 -
 xen/arch/arm/platforms/exynos5.c |  3 ++-
 xen/arch/arm/platforms/seattle.c |  4 ++--
 xen/arch/arm/psci.c              | 37 +++++++++++++++++++++++++------------
 xen/arch/arm/smc.S               | 21 ---------------------
 xen/include/asm-arm/processor.h  |  3 ---
 6 files changed, 29 insertions(+), 40 deletions(-)
 delete mode 100644 xen/arch/arm/smc.S

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index b9b141dc84..37fa8268b3 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -39,7 +39,6 @@ obj-y += processor.o
 obj-y += psci.o
 obj-y += setup.o
 obj-y += shutdown.o
-obj-y += smc.o
 obj-y += smp.o
 obj-y += smpboot.o
 obj-y += sysctl.o
diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
index c15ecf80f5..e2c0b7b878 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -26,6 +26,7 @@
 #include <asm/platforms/exynos5.h>
 #include <asm/platform.h>
 #include <asm/io.h>
+#include <asm/smccc.h>
 
 static bool secure_firmware;
 
@@ -249,7 +250,7 @@ static int exynos5_cpu_up(int cpu)
     iounmap(power);
 
     if ( secure_firmware )
-        call_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0);
+        arm_smccc_smc(SMC_CMD_CPU1BOOT, cpu, NULL);
 
     return cpu_up_send_sgi(cpu);
 }
diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c
index 893cc17972..64cc1868c2 100644
--- a/xen/arch/arm/platforms/seattle.c
+++ b/xen/arch/arm/platforms/seattle.c
@@ -33,12 +33,12 @@ static const char * const seattle_dt_compat[] __initconst =
  */
 static void seattle_system_reset(void)
 {
-    call_smc(PSCI_0_2_FN32_SYSTEM_RESET, 0, 0, 0);
+    arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_RESET, NULL);
 }
 
 static void seattle_system_off(void)
 {
-    call_smc(PSCI_0_2_FN32_SYSTEM_OFF, 0, 0, 0);
+    arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_OFF, NULL);
 }
 
 PLATFORM_START(seattle, "SEATTLE")
diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 941eec921b..02737e6caa 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -42,42 +42,53 @@ uint32_t smccc_ver;
 
 static uint32_t psci_cpu_on_nr;
 
+#define PSCI_RET(res)   ((int32_t)(res).a0)
+
 int call_psci_cpu_on(int cpu)
 {
-    return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary), 0);
+    struct arm_smccc_res res;
+
+    arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary),
+                  &res);
+
+    return (int32_t)res.a0;
 }
 
 void call_psci_cpu_off(void)
 {
     if ( psci_ver > PSCI_VERSION(0, 1) )
     {
-        int errno;
+        struct arm_smccc_res res;
 
         /* If successfull the PSCI cpu_off call doesn't return */
-        errno = call_smc(PSCI_0_2_FN32_CPU_OFF, 0, 0, 0);
+        arm_smccc_smc(PSCI_0_2_FN32_CPU_OFF, &res);
         panic("PSCI cpu off failed for CPU%d err=%d\n", smp_processor_id(),
-              errno);
+              PSCI_RET(res));
     }
 }
 
 void call_psci_system_off(void)
 {
     if ( psci_ver > PSCI_VERSION(0, 1) )
-        call_smc(PSCI_0_2_FN32_SYSTEM_OFF, 0, 0, 0);
+        arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_OFF, NULL);
 }
 
 void call_psci_system_reset(void)
 {
     if ( psci_ver > PSCI_VERSION(0, 1) )
-        call_smc(PSCI_0_2_FN32_SYSTEM_RESET, 0, 0, 0);
+        arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_RESET, NULL);
 }
 
 static int __init psci_features(uint32_t psci_func_id)
 {
+    struct arm_smccc_res res;
+
     if ( psci_ver < PSCI_VERSION(1, 0) )
         return PSCI_NOT_SUPPORTED;
 
-    return call_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, 0, 0);
+    arm_smccc_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, NULL);
+
+    return PSCI_RET(res);
 }
 
 static int __init psci_is_smc_method(const struct dt_device_node *psci)
@@ -112,11 +123,11 @@ static void __init psci_init_smccc(void)
 
     if ( psci_features(ARM_SMCCC_VERSION_FID) != PSCI_NOT_SUPPORTED )
     {
-        uint32_t ret;
+        struct arm_smccc_res res;
 
-        ret = call_smc(ARM_SMCCC_VERSION_FID, 0, 0, 0);
-        if ( ret != ARM_SMCCC_NOT_SUPPORTED )
-            smccc_ver = ret;
+        arm_smccc_smc(ARM_SMCCC_VERSION_FID, &res);
+        if ( PSCI_RET(res) != ARM_SMCCC_NOT_SUPPORTED )
+            smccc_ver = PSCI_RET(res);
     }
 
     if ( smccc_ver >= SMCCC_VERSION(1, 1) )
@@ -165,6 +176,7 @@ static int __init psci_init_0_2(void)
         { /* sentinel */ },
     };
     int ret;
+    struct arm_smccc_res res;
 
     if ( acpi_disabled )
     {
@@ -186,7 +198,8 @@ static int __init psci_init_0_2(void)
         }
     }
 
-    psci_ver = call_smc(PSCI_0_2_FN32_PSCI_VERSION, 0, 0, 0);
+    arm_smccc_smc(PSCI_0_2_FN32_PSCI_VERSION, &res);
+    psci_ver = PSCI_RET(res);
 
     /* For the moment, we only support PSCI 0.2 and PSCI 1.x */
     if ( psci_ver != PSCI_VERSION(0, 2) && PSCI_VERSION_MAJOR(psci_ver) != 1 )
diff --git a/xen/arch/arm/smc.S b/xen/arch/arm/smc.S
deleted file mode 100644
index b8f182272a..0000000000
--- a/xen/arch/arm/smc.S
+++ /dev/null
@@ -1,21 +0,0 @@
-/*
- * xen/arch/arm/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.
- */
-
-#include <asm/macros.h>
-
-ENTRY(call_smc)
-        smc   #0
-        ret
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 222a02dd99..8016cf306f 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -812,9 +812,6 @@ 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 call_smc(register_t function_id, register_t arg0, register_t arg1,
-             register_t arg2);
-
 void do_trap_hyp_serror(struct cpu_user_regs *regs);
 
 void do_trap_guest_serror(struct cpu_user_regs *regs);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/6] xen/arm: smccc-1.1: Make return values unsigned long

[Stefano Stabellini]

On Tue, 25 Sep 2018, Julien Grall wrote:
> From: Marc Zyngier <marc.zyngier@arm.com>
> 
> An unfortunate consequence of having a strong typing for the input
> values to the SMC call is that it also affects the type of the
> return values, limiting r0 to 32 bits and r{1,2,3} to whatever
> was passed as an input.
> 
> Let's turn everything into "unsigned long", which satisfies the
> requirements of both architectures, and allows for the full
> range of return values.
> 
> Reported-by: Stefano Stabellini <stefanos@xilinx.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Add Volodymyr reviewed-by
> ---
>  xen/include/asm-arm/smccc.h | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> index 74c13f8419..a31d67a1de 100644
> --- a/xen/include/asm-arm/smccc.h
> +++ b/xen/include/asm-arm/smccc.h
> @@ -119,35 +119,35 @@ struct arm_smccc_res {
>  
>  #define __declare_arg_0(a0, res)                        \
>      struct arm_smccc_res    *___res = res;              \
> -    register uin32_t        r0 asm("r0") = a0;          \
> +    register unsigned long  r0 asm("r0") = (uint32_t)a0;\
>      register unsigned long  r1 asm("r1");               \
>      register unsigned long  r2 asm("r2");               \
>      register unsigned long  r3 asm("r3")
>  
>  #define __declare_arg_1(a0, a1, res)                    \
>      struct arm_smccc_res    *___res = res;              \
> -    register uint32_t       r0 asm("r0") = a0;          \
> -    register typeof(a1)     r1 asm("r1") = a1;          \
> +    register unsigned long  r0 asm("r0") = (uint32_t)a0;\
> +    register unsigned long  r1 asm("r1") = a1;          \
>      register unsigned long  r2 asm("r2");               \
>      register unsigned long  r3 asm("r3")
>  
>  #define __declare_arg_2(a0, a1, a2, res)                \
>      struct arm_smccc_res    *___res = res;				\
> -    register u32            r0 asm("r0") = a0;          \
> -    register typeof(a1)     r1 asm("r1") = a1;          \
> -    register typeof(a2)     r2 asm("r2") = a2;          \
> +    register unsigned long  r0 asm("r0") = (uint32_t)a0;\
> +    register unsigned long  r1 asm("r1") = a1;          \
> +    register unsigned long  r2 asm("r2") = a2;          \
>      register unsigned long  r3 asm("r3")
>  
>  #define __declare_arg_3(a0, a1, a2, a3, res)            \
>      struct arm_smccc_res    *___res = res;              \
> -    register u32            r0 asm("r0") = a0;          \
> -    register typeof(a1)     r1 asm("r1") = a1;          \
> -    register typeof(a2)     r2 asm("r2") = a2;          \
> -    register typeof(a3)     r3 asm("r3") = a3
> +    register unsigned long  r0 asm("r0") = (uint32_t)a0;\
> +    register unsigned long  r1 asm("r1") = a1;          \
> +    register unsigned long  r2 asm("r2") = a2;          \
> +    register unsigned long  r3 asm("r3") = a3
>  
>  #define __declare_arg_4(a0, a1, a2, a3, a4, res)        \
>      __declare_arg_3(a0, a1, a2, a3, res);               \
> -    register typeof(a4) r4 asm("r4") = a4
> +    register unsigned long r4 asm("r4") = a4
>  
>  #define __declare_arg_5(a0, a1, a2, a3, a4, a5, res)    \
>      __declare_arg_4(a0, a1, a2, a3, a4, res);           \
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/6] xen/arm: smccc-1.1: Handle function result as parameters

[Stefano Stabellini]

On Tue, 25 Sep 2018, Julien Grall wrote:
> From: Marc Zyngier <marc.zyngier@arm.com>
> 
> If someone has the silly idea to write something along those lines:
> 
> 	extern u64 foo(void);
> 
> 	void bar(struct arm_smccc_res *res)
> 	{
> 		arm_smccc_1_1_smc(0xbad, foo(), res);
> 	}
> 
> they are in for a surprise, as this gets compiled as:
> 
> 	0000000000000588 <bar>:
> 	 588:   a9be7bfd        stp     x29, x30, [sp, #-32]!
> 	 58c:   910003fd        mov     x29, sp
> 	 590:   f9000bf3        str     x19, [sp, #16]
> 	 594:   aa0003f3        mov     x19, x0
> 	 598:   aa1e03e0        mov     x0, x30
> 	 59c:   94000000        bl      0 <_mcount>
> 	 5a0:   94000000        bl      0 <foo>
> 	 5a4:   aa0003e1        mov     x1, x0
> 	 5a8:   d4000003        smc     #0x0
> 	 5ac:   b4000073        cbz     x19, 5b8 <bar+0x30>
> 	 5b0:   a9000660        stp     x0, x1, [x19]
> 	 5b4:   a9010e62        stp     x2, x3, [x19, #16]
> 	 5b8:   f9400bf3        ldr     x19, [sp, #16]
> 	 5bc:   a8c27bfd        ldp     x29, x30, [sp], #32
> 	 5c0:   d65f03c0        ret
> 	 5c4:   d503201f        nop
> 
> The call to foo "overwrites" the x0 register for the return value,
> and we end up calling the wrong secure service.
> 
> A solution is to evaluate all the parameters before assigning
> anything to specific registers, leading to the expected result:
> 
> 	0000000000000588 <bar>:
> 	 588:   a9be7bfd        stp     x29, x30, [sp, #-32]!
> 	 58c:   910003fd        mov     x29, sp
> 	 590:   f9000bf3        str     x19, [sp, #16]
> 	 594:   aa0003f3        mov     x19, x0
> 	 598:   aa1e03e0        mov     x0, x30
> 	 59c:   94000000        bl      0 <_mcount>
> 	 5a0:   94000000        bl      0 <foo>
> 	 5a4:   aa0003e1        mov     x1, x0
> 	 5a8:   d28175a0        mov     x0, #0xbad
> 	 5ac:   d4000003        smc     #0x0
> 	 5b0:   b4000073        cbz     x19, 5bc <bar+0x34>
> 	 5b4:   a9000660        stp     x0, x1, [x19]
> 	 5b8:   a9010e62        stp     x2, x3, [x19, #16]
> 	 5bc:   f9400bf3        ldr     x19, [sp, #16]
> 	 5c0:   a8c27bfd        ldp     x29, x30, [sp], #32
> 	 5c4:   d65f03c0        ret
> 
> Reported-by: Stefano Stabellini <stefanos@xilinx.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Add Volodymyr's reviewed-by
> ---
>  xen/include/asm-arm/smccc.h | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> index a31d67a1de..648bef28bd 100644
> --- a/xen/include/asm-arm/smccc.h
> +++ b/xen/include/asm-arm/smccc.h
> @@ -125,41 +125,51 @@ struct arm_smccc_res {
>      register unsigned long  r3 asm("r3")
>  
>  #define __declare_arg_1(a0, a1, res)                    \
> +    typeof(a1) __a1 = a1;                               \
>      struct arm_smccc_res    *___res = res;              \
>      register unsigned long  r0 asm("r0") = (uint32_t)a0;\
> -    register unsigned long  r1 asm("r1") = a1;          \
> +    register unsigned long  r1 asm("r1") = __a1;        \
>      register unsigned long  r2 asm("r2");               \
>      register unsigned long  r3 asm("r3")
>  
>  #define __declare_arg_2(a0, a1, a2, res)                \
> +    typeof(a1) __a1 = a1;                               \
> +    typeof(a2) __a2 = a2;                               \
>      struct arm_smccc_res    *___res = res;				\
>      register unsigned long  r0 asm("r0") = (uint32_t)a0;\
> -    register unsigned long  r1 asm("r1") = a1;          \
> -    register unsigned long  r2 asm("r2") = a2;          \
> +    register unsigned long  r1 asm("r1") = __a1;        \
> +    register unsigned long  r2 asm("r2") = __a2;        \
>      register unsigned long  r3 asm("r3")
>  
>  #define __declare_arg_3(a0, a1, a2, a3, res)            \
> +    typeof(a1) __a1 = a1;                               \
> +    typeof(a2) __a2 = a2;                               \
> +    typeof(a3) __a3 = a3;                               \
>      struct arm_smccc_res    *___res = res;              \
>      register unsigned long  r0 asm("r0") = (uint32_t)a0;\
> -    register unsigned long  r1 asm("r1") = a1;          \
> -    register unsigned long  r2 asm("r2") = a2;          \
> -    register unsigned long  r3 asm("r3") = a3
> +    register unsigned long  r1 asm("r1") = __a1;        \
> +    register unsigned long  r2 asm("r2") = __a2;        \
> +    register unsigned long  r3 asm("r3") = __a3
>  
>  #define __declare_arg_4(a0, a1, a2, a3, a4, res)        \
> +    typeof(a4) __a4 = a4;                               \
>      __declare_arg_3(a0, a1, a2, a3, res);               \
> -    register unsigned long r4 asm("r4") = a4
> +    register unsigned long r4 asm("r4") = __a4
>  
>  #define __declare_arg_5(a0, a1, a2, a3, a4, a5, res)    \
> +    typeof(a5) __a5 = a5;                               \
>      __declare_arg_4(a0, a1, a2, a3, a4, res);           \
> -    register typeof(a5) r5 asm("r5") = a5
> +    register typeof(a5) r5 asm("r5") = __a5
>  
>  #define __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res)    \
> +    typeof(a6) __a6 = a6;                                   \
>      __declare_arg_5(a0, a1, a2, a3, a4, a5, res);           \
> -    register typeof(a6) r6 asm("r6") = a6
> +    register typeof(a6) r6 asm("r6") = __a6
>  
>  #define __declare_arg_7(a0, a1, a2, a3, a4, a5, a6, a7, res)    \
> +    typeof(a7) __a7 = a7;                                       \
>      __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res);           \
> -    register typeof(a7) r7 asm("r7") = a7
> +    register typeof(a7) r7 asm("r7") = __a7
>  
>  #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
>  #define __declare_args(count, ...)  ___declare_args(count, __VA_ARGS__)
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 3/6] xen/arm: add SMC wrapper that is compatible with SMCCC v1.0

[Stefano Stabellini]

On Tue, 25 Sep 2018, Julien Grall wrote:
> From: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> 
> Existing SMC wrapper call_smc() allows only 4 parameters and
> returns only one value. This is enough for existing
> use in PSCI code, but TEE mediator will need a call that is
> fully compatible with ARM SMCCC v1.0.
> 
> This patch adds a wrapper for both arm32 and arm64. In the case of
> arm32, the wrapper is just an alias to the ARM SMCCC v1.1 as the
> convention is the same.
> 
> CC: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> [julien: Rework the wrapper to make it closer to SMCC 1.1 wrapper]
> Signed-off-by: Julien Grall <julien.grall@arm.com>

I have been struggling to find the old doc for SMCCC v1.0, all the
references have been updated to v1.1 online now. Do you have a link?


> ---
>  xen/arch/arm/arm64/Makefile      |  1 +
>  xen/arch/arm/arm64/asm-offsets.c |  5 ++++
>  xen/arch/arm/arm64/smc.S         | 32 +++++++++++++++++++++++++
>  xen/include/asm-arm/smccc.h      | 51 +++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 88 insertions(+), 1 deletion(-)
>  create mode 100644 xen/arch/arm/arm64/smc.S
> 
> diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
> index bb5c610b2a..c4f3a28a0d 100644
> --- a/xen/arch/arm/arm64/Makefile
> +++ b/xen/arch/arm/arm64/Makefile
> @@ -8,6 +8,7 @@ obj-y += domain.o
>  obj-y += entry.o
>  obj-y += insn.o
>  obj-$(CONFIG_LIVEPATCH) += livepatch.o
> +obj-y += smc.o
>  obj-y += smpboot.o
>  obj-y += traps.o
>  obj-y += vfp.o
> diff --git a/xen/arch/arm/arm64/asm-offsets.c b/xen/arch/arm/arm64/asm-offsets.c
> index 62833d8c8b..280ddb55bf 100644
> --- a/xen/arch/arm/arm64/asm-offsets.c
> +++ b/xen/arch/arm/arm64/asm-offsets.c
> @@ -10,6 +10,7 @@
>  #include <xen/bitops.h>
>  #include <public/xen.h>
>  #include <asm/current.h>
> +#include <asm/smccc.h>
>  
>  #define DEFINE(_sym, _val)                                                 \
>      asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \
> @@ -51,6 +52,10 @@ void __dummy__(void)
>  
>     BLANK();
>     OFFSET(INITINFO_stack, struct init_info, stack);
> +
> +   BLANK();
> +   OFFSET(SMCCC_RES_a0, struct arm_smccc_res, a0);
> +   OFFSET(SMCCC_RES_a2, struct arm_smccc_res, a2);
>  }
>  
>  /*
> diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S
> new file mode 100644
> index 0000000000..b0752be57e
> --- /dev/null
> +++ b/xen/arch/arm/arm64/smc.S
> @@ -0,0 +1,32 @@
> +/*
> + * 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.
> + *
> + * 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.
> + */
> +
> +#include <asm/asm_defns.h>
> +#include <asm/macros.h>
> +
> +/*
> + * void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
> + *                          register_t a3, register_t a4, register_t a5,
> + *                          register_t a6, register_t a7,
> + *                          struct arm_smccc_res *res)
> + */
> +ENTRY(__arm_smccc_1_0_smc)
> +        smc     #0
> +        ldr     x4, [sp]
> +        cbz     x4, 1f          /* No need to store the result */
> +        stp     x0, x1, [x4, #SMCCC_RES_a0]
> +        stp     x2, x3, [x4, #SMCCC_RES_a2]
> +1:
> +        ret

As I mentioned, I couldn't find the doc, but it looks like the Linux
implementation always copies back the results
(arch/arm64/kernel/smccc-call.S)? Shouldn't we zero x0-x3 at least?

Other than that, it looks fine.


> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> index 648bef28bd..1ed6cbaa48 100644
> --- a/xen/include/asm-arm/smccc.h
> +++ b/xen/include/asm-arm/smccc.h
> @@ -207,7 +207,56 @@ struct arm_smccc_res {
>          *___res = (typeof(*___res)){r0, r1, r2, r3};            \
>      } while ( 0 )
>  
> -#endif
> +/*
> + * The calling convention for arm32 is the same for both SMCCC v1.0 and
> + * v1.1.
> + */
> +#ifdef CONFIG_ARM_32
> +#define arm_smccc_1_0_smc(...) arm_smccc_1_1_smc(__VA_ARGS__)
> +#else
> +
> +void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
> +                         register_t a3, register_t a4, register_t a5,
> +                         register_t a6, register_t a7,
> +                         struct arm_smccc_res *res);
> +
> +/* Macros to handle variadic parameter for SMCCC v1.0 helper */
> +#define __arm_smccc_1_0_smc_7(a0, a1, a2, a3, a4, a5, a6, a7, res)  \
> +    __arm_smccc_1_0_smc(a0, a1, a2, a3, a4, a5, a6, a7, res)
> +
> +#define __arm_smccc_1_0_smc_6(a0, a1, a2, a3, a4, a5, a6, res)  \
> +    __arm_smccc_1_0_smc_7(a0, a1, a2, a3, a4, a5, a6, 0, res)
> +
> +#define __arm_smccc_1_0_smc_5(a0, a1, a2, a3, a4, a5, res)  \
> +    __arm_smccc_1_0_smc_6(a0, a1, a2, a3, a4, a5, 0, res)
> +
> +#define __arm_smccc_1_0_smc_4(a0, a1, a2, a3, a4, res)  \
> +    __arm_smccc_1_0_smc_5(a0, a1, a2, a3, a4, 0, res)
> +
> +#define __arm_smccc_1_0_smc_3(a0, a1, a2, a3, res)  \
> +    __arm_smccc_1_0_smc_4(a0, a1, a2, a3, 0, res)
> +
> +#define __arm_smccc_1_0_smc_2(a0, a1, a2, res)  \
> +    __arm_smccc_1_0_smc_3(a0, a1, a2, 0, res)
> +
> +#define __arm_smccc_1_0_smc_1(a0, a1, res)  \
> +    __arm_smccc_1_0_smc_2(a0, a1, 0, res)
> +
> +#define __arm_smccc_1_0_smc_0(a0, res)  \
> +    __arm_smccc_1_0_smc_1(a0, 0, res)
> +
> +#define ___arm_smccc_1_0_smc_count(count, ...)    \
> +    __arm_smccc_1_0_smc_ ## count(__VA_ARGS__)
> +
> +#define __arm_smccc_1_0_smc_count(count, ...)   \
> +    ___arm_smccc_1_0_smc_count(count, __VA_ARGS__)
> +
> +#define arm_smccc_1_0_smc(...)                                              \
> +        __arm_smccc_1_0_smc_count(__count_args(__VA_ARGS__), __VA_ARGS__)
> +
> +#endif /* CONFIG_ARM_64 */
> +
> +#endif /* __ASSEMBLY__ */
>  
>  /*
>   * Construct function identifier from call type (fast or standard),
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention

[Stefano Stabellini]

On Tue, 25 Sep 2018, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

> ---
>     Changes in v2:
>         - Invert the condition
>         - Add missing includes
> ---
>  xen/arch/arm/psci.c              |  4 ++++
>  xen/include/asm-arm/cpufeature.h |  3 ++-
>  xen/include/asm-arm/smccc.h      | 11 +++++++++++
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index 3cf5ecf0f3..941eec921b 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -21,6 +21,7 @@
>  #include <xen/types.h>
>  #include <xen/mm.h>
>  #include <xen/smp.h>
> +#include <asm/cpufeature.h>
>  #include <asm/psci.h>
>  #include <asm/acpi.h>
>  
> @@ -118,6 +119,9 @@ static void __init psci_init_smccc(void)
>              smccc_ver = ret;
>      }
>  
> +    if ( smccc_ver >= SMCCC_VERSION(1, 1) )
> +        cpus_set_cap(ARM_SMCCC_1_1);
> +
>      printk(XENLOG_INFO "Using SMC Calling Convention v%u.%u\n",
>             SMCCC_VERSION_MAJOR(smccc_ver), SMCCC_VERSION_MINOR(smccc_ver));
>  }
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index c6cbc2ec84..2d82264427 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -44,8 +44,9 @@
>  #define SKIP_CTXT_SWITCH_SERROR_SYNC 6
>  #define ARM_HARDEN_BRANCH_PREDICTOR 7
>  #define ARM_SSBD 8
> +#define ARM_SMCCC_1_1 9
>  
> -#define ARM_NCAPS           9
> +#define ARM_NCAPS           10
>  
>  #ifndef __ASSEMBLY__
>  
> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> index 1ed6cbaa48..126399dd70 100644
> --- a/xen/include/asm-arm/smccc.h
> +++ b/xen/include/asm-arm/smccc.h
> @@ -16,6 +16,9 @@
>  #ifndef __ASM_ARM_SMCCC_H__
>  #define __ASM_ARM_SMCCC_H__
>  
> +#include <asm/alternative.h>
> +#include <asm/cpufeature.h>
> +
>  #define SMCCC_VERSION_MAJOR_SHIFT            16
>  #define SMCCC_VERSION_MINOR_MASK             \
>          ((1U << SMCCC_VERSION_MAJOR_SHIFT) - 1)
> @@ -213,6 +216,7 @@ struct arm_smccc_res {
>   */
>  #ifdef CONFIG_ARM_32
>  #define arm_smccc_1_0_smc(...) arm_smccc_1_1_smc(__VA_ARGS__)
> +#define arm_smccc_smc(...) arm_smccc_1_1_smc(__VA_ARGS__)
>  #else
>  
>  void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
> @@ -254,6 +258,13 @@ void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
>  #define arm_smccc_1_0_smc(...)                                              \
>          __arm_smccc_1_0_smc_count(__count_args(__VA_ARGS__), __VA_ARGS__)
>  
> +#define arm_smccc_smc(...)                                      \
> +    do {                                                        \
> +        if ( cpus_have_const_cap(ARM_SMCCC_1_1) )               \
> +            arm_smccc_1_1_smc(__VA_ARGS__);                     \
> +        else                                                    \
> +            arm_smccc_1_0_smc(__VA_ARGS__);                     \
> +    } while ( 0 )
>  #endif /* CONFIG_ARM_64 */
>  
>  #endif /* __ASSEMBLY__ */
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 6/6] xen/arm: Replace call_smc with arm_smccc_smc

[Stefano Stabellini]

On Tue, 25 Sep 2018, Julien Grall wrote:
> call_smc is a subset of arm_smccc_smc. Rather than having 2 methods to
> do SMCCC call, replace all call to the former by the later.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/Makefile            |  1 -
>  xen/arch/arm/platforms/exynos5.c |  3 ++-
>  xen/arch/arm/platforms/seattle.c |  4 ++--
>  xen/arch/arm/psci.c              | 37 +++++++++++++++++++++++++------------
>  xen/arch/arm/smc.S               | 21 ---------------------
>  xen/include/asm-arm/processor.h  |  3 ---
>  6 files changed, 29 insertions(+), 40 deletions(-)
>  delete mode 100644 xen/arch/arm/smc.S
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index b9b141dc84..37fa8268b3 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -39,7 +39,6 @@ obj-y += processor.o
>  obj-y += psci.o
>  obj-y += setup.o
>  obj-y += shutdown.o
> -obj-y += smc.o
>  obj-y += smp.o
>  obj-y += smpboot.o
>  obj-y += sysctl.o
> diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
> index c15ecf80f5..e2c0b7b878 100644
> --- a/xen/arch/arm/platforms/exynos5.c
> +++ b/xen/arch/arm/platforms/exynos5.c
> @@ -26,6 +26,7 @@
>  #include <asm/platforms/exynos5.h>
>  #include <asm/platform.h>
>  #include <asm/io.h>
> +#include <asm/smccc.h>
>  
>  static bool secure_firmware;
>  
> @@ -249,7 +250,7 @@ static int exynos5_cpu_up(int cpu)
>      iounmap(power);
>  
>      if ( secure_firmware )
> -        call_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0);
> +        arm_smccc_smc(SMC_CMD_CPU1BOOT, cpu, NULL);
>  
>      return cpu_up_send_sgi(cpu);
>  }
> diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c
> index 893cc17972..64cc1868c2 100644
> --- a/xen/arch/arm/platforms/seattle.c
> +++ b/xen/arch/arm/platforms/seattle.c
> @@ -33,12 +33,12 @@ static const char * const seattle_dt_compat[] __initconst =
>   */
>  static void seattle_system_reset(void)
>  {
> -    call_smc(PSCI_0_2_FN32_SYSTEM_RESET, 0, 0, 0);
> +    arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_RESET, NULL);
>  }
>  
>  static void seattle_system_off(void)
>  {
> -    call_smc(PSCI_0_2_FN32_SYSTEM_OFF, 0, 0, 0);
> +    arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_OFF, NULL);
>  }
>  
>  PLATFORM_START(seattle, "SEATTLE")
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index 941eec921b..02737e6caa 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -42,42 +42,53 @@ uint32_t smccc_ver;
>  
>  static uint32_t psci_cpu_on_nr;
>  
> +#define PSCI_RET(res)   ((int32_t)(res).a0)
> +
>  int call_psci_cpu_on(int cpu)
>  {
> -    return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary), 0);
> +    struct arm_smccc_res res;
> +
> +    arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary),
> +                  &res);
> +
> +    return (int32_t)res.a0;
>  }

Can't we use PSCI_RET(res) here?

Also in general, should we care about the type mismatch int32_t vs. int
which is the return type of this function?



>  void call_psci_cpu_off(void)
>  {
>      if ( psci_ver > PSCI_VERSION(0, 1) )
>      {
> -        int errno;
> +        struct arm_smccc_res res;
>  
>          /* If successfull the PSCI cpu_off call doesn't return */
> -        errno = call_smc(PSCI_0_2_FN32_CPU_OFF, 0, 0, 0);
> +        arm_smccc_smc(PSCI_0_2_FN32_CPU_OFF, &res);
>          panic("PSCI cpu off failed for CPU%d err=%d\n", smp_processor_id(),
> -              errno);
> +              PSCI_RET(res));
>      }
>  }
>  
>  void call_psci_system_off(void)
>  {
>      if ( psci_ver > PSCI_VERSION(0, 1) )
> -        call_smc(PSCI_0_2_FN32_SYSTEM_OFF, 0, 0, 0);
> +        arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_OFF, NULL);
>  }
>  
>  void call_psci_system_reset(void)
>  {
>      if ( psci_ver > PSCI_VERSION(0, 1) )
> -        call_smc(PSCI_0_2_FN32_SYSTEM_RESET, 0, 0, 0);
> +        arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_RESET, NULL);
>  }
>  
>  static int __init psci_features(uint32_t psci_func_id)
>  {
> +    struct arm_smccc_res res;
> +
>      if ( psci_ver < PSCI_VERSION(1, 0) )
>          return PSCI_NOT_SUPPORTED;
>  
> -    return call_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, 0, 0);
> +    arm_smccc_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, NULL);
> +
> +    return PSCI_RET(res);
>  }
>  
>  static int __init psci_is_smc_method(const struct dt_device_node *psci)
> @@ -112,11 +123,11 @@ static void __init psci_init_smccc(void)
>  
>      if ( psci_features(ARM_SMCCC_VERSION_FID) != PSCI_NOT_SUPPORTED )
>      {
> -        uint32_t ret;
> +        struct arm_smccc_res res;
>  
> -        ret = call_smc(ARM_SMCCC_VERSION_FID, 0, 0, 0);
> -        if ( ret != ARM_SMCCC_NOT_SUPPORTED )
> -            smccc_ver = ret;
> +        arm_smccc_smc(ARM_SMCCC_VERSION_FID, &res);
> +        if ( PSCI_RET(res) != ARM_SMCCC_NOT_SUPPORTED )
> +            smccc_ver = PSCI_RET(res);
>      }
>  
>      if ( smccc_ver >= SMCCC_VERSION(1, 1) )
> @@ -165,6 +176,7 @@ static int __init psci_init_0_2(void)
>          { /* sentinel */ },
>      };
>      int ret;
> +    struct arm_smccc_res res;
>  
>      if ( acpi_disabled )
>      {
> @@ -186,7 +198,8 @@ static int __init psci_init_0_2(void)
>          }
>      }
>  
> -    psci_ver = call_smc(PSCI_0_2_FN32_PSCI_VERSION, 0, 0, 0);
> +    arm_smccc_smc(PSCI_0_2_FN32_PSCI_VERSION, &res);
> +    psci_ver = PSCI_RET(res);
>  
>      /* For the moment, we only support PSCI 0.2 and PSCI 1.x */
>      if ( psci_ver != PSCI_VERSION(0, 2) && PSCI_VERSION_MAJOR(psci_ver) != 1 )
> diff --git a/xen/arch/arm/smc.S b/xen/arch/arm/smc.S
> deleted file mode 100644
> index b8f182272a..0000000000
> --- a/xen/arch/arm/smc.S
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -/*
> - * xen/arch/arm/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.
> - */
> -
> -#include <asm/macros.h>
> -
> -ENTRY(call_smc)
> -        smc   #0
> -        ret
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 222a02dd99..8016cf306f 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -812,9 +812,6 @@ 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 call_smc(register_t function_id, register_t arg0, register_t arg1,
> -             register_t arg2);
> -
>  void do_trap_hyp_serror(struct cpu_user_regs *regs);
>  
>  void do_trap_guest_serror(struct cpu_user_regs *regs);
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention

[Volodymyr Babchuk]

Hi Julien,

On 25.09.18 20:20, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

> 
> ---
>      Changes in v2:
>          - Invert the condition
>          - Add missing includes
> ---
>   xen/arch/arm/psci.c              |  4 ++++
>   xen/include/asm-arm/cpufeature.h |  3 ++-
>   xen/include/asm-arm/smccc.h      | 11 +++++++++++
>   3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index 3cf5ecf0f3..941eec921b 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -21,6 +21,7 @@
>   #include <xen/types.h>
>   #include <xen/mm.h>
>   #include <xen/smp.h>
> +#include <asm/cpufeature.h>
>   #include <asm/psci.h>
>   #include <asm/acpi.h>
>   
> @@ -118,6 +119,9 @@ static void __init psci_init_smccc(void)
>               smccc_ver = ret;
>       }
>   
> +    if ( smccc_ver >= SMCCC_VERSION(1, 1) )
> +        cpus_set_cap(ARM_SMCCC_1_1);
> +
>       printk(XENLOG_INFO "Using SMC Calling Convention v%u.%u\n",
>              SMCCC_VERSION_MAJOR(smccc_ver), SMCCC_VERSION_MINOR(smccc_ver));
>   }
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index c6cbc2ec84..2d82264427 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -44,8 +44,9 @@
>   #define SKIP_CTXT_SWITCH_SERROR_SYNC 6
>   #define ARM_HARDEN_BRANCH_PREDICTOR 7
>   #define ARM_SSBD 8
> +#define ARM_SMCCC_1_1 9
>   
> -#define ARM_NCAPS           9
> +#define ARM_NCAPS           10
>   
>   #ifndef __ASSEMBLY__
>   
> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> index 1ed6cbaa48..126399dd70 100644
> --- a/xen/include/asm-arm/smccc.h
> +++ b/xen/include/asm-arm/smccc.h
> @@ -16,6 +16,9 @@
>   #ifndef __ASM_ARM_SMCCC_H__
>   #define __ASM_ARM_SMCCC_H__
>   
> +#include <asm/alternative.h>
> +#include <asm/cpufeature.h>
> +
>   #define SMCCC_VERSION_MAJOR_SHIFT            16
>   #define SMCCC_VERSION_MINOR_MASK             \
>           ((1U << SMCCC_VERSION_MAJOR_SHIFT) - 1)
> @@ -213,6 +216,7 @@ struct arm_smccc_res {
>    */
>   #ifdef CONFIG_ARM_32
>   #define arm_smccc_1_0_smc(...) arm_smccc_1_1_smc(__VA_ARGS__)
> +#define arm_smccc_smc(...) arm_smccc_1_1_smc(__VA_ARGS__)
>   #else
>   
>   void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
> @@ -254,6 +258,13 @@ void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
>   #define arm_smccc_1_0_smc(...)                                              \
>           __arm_smccc_1_0_smc_count(__count_args(__VA_ARGS__), __VA_ARGS__)
>   
> +#define arm_smccc_smc(...)                                      \
> +    do {                                                        \
> +        if ( cpus_have_const_cap(ARM_SMCCC_1_1) )               \
> +            arm_smccc_1_1_smc(__VA_ARGS__);                     \
> +        else                                                    \
> +            arm_smccc_1_0_smc(__VA_ARGS__);                     \
> +    } while ( 0 )
>   #endif /* CONFIG_ARM_64 */
>   
>   #endif /* __ASSEMBLY__ */
> 

-- 
Volodymyr Babchuk

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 4/6] xen/arm: cpufeature: Add helper to check constant caps

[Stefano Stabellini]

On Tue, 25 Sep 2018, Julien Grall wrote:
> Some capababilities are set right during boot and will never change
> afterwards. At the moment, the function cpu_have_caps will check whether
> the cap is enabled from the memory.
> 
> It is possible to avoid the load from the memory by using an
> ALTERNATIVE. With that the check is just reduced to 1 instruction.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

I enjoyed reading the patch :-)  But I don't think it is worth going
into this extreme level of optimization. test_bit is efficient enough,
right? What do you think we need to use alternatives just to check one
bit?


> ---
> 
> This is the static key for the poor. At some point we might want to
> introduce something similar to static key in Xen.
> 
>     Changes in v2:
>         - Use unlikely
> ---
>  xen/include/asm-arm/cpufeature.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index 3de6b54301..c6cbc2ec84 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -63,6 +63,18 @@ static inline bool cpus_have_cap(unsigned int num)
>      return test_bit(num, cpu_hwcaps);
>  }
>  
> +/* System capability check for constant cap */
> +#define cpus_have_const_cap(num) ({                 \
> +        bool __ret;                                 \
> +                                                    \
> +        asm volatile (ALTERNATIVE("mov %0, #0",     \
> +                                  "mov %0, #1",     \
> +                                  num)              \
> +                      : "=r" (__ret));              \
> +                                                    \
> +        unlikely(__ret);                            \
> +        })
> +
>  static inline void cpus_set_cap(unsigned int num)
>  {
>      if (num >= ARM_NCAPS)
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 3/6] xen/arm: add SMC wrapper that is compatible with SMCCC v1.0

[Julien Grall]

Hi Stefano,

On 09/26/2018 12:50 AM, Stefano Stabellini wrote:
> On Tue, 25 Sep 2018, Julien Grall wrote:
>> From: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>
>> Existing SMC wrapper call_smc() allows only 4 parameters and
>> returns only one value. This is enough for existing
>> use in PSCI code, but TEE mediator will need a call that is
>> fully compatible with ARM SMCCC v1.0.
>>
>> This patch adds a wrapper for both arm32 and arm64. In the case of
>> arm32, the wrapper is just an alias to the ARM SMCCC v1.1 as the
>> convention is the same.
>>
>> CC: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> [julien: Rework the wrapper to make it closer to SMCC 1.1 wrapper]
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> I have been struggling to find the old doc for SMCCC v1.0, all the
> references have been updated to v1.1 online now. Do you have a link?

Are you sure? All the references are still to v1.0 (DEN 0028B). See [1].

>> diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S
>> new file mode 100644
>> index 0000000000..b0752be57e
>> --- /dev/null
>> +++ b/xen/arch/arm/arm64/smc.S
>> @@ -0,0 +1,32 @@
>> +/*
>> + * 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.
>> + *
>> + * 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.
>> + */
>> +
>> +#include <asm/asm_defns.h>
>> +#include <asm/macros.h>
>> +
>> +/*
>> + * void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
>> + *                          register_t a3, register_t a4, register_t a5,
>> + *                          register_t a6, register_t a7,
>> + *                          struct arm_smccc_res *res)
>> + */
>> +ENTRY(__arm_smccc_1_0_smc)
>> +        smc     #0
>> +        ldr     x4, [sp]
>> +        cbz     x4, 1f          /* No need to store the result */
>> +        stp     x0, x1, [x4, #SMCCC_RES_a0]
>> +        stp     x2, x3, [x4, #SMCCC_RES_a2]
>> +1:
>> +        ret
> 
> As I mentioned, I couldn't find the doc, but it looks like the Linux
> implementation always copies back the results
> (arch/arm64/kernel/smccc-call.S)? Shouldn't we zero x0-x3 at least?
Could you provide more details on what looks wrong?

The results are copied in the array res using stp instructions. The only 
difference with Linux implementation is we don't handle quirk.

Cheers,

[1] 
https://developer.arm.com/products/architecture/cpu-architecture/a-profile/docs/den0028/latest/smc-calling-convention-system-software-on-arm-platforms

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 4/6] xen/arm: cpufeature: Add helper to check constant caps

[Julien Grall]

Hi Stefano,

On 09/26/2018 05:53 PM, Stefano Stabellini wrote:
> On Tue, 25 Sep 2018, Julien Grall wrote:
>> Some capababilities are set right during boot and will never change
>> afterwards. At the moment, the function cpu_have_caps will check whether
>> the cap is enabled from the memory.
>>
>> It is possible to avoid the load from the memory by using an
>> ALTERNATIVE. With that the check is just reduced to 1 instruction.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> I enjoyed reading the patch :-)  But I don't think it is worth going
> into this extreme level of optimization. test_bit is efficient enough,
> right? What do you think we need to use alternatives just to check one
> bit?

We already have an helper using test_bit (see cpus_have_cap). However 
test_bit requires to load the word from the memory. This is an overhead 
when the decision never change after boot.

One load may be insignificant by itself (thought may have a cache miss), 
but if you are in hotpath this is a win in long term.

The mechanism is very similar to static key but for the poor (I don't 
have much time to implement better for now). We already use similar 
construction on other places (see CHECK_WORKAROUND_HELPER).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 6/6] xen/arm: Replace call_smc with arm_smccc_smc

[Julien Grall]

Hi Stefano,

On 09/26/2018 12:57 AM, Stefano Stabellini wrote:
> On Tue, 25 Sep 2018, Julien Grall wrote:
>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>> index 941eec921b..02737e6caa 100644
>> --- a/xen/arch/arm/psci.c
>> +++ b/xen/arch/arm/psci.c
>> @@ -42,42 +42,53 @@ uint32_t smccc_ver;
>>   
>>   static uint32_t psci_cpu_on_nr;
>>   
>> +#define PSCI_RET(res)   ((int32_t)(res).a0)
>> +
>>   int call_psci_cpu_on(int cpu)
>>   {
>> -    return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary), 0);
>> +    struct arm_smccc_res res;
>> +
>> +    arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary),
>> +                  &res);
>> +
>> +    return (int32_t)res.a0;
>>   }
> 
> Can't we use PSCI_RET(res) here?

I missed that one. I will update it.

> 
> Also in general, should we care about the type mismatch int32_t vs. int
> which is the return type of this function?

The only issue I could see is if sizeof(int) < sizeof(int32_t). If that 
happen, then psci.c would be our least concern as we always assume int 
would at least 32-bit wide.

I would prefer to keep the return of the function int and casting the 
result with (int32_t). The latter is helpful to know what is the size of 
the result (a0 is 64-bit).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 4/6] xen/arm: cpufeature: Add helper to check constant caps

[Stefano Stabellini]

On Wed, 26 Sep 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 09/26/2018 05:53 PM, Stefano Stabellini wrote:
> > On Tue, 25 Sep 2018, Julien Grall wrote:
> > > Some capababilities are set right during boot and will never change
> > > afterwards. At the moment, the function cpu_have_caps will check whether
> > > the cap is enabled from the memory.
> > > 
> > > It is possible to avoid the load from the memory by using an
> > > ALTERNATIVE. With that the check is just reduced to 1 instruction.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > 
> > I enjoyed reading the patch :-)  But I don't think it is worth going
> > into this extreme level of optimization. test_bit is efficient enough,
> > right? What do you think we need to use alternatives just to check one
> > bit?
> 
> We already have an helper using test_bit (see cpus_have_cap). However test_bit
> requires to load the word from the memory. This is an overhead when the
> decision never change after boot.
> 
> One load may be insignificant by itself (thought may have a cache miss), but
> if you are in hotpath this is a win in long term.
> 
> The mechanism is very similar to static key but for the poor (I don't have
> much time to implement better for now). We already use similar construction on
> other places (see CHECK_WORKAROUND_HELPER).

I like the mechanism and the little ALTERNATIVE trick.  I can see it
could very useful in some cases. It is just that it is a bit of a waste
to use it on SMCCC virtualization -- SMCCC calls cannot be done often
enough for this to make a difference.

But who am I to complain when you make things faster? :-)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 6/6] xen/arm: Replace call_smc with arm_smccc_smc

[Stefano Stabellini]

On Wed, 26 Sep 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 09/26/2018 12:57 AM, Stefano Stabellini wrote:
> > On Tue, 25 Sep 2018, Julien Grall wrote:
> > > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> > > index 941eec921b..02737e6caa 100644
> > > --- a/xen/arch/arm/psci.c
> > > +++ b/xen/arch/arm/psci.c
> > > @@ -42,42 +42,53 @@ uint32_t smccc_ver;
> > >     static uint32_t psci_cpu_on_nr;
> > >   +#define PSCI_RET(res)   ((int32_t)(res).a0)
> > > +
> > >   int call_psci_cpu_on(int cpu)
> > >   {
> > > -    return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu),
> > > __pa(init_secondary), 0);
> > > +    struct arm_smccc_res res;
> > > +
> > > +    arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu),
> > > __pa(init_secondary),
> > > +                  &res);
> > > +
> > > +    return (int32_t)res.a0;
> > >   }
> > 
> > Can't we use PSCI_RET(res) here?
> 
> I missed that one. I will update it.
> 
> > 
> > Also in general, should we care about the type mismatch int32_t vs. int
> > which is the return type of this function?
> 
> The only issue I could see is if sizeof(int) < sizeof(int32_t). If that
> happen, then psci.c would be our least concern as we always assume int would
> at least 32-bit wide.
> 
> I would prefer to keep the return of the function int and casting the result
> with (int32_t). The latter is helpful to know what is the size of the result
> (a0 is 64-bit).

It is a good idea to keep the cast. I don't have a strong opinion on
whether the functions should return int or int32_t. The only question is
whether we want to cast to (int32_t) or to (int). I would prefer to cast
to the same type of the return of the function. So if we keep int as
return type, then casting to (int). However, given that in practice it
makes no difference, I can ack the patch in any case.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 3/6] xen/arm: add SMC wrapper that is compatible with SMCCC v1.0

[Stefano Stabellini]

On Wed, 26 Sep 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 09/26/2018 12:50 AM, Stefano Stabellini wrote:
> > On Tue, 25 Sep 2018, Julien Grall wrote:
> > > From: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> > > 
> > > Existing SMC wrapper call_smc() allows only 4 parameters and
> > > returns only one value. This is enough for existing
> > > use in PSCI code, but TEE mediator will need a call that is
> > > fully compatible with ARM SMCCC v1.0.
> > > 
> > > This patch adds a wrapper for both arm32 and arm64. In the case of
> > > arm32, the wrapper is just an alias to the ARM SMCCC v1.1 as the
> > > convention is the same.
> > > 
> > > CC: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> > > [julien: Rework the wrapper to make it closer to SMCC 1.1 wrapper]
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > 
> > I have been struggling to find the old doc for SMCCC v1.0, all the
> > references have been updated to v1.1 online now. Do you have a link?
> 
> Are you sure? All the references are still to v1.0 (DEN 0028B). See [1].

The lack of version in the doc confused me.


> > > diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S
> > > new file mode 100644
> > > index 0000000000..b0752be57e
> > > --- /dev/null
> > > +++ b/xen/arch/arm/arm64/smc.S
> > > @@ -0,0 +1,32 @@
> > > +/*
> > > + * 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.
> > > + *
> > > + * 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.
> > > + */
> > > +
> > > +#include <asm/asm_defns.h>
> > > +#include <asm/macros.h>
> > > +
> > > +/*
> > > + * void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
> > > + *                          register_t a3, register_t a4, register_t a5,
> > > + *                          register_t a6, register_t a7,
> > > + *                          struct arm_smccc_res *res)
> > > + */
> > > +ENTRY(__arm_smccc_1_0_smc)
> > > +        smc     #0
> > > +        ldr     x4, [sp]
> > > +        cbz     x4, 1f          /* No need to store the result */
> > > +        stp     x0, x1, [x4, #SMCCC_RES_a0]
> > > +        stp     x2, x3, [x4, #SMCCC_RES_a2]
> > > +1:
> > > +        ret
> > 
> > As I mentioned, I couldn't find the doc, but it looks like the Linux
> > implementation always copies back the results
> > (arch/arm64/kernel/smccc-call.S)? Shouldn't we zero x0-x3 at least?
> Could you provide more details on what looks wrong?
> 
> The results are copied in the array res using stp instructions. The only
> difference with Linux implementation is we don't handle quirk.

The only difference is that in this implementation we handle `res' being
NULL, which I think is a good idea:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 3/6] xen/arm: add SMC wrapper that is compatible with SMCCC v1.0

[Julien Grall]

Hi Stefano,

On 09/28/2018 12:02 AM, Stefano Stabellini wrote:
> On Wed, 26 Sep 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 09/26/2018 12:50 AM, Stefano Stabellini wrote:
>>> On Tue, 25 Sep 2018, Julien Grall wrote:
>>>> From: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>>>
>>>> Existing SMC wrapper call_smc() allows only 4 parameters and
>>>> returns only one value. This is enough for existing
>>>> use in PSCI code, but TEE mediator will need a call that is
>>>> fully compatible with ARM SMCCC v1.0.
>>>>
>>>> This patch adds a wrapper for both arm32 and arm64. In the case of
>>>> arm32, the wrapper is just an alias to the ARM SMCCC v1.1 as the
>>>> convention is the same.
>>>>
>>>> CC: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>>> [julien: Rework the wrapper to make it closer to SMCC 1.1 wrapper]
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>
>>> I have been struggling to find the old doc for SMCCC v1.0, all the
>>> references have been updated to v1.1 online now. Do you have a link?
>>
>> Are you sure? All the references are still to v1.0 (DEN 0028B). See [1].
> 
> The lack of version in the doc confused me.
> 
> 
>>>> diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S
>>>> new file mode 100644
>>>> index 0000000000..b0752be57e
>>>> --- /dev/null
>>>> +++ b/xen/arch/arm/arm64/smc.S
>>>> @@ -0,0 +1,32 @@
>>>> +/*
>>>> + * 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.
>>>> + *
>>>> + * 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.
>>>> + */
>>>> +
>>>> +#include <asm/asm_defns.h>
>>>> +#include <asm/macros.h>
>>>> +
>>>> +/*
>>>> + * void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
>>>> + *                          register_t a3, register_t a4, register_t a5,
>>>> + *                          register_t a6, register_t a7,
>>>> + *                          struct arm_smccc_res *res)
>>>> + */
>>>> +ENTRY(__arm_smccc_1_0_smc)
>>>> +        smc     #0
>>>> +        ldr     x4, [sp]
>>>> +        cbz     x4, 1f          /* No need to store the result */
>>>> +        stp     x0, x1, [x4, #SMCCC_RES_a0]
>>>> +        stp     x2, x3, [x4, #SMCCC_RES_a2]
>>>> +1:
>>>> +        ret
>>>
>>> As I mentioned, I couldn't find the doc, but it looks like the Linux
>>> implementation always copies back the results
>>> (arch/arm64/kernel/smccc-call.S)? Shouldn't we zero x0-x3 at least?
>> Could you provide more details on what looks wrong?
>>
>> The results are copied in the array res using stp instructions. The only
>> difference with Linux implementation is we don't handle quirk.
> 
> The only difference is that in this implementation we handle `res' being
> NULL, which I think is a good idea:

Oh yeah I forgot that difference :/. I added it to keep the behavior 
similar to the SMCCC v1.1 implementation.

> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 

Thank you!

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 4/6] xen/arm: cpufeature: Add helper to check constant caps

[Julien Grall]

Hi,

On 09/27/2018 11:34 PM, Stefano Stabellini wrote:
> On Wed, 26 Sep 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 09/26/2018 05:53 PM, Stefano Stabellini wrote:
>>> On Tue, 25 Sep 2018, Julien Grall wrote:
>>>> Some capababilities are set right during boot and will never change
>>>> afterwards. At the moment, the function cpu_have_caps will check whether
>>>> the cap is enabled from the memory.
>>>>
>>>> It is possible to avoid the load from the memory by using an
>>>> ALTERNATIVE. With that the check is just reduced to 1 instruction.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>
>>> I enjoyed reading the patch :-)  But I don't think it is worth going
>>> into this extreme level of optimization. test_bit is efficient enough,
>>> right? What do you think we need to use alternatives just to check one
>>> bit?
>>
>> We already have an helper using test_bit (see cpus_have_cap). However test_bit
>> requires to load the word from the memory. This is an overhead when the
>> decision never change after boot.
>>
>> One load may be insignificant by itself (thought may have a cache miss), but
>> if you are in hotpath this is a win in long term.
>>
>> The mechanism is very similar to static key but for the poor (I don't have
>> much time to implement better for now). We already use similar construction on
>> other places (see CHECK_WORKAROUND_HELPER).
> 
> I like the mechanism and the little ALTERNATIVE trick.  I can see it
> could very useful in some cases. It is just that it is a bit of a waste
> to use it on SMCCC virtualization -- SMCCC calls cannot be done often
> enough for this to make a difference.

I would not be so sure ;). SMC can be called at *every* entry and exit 
into the hypervisor to apply SSBD workaround.

While SSBD will directly call arm_smccc_1_1_smc (and not the generic 
one), I would not rule out more hotpath with SMC call in it.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 6/6] xen/arm: Replace call_smc with arm_smccc_smc

[Julien Grall]

Hi Stefano,

On 09/27/2018 11:41 PM, Stefano Stabellini wrote:
> On Wed, 26 Sep 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 09/26/2018 12:57 AM, Stefano Stabellini wrote:
>>> On Tue, 25 Sep 2018, Julien Grall wrote:
>>>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>>>> index 941eec921b..02737e6caa 100644
>>>> --- a/xen/arch/arm/psci.c
>>>> +++ b/xen/arch/arm/psci.c
>>>> @@ -42,42 +42,53 @@ uint32_t smccc_ver;
>>>>      static uint32_t psci_cpu_on_nr;
>>>>    +#define PSCI_RET(res)   ((int32_t)(res).a0)
>>>> +
>>>>    int call_psci_cpu_on(int cpu)
>>>>    {
>>>> -    return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu),
>>>> __pa(init_secondary), 0);
>>>> +    struct arm_smccc_res res;
>>>> +
>>>> +    arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu),
>>>> __pa(init_secondary),
>>>> +                  &res);
>>>> +
>>>> +    return (int32_t)res.a0;
>>>>    }
>>>
>>> Can't we use PSCI_RET(res) here?
>>
>> I missed that one. I will update it.
>>
>>>
>>> Also in general, should we care about the type mismatch int32_t vs. int
>>> which is the return type of this function?
>>
>> The only issue I could see is if sizeof(int) < sizeof(int32_t). If that
>> happen, then psci.c would be our least concern as we always assume int would
>> at least 32-bit wide.
>>
>> I would prefer to keep the return of the function int and casting the result
>> with (int32_t). The latter is helpful to know what is the size of the result
>> (a0 is 64-bit).
> 
> It is a good idea to keep the cast. I don't have a strong opinion on
> whether the functions should return int or int32_t. The only question is
> whether we want to cast to (int32_t) or to (int). I would prefer to cast
> to the same type of the return of the function. So if we keep int as
> return type, then casting to (int). However, given that in practice it
> makes no difference, I can ack the patch in any case.

I would prefer to stick with the current approach.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel