[[PATCH v3] 0/4] xen/arm: SMCCC fixup and improvement

[[PATCH v3] 0/4] xen/arm: SMCCC fixup and improvement

[Julien Grall]

Hi all,

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

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

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      | 62 +++++++++++++++++++++++++++++++++++++++-
 11 files changed, 146 insertions(+), 42 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 v3] 1/4] 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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v3:
        - Add Stefano's reviewed-by
---
 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 v3] 2/4] 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 v3] 3/4] xen/arm: smccc: Add wrapper to automatically select the calling convention

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

---
    Changes in v3:
        - Add Stefano's and Volodymyr's reviewed-by

    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 v3] 4/4] 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>

---

    Changes in v3:
        - Use PSCI_RET where needed
---
 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..4ae6504f3e 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 PSCI_RET(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 v3] 4/4] xen/arm: Replace call_smc with arm_smccc_smc

[Andrew Cooper]

On 01/10/18 13:46, 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>
>
> ---
>
>     Changes in v3:
>         - Use PSCI_RET where needed
> ---
>  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..4ae6504f3e 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 PSCI_RET(res.a0);
>  }

Sorry if I'm jumping into the middle of a conversation, but why force
all callers to manually extract the return value when it is a fixed
register?

Wouldn't it be far easier to do this:

#define arcm_smccc_smc(...)                         \
    ({                                              \
        struct arm_smccc_res res;                   \
                                                    \
        if ( cpus_have_const_cap(ARM_SMCCC_1_1) )   \
            res = arm_smccc_1_1_smc(__VA_ARGS__);   \
        else                                        \
            res = arm_smccc_1_0_smc(__VA_ARGS__);   \
                                                    \
        (int)res.a0;                                \
    })

Which also allows the compiler to optimise out the structure if it isn't
read, and also avoids the caller needing to pass a NULL pointer for "I
don't want the result".

~Andrew

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

Re: [[PATCH v3] 4/4] xen/arm: Replace call_smc with arm_smccc_smc

[Julien Grall]

Hi,

On 10/01/2018 01:46 PM, Julien Grall wrote:
>   PLATFORM_START(seattle, "SEATTLE")
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index 941eec921b..4ae6504f3e 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 PSCI_RET(res.a0);

Hmmm this should have been PSCI_RET(res). I guess this could be fixed if 
not other change.

Cheers,

-- 
Julien Grall

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

Re: [[PATCH v3] 4/4] xen/arm: Replace call_smc with arm_smccc_smc

[Julien Grall]

Hi Andrew,

On 10/01/2018 02:11 PM, Andrew Cooper wrote:
> On 01/10/18 13:46, 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>
>>
>> ---
>>
>>      Changes in v3:
>>          - Use PSCI_RET where needed
>> ---
>>   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..4ae6504f3e 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 PSCI_RET(res.a0);
>>   }
> 
> Sorry if I'm jumping into the middle of a conversation, but why force
> all callers to manually extract the return value when it is a fixed
> register?

The SMCCC allows up to 4 return value (a0 ... a3). At the moment, most 
of the caller in Xen only care about one result value. But this will 
change soon (see OP-TEE support in Xen).

> 
> Wouldn't it be far easier to do this:
> 
> #define arcm_smccc_smc(...)                         \
>      ({                                              \
>          struct arm_smccc_res res;                   \
>                                                      \
>          if ( cpus_have_const_cap(ARM_SMCCC_1_1) )   \
>              res = arm_smccc_1_1_smc(__VA_ARGS__);   \
>          else                                        \
>              res = arm_smccc_1_0_smc(__VA_ARGS__);   \
>                                                      \
>          (int)res.a0;                                \

We can't possibly cast here. The interpretation of a0 is different from 
one call to another.

>      })
> 
> Which also allows the compiler to optimise out the structure if it isn't
> read, and also avoids the caller needing to pass a NULL pointer for "I
> don't want the result".

While I understand the value, I don't think this would be correct if we 
want to implement an interface with the SMCCC.

Cheers,

-- 
Julien Grall

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

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

[Stefano Stabellini]

On Mon, 1 Oct 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>

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

> ---
> 
> 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 v3] 4/4] xen/arm: Replace call_smc with arm_smccc_smc

[Stefano Stabellini]

On Mon, 1 Oct 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>

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

(you might want to make sure the double [[ is removed on commit.)

> ---
> 
>     Changes in v3:
>         - Use PSCI_RET where needed
> ---
>  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..4ae6504f3e 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 PSCI_RET(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 v3] 4/4] xen/arm: Replace call_smc with arm_smccc_smc

[Stefano Stabellini]

On Mon, 1 Oct 2018, Stefano Stabellini wrote:
> On Mon, 1 Oct 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>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> (you might want to make sure the double [[ is removed on commit.)

Hey Julien,

I was going to commit the series, but I am getting a build error:

psci.c: In function 'call_psci_cpu_on':
psci.c:45:40: error: request for member 'a0' in something not a structure or union
 #define PSCI_RET(res)   ((int32_t)(res).a0)
                                        ^
psci.c:54:12: note: in expansion of macro 'PSCI_RET'
     return PSCI_RET(res.a0);
            ^
psci.c:55:1: error: control reaches end of non-void function [-Werror=return-type]
 }



> > ---
> > 
> >     Changes in v3:
> >         - Use PSCI_RET where needed
> > ---
> >  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..4ae6504f3e 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 PSCI_RET(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 v3] 4/4] xen/arm: Replace call_smc with arm_smccc_smc

[Julien Grall]

Hi Stefano,

On 10/01/2018 09:52 PM, Stefano Stabellini wrote:
> On Mon, 1 Oct 2018, Stefano Stabellini wrote:
>> On Mon, 1 Oct 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>
>>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>> (you might want to make sure the double [[ is removed on commit.)
> 
> Hey Julien,
> 
> I was going to commit the series, but I am getting a build error:
> 
> psci.c: In function 'call_psci_cpu_on':
> psci.c:45:40: error: request for member 'a0' in something not a structure or union
>   #define PSCI_RET(res)   ((int32_t)(res).a0)
>                                          ^
> psci.c:54:12: note: in expansion of macro 'PSCI_RET'
>       return PSCI_RET(res.a0);
>              ^
> psci.c:55:1: error: control reaches end of non-void function [-Werror=return-type]
>   }

I already mentioned this error in reply to the patch [1] and suggested 
it could be fixed on commit...

Cheers,

[1] <9e0160ad-853c-6efa-1a04-15f801ef06da@arm.com>

-- 
Julien Grall

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