* [PATCH 0/6] xen/arm: SMCCC fixup and improvement
@ 2018-08-24 16:58 Julien Grall
2018-08-24 16:58 ` [PATCH 1/6] xen/arm: smccc-1.1: Make return values unsigned long Julien Grall
` (6 more replies)
0 siblings, 7 replies; 26+ messages in thread
From: Julien Grall @ 2018-08-24 16:58 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, sstabellini, volodymyr_babchuk
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 | 97 +++++++++++++++++++++++++++++++++-------
11 files changed, 167 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/6] xen/arm: smccc-1.1: Make return values unsigned long
2018-08-24 16:58 [PATCH 0/6] xen/arm: SMCCC fixup and improvement Julien Grall
@ 2018-08-24 16:58 ` Julien Grall
2018-08-27 14:03 ` Volodymyr Babchuk
2018-08-24 16:58 ` [PATCH 2/6] xen/arm: smccc-1.1: Handle function result as parameters Julien Grall
` (5 subsequent siblings)
6 siblings, 1 reply; 26+ messages in thread
From: Julien Grall @ 2018-08-24 16:58 UTC (permalink / raw)
To: xen-devel; +Cc: Marc Zyngier, Julien Grall, sstabellini, volodymyr_babchuk
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>
---
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/6] xen/arm: smccc-1.1: Handle function result as parameters
2018-08-24 16:58 [PATCH 0/6] xen/arm: SMCCC fixup and improvement Julien Grall
2018-08-24 16:58 ` [PATCH 1/6] xen/arm: smccc-1.1: Make return values unsigned long Julien Grall
@ 2018-08-24 16:58 ` Julien Grall
2018-08-27 14:05 ` Volodymyr Babchuk
2018-08-24 16:58 ` [PATCH 3/6] xen/arm: add SMC wrapper that is compatible with SMCCC v1.0 Julien Grall
` (4 subsequent siblings)
6 siblings, 1 reply; 26+ messages in thread
From: Julien Grall @ 2018-08-24 16:58 UTC (permalink / raw)
To: xen-devel; +Cc: Marc Zyngier, sstabellini, volodymyr_babchuk
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>
---
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 3/6] xen/arm: add SMC wrapper that is compatible with SMCCC v1.0
2018-08-24 16:58 [PATCH 0/6] xen/arm: SMCCC fixup and improvement Julien Grall
2018-08-24 16:58 ` [PATCH 1/6] xen/arm: smccc-1.1: Make return values unsigned long Julien Grall
2018-08-24 16:58 ` [PATCH 2/6] xen/arm: smccc-1.1: Handle function result as parameters Julien Grall
@ 2018-08-24 16:58 ` Julien Grall
2018-08-24 16:58 ` [PATCH 4/6] xen/arm: cpufeature: Add helper to check constant caps Julien Grall
` (3 subsequent siblings)
6 siblings, 0 replies; 26+ messages in thread
From: Julien Grall @ 2018-08-24 16:58 UTC (permalink / raw)
To: xen-devel; +Cc: Edgar E. Iglesias, Julien Grall, sstabellini, volodymyr_babchuk
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 4/6] xen/arm: cpufeature: Add helper to check constant caps
2018-08-24 16:58 [PATCH 0/6] xen/arm: SMCCC fixup and improvement Julien Grall
` (2 preceding siblings ...)
2018-08-24 16:58 ` [PATCH 3/6] xen/arm: add SMC wrapper that is compatible with SMCCC v1.0 Julien Grall
@ 2018-08-24 16:58 ` Julien Grall
2018-08-30 17:43 ` Volodymyr Babchuk
2018-08-24 16:58 ` [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention Julien Grall
` (2 subsequent siblings)
6 siblings, 1 reply; 26+ messages in thread
From: Julien Grall @ 2018-08-24 16:58 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, sstabellini, volodymyr_babchuk
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.
---
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..9c297c521c 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)); \
+ \
+ __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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention
2018-08-24 16:58 [PATCH 0/6] xen/arm: SMCCC fixup and improvement Julien Grall
` (3 preceding siblings ...)
2018-08-24 16:58 ` [PATCH 4/6] xen/arm: cpufeature: Add helper to check constant caps Julien Grall
@ 2018-08-24 16:58 ` Julien Grall
2018-08-27 14:15 ` Volodymyr Babchuk
2018-08-30 17:45 ` Volodymyr Babchuk
2018-08-24 16:58 ` [PATCH 6/6] xen/arm: Replace call_smc with arm_smccc_smc Julien Grall
2018-08-30 16:41 ` [PATCH 0/6] xen/arm: SMCCC fixup and improvement Volodymyr Babchuk
6 siblings, 2 replies; 26+ messages in thread
From: Julien Grall @ 2018-08-24 16:58 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, sstabellini, volodymyr_babchuk
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
xen/arch/arm/psci.c | 4 ++++
xen/include/asm-arm/cpufeature.h | 3 ++-
xen/include/asm-arm/smccc.h | 8 ++++++++
3 files changed, 14 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 9c297c521c..c9c4046f5f 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..7c39c530e2 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -213,6 +213,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 +255,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_0_smc(__VA_ARGS__); \
+ else \
+ arm_smccc_1_1_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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 6/6] xen/arm: Replace call_smc with arm_smccc_smc
2018-08-24 16:58 [PATCH 0/6] xen/arm: SMCCC fixup and improvement Julien Grall
` (4 preceding siblings ...)
2018-08-24 16:58 ` [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention Julien Grall
@ 2018-08-24 16:58 ` Julien Grall
2018-08-27 13:53 ` Volodymyr Babchuk
2018-08-30 16:41 ` [PATCH 0/6] xen/arm: SMCCC fixup and improvement Volodymyr Babchuk
6 siblings, 1 reply; 26+ messages in thread
From: Julien Grall @ 2018-08-24 16:58 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, sstabellini, volodymyr_babchuk
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 6/6] xen/arm: Replace call_smc with arm_smccc_smc
2018-08-24 16:58 ` [PATCH 6/6] xen/arm: Replace call_smc with arm_smccc_smc Julien Grall
@ 2018-08-27 13:53 ` Volodymyr Babchuk
0 siblings, 0 replies; 26+ messages in thread
From: Volodymyr Babchuk @ 2018-08-27 13:53 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: sstabellini
Hi Julien,
On 24.08.18 19:58, 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)
Do we really need this macro?
> +
> 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;
If you still want to introduce it, then there should be
+return PSCI_RET(res);
> }
>
> 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);
>
--
Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/6] xen/arm: smccc-1.1: Make return values unsigned long
2018-08-24 16:58 ` [PATCH 1/6] xen/arm: smccc-1.1: Make return values unsigned long Julien Grall
@ 2018-08-27 14:03 ` Volodymyr Babchuk
2018-08-27 15:23 ` Julien Grall
0 siblings, 1 reply; 26+ messages in thread
From: Volodymyr Babchuk @ 2018-08-27 14:03 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: Marc Zyngier, sstabellini
Hi Julien, Marc
On 24.08.18 19:58, 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.
Maybe it better to use register_t then? By definition register_t has the
same size as a CPU register and SMC uses CPU registers to pass
parameters/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>
> ---
> 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;\
Do you really want to silently drop upper 32 bits of the argument?
I know, that SMCCC states that function id is a 32-bit value,
but I don't think that it is a good place to enforce this
behavior.
I think it is better to allow user to shoot in his leg in this case.
> 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); \
>
--
Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/6] xen/arm: smccc-1.1: Handle function result as parameters
2018-08-24 16:58 ` [PATCH 2/6] xen/arm: smccc-1.1: Handle function result as parameters Julien Grall
@ 2018-08-27 14:05 ` Volodymyr Babchuk
0 siblings, 0 replies; 26+ messages in thread
From: Volodymyr Babchuk @ 2018-08-27 14:05 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: Marc Zyngier, sstabellini
Hi,
On 24.08.18 19:58, 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 <volodymr_babchuk@epam.com>
> ---
> 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__)
>
--
Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention
2018-08-24 16:58 ` [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention Julien Grall
@ 2018-08-27 14:15 ` Volodymyr Babchuk
2018-08-27 15:29 ` Julien Grall
2018-08-30 17:45 ` Volodymyr Babchuk
1 sibling, 1 reply; 26+ messages in thread
From: Volodymyr Babchuk @ 2018-08-27 14:15 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: sstabellini
Hi Julien,
On 24.08.18 19:58, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
> xen/arch/arm/psci.c | 4 ++++
> xen/include/asm-arm/cpufeature.h | 3 ++-
> xen/include/asm-arm/smccc.h | 8 ++++++++
> 3 files changed, 14 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 9c297c521c..c9c4046f5f 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..7c39c530e2 100644
> --- a/xen/include/asm-arm/smccc.h
> +++ b/xen/include/asm-arm/smccc.h
> @@ -213,6 +213,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 +255,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_0_smc(__VA_ARGS__); \
> + else \
> + arm_smccc_1_1_smc(__VA_ARGS__); \
> + } while ( 0 )
> #endif /* CONFIG_ARM_64 */
>
This will generate lots of code for every arm_smccc_smc(). Can we have
function pointer arm_smccc_smc instead and assign it to either
arm_smccc_1_1_smc() or arm_asmccc_1_0_smc() at boot?
I know that currently we have no function arm_smccc_1_1_smc() because it
is being constructed inline every time. But we can write it explicitly
and then have one indirect call to (maybe cached) function instead of
lots inlined code with conditional branches.
> #endif /* __ASSEMBLY__ */
>
--
Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/6] xen/arm: smccc-1.1: Make return values unsigned long
2018-08-27 14:03 ` Volodymyr Babchuk
@ 2018-08-27 15:23 ` Julien Grall
2018-08-27 16:33 ` Volodymyr Babchuk
0 siblings, 1 reply; 26+ messages in thread
From: Julien Grall @ 2018-08-27 15:23 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: Marc Zyngier, nd, sstabellini
On 27/08/2018 15:03, Volodymyr Babchuk wrote:
> Hi Julien, Marc
Hi,
> On 24.08.18 19:58, 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.
> Maybe it better to use register_t then? By definition register_t has the
> same size as a CPU register and SMC uses CPU registers to pass
> parameters/return values.
The code is based on Linux series (posted last Friday). I don't much
want to diverge too much from it. So unsigned "unsigned long" here is ok.
>
>> 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>
>> ---
>> 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;\
> Do you really want to silently drop upper 32 bits of the argument?
> I know, that SMCCC states that function id is a 32-bit value,
> but I don't think that it is a good place to enforce this
> behavior.
> I think it is better to allow user to shoot in his leg in this case.
I don't see any issue with casting here. This is what the SMCCC request
for x0 and after all you silently drop upper 32-bit when using a static
inline function...
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention
2018-08-27 14:15 ` Volodymyr Babchuk
@ 2018-08-27 15:29 ` Julien Grall
2018-08-27 16:50 ` Volodymyr Babchuk
0 siblings, 1 reply; 26+ messages in thread
From: Julien Grall @ 2018-08-27 15:29 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: nd, sstabellini
On 27/08/2018 15:15, Volodymyr Babchuk wrote:
> Hi Julien,
Hi,
> On 24.08.18 19:58, Julien Grall wrote:
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>> xen/arch/arm/psci.c | 4 ++++
>> xen/include/asm-arm/cpufeature.h | 3 ++-
>> xen/include/asm-arm/smccc.h | 8 ++++++++
>> 3 files changed, 14 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 9c297c521c..c9c4046f5f 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..7c39c530e2 100644
>> --- a/xen/include/asm-arm/smccc.h
>> +++ b/xen/include/asm-arm/smccc.h
>> @@ -213,6 +213,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 +255,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_0_smc(__VA_ARGS__); \
>> + else \
>> + arm_smccc_1_1_smc(__VA_ARGS__); \
>> + } while ( 0 )
>> #endif /* CONFIG_ARM_64 */
> This will generate lots of code for every arm_smccc_smc(). Can we have
> function pointer arm_smccc_smc instead and assign it to either
> arm_smccc_1_1_smc() or arm_asmccc_1_0_smc() at boot?
>
> I know that currently we have no function arm_smccc_1_1_smc() because it
> is being constructed inline every time. But we can write it explicitly
> and then have one indirect call to (maybe cached) function instead of
> lots inlined code with conditional branches.
This is indeed increasing the size of the function, but with a better
performance when you perform an SMC with 1.1.
The main goal of SMCCC 1.1 is to limit the number of registers saved
when calling an SMC. So if you provide provide a wrapper using a
function, then you are better off sticking with SMCCC 1.0.
The idea of this code is to provide a faster call on the presence of
SMCCC 1.1 while providing a fallback for other case. The function
cpus_have_const_cap is implemented using an ALTERNATIVE (similar to
static key) that will make if close to a NOP. I am saying close because
this is not quite a static key as we don't have it in Xen. Instead, you
replace a memory load by a constant.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/6] xen/arm: smccc-1.1: Make return values unsigned long
2018-08-27 15:23 ` Julien Grall
@ 2018-08-27 16:33 ` Volodymyr Babchuk
0 siblings, 0 replies; 26+ messages in thread
From: Volodymyr Babchuk @ 2018-08-27 16:33 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: Marc Zyngier, nd, sstabellini
Julien,
On 27.08.18 18:23, Julien Grall wrote:
>
>
> On 27/08/2018 15:03, Volodymyr Babchuk wrote:
>> Hi Julien, Marc
>
> Hi,
>
>> On 24.08.18 19:58, 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.
>> Maybe it better to use register_t then? By definition register_t has
>> the same size as a CPU register and SMC uses CPU registers to pass
>> parameters/return values.
>
> The code is based on Linux series (posted last Friday). I don't much
> want to diverge too much from it. So unsigned "unsigned long" here is ok.
>
>>
>>> 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>
>>> ---
>>> 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;\
>> Do you really want to silently drop upper 32 bits of the argument?
>> I know, that SMCCC states that function id is a 32-bit value,
>> but I don't think that it is a good place to enforce this
>> behavior.
>> I think it is better to allow user to shoot in his leg in this case.
>
> I don't see any issue with casting here. This is what the SMCCC request
> for x0 and after all you silently drop upper 32-bit when using a static
> inline function...
>
Yes, you are right.
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
--
Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention
2018-08-27 15:29 ` Julien Grall
@ 2018-08-27 16:50 ` Volodymyr Babchuk
2018-08-28 14:05 ` Julien Grall
0 siblings, 1 reply; 26+ messages in thread
From: Volodymyr Babchuk @ 2018-08-27 16:50 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: nd, sstabellini
Hi,
On 27.08.18 18:29, Julien Grall wrote:
>
>
> On 27/08/2018 15:15, Volodymyr Babchuk wrote:
>> Hi Julien,
>
> Hi,
>
>> On 24.08.18 19:58, Julien Grall wrote:
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>> ---
>>> xen/arch/arm/psci.c | 4 ++++
>>> xen/include/asm-arm/cpufeature.h | 3 ++-
>>> xen/include/asm-arm/smccc.h | 8 ++++++++
>>> 3 files changed, 14 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 9c297c521c..c9c4046f5f 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..7c39c530e2 100644
>>> --- a/xen/include/asm-arm/smccc.h
>>> +++ b/xen/include/asm-arm/smccc.h
>>> @@ -213,6 +213,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 +255,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_0_smc(__VA_ARGS__); \
>>> + else \
>>> + arm_smccc_1_1_smc(__VA_ARGS__); \
>>> + } while ( 0 )
>>> #endif /* CONFIG_ARM_64 */
>> This will generate lots of code for every arm_smccc_smc(). Can we have
>> function pointer arm_smccc_smc instead and assign it to either
>> arm_smccc_1_1_smc() or arm_asmccc_1_0_smc() at boot?
>>
>> I know that currently we have no function arm_smccc_1_1_smc() because
>> it is being constructed inline every time. But we can write it
>> explicitly and then have one indirect call to (maybe cached) function
>> instead of
>> lots inlined code with conditional branches.
>
> This is indeed increasing the size of the function, but with a better
> performance when you perform an SMC with 1.1.
>
> The main goal of SMCCC 1.1 is to limit the number of registers saved
> when calling an SMC. So if you provide provide a wrapper using a
> function, then you are better off sticking with SMCCC 1.0.
>
> The idea of this code is to provide a faster call on the presence of
> SMCCC 1.1 while providing a fallback for other case. The function
> cpus_have_const_cap is implemented using an ALTERNATIVE (similar to
> static key) that will make if close to a NOP. I am saying close because
> this is not quite a static key as we don't have it in Xen. Instead, you
> replace a memory load by a constant.
Ah, yes, true.
I checked how static keys are working. Seems, they use asm goto feature.
Now I think: can we optimize this more? Something like that:
#define arm_smccc_smc(...)
do {
asm goto (ALTERNATIVE(nop",
"b %l[smccc_1_1]",
ARM_SMCCC_1_1));
arm_smccc_1_0_smc(__VA_ARGS__);
break;
smccc_1_1:
arm_smccc_1_1_smc(__VA_ARGS__);
} while ( 0 )
This will save use additional conditional branch.
--
Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention
2018-08-27 16:50 ` Volodymyr Babchuk
@ 2018-08-28 14:05 ` Julien Grall
2018-08-28 14:40 ` Volodymyr Babchuk
0 siblings, 1 reply; 26+ messages in thread
From: Julien Grall @ 2018-08-28 14:05 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: sstabellini
Hi Volodymyr,
On 27/08/18 17:50, Volodymyr Babchuk wrote:
> On 27.08.18 18:29, Julien Grall wrote:
>> On 27/08/2018 15:15, Volodymyr Babchuk wrote:
>>> On 24.08.18 19:58, Julien Grall wrote:
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> This is indeed increasing the size of the function, but with a better
>> performance when you perform an SMC with 1.1.
>>
>> The main goal of SMCCC 1.1 is to limit the number of registers saved
>> when calling an SMC. So if you provide provide a wrapper using a
>> function, then you are better off sticking with SMCCC 1.0.
>>
>> The idea of this code is to provide a faster call on the presence of
>> SMCCC 1.1 while providing a fallback for other case. The function
>> cpus_have_const_cap is implemented using an ALTERNATIVE (similar to
>> static key) that will make if close to a NOP. I am saying close
>> because this is not quite a static key as we don't have it in Xen.
>> Instead, you replace a memory load by a constant.
> Ah, yes, true.
>
> I checked how static keys are working. Seems, they use asm goto feature.
> Now I think: can we optimize this more? Something like that:
>
> #define arm_smccc_smc(...)
> do {
>
> asm goto (ALTERNATIVE(nop",
> "b %l[smccc_1_1]",
> ARM_SMCCC_1_1));
You would need to list the label in GotoLabels (see 6.45.2.7 [1]).
> arm_smccc_1_0_smc(__VA_ARGS__);
> break;
> smccc_1_1:
The label will get redefined if you have multiple SMC call in the same
function. We could possibly generate label based on the file/line.
> arm_smccc_1_1_smc(__VA_ARGS__);
> } while ( 0 )
>
> This will save use additional conditional branch.
The code with this patch looks like:
mov x0, #0 mov x0, #1
cbz w0, label cbz w0, label
The mov + conditional branch is likely to be negligible over the cost of
switching to EL3. Overall, I am not really convinced that it would be
worth the cost of open-coding it. I would prefer if we keep the call to
cpus_have_const_cap() and see if it can be optimized.
I have looked at cpus_have_const_cap() and haven't found good way to
optimize it with the current infrastructure in Xen. Feel free to suggest
improvement.
Cheers,
[1] https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention
2018-08-28 14:05 ` Julien Grall
@ 2018-08-28 14:40 ` Volodymyr Babchuk
2018-08-28 14:43 ` Julien Grall
0 siblings, 1 reply; 26+ messages in thread
From: Volodymyr Babchuk @ 2018-08-28 14:40 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: sstabellini
Hi Julien,
On 28.08.18 17:05, Julien Grall wrote:
> Hi Volodymyr,
>
> On 27/08/18 17:50, Volodymyr Babchuk wrote:
>> On 27.08.18 18:29, Julien Grall wrote:
>>> On 27/08/2018 15:15, Volodymyr Babchuk wrote:
>>>> On 24.08.18 19:58, Julien Grall wrote:
>>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>> This is indeed increasing the size of the function, but with a better
>>> performance when you perform an SMC with 1.1.
>>>
>>> The main goal of SMCCC 1.1 is to limit the number of registers saved
>>> when calling an SMC. So if you provide provide a wrapper using a
>>> function, then you are better off sticking with SMCCC 1.0.
>>>
>>> The idea of this code is to provide a faster call on the presence of
>>> SMCCC 1.1 while providing a fallback for other case. The function
>>> cpus_have_const_cap is implemented using an ALTERNATIVE (similar to
>>> static key) that will make if close to a NOP. I am saying close
>>> because this is not quite a static key as we don't have it in Xen.
>>> Instead, you replace a memory load by a constant.
>> Ah, yes, true.
>>
>> I checked how static keys are working. Seems, they use asm goto
>> feature. Now I think: can we optimize this more? Something like that:
>>
>> #define arm_smccc_smc(...)
>> do {
>>
>> asm goto (ALTERNATIVE(nop",
>> "b %l[smccc_1_1]",
>> ARM_SMCCC_1_1));
>
> You would need to list the label in GotoLabels (see 6.45.2.7 [1]).
Yes, but as you said, we can use __LINE__ there.
>> arm_smccc_1_0_smc(__VA_ARGS__);
>> break;
>> smccc_1_1:
> The label will get redefined if you have multiple SMC call in the same
> function. We could possibly generate label based on the file/line.
>
>> arm_smccc_1_1_smc(__VA_ARGS__);
>> } while ( 0 )
>>
>> This will save use additional conditional branch.
> The code with this patch looks like:
>
> mov x0, #0 mov x0, #1
> cbz w0, label cbz w0, label
>
> The mov + conditional branch is likely to be negligible over the cost of
> switching to EL3. Overall, I am not really convinced that it would be
> worth the cost of open-coding it. I would prefer if we keep the call to
> cpus_have_const_cap() and see if it can be optimized.
Okay. I'm fine with this. I just saw one more way to optimize and wanted
to share it with you. Anyways:
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> I have looked at cpus_have_const_cap() and haven't found good way to
> optimize it with the current infrastructure in Xen. Feel free to suggest
> improvement.
Another thing: maybe it is worth to branch to 1.0 code and leave 1.1 in
a straight path of execution? This will save you one more instruction
for SMCCC 1.1 call.
--
Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention
2018-08-28 14:40 ` Volodymyr Babchuk
@ 2018-08-28 14:43 ` Julien Grall
2018-08-28 15:10 ` Volodymyr Babchuk
0 siblings, 1 reply; 26+ messages in thread
From: Julien Grall @ 2018-08-28 14:43 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: sstabellini
On 28/08/18 15:40, Volodymyr Babchuk wrote:
> Hi Julien,
>
> On 28.08.18 17:05, Julien Grall wrote:
>> Hi Volodymyr,
>>
>> On 27/08/18 17:50, Volodymyr Babchuk wrote:
>>> On 27.08.18 18:29, Julien Grall wrote:
>>>> On 27/08/2018 15:15, Volodymyr Babchuk wrote:
>>>>> On 24.08.18 19:58, Julien Grall wrote:
>>>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>> This is indeed increasing the size of the function, but with a
>>>> better performance when you perform an SMC with 1.1.
>>>>
>>>> The main goal of SMCCC 1.1 is to limit the number of registers saved
>>>> when calling an SMC. So if you provide provide a wrapper using a
>>>> function, then you are better off sticking with SMCCC 1.0.
>>>>
>>>> The idea of this code is to provide a faster call on the presence of
>>>> SMCCC 1.1 while providing a fallback for other case. The function
>>>> cpus_have_const_cap is implemented using an ALTERNATIVE (similar to
>>>> static key) that will make if close to a NOP. I am saying close
>>>> because this is not quite a static key as we don't have it in Xen.
>>>> Instead, you replace a memory load by a constant.
>>> Ah, yes, true.
>>>
>>> I checked how static keys are working. Seems, they use asm goto
>>> feature. Now I think: can we optimize this more? Something like that:
>>>
>>> #define arm_smccc_smc(...)
>>> do {
>>>
>>> asm goto (ALTERNATIVE(nop",
>>> "b %l[smccc_1_1]",
>>> ARM_SMCCC_1_1));
>>
>> You would need to list the label in GotoLabels (see 6.45.2.7 [1]).
> Yes, but as you said, we can use __LINE__ there.
>
>>> arm_smccc_1_0_smc(__VA_ARGS__);
>>> break;
>>> smccc_1_1:
>> The label will get redefined if you have multiple SMC call in the same
>> function. We could possibly generate label based on the file/line.
>>
>>> arm_smccc_1_1_smc(__VA_ARGS__);
>>> } while ( 0 )
>>>
>>> This will save use additional conditional branch.
>> The code with this patch looks like:
>>
>> mov x0, #0 mov x0, #1
>> cbz w0, label cbz w0, label
>>
>> The mov + conditional branch is likely to be negligible over the cost
>> of switching to EL3. Overall, I am not really convinced that it would
>> be worth the cost of open-coding it. I would prefer if we keep the
>> call to cpus_have_const_cap() and see if it can be optimized.
> Okay. I'm fine with this. I just saw one more way to optimize and wanted
> to share it with you. Anyways:
>
> Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Thank you!
>> I have looked at cpus_have_const_cap() and haven't found good way to
>> optimize it with the current infrastructure in Xen. Feel free to
>> suggest improvement.
>
> Another thing: maybe it is worth to branch to 1.0 code and leave 1.1 in
> a straight path of execution? This will save you one more instruction
> for SMCCC 1.1 call.
I am not sure to understand your suggestion here. Could you expand?
However, why SMCCC 1.1 should be in the straight path of execution?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention
2018-08-28 14:43 ` Julien Grall
@ 2018-08-28 15:10 ` Volodymyr Babchuk
2018-08-28 15:27 ` Julien Grall
0 siblings, 1 reply; 26+ messages in thread
From: Volodymyr Babchuk @ 2018-08-28 15:10 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: sstabellini
On 28.08.18 17:43, Julien Grall wrote:
[...]
>
>>> I have looked at cpus_have_const_cap() and haven't found good way to
>>> optimize it with the current infrastructure in Xen. Feel free to
>>> suggest improvement.
>>
>> Another thing: maybe it is worth to branch to 1.0 code and leave 1.1
>> in a straight path of execution? This will save you one more
>> instruction for SMCCC 1.1 call.
>
> I am not sure to understand your suggestion here. Could you expand?
+#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 )
> However, why SMCCC 1.1 should be in the straight path of execution?
It is easier to read - no negation in if(). Also, I think, branch
predictor would be happy. At least, if the following is true:
" If the branch information is not contained in the BTAC, static branch
prediction is used, whereby we assume the branch will be taken if the
branch is a conditional backwards branch or not taken if the branch is a
conditional forwards branch." [1]
[1]
http://linuxkernelhacker.blogspot.com/2014/07/branch-prediction-logic-in-arm.html
--
Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention
2018-08-28 15:10 ` Volodymyr Babchuk
@ 2018-08-28 15:27 ` Julien Grall
2018-08-28 15:50 ` Volodymyr Babchuk
0 siblings, 1 reply; 26+ messages in thread
From: Julien Grall @ 2018-08-28 15:27 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: sstabellini
Hi Volodymyr,
On 28/08/18 16:10, Volodymyr Babchuk wrote:
>
>
> On 28.08.18 17:43, Julien Grall wrote:
> [...]
>
>>
>>>> I have looked at cpus_have_const_cap() and haven't found good way to
>>>> optimize it with the current infrastructure in Xen. Feel free to
>>>> suggest improvement.
>>>
>>> Another thing: maybe it is worth to branch to 1.0 code and leave 1.1
>>> in a straight path of execution? This will save you one more
>>> instruction for SMCCC 1.1 call.
>>
>> I am not sure to understand your suggestion here. Could you expand?
>
> +#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 )
>
>
>> However, why SMCCC 1.1 should be in the straight path of execution?
>
> It is easier to read - no negation in if().
I can do that. I will also probably add an unlikely in
cpus_have_const_cap(...).
> Also, I think, branch
> predictor would be happy. At least, if the following is true:
>
> " If the branch information is not contained in the BTAC, static branch
> prediction is used, whereby we assume the branch will be taken if the
> branch is a conditional backwards branch or not taken if the branch is a
> conditional forwards branch." [1]
The Arm Arm does not provide any details on the branch predictor
implementation. An implementer is free to do whatever he wants and could
design a branch predicator doing the opposite as this statement.
You also can't assume how the compiler will compile the code, it may end
up to generate the else branch first because it is predicted to be taken
more often. This is why GCC provide __builtin_expect (commonly used as
unlikely/likely) to influence the compiler choice for branch prediction.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention
2018-08-28 15:27 ` Julien Grall
@ 2018-08-28 15:50 ` Volodymyr Babchuk
2018-08-28 15:57 ` Julien Grall
0 siblings, 1 reply; 26+ messages in thread
From: Volodymyr Babchuk @ 2018-08-28 15:50 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: sstabellini
Hi Julien,
On 28.08.18 18:27, Julien Grall wrote:
> Hi Volodymyr,
>
> On 28/08/18 16:10, Volodymyr Babchuk wrote:
>>
>>
>> On 28.08.18 17:43, Julien Grall wrote:
>> [...]
>>
>>>
>>>>> I have looked at cpus_have_const_cap() and haven't found good way
>>>>> to optimize it with the current infrastructure in Xen. Feel free to
>>>>> suggest improvement.
>>>>
>>>> Another thing: maybe it is worth to branch to 1.0 code and leave 1.1
>>>> in a straight path of execution? This will save you one more
>>>> instruction for SMCCC 1.1 call.
>>>
>>> I am not sure to understand your suggestion here. Could you expand?
>>
>> +#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 )
>>
>>
>>> However, why SMCCC 1.1 should be in the straight path of execution?
>>
>> It is easier to read - no negation in if().
>
> I can do that. I will also probably add an unlikely in
> cpus_have_const_cap(...).
That would be great.
>
>> Also, I think, branch predictor would be happy. At least, if the
>> following is true:
>>
>> " If the branch information is not contained in the BTAC, static
>> branch prediction is used, whereby we assume the branch will be taken
>> if the branch is a conditional backwards branch or not taken if the
>> branch is a conditional forwards branch." [1]
>
> The Arm Arm does not provide any details on the branch predictor
> implementation. An implementer is free to do whatever he wants and could
> design a branch predicator doing the opposite as this statement.
Generally speaking - yes. But, AFAIK, most static predictors behave in
the way described above. Anyways, as you pointed below, better to leave
hint for the compiler.
>
> You also can't assume how the compiler will compile the code, it may end
> up to generate the else branch first because it is predicted to be taken
> more often. This is why GCC provide __builtin_expect (commonly used as
> unlikely/likely) to influence the compiler choice for branch prediction.
Yes, this is the good point. So, you can add likely/unlikely not only in
cpus_have_const_cap(...) but also in #define arm_smccc_smc(...)
--
Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention
2018-08-28 15:50 ` Volodymyr Babchuk
@ 2018-08-28 15:57 ` Julien Grall
0 siblings, 0 replies; 26+ messages in thread
From: Julien Grall @ 2018-08-28 15:57 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: sstabellini
On 28/08/18 16:50, Volodymyr Babchuk wrote:
> Hi Julien,
Hi,
>
> On 28.08.18 18:27, Julien Grall wrote:
>> Hi Volodymyr,
>>
>> On 28/08/18 16:10, Volodymyr Babchuk wrote:
>>>
>>>
>>> On 28.08.18 17:43, Julien Grall wrote:
>>> [...]
>>>
>>>>
>>>>>> I have looked at cpus_have_const_cap() and haven't found good way
>>>>>> to optimize it with the current infrastructure in Xen. Feel free
>>>>>> to suggest improvement.
>>>>>
>>>>> Another thing: maybe it is worth to branch to 1.0 code and leave
>>>>> 1.1 in a straight path of execution? This will save you one more
>>>>> instruction for SMCCC 1.1 call.
>>>>
>>>> I am not sure to understand your suggestion here. Could you expand?
>>>
>>> +#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 )
>>>
>>>
>>>> However, why SMCCC 1.1 should be in the straight path of execution?
>>>
>>> It is easier to read - no negation in if().
>>
>> I can do that. I will also probably add an unlikely in
>> cpus_have_const_cap(...).
> That would be great.
>
>>
>>> Also, I think, branch predictor would be happy. At least, if the
>>> following is true:
>>>
>>> " If the branch information is not contained in the BTAC, static
>>> branch prediction is used, whereby we assume the branch will be taken
>>> if the branch is a conditional backwards branch or not taken if the
>>> branch is a conditional forwards branch." [1]
>>
>> The Arm Arm does not provide any details on the branch predictor
>> implementation. An implementer is free to do whatever he wants and
>> could design a branch predicator doing the opposite as this statement.
> Generally speaking - yes. But, AFAIK, most static predictors behave in
> the way described above. Anyways, as you pointed below, better to leave
> hint for the compiler.
Spectre would not have existed if the branch predictor was so easy ;).
>
>>
>> You also can't assume how the compiler will compile the code, it may
>> end up to generate the else branch first because it is predicted to be
>> taken more often. This is why GCC provide __builtin_expect (commonly
>> used as unlikely/likely) to influence the compiler choice for branch
>> prediction.
> Yes, this is the good point. So, you can add likely/unlikely not only in
> cpus_have_const_cap(...) but also in #define arm_smccc_smc(...)
There are no need to have likely/unlikely in arm_smccc_smc if it is
already present in cpus_have_const_cap.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/6] xen/arm: SMCCC fixup and improvement
2018-08-24 16:58 [PATCH 0/6] xen/arm: SMCCC fixup and improvement Julien Grall
` (5 preceding siblings ...)
2018-08-24 16:58 ` [PATCH 6/6] xen/arm: Replace call_smc with arm_smccc_smc Julien Grall
@ 2018-08-30 16:41 ` Volodymyr Babchuk
6 siblings, 0 replies; 26+ messages in thread
From: Volodymyr Babchuk @ 2018-08-30 16:41 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: sstabellini
Hi Julien,
On 24.08.18 19:58, Julien Grall wrote:
> Hi all,
>
> This patch series contains fixup and improvement for the SMCCC subsystem.
>
> Patch #1 - #2 are candidates for backporting.
I tested this patches together with my TEE patch series and all is
working fine both with SMCCC 1.0 and SMCCC 1.1.
So you can have my
Tested-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
for this this 6 patches.
> 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 | 97 +++++++++++++++++++++++++++++++++-------
> 11 files changed, 167 insertions(+), 56 deletions(-)
> create mode 100644 xen/arch/arm/arm64/smc.S
> delete mode 100644 xen/arch/arm/smc.S
>
--
Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/6] xen/arm: cpufeature: Add helper to check constant caps
2018-08-24 16:58 ` [PATCH 4/6] xen/arm: cpufeature: Add helper to check constant caps Julien Grall
@ 2018-08-30 17:43 ` Volodymyr Babchuk
2018-09-25 16:53 ` Julien Grall
0 siblings, 1 reply; 26+ messages in thread
From: Volodymyr Babchuk @ 2018-08-30 17:43 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: sstabellini
Hi Julien,
On 24.08.18 19:58, 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>
>
> ---
>
> This is the static key for the poor. At some point we might want to
> introduce something similar to static key in Xen.
> ---
> 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..9c297c521c 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);
> }
>
+#include <asm/alternative.h>
> +/* 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)); \
> + \
> + __ret; \
> + })
> +
> static inline void cpus_set_cap(unsigned int num)
> {
> if (num >= ARM_NCAPS)
>
--
Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention
2018-08-24 16:58 ` [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention Julien Grall
2018-08-27 14:15 ` Volodymyr Babchuk
@ 2018-08-30 17:45 ` Volodymyr Babchuk
1 sibling, 0 replies; 26+ messages in thread
From: Volodymyr Babchuk @ 2018-08-30 17:45 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: sstabellini
Hi Julien,
On 24.08.18 19:58, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
> xen/arch/arm/psci.c | 4 ++++
> xen/include/asm-arm/cpufeature.h | 3 ++-
> xen/include/asm-arm/smccc.h | 8 ++++++++
> 3 files changed, 14 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 9c297c521c..c9c4046f5f 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..7c39c530e2 100644
> --- a/xen/include/asm-arm/smccc.h
> +++ b/xen/include/asm-arm/smccc.h
+#include <asm/cpufeature.h>
> @@ -213,6 +213,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 +255,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_0_smc(__VA_ARGS__); \
> + else \
> + arm_smccc_1_1_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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/6] xen/arm: cpufeature: Add helper to check constant caps
2018-08-30 17:43 ` Volodymyr Babchuk
@ 2018-09-25 16:53 ` Julien Grall
0 siblings, 0 replies; 26+ messages in thread
From: Julien Grall @ 2018-09-25 16:53 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: sstabellini
On 30/08/18 18:43, Volodymyr Babchuk wrote:
> Hi Julien,
Hi,
> On 24.08.18 19:58, 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>
>>
>> ---
>>
>> This is the static key for the poor. At some point we might want to
>> introduce something similar to static key in Xen.
>> ---
>> 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..9c297c521c 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);
>> }
>
> +#include <asm/alternative.h>
I would rather not, alternative.h already includes cpufeature.h.
My preference would be the user of cpus_have_const_cap() to include
alternative.h.
Cheers,
>
>
>> +/* 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)); \
>> + \
>> + __ret; \
>> + })
>> +
>> static inline void cpus_set_cap(unsigned int num)
>> {
>> if (num >= ARM_NCAPS)
>>
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2018-09-25 16:53 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-24 16:58 [PATCH 0/6] xen/arm: SMCCC fixup and improvement Julien Grall
2018-08-24 16:58 ` [PATCH 1/6] xen/arm: smccc-1.1: Make return values unsigned long Julien Grall
2018-08-27 14:03 ` Volodymyr Babchuk
2018-08-27 15:23 ` Julien Grall
2018-08-27 16:33 ` Volodymyr Babchuk
2018-08-24 16:58 ` [PATCH 2/6] xen/arm: smccc-1.1: Handle function result as parameters Julien Grall
2018-08-27 14:05 ` Volodymyr Babchuk
2018-08-24 16:58 ` [PATCH 3/6] xen/arm: add SMC wrapper that is compatible with SMCCC v1.0 Julien Grall
2018-08-24 16:58 ` [PATCH 4/6] xen/arm: cpufeature: Add helper to check constant caps Julien Grall
2018-08-30 17:43 ` Volodymyr Babchuk
2018-09-25 16:53 ` Julien Grall
2018-08-24 16:58 ` [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention Julien Grall
2018-08-27 14:15 ` Volodymyr Babchuk
2018-08-27 15:29 ` Julien Grall
2018-08-27 16:50 ` Volodymyr Babchuk
2018-08-28 14:05 ` Julien Grall
2018-08-28 14:40 ` Volodymyr Babchuk
2018-08-28 14:43 ` Julien Grall
2018-08-28 15:10 ` Volodymyr Babchuk
2018-08-28 15:27 ` Julien Grall
2018-08-28 15:50 ` Volodymyr Babchuk
2018-08-28 15:57 ` Julien Grall
2018-08-30 17:45 ` Volodymyr Babchuk
2018-08-24 16:58 ` [PATCH 6/6] xen/arm: Replace call_smc with arm_smccc_smc Julien Grall
2018-08-27 13:53 ` Volodymyr Babchuk
2018-08-30 16:41 ` [PATCH 0/6] xen/arm: SMCCC fixup and improvement Volodymyr Babchuk
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.