* [PATCH 0/5] xen/arm: Fixes and improvements to SMCCC
@ 2026-08-31 12:19 Andrew Cooper
2026-08-31 12:19 ` [PATCH 1/6] xen/arm: Fix evaluation of parameters for SMCCC calls Andrew Cooper
` (5 more replies)
0 siblings, 6 replies; 15+ messages in thread
From: Andrew Cooper @ 2026-08-31 12:19 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Bertrand Marquis, Michal Orzel,
Jan Setje-Eilers
Patch 1 is a bugfix for the issue reported by Jan Setje-Eilers. It needs
backporting to Xen 4.22.
Everything else is because I couldn't bear to leave the code generation in
such a bad state.
https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/2805409790
Andrew Cooper (5):
xen/arm: Fix evaluation of parameters for SMCCC calls
xen/arm: Introduce arm_smccc_guest_smc()
xen/arm: Clean up 32bit arm_smccc_1_1_smc()
xen/arm: Rewrite arm_smccc_smc() for arm64
xen/arm: Rewrite arm_smccc_*() to return by value
xen/arch/arm/arm64/smc.S | 16 --
xen/arch/arm/cpuerrata.c | 18 +-
xen/arch/arm/firmware/scmi-smc.c | 16 +-
xen/arch/arm/include/asm/smccc.h | 241 +++++++++++---------
xen/arch/arm/platforms/exynos5.c | 2 +-
xen/arch/arm/platforms/imx8m.c | 16 +-
xen/arch/arm/platforms/imx8qm.c | 16 +-
xen/arch/arm/platforms/seattle.c | 4 +-
xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 17 +-
xen/arch/arm/psci.c | 17 +-
xen/arch/arm/traps.c | 4 +-
11 files changed, 165 insertions(+), 202 deletions(-)
--
2.39.5
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/6] xen/arm: Fix evaluation of parameters for SMCCC calls
2026-08-31 12:19 [PATCH 0/5] xen/arm: Fixes and improvements to SMCCC Andrew Cooper
@ 2026-08-31 12:19 ` Andrew Cooper
2026-09-03 11:46 ` Bertrand Marquis
2026-08-31 12:19 ` [PATCH 2/6] xen/arm: Introduce arm_smccc_guest_smc() Andrew Cooper
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2026-08-31 12:19 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Jan Setje-Eilers, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Bertrand Marquis, Michal Orzel
Contrary to what was claimed in commit 67bcf5eae709 ("xen/arm: Simplify type
handling for SMCCC declarations"), there is an important reason to retain the
intermediate variable. It is unsafe to have any logic between the assignment
of the register variabes and the asm() block they're used in.
This logically reverts commit 67bcf5eae709 ("xen/arm: Simplify type handling
for SMCCC declarations") while retaining the conversions from commit
7f15d5d13221 ("xen/treewide: More typeof() -> auto conversions").
Adjust __declare_arg_0() to match. It happens to be safe because it's the
first register expression once all macros are expanded, but it really should
be consistent with the others.
Leave a comment explaining why they must be written like this.
Fixes: 67bcf5eae709 ("xen/arm: Simplify type handling for SMCCC declarations")
Reported-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
---
xen/arch/arm/include/asm/smccc.h | 31 +++++++++++++++++++++++--------
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/xen/arch/arm/include/asm/smccc.h b/xen/arch/arm/include/asm/smccc.h
index 62c6985e7315..53cdddb690b7 100644
--- a/xen/arch/arm/include/asm/smccc.h
+++ b/xen/arch/arm/include/asm/smccc.h
@@ -108,37 +108,52 @@ struct arm_smccc_res {
#define __constraint_read_6 __constraint_read_5, "r" (arg6)
#define __constraint_read_7 __constraint_read_6, "r" (arg7)
+/*
+ * Macro arguments MUST be evaluated before being assigned to a register
+ * variable.
+ *
+ * This is manual register scheduling for the asm() statement, and any other
+ * logic to evaluate may clobber the already-scheduled registers.
+ */
#define __declare_arg_0(a0, res) \
+ auto __a0 = (uint32_t)(a0); \
struct arm_smccc_res *___res = (res); \
- register unsigned long arg0 ASM_REG(0) = (uint32_t)(a0)
+ register unsigned long arg0 ASM_REG(0) = __a0
#define __declare_arg_1(a0, a1, res) \
+ auto __a1 = (a1); \
__declare_arg_0(a0, res); \
- register auto arg1 ASM_REG(1) = (a1)
+ register auto arg1 ASM_REG(1) = __a1
#define __declare_arg_2(a0, a1, a2, res) \
+ auto __a2 = (a2); \
__declare_arg_1(a0, a1, res); \
- register auto arg2 ASM_REG(2) = (a2)
+ register auto arg2 ASM_REG(2) = __a2
#define __declare_arg_3(a0, a1, a2, a3, res) \
+ auto __a3 = (a3); \
__declare_arg_2(a0, a1, a2, res); \
- register auto arg3 ASM_REG(3) = (a3)
+ register auto arg3 ASM_REG(3) = __a3
#define __declare_arg_4(a0, a1, a2, a3, a4, res) \
+ auto __a4 = (a4); \
__declare_arg_3(a0, a1, a2, a3, res); \
- register auto arg4 ASM_REG(4) = (a4)
+ register auto arg4 ASM_REG(4) = __a4
#define __declare_arg_5(a0, a1, a2, a3, a4, a5, res) \
+ auto __a5 = (a5); \
__declare_arg_4(a0, a1, a2, a3, a4, res); \
- register auto arg5 ASM_REG(5) = (a5)
+ register auto arg5 ASM_REG(5) = __a5
#define __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res) \
+ auto __a6 = (a6); \
__declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
- register auto arg6 ASM_REG(6) = (a6)
+ register auto arg6 ASM_REG(6) = __a6
#define __declare_arg_7(a0, a1, a2, a3, a4, a5, a6, a7, res) \
+ auto __a7 = (a7); \
__declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
- register auto arg7 ASM_REG(7) = (a7)
+ register auto arg7 ASM_REG(7) = __a7
#define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
#define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
base-commit: 79225a0c77e13b693b4d2b903a88289704b79db6
--
2.39.5
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/6] xen/arm: Introduce arm_smccc_guest_smc()
2026-08-31 12:19 [PATCH 0/5] xen/arm: Fixes and improvements to SMCCC Andrew Cooper
2026-08-31 12:19 ` [PATCH 1/6] xen/arm: Fix evaluation of parameters for SMCCC calls Andrew Cooper
@ 2026-08-31 12:19 ` Andrew Cooper
2026-09-03 11:49 ` Bertrand Marquis
2026-08-31 12:19 ` [PATCH 3/6] xen/arm: Clean up 32bit arm_smccc_1_1_smc() Andrew Cooper
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2026-08-31 12:19 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Bertrand Marquis, Michal Orzel,
Jan Setje-Eilers
Both {get,set}_user_reg() are out-of-line functions, leading to awful code
generation.
Introduce arm_smccc_guest_smc() to operate directly on guest registers.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
For arm64:
add/remove: 0/0 grow/shrink: 2/4 up/down: 27/-864 (-837)
Function old new delta
symbols_addresses 35096 35120 +24
symbols_names 42958 42961 +3
imx8qm_smc 544 348 -196
scmi_handle_smc 372 152 -220
imx8m_smc 576 356 -220
zynqmp_eemi 864 636 -228
For arm32:
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-160 (-160)
Function old new delta
scmi_handle_smc 392 232 -160
---
xen/arch/arm/firmware/scmi-smc.c | 16 +-----------
xen/arch/arm/include/asm/smccc.h | 29 +++++++++++++++++++++
xen/arch/arm/platforms/imx8m.c | 16 +-----------
xen/arch/arm/platforms/imx8qm.c | 16 +-----------
xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 17 ++----------
5 files changed, 34 insertions(+), 60 deletions(-)
diff --git a/xen/arch/arm/firmware/scmi-smc.c b/xen/arch/arm/firmware/scmi-smc.c
index 0835ddeeeccc..a0cc6c6192f8 100644
--- a/xen/arch/arm/firmware/scmi-smc.c
+++ b/xen/arch/arm/firmware/scmi-smc.c
@@ -50,7 +50,6 @@ static bool scmi_is_valid_smc_id(uint32_t fid)
static bool scmi_handle_smc(struct cpu_user_regs *regs)
{
uint32_t fid = (uint32_t)get_user_reg(regs, 0);
- struct arm_smccc_res res;
if ( !scmi_is_valid_smc_id(fid) )
return false;
@@ -63,20 +62,7 @@ static bool scmi_handle_smc(struct cpu_user_regs *regs)
}
/* For the moment, forward the SCMI Request to FW running at EL3 */
- arm_smccc_1_1_smc(fid,
- get_user_reg(regs, 1),
- get_user_reg(regs, 2),
- get_user_reg(regs, 3),
- get_user_reg(regs, 4),
- get_user_reg(regs, 5),
- get_user_reg(regs, 6),
- get_user_reg(regs, 7),
- &res);
-
- set_user_reg(regs, 0, res.a0);
- set_user_reg(regs, 1, res.a1);
- set_user_reg(regs, 2, res.a2);
- set_user_reg(regs, 3, res.a3);
+ arm_smccc_guest_smc(regs);
return true;
}
diff --git a/xen/arch/arm/include/asm/smccc.h b/xen/arch/arm/include/asm/smccc.h
index 53cdddb690b7..832157f43734 100644
--- a/xen/arch/arm/include/asm/smccc.h
+++ b/xen/arch/arm/include/asm/smccc.h
@@ -202,6 +202,21 @@ 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__)
+
+/* Make an SMCCC v1.1 compliant SMC call with guest register state. */
+static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
+{
+ struct arm_smccc_res res;
+
+ arm_smccc_1_1_smc(regs->r0, regs->r1, regs->r2, regs->r3,
+ regs->r4, regs->r5, regs->r6, regs->r7, &res);
+
+ regs->r0 = res.a0;
+ regs->r1 = res.a1;
+ regs->r2 = res.a2;
+ regs->r3 = res.a3;
+}
+
#else
void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
@@ -251,6 +266,20 @@ void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
arm_smccc_1_0_smc(__VA_ARGS__); \
} while ( 0 )
+/* Make an SMCCC v1.1 compliant SMC call with guest register state. */
+static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
+{
+ struct arm_smccc_res res;
+
+ arm_smccc_1_1_smc(regs->x0, regs->x1, regs->x2, regs->x3,
+ regs->x4, regs->x5, regs->x6, regs->x7, &res);
+
+ regs->x0 = res.a0;
+ regs->x1 = res.a1;
+ regs->x2 = res.a2;
+ regs->x3 = res.a3;
+}
+
/*
* struct arm_smccc_1_2_regs - Arguments for or Results from SMC call
* @a0-a17 argument values from registers 0 to 17
diff --git a/xen/arch/arm/platforms/imx8m.c b/xen/arch/arm/platforms/imx8m.c
index 669dd517e057..efb0ad20d6e8 100644
--- a/xen/arch/arm/platforms/imx8m.c
+++ b/xen/arch/arm/platforms/imx8m.c
@@ -50,7 +50,6 @@ static bool imx8m_smc(struct cpu_user_regs *regs)
{
uint32_t function_id = get_user_reg(regs, 0);
uint32_t subfunction_id = get_user_reg(regs, 1);
- struct arm_smccc_res res;
if ( !cpus_have_const_cap(ARM_SMCCC_1_1) )
{
@@ -122,20 +121,7 @@ static bool imx8m_smc(struct cpu_user_regs *regs)
return false;
}
- arm_smccc_1_1_smc(function_id,
- subfunction_id,
- get_user_reg(regs, 2),
- get_user_reg(regs, 3),
- get_user_reg(regs, 4),
- get_user_reg(regs, 5),
- get_user_reg(regs, 6),
- get_user_reg(regs, 7),
- &res);
-
- set_user_reg(regs, 0, res.a0);
- set_user_reg(regs, 1, res.a1);
- set_user_reg(regs, 2, res.a2);
- set_user_reg(regs, 3, res.a3);
+ arm_smccc_guest_smc(regs);
return true;
}
diff --git a/xen/arch/arm/platforms/imx8qm.c b/xen/arch/arm/platforms/imx8qm.c
index 3600a073e8ba..7249e14ab640 100644
--- a/xen/arch/arm/platforms/imx8qm.c
+++ b/xen/arch/arm/platforms/imx8qm.c
@@ -67,7 +67,6 @@ static bool imx8qm_smc(struct cpu_user_regs *regs)
{
uint32_t function_id = get_user_reg(regs, 0);
uint32_t subfunction_id = get_user_reg(regs, 1);
- struct arm_smccc_res res;
if ( !cpus_have_const_cap(ARM_SMCCC_1_1) )
{
@@ -106,20 +105,7 @@ static bool imx8qm_smc(struct cpu_user_regs *regs)
}
allow_call:
- arm_smccc_1_1_smc(function_id,
- subfunction_id,
- get_user_reg(regs, 2),
- get_user_reg(regs, 3),
- get_user_reg(regs, 4),
- get_user_reg(regs, 5),
- get_user_reg(regs, 6),
- get_user_reg(regs, 7),
- &res);
-
- set_user_reg(regs, 0, res.a0);
- set_user_reg(regs, 1, res.a1);
- set_user_reg(regs, 2, res.a2);
- set_user_reg(regs, 3, res.a3);
+ arm_smccc_guest_smc(regs);
return true;
}
diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
index 2053ed7ac5f6..326c8a1ba6e5 100644
--- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
+++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
@@ -51,7 +51,6 @@ static inline bool domain_has_reset_access(struct domain *d, uint32_t rst)
bool zynqmp_eemi(struct cpu_user_regs *regs)
{
- struct arm_smccc_res res;
uint32_t fid = get_user_reg(regs, 0);
uint32_t nodeid = get_user_reg(regs, 1);
unsigned int pm_fn = fid & 0xFFFF;
@@ -187,20 +186,8 @@ bool zynqmp_eemi(struct cpu_user_regs *regs)
* can forward the whole command to firmware without additional
* parameters checks.
*/
- arm_smccc_1_1_smc(get_user_reg(regs, 0),
- get_user_reg(regs, 1),
- get_user_reg(regs, 2),
- get_user_reg(regs, 3),
- get_user_reg(regs, 4),
- get_user_reg(regs, 5),
- get_user_reg(regs, 6),
- get_user_reg(regs, 7),
- &res);
-
- set_user_reg(regs, 0, res.a0);
- set_user_reg(regs, 1, res.a1);
- set_user_reg(regs, 2, res.a2);
- set_user_reg(regs, 3, res.a3);
+ arm_smccc_guest_smc(regs);
+
return true;
done:
--
2.39.5
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/6] xen/arm: Clean up 32bit arm_smccc_1_1_smc()
2026-08-31 12:19 [PATCH 0/5] xen/arm: Fixes and improvements to SMCCC Andrew Cooper
2026-08-31 12:19 ` [PATCH 1/6] xen/arm: Fix evaluation of parameters for SMCCC calls Andrew Cooper
2026-08-31 12:19 ` [PATCH 2/6] xen/arm: Introduce arm_smccc_guest_smc() Andrew Cooper
@ 2026-08-31 12:19 ` Andrew Cooper
2026-09-03 11:52 ` Bertrand Marquis
2026-08-31 12:19 ` [PATCH 4/6] xen/arm: Rewrite arm_smccc_smc() for arm64 Andrew Cooper
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2026-08-31 12:19 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Bertrand Marquis, Michal Orzel,
Jan Setje-Eilers
... before making a related copy of it.
* Drop __constraints() so the output parameters are visible in the same block
as they're defined. Use PASTE() rather than opencoding it.
* Adust the indentation of trailing \'s for consistency.
* Drop the newline at the end of the instruction.
* Indent the if condition correctly. ___res is always of type
arm_smccc_res (declared in __declare_arg_0()), so drop the typeof().
* Drop arm_smccc_1_0_smc() as it has no users.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
---
xen/arch/arm/include/asm/smccc.h | 45 ++++++++++++++++----------------
1 file changed, 22 insertions(+), 23 deletions(-)
diff --git a/xen/arch/arm/include/asm/smccc.h b/xen/arch/arm/include/asm/smccc.h
index 832157f43734..5fe54013ac83 100644
--- a/xen/arch/arm/include/asm/smccc.h
+++ b/xen/arch/arm/include/asm/smccc.h
@@ -56,6 +56,8 @@
#ifndef __ASSEMBLER__
+#include <xen/macros.h>
+
extern uint32_t smccc_ver;
/* Check if this is fast call. */
@@ -115,24 +117,24 @@ struct arm_smccc_res {
* This is manual register scheduling for the asm() statement, and any other
* logic to evaluate may clobber the already-scheduled registers.
*/
-#define __declare_arg_0(a0, res) \
- auto __a0 = (uint32_t)(a0); \
- struct arm_smccc_res *___res = (res); \
+#define __declare_arg_0(a0, res) \
+ auto __a0 = (uint32_t)(a0); \
+ struct arm_smccc_res *___res = (res); \
register unsigned long arg0 ASM_REG(0) = __a0
-#define __declare_arg_1(a0, a1, res) \
- auto __a1 = (a1); \
- __declare_arg_0(a0, res); \
+#define __declare_arg_1(a0, a1, res) \
+ auto __a1 = (a1); \
+ __declare_arg_0(a0, res); \
register auto arg1 ASM_REG(1) = __a1
-#define __declare_arg_2(a0, a1, a2, res) \
- auto __a2 = (a2); \
- __declare_arg_1(a0, a1, res); \
+#define __declare_arg_2(a0, a1, a2, res) \
+ auto __a2 = (a2); \
+ __declare_arg_1(a0, a1, res); \
register auto arg2 ASM_REG(2) = __a2
-#define __declare_arg_3(a0, a1, a2, a3, res) \
- auto __a3 = (a3); \
- __declare_arg_2(a0, a1, a2, res); \
+#define __declare_arg_3(a0, a1, a2, a3, res) \
+ auto __a3 = (a3); \
+ __declare_arg_2(a0, a1, a2, res); \
register auto arg3 ASM_REG(3) = __a3
#define __declare_arg_4(a0, a1, a2, a3, a4, res) \
@@ -158,12 +160,6 @@ struct arm_smccc_res {
#define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
#define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
-#define ___constraints(count) \
- : "=r" (r0), "=r" (r1), "=r" (r2), "=r" (r3) \
- : __constraint_read_ ## count \
- : "memory"
-#define __constraints(count) ___constraints(count)
-
/*
* arm_smccc_1_1_smc() - make an SMCCC v1.1 compliant SMC call
*
@@ -189,10 +185,14 @@ struct arm_smccc_res {
register unsigned long r2 ASM_REG(2); \
register unsigned long r3 ASM_REG(3); \
__declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
- asm volatile("smc #0\n" \
- __constraints(__count_args(__VA_ARGS__))); \
+ asm volatile ( \
+ "smc #0" \
+ : "=r" (r0), "=r" (r1), "=r" (r2), "=r" (r3) \
+ : PASTE(__constraint_read_, \
+ __count_args(__VA_ARGS__)) \
+ : "memory" ); \
if ( ___res ) \
- *___res = (typeof(*___res)){r0, r1, r2, r3}; \
+ *___res = (struct arm_smccc_res){ r0, r1, r2, r3 }; \
} while ( 0 )
/*
@@ -200,7 +200,6 @@ struct arm_smccc_res {
* v1.1.
*/
#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__)
/* Make an SMCCC v1.1 compliant SMC call with guest register state. */
@@ -217,7 +216,7 @@ static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
regs->r3 = res.a3;
}
-#else
+#else /* CONFIG_ARM_64 */
void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
register_t a3, register_t a4, register_t a5,
--
2.39.5
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 4/6] xen/arm: Rewrite arm_smccc_smc() for arm64
2026-08-31 12:19 [PATCH 0/5] xen/arm: Fixes and improvements to SMCCC Andrew Cooper
` (2 preceding siblings ...)
2026-08-31 12:19 ` [PATCH 3/6] xen/arm: Clean up 32bit arm_smccc_1_1_smc() Andrew Cooper
@ 2026-08-31 12:19 ` Andrew Cooper
2026-09-03 12:16 ` Bertrand Marquis
2026-08-31 12:19 ` [PATCH 5/6] xen/arm: Rewrite arm_smccc_*() to return by value Andrew Cooper
2026-09-03 9:20 ` [PATCH 0/5] xen/arm: Fixes and improvements to SMCCC Bertrand Marquis
5 siblings, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2026-08-31 12:19 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Bertrand Marquis, Michal Orzel,
Jan Setje-Eilers
PSCI v1.0 says that x4 thru x17 may be clobbered. PSCI v1.1 says they are
strictly preserved unless they contain return values.
Xen deals with this by having __arm_smccc_1_0_smc() as an out-of-line
function, but this causes awful code generation in arm_smccc_smc().
cpus_have_const_cap() is opaque to the optimiser, so we end up with one basic
block doing the reasonably-ok arm_smccc_1_1_smc() code generation and a second
basic block setting up all 8 input registers even when they're not needed,
spilling or discarding x8 thru x17, and calling an out-of-line function.
Remove __arm_smccc_1_0_smc() entirely, and rewrite arm_smccc_smc() to declare
x4 thru x17 as clobbered.
This fully inlines the SMC, is a single basic block which instructs the
compiler to spill or discard the potentially clobbered registers, and only
sets up the necessary number of arguments for the call.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
Bloat-o-meter reports:
add/remove: 0/1 grow/shrink: 0/10 up/down: 0/-795 (-795)
Function old new delta
symbols_sorted_offsets 23832 23824 -8
symbols_names 42962 42943 -19
symbols_addresses 35128 35104 -24
__arm_smccc_1_0_smc 32 - -32
call_psci_cpu_off 144 76 -68
seattle_system_reset 92 16 -76
seattle_system_off 92 16 -76
call_psci_system_reset 112 32 -80
call_psci_system_off 112 32 -80
call_psci_cpu_on 252 124 -128
psci_init 628 424 -204
An alternative way to do this would be to have x8 thru x17 in the clobber list
rather than the output list which would reduce the source size, but this form
is more amenable to having PSCI v1.2 worked into it too.
---
xen/arch/arm/arm64/smc.S | 16 ------
xen/arch/arm/include/asm/smccc.h | 95 ++++++++++++++++----------------
2 files changed, 48 insertions(+), 63 deletions(-)
diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S
index 68b05e8ddd12..65b4eabe4f87 100644
--- a/xen/arch/arm/arm64/smc.S
+++ b/xen/arch/arm/arm64/smc.S
@@ -13,22 +13,6 @@
* GNU General Public License for more details.
*/
-/*
- * 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)
- */
-FUNC(__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
-END(__arm_smccc_1_0_smc)
-
/*
* void arm_smccc_1_2_smc(const struct arm_smccc_1_2_regs *args,
* struct arm_smccc_1_2_regs *res)
diff --git a/xen/arch/arm/include/asm/smccc.h b/xen/arch/arm/include/asm/smccc.h
index 5fe54013ac83..8920c54b09a6 100644
--- a/xen/arch/arm/include/asm/smccc.h
+++ b/xen/arch/arm/include/asm/smccc.h
@@ -16,9 +16,6 @@
#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)
@@ -57,6 +54,9 @@
#ifndef __ASSEMBLER__
#include <xen/macros.h>
+#include <xen/types.h>
+
+#include <asm/asm_defns.h>
extern uint32_t smccc_ver;
@@ -160,6 +160,8 @@ struct arm_smccc_res {
#define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
#define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
+#ifdef CONFIG_ARM_32
+
/*
* arm_smccc_1_1_smc() - make an SMCCC v1.1 compliant SMC call
*
@@ -199,7 +201,6 @@ struct arm_smccc_res {
* The calling convention for arm32 is the same for both SMCCC v1.0 and
* v1.1.
*/
-#ifdef CONFIG_ARM_32
#define arm_smccc_smc(...) arm_smccc_1_1_smc(__VA_ARGS__)
/* Make an SMCCC v1.1 compliant SMC call with guest register state. */
@@ -218,53 +219,53 @@ static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
#else /* CONFIG_ARM_64 */
-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__)
-
+/*
+ * Make an SMCCC call compatible with both PSCI v1.1 and v1.0.
+ *
+ * PSCI v1.1 says that x4 through x17 are strictly preserved unless they
+ * contain return data. PSCI v1.0 says they clobbered.
+ *
+ * Xen doesn't make PSCI v1.1 calls which expect more than 4 return registers,
+ * so imply list x4 through x17 as clobbered.
+ */
#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__); \
+ register unsigned long r0 ASM_REG(0); \
+ register unsigned long r1 ASM_REG(1); \
+ register unsigned long r2 ASM_REG(2); \
+ register unsigned long r3 ASM_REG(3); \
+ /* Potentially clobbered in PSCI 1.0 */ \
+ register unsigned long c4 ASM_REG(4); \
+ register unsigned long c5 ASM_REG(5); \
+ register unsigned long c6 ASM_REG(6); \
+ register unsigned long c7 ASM_REG(7); \
+ register unsigned long c8 ASM_REG(8); \
+ register unsigned long c9 ASM_REG(9); \
+ register unsigned long c10 ASM_REG(10); \
+ register unsigned long c11 ASM_REG(11); \
+ register unsigned long c12 ASM_REG(12); \
+ register unsigned long c13 ASM_REG(13); \
+ register unsigned long c14 ASM_REG(14); \
+ register unsigned long c15 ASM_REG(15); \
+ register unsigned long c16 ASM_REG(16); \
+ register unsigned long c17 ASM_REG(17); \
+ __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
+ asm volatile ( \
+ "smc #0" \
+ : "=r" (r0), "=r" (r1), "=r" (r2), "=r" (r3), \
+ "=r" (c4), "=r" (c5), "=r" (c6), "=r" (c7), \
+ "=r" (c8), "=r" (c9), "=r" (c10), "=r" (c11), \
+ "=r" (c12), "=r" (c13), "=r" (c14), "=r" (c15), \
+ "=r" (c16), "=r" (c17) \
+ : PASTE(__constraint_read_, \
+ __count_args(__VA_ARGS__)) \
+ : "memory" ); \
+ if ( ___res ) \
+ *___res = (struct arm_smccc_res){ r0, r1, r2, r3 }; \
} while ( 0 )
+#define arm_smccc_1_1_smc(...) arm_smccc_smc(__VA_ARGS__)
+
/* Make an SMCCC v1.1 compliant SMC call with guest register state. */
static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
{
--
2.39.5
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 5/6] xen/arm: Rewrite arm_smccc_*() to return by value
2026-08-31 12:19 [PATCH 0/5] xen/arm: Fixes and improvements to SMCCC Andrew Cooper
` (3 preceding siblings ...)
2026-08-31 12:19 ` [PATCH 4/6] xen/arm: Rewrite arm_smccc_smc() for arm64 Andrew Cooper
@ 2026-08-31 12:19 ` Andrew Cooper
2026-09-03 12:21 ` Bertrand Marquis
2026-09-03 9:20 ` [PATCH 0/5] xen/arm: Fixes and improvements to SMCCC Bertrand Marquis
5 siblings, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2026-08-31 12:19 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Bertrand Marquis, Michal Orzel,
Jan Setje-Eilers
Use statement expressions to return struct arm_smccc_res which makes the code
read a lot more normally, and avoids needing to pass in NULL in order to skip
return information.
More importantly, it removes the local implementation of __count_args() which
is off by two and deeply confusing to try and follow.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
Xen compiles identically before and after this change, for both arm32 and arm64.
---
xen/arch/arm/cpuerrata.c | 18 +++----
xen/arch/arm/include/asm/smccc.h | 87 ++++++++++++++------------------
xen/arch/arm/platforms/exynos5.c | 2 +-
xen/arch/arm/platforms/seattle.c | 4 +-
xen/arch/arm/psci.c | 17 +++----
xen/arch/arm/tee/optee.c | 47 +++++++++--------
xen/arch/arm/traps.c | 4 +-
7 files changed, 84 insertions(+), 95 deletions(-)
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index 3a32183618dc..35ad98d29d14 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -179,8 +179,8 @@ static int enable_smccc_arch_workaround_1(void *data)
if ( smccc_ver < SMCCC_VERSION(1, 1) )
goto warn;
- arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID,
- ARM_SMCCC_ARCH_WORKAROUND_1_FID, &res);
+ res = arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID,
+ ARM_SMCCC_ARCH_WORKAROUND_1_FID);
/* The return value is in the lower 32-bits. */
if ( (int)res.a0 < 0 )
goto warn;
@@ -256,8 +256,8 @@ static int enable_spectre_bhb_workaround(void *data)
if ( smccc_ver < SMCCC_VERSION(1, 1) )
goto warn;
- arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID,
- ARM_SMCCC_ARCH_WORKAROUND_3_FID, &res);
+ res = arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID,
+ ARM_SMCCC_ARCH_WORKAROUND_3_FID);
/* The return value is in the lower 32-bits. */
if ( (int)res.a0 < 0 )
{
@@ -398,8 +398,8 @@ static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry)
if ( smccc_ver < SMCCC_VERSION(1, 1) )
return false;
- arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID,
- ARM_SMCCC_ARCH_WORKAROUND_2_FID, &res);
+ res = arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID,
+ ARM_SMCCC_ARCH_WORKAROUND_2_FID);
switch ( (int)res.a0 )
{
@@ -429,7 +429,7 @@ static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry)
case ARM_SSBD_FORCE_DISABLE:
printk_once("%s disabled from command-line\n", entry->desc);
- arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 0, NULL);
+ arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 0);
required = false;
break;
@@ -437,7 +437,7 @@ static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry)
if ( required )
{
this_cpu(ssbd_callback_required) = 1;
- arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL);
+ arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1);
}
break;
@@ -445,7 +445,7 @@ static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry)
case ARM_SSBD_FORCE_ENABLE:
printk_once("%s forced from command-line\n", entry->desc);
- arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL);
+ arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1);
required = true;
break;
diff --git a/xen/arch/arm/include/asm/smccc.h b/xen/arch/arm/include/asm/smccc.h
index 8920c54b09a6..ebed2ff7c7d2 100644
--- a/xen/arch/arm/include/asm/smccc.h
+++ b/xen/arch/arm/include/asm/smccc.h
@@ -95,20 +95,14 @@ struct arm_smccc_res {
unsigned long a3;
};
-/* SMCCC v1.1 implementation madness follows */
-#define ___count_args(_0, _1, _2, _3, _4, _5, _6, _7, _8, x, ...) x
-
-#define __count_args(...) \
- ___count_args(__VA_ARGS__, 7, 6, 5, 4, 3, 2, 1, 0)
-
-#define __constraint_read_0 "r" (arg0)
-#define __constraint_read_1 __constraint_read_0, "r" (arg1)
-#define __constraint_read_2 __constraint_read_1, "r" (arg2)
-#define __constraint_read_3 __constraint_read_2, "r" (arg3)
-#define __constraint_read_4 __constraint_read_3, "r" (arg4)
-#define __constraint_read_5 __constraint_read_4, "r" (arg5)
-#define __constraint_read_6 __constraint_read_5, "r" (arg6)
-#define __constraint_read_7 __constraint_read_6, "r" (arg7)
+#define __constraint_read_1 "r" (arg0)
+#define __constraint_read_2 __constraint_read_1, "r" (arg1)
+#define __constraint_read_3 __constraint_read_2, "r" (arg2)
+#define __constraint_read_4 __constraint_read_3, "r" (arg3)
+#define __constraint_read_5 __constraint_read_4, "r" (arg4)
+#define __constraint_read_6 __constraint_read_5, "r" (arg5)
+#define __constraint_read_7 __constraint_read_6, "r" (arg6)
+#define __constraint_read_8 __constraint_read_7, "r" (arg7)
/*
* Macro arguments MUST be evaluated before being assigned to a register
@@ -117,44 +111,43 @@ struct arm_smccc_res {
* This is manual register scheduling for the asm() statement, and any other
* logic to evaluate may clobber the already-scheduled registers.
*/
-#define __declare_arg_0(a0, res) \
+#define __declare_arg_1(a0) \
auto __a0 = (uint32_t)(a0); \
- struct arm_smccc_res *___res = (res); \
register unsigned long arg0 ASM_REG(0) = __a0
-#define __declare_arg_1(a0, a1, res) \
+#define __declare_arg_2(a0, a1) \
auto __a1 = (a1); \
- __declare_arg_0(a0, res); \
+ __declare_arg_1(a0); \
register auto arg1 ASM_REG(1) = __a1
-#define __declare_arg_2(a0, a1, a2, res) \
+#define __declare_arg_3(a0, a1, a2) \
auto __a2 = (a2); \
- __declare_arg_1(a0, a1, res); \
+ __declare_arg_2(a0, a1); \
register auto arg2 ASM_REG(2) = __a2
-#define __declare_arg_3(a0, a1, a2, a3, res) \
+#define __declare_arg_4(a0, a1, a2, a3) \
auto __a3 = (a3); \
- __declare_arg_2(a0, a1, a2, res); \
+ __declare_arg_3(a0, a1, a2); \
register auto arg3 ASM_REG(3) = __a3
-#define __declare_arg_4(a0, a1, a2, a3, a4, res) \
+#define __declare_arg_5(a0, a1, a2, a3, a4) \
auto __a4 = (a4); \
- __declare_arg_3(a0, a1, a2, a3, res); \
+ __declare_arg_4(a0, a1, a2, a3); \
register auto arg4 ASM_REG(4) = __a4
-#define __declare_arg_5(a0, a1, a2, a3, a4, a5, res) \
+#define __declare_arg_6(a0, a1, a2, a3, a4, a5) \
auto __a5 = (a5); \
- __declare_arg_4(a0, a1, a2, a3, a4, res); \
+ __declare_arg_5(a0, a1, a2, a3, a4); \
register auto arg5 ASM_REG(5) = __a5
-#define __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res) \
- auto __a6 = (a6); \
- __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
+#define __declare_arg_7(a0, a1, a2, a3, a4, a5, a6) \
+ auto __a6 = (a6); \
+ __declare_arg_6(a0, a1, a2, a3, a4, a5); \
register auto arg6 ASM_REG(6) = __a6
-#define __declare_arg_7(a0, a1, a2, a3, a4, a5, a6, a7, res) \
- auto __a7 = (a7); \
- __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
+#define __declare_arg_8(a0, a1, a2, a3, a4, a5, a6, a7) \
+ auto __a7 = (a7); \
+ __declare_arg_7(a0, a1, a2, a3, a4, a5, a6); \
register auto arg7 ASM_REG(7) = __a7
#define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
@@ -181,21 +174,20 @@ struct arm_smccc_res {
* makes it stick.
*/
#define arm_smccc_1_1_smc(...) \
- do { \
+ ({ \
register unsigned long r0 ASM_REG(0); \
register unsigned long r1 ASM_REG(1); \
register unsigned long r2 ASM_REG(2); \
register unsigned long r3 ASM_REG(3); \
- __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
+ __declare_args(count_args(__VA_ARGS__), __VA_ARGS__); \
asm volatile ( \
"smc #0" \
: "=r" (r0), "=r" (r1), "=r" (r2), "=r" (r3) \
: PASTE(__constraint_read_, \
- __count_args(__VA_ARGS__)) \
+ count_args(__VA_ARGS__)) \
: "memory" ); \
- if ( ___res ) \
- *___res = (struct arm_smccc_res){ r0, r1, r2, r3 }; \
- } while ( 0 )
+ (struct arm_smccc_res){ r0, r1, r2, r3 }; \
+ })
/*
* The calling convention for arm32 is the same for both SMCCC v1.0 and
@@ -208,8 +200,8 @@ static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
{
struct arm_smccc_res res;
- arm_smccc_1_1_smc(regs->r0, regs->r1, regs->r2, regs->r3,
- regs->r4, regs->r5, regs->r6, regs->r7, &res);
+ res = arm_smccc_1_1_smc(regs->r0, regs->r1, regs->r2, regs->r3,
+ regs->r4, regs->r5, regs->r6, regs->r7);
regs->r0 = res.a0;
regs->r1 = res.a1;
@@ -229,7 +221,7 @@ static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
* so imply list x4 through x17 as clobbered.
*/
#define arm_smccc_smc(...) \
- do { \
+ ({ \
register unsigned long r0 ASM_REG(0); \
register unsigned long r1 ASM_REG(1); \
register unsigned long r2 ASM_REG(2); \
@@ -249,7 +241,7 @@ static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
register unsigned long c15 ASM_REG(15); \
register unsigned long c16 ASM_REG(16); \
register unsigned long c17 ASM_REG(17); \
- __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
+ __declare_args(count_args(__VA_ARGS__), __VA_ARGS__); \
asm volatile ( \
"smc #0" \
: "=r" (r0), "=r" (r1), "=r" (r2), "=r" (r3), \
@@ -258,11 +250,10 @@ static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
"=r" (c12), "=r" (c13), "=r" (c14), "=r" (c15), \
"=r" (c16), "=r" (c17) \
: PASTE(__constraint_read_, \
- __count_args(__VA_ARGS__)) \
+ count_args(__VA_ARGS__)) \
: "memory" ); \
- if ( ___res ) \
- *___res = (struct arm_smccc_res){ r0, r1, r2, r3 }; \
- } while ( 0 )
+ (struct arm_smccc_res){ r0, r1, r2, r3 }; \
+ })
#define arm_smccc_1_1_smc(...) arm_smccc_smc(__VA_ARGS__)
@@ -271,8 +262,8 @@ static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
{
struct arm_smccc_res res;
- arm_smccc_1_1_smc(regs->x0, regs->x1, regs->x2, regs->x3,
- regs->x4, regs->x5, regs->x6, regs->x7, &res);
+ res = arm_smccc_1_1_smc(regs->x0, regs->x1, regs->x2, regs->x3,
+ regs->x4, regs->x5, regs->x6, regs->x7);
regs->x0 = res.a0;
regs->x1 = res.a1;
diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
index f7c09520675e..f08d50c1fe38 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -249,7 +249,7 @@ static int exynos5_cpu_up(int cpu)
iounmap(power);
if ( secure_firmware )
- arm_smccc_smc(SMC_CMD_CPU1BOOT, cpu, NULL);
+ arm_smccc_smc(SMC_CMD_CPU1BOOT, cpu);
return cpu_up_send_sgi(cpu);
}
diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c
index 64cc1868c24b..dfa5cf4265c0 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)
{
- arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_RESET, NULL);
+ arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_RESET);
}
static void seattle_system_off(void)
{
- arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_OFF, NULL);
+ arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_OFF);
}
PLATFORM_START(seattle, "SEATTLE")
diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index b6860a776031..634d0d7467cf 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -41,8 +41,8 @@ int call_psci_cpu_on(int cpu)
{
struct arm_smccc_res res;
- arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary),
- &res);
+ res = arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu),
+ __pa(init_secondary));
return PSCI_RET(res);
}
@@ -54,7 +54,7 @@ void call_psci_cpu_off(void)
struct arm_smccc_res res;
/* If successfull the PSCI cpu_off call doesn't return */
- arm_smccc_smc(PSCI_0_2_FN32_CPU_OFF, &res);
+ res = arm_smccc_smc(PSCI_0_2_FN32_CPU_OFF);
panic("PSCI cpu off failed for CPU%d err=%d\n", smp_processor_id(),
PSCI_RET(res));
}
@@ -63,13 +63,13 @@ void call_psci_cpu_off(void)
void call_psci_system_off(void)
{
if ( psci_ver > PSCI_VERSION(0, 1) )
- arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_OFF, NULL);
+ arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_OFF);
}
void call_psci_system_reset(void)
{
if ( psci_ver > PSCI_VERSION(0, 1) )
- arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_RESET, NULL);
+ arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_RESET);
}
static int __init psci_features(uint32_t psci_func_id)
@@ -79,7 +79,7 @@ static int __init psci_features(uint32_t psci_func_id)
if ( psci_ver < PSCI_VERSION(1, 0) )
return PSCI_NOT_SUPPORTED;
- arm_smccc_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, &res);
+ res = arm_smccc_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id);
return PSCI_RET(res);
}
@@ -116,9 +116,8 @@ static void __init psci_init_smccc(void)
if ( psci_features(ARM_SMCCC_VERSION_FID) != PSCI_NOT_SUPPORTED )
{
- struct arm_smccc_res res;
+ struct arm_smccc_res res = arm_smccc_smc(ARM_SMCCC_VERSION_FID);
- arm_smccc_smc(ARM_SMCCC_VERSION_FID, &res);
if ( PSCI_RET(res) != ARM_SMCCC_NOT_SUPPORTED )
smccc_ver = PSCI_RET(res);
}
@@ -191,7 +190,7 @@ static int __init psci_init_0_2(void)
}
}
- arm_smccc_smc(PSCI_0_2_FN32_PSCI_VERSION, &res);
+ res = arm_smccc_smc(PSCI_0_2_FN32_PSCI_VERSION);
psci_ver = PSCI_RET(res);
/* For the moment, we only support PSCI 0.2 and PSCI 1.x */
diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index 3d2633237074..e38223d49801 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -178,7 +178,7 @@ static bool optee_probe(void)
return false;
/* Check UID */
- arm_smccc_smc(ARM_SMCCC_CALL_UID_FID(TRUSTED_OS_END), &resp);
+ resp = arm_smccc_smc(ARM_SMCCC_CALL_UID_FID(TRUSTED_OS_END));
if ( (uint32_t)resp.a0 != OPTEE_MSG_UID_0 ||
(uint32_t)resp.a1 != OPTEE_MSG_UID_1 ||
@@ -209,7 +209,7 @@ static bool optee_probe(void)
* call. It will return OPTEE_SMC_RETURN_UNKNOWN_FUNCTION if
* OP-TEE have no virtualization support enabled.
*/
- arm_smccc_smc(OPTEE_SMC_VM_DESTROYED, 0, 0, 0, 0, 0, 0, 0, &resp);
+ resp = arm_smccc_smc(OPTEE_SMC_VM_DESTROYED, 0, 0, 0, 0, 0, 0, 0);
if ( resp.a0 == OPTEE_SMC_RETURN_UNKNOWN_FUNCTION )
return false;
@@ -243,8 +243,8 @@ static int optee_domain_init(struct domain *d)
*
* a7 should be 0, so we can't skip last 6 parameters of arm_smccc_smc()
*/
- arm_smccc_smc(OPTEE_SMC_VM_CREATED, OPTEE_CLIENT_ID(d), 0, 0, 0, 0, 0, 0,
- &resp);
+ resp = arm_smccc_smc(OPTEE_SMC_VM_CREATED, OPTEE_CLIENT_ID(d),
+ 0, 0, 0, 0, 0, 0);
if ( resp.a0 != OPTEE_SMC_RETURN_OK )
{
printk(XENLOG_WARNING "%pd: Unable to create OPTEE client: rc = 0x%X\n",
@@ -681,8 +681,8 @@ static int optee_relinquish_resources(struct domain *d)
*
* a7 should be 0, so we can't skip last 6 parameters of arm_smccc_smc()
*/
- arm_smccc_smc(OPTEE_SMC_VM_DESTROYED, OPTEE_CLIENT_ID(d), 0, 0, 0, 0, 0, 0,
- &resp);
+ resp = arm_smccc_smc(OPTEE_SMC_VM_DESTROYED, OPTEE_CLIENT_ID(d),
+ 0, 0, 0, 0, 0, 0);
ASSERT(!spin_is_locked(&ctx->lock));
ASSERT(!atomic_read(&ctx->call_count));
@@ -1171,15 +1171,14 @@ static void do_call_with_arg(struct optee_domain *ctx,
{
struct arm_smccc_res res;
- arm_smccc_smc(a0, a1, a2, a3, a4, a5, 0, OPTEE_CLIENT_ID(current->domain),
- &res);
+ res = arm_smccc_smc(a0, a1, a2, a3, a4, a5, 0, OPTEE_CLIENT_ID(current->domain));
if ( OPTEE_SMC_RETURN_IS_RPC(res.a0) )
{
while ( handle_rpc_return(ctx, &res, regs, call) == -ERESTART )
{
- arm_smccc_smc(res.a0, res.a1, res.a2, res.a3, 0, 0, 0,
- OPTEE_CLIENT_ID(current->domain), &res);
+ res = arm_smccc_smc(res.a0, res.a1, res.a2, res.a3, 0, 0, 0,
+ OPTEE_CLIENT_ID(current->domain));
if ( !OPTEE_SMC_RETURN_IS_RPC(res.a0) )
break;
@@ -1619,8 +1618,8 @@ static void handle_exchange_capabilities(struct cpu_user_regs *regs)
caps = get_user_reg(regs, 1);
caps &= OPTEE_KNOWN_NSEC_CAPS;
- arm_smccc_smc(OPTEE_SMC_EXCHANGE_CAPABILITIES, caps, 0, 0, 0, 0, 0,
- OPTEE_CLIENT_ID(current->domain), &resp);
+ resp = arm_smccc_smc(OPTEE_SMC_EXCHANGE_CAPABILITIES, caps, 0, 0, 0, 0, 0,
+ OPTEE_CLIENT_ID(current->domain));
if ( resp.a0 != OPTEE_SMC_RETURN_OK ) {
set_user_reg(regs, 0, resp.a0);
return;
@@ -1664,8 +1663,8 @@ static bool optee_handle_call(struct cpu_user_regs *regs)
return true;
case OPTEE_SMC_CALLS_UID:
- arm_smccc_smc(OPTEE_SMC_CALLS_UID, 0, 0, 0, 0, 0, 0,
- OPTEE_CLIENT_ID(current->domain), &resp);
+ resp = arm_smccc_smc(OPTEE_SMC_CALLS_UID, 0, 0, 0, 0, 0, 0,
+ OPTEE_CLIENT_ID(current->domain));
set_user_reg(regs, 0, resp.a0);
set_user_reg(regs, 1, resp.a1);
set_user_reg(regs, 2, resp.a2);
@@ -1673,15 +1672,15 @@ static bool optee_handle_call(struct cpu_user_regs *regs)
return true;
case OPTEE_SMC_CALLS_REVISION:
- arm_smccc_smc(OPTEE_SMC_CALLS_REVISION, 0, 0, 0, 0, 0, 0,
- OPTEE_CLIENT_ID(current->domain), &resp);
+ resp = arm_smccc_smc(OPTEE_SMC_CALLS_REVISION, 0, 0, 0, 0, 0, 0,
+ OPTEE_CLIENT_ID(current->domain));
set_user_reg(regs, 0, resp.a0);
set_user_reg(regs, 1, resp.a1);
return true;
case OPTEE_SMC_CALL_GET_OS_UUID:
- arm_smccc_smc(OPTEE_SMC_CALL_GET_OS_UUID, 0, 0, 0, 0, 0, 0,
- OPTEE_CLIENT_ID(current->domain),&resp);
+ resp = arm_smccc_smc(OPTEE_SMC_CALL_GET_OS_UUID, 0, 0, 0, 0, 0, 0,
+ OPTEE_CLIENT_ID(current->domain));
set_user_reg(regs, 0, resp.a0);
set_user_reg(regs, 1, resp.a1);
set_user_reg(regs, 2, resp.a2);
@@ -1689,21 +1688,21 @@ static bool optee_handle_call(struct cpu_user_regs *regs)
return true;
case OPTEE_SMC_CALL_GET_OS_REVISION:
- arm_smccc_smc(OPTEE_SMC_CALL_GET_OS_REVISION, 0, 0, 0, 0, 0, 0,
- OPTEE_CLIENT_ID(current->domain), &resp);
+ resp = arm_smccc_smc(OPTEE_SMC_CALL_GET_OS_REVISION, 0, 0, 0, 0, 0, 0,
+ OPTEE_CLIENT_ID(current->domain));
set_user_reg(regs, 0, resp.a0);
set_user_reg(regs, 1, resp.a1);
return true;
case OPTEE_SMC_ENABLE_SHM_CACHE:
- arm_smccc_smc(OPTEE_SMC_ENABLE_SHM_CACHE, 0, 0, 0, 0, 0, 0,
- OPTEE_CLIENT_ID(current->domain), &resp);
+ resp = arm_smccc_smc(OPTEE_SMC_ENABLE_SHM_CACHE, 0, 0, 0, 0, 0, 0,
+ OPTEE_CLIENT_ID(current->domain));
set_user_reg(regs, 0, resp.a0);
return true;
case OPTEE_SMC_DISABLE_SHM_CACHE:
- arm_smccc_smc(OPTEE_SMC_DISABLE_SHM_CACHE, 0, 0, 0, 0, 0, 0,
- OPTEE_CLIENT_ID(current->domain), &resp);
+ resp = arm_smccc_smc(OPTEE_SMC_DISABLE_SHM_CACHE, 0, 0, 0, 0, 0, 0,
+ OPTEE_CLIENT_ID(current->domain));
set_user_reg(regs, 0, resp.a0);
if ( resp.a0 == OPTEE_SMC_RETURN_OK ) {
free_shm_rpc(ctx, regpair_to_uint64(resp.a1, resp.a2));
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 625d229396bb..6a5dcef2aa87 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1991,7 +1991,7 @@ void asmlinkage enter_hypervisor_from_guest_preirq(void)
/* If the guest has disabled the workaround, bring it back on. */
if ( needs_ssbd_flip(v) )
- arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL);
+ arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1);
}
/*
@@ -2334,7 +2334,7 @@ void asmlinkage leave_hypervisor_to_guest(void)
* If the guest wants it disabled, so be it...
*/
if ( needs_ssbd_flip(current) )
- arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 0, NULL);
+ arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 0);
}
/*
--
2.39.5
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] xen/arm: Fixes and improvements to SMCCC
2026-08-31 12:19 [PATCH 0/5] xen/arm: Fixes and improvements to SMCCC Andrew Cooper
` (4 preceding siblings ...)
2026-08-31 12:19 ` [PATCH 5/6] xen/arm: Rewrite arm_smccc_*() to return by value Andrew Cooper
@ 2026-09-03 9:20 ` Bertrand Marquis
2026-09-03 9:35 ` Andrew Cooper
5 siblings, 1 reply; 15+ messages in thread
From: Bertrand Marquis @ 2026-09-03 9:20 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Michal Orzel, Jan Setje-Eilers
Hi Andrew,
While reviewing those patches i found out that cover letter is saying that the serie has 5 patches but then the patches are number out of 6.
Is there a patch missing or something went wrong in the numbering and there is actually on 5 patches ?
Regards
Bertrand
> On 31 Aug 2026, at 14:19, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> Patch 1 is a bugfix for the issue reported by Jan Setje-Eilers. It needs
> backporting to Xen 4.22.
>
> Everything else is because I couldn't bear to leave the code generation in
> such a bad state.
>
> https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/2805409790
>
> Andrew Cooper (5):
> xen/arm: Fix evaluation of parameters for SMCCC calls
> xen/arm: Introduce arm_smccc_guest_smc()
> xen/arm: Clean up 32bit arm_smccc_1_1_smc()
> xen/arm: Rewrite arm_smccc_smc() for arm64
> xen/arm: Rewrite arm_smccc_*() to return by value
>
> xen/arch/arm/arm64/smc.S | 16 --
> xen/arch/arm/cpuerrata.c | 18 +-
> xen/arch/arm/firmware/scmi-smc.c | 16 +-
> xen/arch/arm/include/asm/smccc.h | 241 +++++++++++---------
> xen/arch/arm/platforms/exynos5.c | 2 +-
> xen/arch/arm/platforms/imx8m.c | 16 +-
> xen/arch/arm/platforms/imx8qm.c | 16 +-
> xen/arch/arm/platforms/seattle.c | 4 +-
> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 17 +-
> xen/arch/arm/psci.c | 17 +-
> xen/arch/arm/traps.c | 4 +-
> 11 files changed, 165 insertions(+), 202 deletions(-)
>
> --
> 2.39.5
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] xen/arm: Fixes and improvements to SMCCC
2026-09-03 9:20 ` [PATCH 0/5] xen/arm: Fixes and improvements to SMCCC Bertrand Marquis
@ 2026-09-03 9:35 ` Andrew Cooper
2026-09-03 10:26 ` Bertrand Marquis
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2026-09-03 9:35 UTC (permalink / raw)
To: Bertrand Marquis
Cc: Andrew Cooper, Xen-devel, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Michal Orzel, Jan Setje-Eilers
On 03/09/2026 10:20 am, Bertrand Marquis wrote:
> Hi Andrew,
>
> While reviewing those patches i found out that cover letter is saying that the serie has 5 patches but then the patches are number out of 6.
> Is there a patch missing or something went wrong in the numbering and there is actually on 5 patches ?
Oops, that was a mistake with git-format-patch, I expect.
The 6th patch in this series was
https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=5504b16eea5e661b67d4fba3cf9283d85b9dd3dc
but I ended up submitting it separately.
~Andrew
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] xen/arm: Fixes and improvements to SMCCC
2026-09-03 9:35 ` Andrew Cooper
@ 2026-09-03 10:26 ` Bertrand Marquis
0 siblings, 0 replies; 15+ messages in thread
From: Bertrand Marquis @ 2026-09-03 10:26 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Michal Orzel, Jan Setje-Eilers
> On 3 Sep 2026, at 11:35, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 03/09/2026 10:20 am, Bertrand Marquis wrote:
>> Hi Andrew,
>>
>> While reviewing those patches i found out that cover letter is saying that the serie has 5 patches but then the patches are number out of 6.
>> Is there a patch missing or something went wrong in the numbering and there is actually on 5 patches ?
>
> Oops, that was a mistake with git-format-patch, I expect.
>
> The 6th patch in this series was
> https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=5504b16eea5e661b67d4fba3cf9283d85b9dd3dc
> but I ended up submitting it separately.
Ok then 5 patches it is :-)
Cheers
Bertrand
>
> ~Andrew
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/6] xen/arm: Fix evaluation of parameters for SMCCC calls
2026-08-31 12:19 ` [PATCH 1/6] xen/arm: Fix evaluation of parameters for SMCCC calls Andrew Cooper
@ 2026-09-03 11:46 ` Bertrand Marquis
0 siblings, 0 replies; 15+ messages in thread
From: Bertrand Marquis @ 2026-09-03 11:46 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Jan Setje-Eilers, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Michal Orzel
Hi Andrew,
> On 31 Aug 2026, at 14:19, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> Contrary to what was claimed in commit 67bcf5eae709 ("xen/arm: Simplify type
> handling for SMCCC declarations"), there is an important reason to retain the
> intermediate variable. It is unsafe to have any logic between the assignment
> of the register variabes and the asm() block they're used in.
>
> This logically reverts commit 67bcf5eae709 ("xen/arm: Simplify type handling
> for SMCCC declarations") while retaining the conversions from commit
> 7f15d5d13221 ("xen/treewide: More typeof() -> auto conversions").
>
> Adjust __declare_arg_0() to match. It happens to be safe because it's the
> first register expression once all macros are expanded, but it really should
> be consistent with the others.
>
> Leave a comment explaining why they must be written like this.
>
> Fixes: 67bcf5eae709 ("xen/arm: Simplify type handling for SMCCC declarations")
> Reported-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Code looks good and my tests are passing:
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Cheers
Bertrand
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
> ---
> xen/arch/arm/include/asm/smccc.h | 31 +++++++++++++++++++++++--------
> 1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/smccc.h b/xen/arch/arm/include/asm/smccc.h
> index 62c6985e7315..53cdddb690b7 100644
> --- a/xen/arch/arm/include/asm/smccc.h
> +++ b/xen/arch/arm/include/asm/smccc.h
> @@ -108,37 +108,52 @@ struct arm_smccc_res {
> #define __constraint_read_6 __constraint_read_5, "r" (arg6)
> #define __constraint_read_7 __constraint_read_6, "r" (arg7)
>
> +/*
> + * Macro arguments MUST be evaluated before being assigned to a register
> + * variable.
> + *
> + * This is manual register scheduling for the asm() statement, and any other
> + * logic to evaluate may clobber the already-scheduled registers.
> + */
> #define __declare_arg_0(a0, res) \
> + auto __a0 = (uint32_t)(a0); \
> struct arm_smccc_res *___res = (res); \
> - register unsigned long arg0 ASM_REG(0) = (uint32_t)(a0)
> + register unsigned long arg0 ASM_REG(0) = __a0
>
> #define __declare_arg_1(a0, a1, res) \
> + auto __a1 = (a1); \
> __declare_arg_0(a0, res); \
> - register auto arg1 ASM_REG(1) = (a1)
> + register auto arg1 ASM_REG(1) = __a1
>
> #define __declare_arg_2(a0, a1, a2, res) \
> + auto __a2 = (a2); \
> __declare_arg_1(a0, a1, res); \
> - register auto arg2 ASM_REG(2) = (a2)
> + register auto arg2 ASM_REG(2) = __a2
>
> #define __declare_arg_3(a0, a1, a2, a3, res) \
> + auto __a3 = (a3); \
> __declare_arg_2(a0, a1, a2, res); \
> - register auto arg3 ASM_REG(3) = (a3)
> + register auto arg3 ASM_REG(3) = __a3
>
> #define __declare_arg_4(a0, a1, a2, a3, a4, res) \
> + auto __a4 = (a4); \
> __declare_arg_3(a0, a1, a2, a3, res); \
> - register auto arg4 ASM_REG(4) = (a4)
> + register auto arg4 ASM_REG(4) = __a4
>
> #define __declare_arg_5(a0, a1, a2, a3, a4, a5, res) \
> + auto __a5 = (a5); \
> __declare_arg_4(a0, a1, a2, a3, a4, res); \
> - register auto arg5 ASM_REG(5) = (a5)
> + register auto arg5 ASM_REG(5) = __a5
>
> #define __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res) \
> + auto __a6 = (a6); \
> __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
> - register auto arg6 ASM_REG(6) = (a6)
> + register auto arg6 ASM_REG(6) = __a6
>
> #define __declare_arg_7(a0, a1, a2, a3, a4, a5, a6, a7, res) \
> + auto __a7 = (a7); \
> __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
> - register auto arg7 ASM_REG(7) = (a7)
> + register auto arg7 ASM_REG(7) = __a7
>
> #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
> #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
>
> base-commit: 79225a0c77e13b693b4d2b903a88289704b79db6
> --
> 2.39.5
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/6] xen/arm: Introduce arm_smccc_guest_smc()
2026-08-31 12:19 ` [PATCH 2/6] xen/arm: Introduce arm_smccc_guest_smc() Andrew Cooper
@ 2026-09-03 11:49 ` Bertrand Marquis
0 siblings, 0 replies; 15+ messages in thread
From: Bertrand Marquis @ 2026-09-03 11:49 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Michal Orzel, Jan Setje-Eilers
Hi Andrew,
> On 31 Aug 2026, at 14:19, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> Both {get,set}_user_reg() are out-of-line functions, leading to awful code
> generation.
>
> Introduce arm_smccc_guest_smc() to operate directly on guest registers.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Looks good to me.
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Cheers
Bertrand
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
>
> For arm64:
>
> add/remove: 0/0 grow/shrink: 2/4 up/down: 27/-864 (-837)
> Function old new delta
> symbols_addresses 35096 35120 +24
> symbols_names 42958 42961 +3
> imx8qm_smc 544 348 -196
> scmi_handle_smc 372 152 -220
> imx8m_smc 576 356 -220
> zynqmp_eemi 864 636 -228
>
> For arm32:
>
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-160 (-160)
> Function old new delta
> scmi_handle_smc 392 232 -160
> ---
> xen/arch/arm/firmware/scmi-smc.c | 16 +-----------
> xen/arch/arm/include/asm/smccc.h | 29 +++++++++++++++++++++
> xen/arch/arm/platforms/imx8m.c | 16 +-----------
> xen/arch/arm/platforms/imx8qm.c | 16 +-----------
> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 17 ++----------
> 5 files changed, 34 insertions(+), 60 deletions(-)
>
> diff --git a/xen/arch/arm/firmware/scmi-smc.c b/xen/arch/arm/firmware/scmi-smc.c
> index 0835ddeeeccc..a0cc6c6192f8 100644
> --- a/xen/arch/arm/firmware/scmi-smc.c
> +++ b/xen/arch/arm/firmware/scmi-smc.c
> @@ -50,7 +50,6 @@ static bool scmi_is_valid_smc_id(uint32_t fid)
> static bool scmi_handle_smc(struct cpu_user_regs *regs)
> {
> uint32_t fid = (uint32_t)get_user_reg(regs, 0);
> - struct arm_smccc_res res;
>
> if ( !scmi_is_valid_smc_id(fid) )
> return false;
> @@ -63,20 +62,7 @@ static bool scmi_handle_smc(struct cpu_user_regs *regs)
> }
>
> /* For the moment, forward the SCMI Request to FW running at EL3 */
> - arm_smccc_1_1_smc(fid,
> - get_user_reg(regs, 1),
> - get_user_reg(regs, 2),
> - get_user_reg(regs, 3),
> - get_user_reg(regs, 4),
> - get_user_reg(regs, 5),
> - get_user_reg(regs, 6),
> - get_user_reg(regs, 7),
> - &res);
> -
> - set_user_reg(regs, 0, res.a0);
> - set_user_reg(regs, 1, res.a1);
> - set_user_reg(regs, 2, res.a2);
> - set_user_reg(regs, 3, res.a3);
> + arm_smccc_guest_smc(regs);
>
> return true;
> }
> diff --git a/xen/arch/arm/include/asm/smccc.h b/xen/arch/arm/include/asm/smccc.h
> index 53cdddb690b7..832157f43734 100644
> --- a/xen/arch/arm/include/asm/smccc.h
> +++ b/xen/arch/arm/include/asm/smccc.h
> @@ -202,6 +202,21 @@ 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__)
> +
> +/* Make an SMCCC v1.1 compliant SMC call with guest register state. */
> +static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_1_1_smc(regs->r0, regs->r1, regs->r2, regs->r3,
> + regs->r4, regs->r5, regs->r6, regs->r7, &res);
> +
> + regs->r0 = res.a0;
> + regs->r1 = res.a1;
> + regs->r2 = res.a2;
> + regs->r3 = res.a3;
> +}
> +
> #else
>
> void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
> @@ -251,6 +266,20 @@ void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
> arm_smccc_1_0_smc(__VA_ARGS__); \
> } while ( 0 )
>
> +/* Make an SMCCC v1.1 compliant SMC call with guest register state. */
> +static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_1_1_smc(regs->x0, regs->x1, regs->x2, regs->x3,
> + regs->x4, regs->x5, regs->x6, regs->x7, &res);
> +
> + regs->x0 = res.a0;
> + regs->x1 = res.a1;
> + regs->x2 = res.a2;
> + regs->x3 = res.a3;
> +}
> +
> /*
> * struct arm_smccc_1_2_regs - Arguments for or Results from SMC call
> * @a0-a17 argument values from registers 0 to 17
> diff --git a/xen/arch/arm/platforms/imx8m.c b/xen/arch/arm/platforms/imx8m.c
> index 669dd517e057..efb0ad20d6e8 100644
> --- a/xen/arch/arm/platforms/imx8m.c
> +++ b/xen/arch/arm/platforms/imx8m.c
> @@ -50,7 +50,6 @@ static bool imx8m_smc(struct cpu_user_regs *regs)
> {
> uint32_t function_id = get_user_reg(regs, 0);
> uint32_t subfunction_id = get_user_reg(regs, 1);
> - struct arm_smccc_res res;
>
> if ( !cpus_have_const_cap(ARM_SMCCC_1_1) )
> {
> @@ -122,20 +121,7 @@ static bool imx8m_smc(struct cpu_user_regs *regs)
> return false;
> }
>
> - arm_smccc_1_1_smc(function_id,
> - subfunction_id,
> - get_user_reg(regs, 2),
> - get_user_reg(regs, 3),
> - get_user_reg(regs, 4),
> - get_user_reg(regs, 5),
> - get_user_reg(regs, 6),
> - get_user_reg(regs, 7),
> - &res);
> -
> - set_user_reg(regs, 0, res.a0);
> - set_user_reg(regs, 1, res.a1);
> - set_user_reg(regs, 2, res.a2);
> - set_user_reg(regs, 3, res.a3);
> + arm_smccc_guest_smc(regs);
>
> return true;
> }
> diff --git a/xen/arch/arm/platforms/imx8qm.c b/xen/arch/arm/platforms/imx8qm.c
> index 3600a073e8ba..7249e14ab640 100644
> --- a/xen/arch/arm/platforms/imx8qm.c
> +++ b/xen/arch/arm/platforms/imx8qm.c
> @@ -67,7 +67,6 @@ static bool imx8qm_smc(struct cpu_user_regs *regs)
> {
> uint32_t function_id = get_user_reg(regs, 0);
> uint32_t subfunction_id = get_user_reg(regs, 1);
> - struct arm_smccc_res res;
>
> if ( !cpus_have_const_cap(ARM_SMCCC_1_1) )
> {
> @@ -106,20 +105,7 @@ static bool imx8qm_smc(struct cpu_user_regs *regs)
> }
>
> allow_call:
> - arm_smccc_1_1_smc(function_id,
> - subfunction_id,
> - get_user_reg(regs, 2),
> - get_user_reg(regs, 3),
> - get_user_reg(regs, 4),
> - get_user_reg(regs, 5),
> - get_user_reg(regs, 6),
> - get_user_reg(regs, 7),
> - &res);
> -
> - set_user_reg(regs, 0, res.a0);
> - set_user_reg(regs, 1, res.a1);
> - set_user_reg(regs, 2, res.a2);
> - set_user_reg(regs, 3, res.a3);
> + arm_smccc_guest_smc(regs);
>
> return true;
> }
> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> index 2053ed7ac5f6..326c8a1ba6e5 100644
> --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> @@ -51,7 +51,6 @@ static inline bool domain_has_reset_access(struct domain *d, uint32_t rst)
>
> bool zynqmp_eemi(struct cpu_user_regs *regs)
> {
> - struct arm_smccc_res res;
> uint32_t fid = get_user_reg(regs, 0);
> uint32_t nodeid = get_user_reg(regs, 1);
> unsigned int pm_fn = fid & 0xFFFF;
> @@ -187,20 +186,8 @@ bool zynqmp_eemi(struct cpu_user_regs *regs)
> * can forward the whole command to firmware without additional
> * parameters checks.
> */
> - arm_smccc_1_1_smc(get_user_reg(regs, 0),
> - get_user_reg(regs, 1),
> - get_user_reg(regs, 2),
> - get_user_reg(regs, 3),
> - get_user_reg(regs, 4),
> - get_user_reg(regs, 5),
> - get_user_reg(regs, 6),
> - get_user_reg(regs, 7),
> - &res);
> -
> - set_user_reg(regs, 0, res.a0);
> - set_user_reg(regs, 1, res.a1);
> - set_user_reg(regs, 2, res.a2);
> - set_user_reg(regs, 3, res.a3);
> + arm_smccc_guest_smc(regs);
> +
> return true;
>
> done:
> --
> 2.39.5
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/6] xen/arm: Clean up 32bit arm_smccc_1_1_smc()
2026-08-31 12:19 ` [PATCH 3/6] xen/arm: Clean up 32bit arm_smccc_1_1_smc() Andrew Cooper
@ 2026-09-03 11:52 ` Bertrand Marquis
0 siblings, 0 replies; 15+ messages in thread
From: Bertrand Marquis @ 2026-09-03 11:52 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Michal Orzel, Jan Setje-Eilers
Hi Andrew,
> On 31 Aug 2026, at 14:19, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> ... before making a related copy of it.
>
> * Drop __constraints() so the output parameters are visible in the same block
> as they're defined. Use PASTE() rather than opencoding it.
> * Adust the indentation of trailing \'s for consistency.
> * Drop the newline at the end of the instruction.
> * Indent the if condition correctly. ___res is always of type
> arm_smccc_res (declared in __declare_arg_0()), so drop the typeof().
> * Drop arm_smccc_1_0_smc() as it has no users.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Looks good and i was not aware of this PASTE, result looks nit.
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Cheers
Bertrand
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
> ---
> xen/arch/arm/include/asm/smccc.h | 45 ++++++++++++++++----------------
> 1 file changed, 22 insertions(+), 23 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/smccc.h b/xen/arch/arm/include/asm/smccc.h
> index 832157f43734..5fe54013ac83 100644
> --- a/xen/arch/arm/include/asm/smccc.h
> +++ b/xen/arch/arm/include/asm/smccc.h
> @@ -56,6 +56,8 @@
>
> #ifndef __ASSEMBLER__
>
> +#include <xen/macros.h>
> +
> extern uint32_t smccc_ver;
>
> /* Check if this is fast call. */
> @@ -115,24 +117,24 @@ struct arm_smccc_res {
> * This is manual register scheduling for the asm() statement, and any other
> * logic to evaluate may clobber the already-scheduled registers.
> */
> -#define __declare_arg_0(a0, res) \
> - auto __a0 = (uint32_t)(a0); \
> - struct arm_smccc_res *___res = (res); \
> +#define __declare_arg_0(a0, res) \
> + auto __a0 = (uint32_t)(a0); \
> + struct arm_smccc_res *___res = (res); \
> register unsigned long arg0 ASM_REG(0) = __a0
>
> -#define __declare_arg_1(a0, a1, res) \
> - auto __a1 = (a1); \
> - __declare_arg_0(a0, res); \
> +#define __declare_arg_1(a0, a1, res) \
> + auto __a1 = (a1); \
> + __declare_arg_0(a0, res); \
> register auto arg1 ASM_REG(1) = __a1
>
> -#define __declare_arg_2(a0, a1, a2, res) \
> - auto __a2 = (a2); \
> - __declare_arg_1(a0, a1, res); \
> +#define __declare_arg_2(a0, a1, a2, res) \
> + auto __a2 = (a2); \
> + __declare_arg_1(a0, a1, res); \
> register auto arg2 ASM_REG(2) = __a2
>
> -#define __declare_arg_3(a0, a1, a2, a3, res) \
> - auto __a3 = (a3); \
> - __declare_arg_2(a0, a1, a2, res); \
> +#define __declare_arg_3(a0, a1, a2, a3, res) \
> + auto __a3 = (a3); \
> + __declare_arg_2(a0, a1, a2, res); \
> register auto arg3 ASM_REG(3) = __a3
>
> #define __declare_arg_4(a0, a1, a2, a3, a4, res) \
> @@ -158,12 +160,6 @@ struct arm_smccc_res {
> #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
> #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
>
> -#define ___constraints(count) \
> - : "=r" (r0), "=r" (r1), "=r" (r2), "=r" (r3) \
> - : __constraint_read_ ## count \
> - : "memory"
> -#define __constraints(count) ___constraints(count)
> -
> /*
> * arm_smccc_1_1_smc() - make an SMCCC v1.1 compliant SMC call
> *
> @@ -189,10 +185,14 @@ struct arm_smccc_res {
> register unsigned long r2 ASM_REG(2); \
> register unsigned long r3 ASM_REG(3); \
> __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
> - asm volatile("smc #0\n" \
> - __constraints(__count_args(__VA_ARGS__))); \
> + asm volatile ( \
> + "smc #0" \
> + : "=r" (r0), "=r" (r1), "=r" (r2), "=r" (r3) \
> + : PASTE(__constraint_read_, \
> + __count_args(__VA_ARGS__)) \
> + : "memory" ); \
> if ( ___res ) \
> - *___res = (typeof(*___res)){r0, r1, r2, r3}; \
> + *___res = (struct arm_smccc_res){ r0, r1, r2, r3 }; \
> } while ( 0 )
>
> /*
> @@ -200,7 +200,6 @@ struct arm_smccc_res {
> * v1.1.
> */
> #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__)
>
> /* Make an SMCCC v1.1 compliant SMC call with guest register state. */
> @@ -217,7 +216,7 @@ static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
> regs->r3 = res.a3;
> }
>
> -#else
> +#else /* CONFIG_ARM_64 */
>
> void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
> register_t a3, register_t a4, register_t a5,
> --
> 2.39.5
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/6] xen/arm: Rewrite arm_smccc_smc() for arm64
2026-08-31 12:19 ` [PATCH 4/6] xen/arm: Rewrite arm_smccc_smc() for arm64 Andrew Cooper
@ 2026-09-03 12:16 ` Bertrand Marquis
2026-09-04 10:50 ` Andrew Cooper
0 siblings, 1 reply; 15+ messages in thread
From: Bertrand Marquis @ 2026-09-03 12:16 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Michal Orzel, Jan Setje-Eilers
Hi Andrew,
> On 31 Aug 2026, at 14:19, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> PSCI v1.0 says that x4 thru x17 may be clobbered. PSCI v1.1 says they are
> strictly preserved unless they contain return values.
I think the PSCI reference here and in the rest of the commit message and the patch
should not be used and should be SMCCC references instead (which would also be
more coherent with the fact that this code is not PSCI but SMCCC and is used by
others than PSCI).
PSCI does not define the preservation rules, this is defined in SMCCC spec and we
have a way to detect the SMCCC version.
SMCCC defines the following rules:
- 1.0: x4-x17 have an unpredictable return state.
- 1.1: x4-x17 must be preserved.
- 1.2: x4-x17 may additionally be defined as result registers, otherwise they remain preserved.
So the commit message and comments should consistently say SMCCC instead of PSCI.
Other than this the code is right i think.
Cheers
Bertrand
>
> Xen deals with this by having __arm_smccc_1_0_smc() as an out-of-line
> function, but this causes awful code generation in arm_smccc_smc().
> cpus_have_const_cap() is opaque to the optimiser, so we end up with one basic
> block doing the reasonably-ok arm_smccc_1_1_smc() code generation and a second
> basic block setting up all 8 input registers even when they're not needed,
> spilling or discarding x8 thru x17, and calling an out-of-line function.
>
> Remove __arm_smccc_1_0_smc() entirely, and rewrite arm_smccc_smc() to declare
> x4 thru x17 as clobbered.
>
> This fully inlines the SMC, is a single basic block which instructs the
> compiler to spill or discard the potentially clobbered registers, and only
> sets up the necessary number of arguments for the call.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
>
> Bloat-o-meter reports:
>
> add/remove: 0/1 grow/shrink: 0/10 up/down: 0/-795 (-795)
> Function old new delta
> symbols_sorted_offsets 23832 23824 -8
> symbols_names 42962 42943 -19
> symbols_addresses 35128 35104 -24
> __arm_smccc_1_0_smc 32 - -32
> call_psci_cpu_off 144 76 -68
> seattle_system_reset 92 16 -76
> seattle_system_off 92 16 -76
> call_psci_system_reset 112 32 -80
> call_psci_system_off 112 32 -80
> call_psci_cpu_on 252 124 -128
> psci_init 628 424 -204
>
> An alternative way to do this would be to have x8 thru x17 in the clobber list
> rather than the output list which would reduce the source size, but this form
> is more amenable to having PSCI v1.2 worked into it too.
> ---
> xen/arch/arm/arm64/smc.S | 16 ------
> xen/arch/arm/include/asm/smccc.h | 95 ++++++++++++++++----------------
> 2 files changed, 48 insertions(+), 63 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S
> index 68b05e8ddd12..65b4eabe4f87 100644
> --- a/xen/arch/arm/arm64/smc.S
> +++ b/xen/arch/arm/arm64/smc.S
> @@ -13,22 +13,6 @@
> * GNU General Public License for more details.
> */
>
> -/*
> - * 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)
> - */
> -FUNC(__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
> -END(__arm_smccc_1_0_smc)
> -
> /*
> * void arm_smccc_1_2_smc(const struct arm_smccc_1_2_regs *args,
> * struct arm_smccc_1_2_regs *res)
> diff --git a/xen/arch/arm/include/asm/smccc.h b/xen/arch/arm/include/asm/smccc.h
> index 5fe54013ac83..8920c54b09a6 100644
> --- a/xen/arch/arm/include/asm/smccc.h
> +++ b/xen/arch/arm/include/asm/smccc.h
> @@ -16,9 +16,6 @@
> #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)
> @@ -57,6 +54,9 @@
> #ifndef __ASSEMBLER__
>
> #include <xen/macros.h>
> +#include <xen/types.h>
> +
> +#include <asm/asm_defns.h>
>
> extern uint32_t smccc_ver;
>
> @@ -160,6 +160,8 @@ struct arm_smccc_res {
> #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
> #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
>
> +#ifdef CONFIG_ARM_32
> +
> /*
> * arm_smccc_1_1_smc() - make an SMCCC v1.1 compliant SMC call
> *
> @@ -199,7 +201,6 @@ struct arm_smccc_res {
> * The calling convention for arm32 is the same for both SMCCC v1.0 and
> * v1.1.
> */
> -#ifdef CONFIG_ARM_32
> #define arm_smccc_smc(...) arm_smccc_1_1_smc(__VA_ARGS__)
>
> /* Make an SMCCC v1.1 compliant SMC call with guest register state. */
> @@ -218,53 +219,53 @@ static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
>
> #else /* CONFIG_ARM_64 */
>
> -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__)
> -
> +/*
> + * Make an SMCCC call compatible with both PSCI v1.1 and v1.0.
> + *
> + * PSCI v1.1 says that x4 through x17 are strictly preserved unless they
> + * contain return data. PSCI v1.0 says they clobbered.
> + *
> + * Xen doesn't make PSCI v1.1 calls which expect more than 4 return registers,
> + * so imply list x4 through x17 as clobbered.
> + */
> #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__); \
> + register unsigned long r0 ASM_REG(0); \
> + register unsigned long r1 ASM_REG(1); \
> + register unsigned long r2 ASM_REG(2); \
> + register unsigned long r3 ASM_REG(3); \
> + /* Potentially clobbered in PSCI 1.0 */ \
> + register unsigned long c4 ASM_REG(4); \
> + register unsigned long c5 ASM_REG(5); \
> + register unsigned long c6 ASM_REG(6); \
> + register unsigned long c7 ASM_REG(7); \
> + register unsigned long c8 ASM_REG(8); \
> + register unsigned long c9 ASM_REG(9); \
> + register unsigned long c10 ASM_REG(10); \
> + register unsigned long c11 ASM_REG(11); \
> + register unsigned long c12 ASM_REG(12); \
> + register unsigned long c13 ASM_REG(13); \
> + register unsigned long c14 ASM_REG(14); \
> + register unsigned long c15 ASM_REG(15); \
> + register unsigned long c16 ASM_REG(16); \
> + register unsigned long c17 ASM_REG(17); \
> + __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
> + asm volatile ( \
> + "smc #0" \
> + : "=r" (r0), "=r" (r1), "=r" (r2), "=r" (r3), \
> + "=r" (c4), "=r" (c5), "=r" (c6), "=r" (c7), \
> + "=r" (c8), "=r" (c9), "=r" (c10), "=r" (c11), \
> + "=r" (c12), "=r" (c13), "=r" (c14), "=r" (c15), \
> + "=r" (c16), "=r" (c17) \
> + : PASTE(__constraint_read_, \
> + __count_args(__VA_ARGS__)) \
> + : "memory" ); \
> + if ( ___res ) \
> + *___res = (struct arm_smccc_res){ r0, r1, r2, r3 }; \
> } while ( 0 )
>
> +#define arm_smccc_1_1_smc(...) arm_smccc_smc(__VA_ARGS__)
> +
> /* Make an SMCCC v1.1 compliant SMC call with guest register state. */
> static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
> {
> --
> 2.39.5
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 5/6] xen/arm: Rewrite arm_smccc_*() to return by value
2026-08-31 12:19 ` [PATCH 5/6] xen/arm: Rewrite arm_smccc_*() to return by value Andrew Cooper
@ 2026-09-03 12:21 ` Bertrand Marquis
0 siblings, 0 replies; 15+ messages in thread
From: Bertrand Marquis @ 2026-09-03 12:21 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Michal Orzel, Jan Setje-Eilers
Hi Andrew,
> On 31 Aug 2026, at 14:19, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> Use statement expressions to return struct arm_smccc_res which makes the code
> read a lot more normally, and avoids needing to pass in NULL in order to skip
> return information.
>
> More importantly, it removes the local implementation of __count_args() which
> is off by two and deeply confusing to try and follow.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
>
> Xen compiles identically before and after this change, for both arm32 and arm64.
> ---
> xen/arch/arm/cpuerrata.c | 18 +++----
> xen/arch/arm/include/asm/smccc.h | 87 ++++++++++++++------------------
> xen/arch/arm/platforms/exynos5.c | 2 +-
> xen/arch/arm/platforms/seattle.c | 4 +-
> xen/arch/arm/psci.c | 17 +++----
> xen/arch/arm/tee/optee.c | 47 +++++++++--------
> xen/arch/arm/traps.c | 4 +-
> 7 files changed, 84 insertions(+), 95 deletions(-)
>
> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> index 3a32183618dc..35ad98d29d14 100644
> --- a/xen/arch/arm/cpuerrata.c
> +++ b/xen/arch/arm/cpuerrata.c
> @@ -179,8 +179,8 @@ static int enable_smccc_arch_workaround_1(void *data)
> if ( smccc_ver < SMCCC_VERSION(1, 1) )
> goto warn;
>
> - arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID,
> - ARM_SMCCC_ARCH_WORKAROUND_1_FID, &res);
> + res = arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID,
> + ARM_SMCCC_ARCH_WORKAROUND_1_FID);
> /* The return value is in the lower 32-bits. */
> if ( (int)res.a0 < 0 )
> goto warn;
> @@ -256,8 +256,8 @@ static int enable_spectre_bhb_workaround(void *data)
> if ( smccc_ver < SMCCC_VERSION(1, 1) )
> goto warn;
>
> - arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID,
> - ARM_SMCCC_ARCH_WORKAROUND_3_FID, &res);
> + res = arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID,
> + ARM_SMCCC_ARCH_WORKAROUND_3_FID);
> /* The return value is in the lower 32-bits. */
> if ( (int)res.a0 < 0 )
> {
> @@ -398,8 +398,8 @@ static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry)
> if ( smccc_ver < SMCCC_VERSION(1, 1) )
> return false;
>
> - arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID,
> - ARM_SMCCC_ARCH_WORKAROUND_2_FID, &res);
> + res = arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID,
> + ARM_SMCCC_ARCH_WORKAROUND_2_FID);
>
> switch ( (int)res.a0 )
> {
> @@ -429,7 +429,7 @@ static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry)
> case ARM_SSBD_FORCE_DISABLE:
> printk_once("%s disabled from command-line\n", entry->desc);
>
> - arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 0, NULL);
> + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 0);
> required = false;
> break;
>
> @@ -437,7 +437,7 @@ static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry)
> if ( required )
> {
> this_cpu(ssbd_callback_required) = 1;
> - arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL);
> + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1);
> }
>
> break;
> @@ -445,7 +445,7 @@ static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry)
> case ARM_SSBD_FORCE_ENABLE:
> printk_once("%s forced from command-line\n", entry->desc);
>
> - arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL);
> + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1);
> required = true;
> break;
>
> diff --git a/xen/arch/arm/include/asm/smccc.h b/xen/arch/arm/include/asm/smccc.h
> index 8920c54b09a6..ebed2ff7c7d2 100644
> --- a/xen/arch/arm/include/asm/smccc.h
> +++ b/xen/arch/arm/include/asm/smccc.h
> @@ -95,20 +95,14 @@ struct arm_smccc_res {
> unsigned long a3;
> };
>
> -/* SMCCC v1.1 implementation madness follows */
> -#define ___count_args(_0, _1, _2, _3, _4, _5, _6, _7, _8, x, ...) x
> -
> -#define __count_args(...) \
> - ___count_args(__VA_ARGS__, 7, 6, 5, 4, 3, 2, 1, 0)
> -
> -#define __constraint_read_0 "r" (arg0)
> -#define __constraint_read_1 __constraint_read_0, "r" (arg1)
> -#define __constraint_read_2 __constraint_read_1, "r" (arg2)
> -#define __constraint_read_3 __constraint_read_2, "r" (arg3)
> -#define __constraint_read_4 __constraint_read_3, "r" (arg4)
> -#define __constraint_read_5 __constraint_read_4, "r" (arg5)
> -#define __constraint_read_6 __constraint_read_5, "r" (arg6)
> -#define __constraint_read_7 __constraint_read_6, "r" (arg7)
> +#define __constraint_read_1 "r" (arg0)
> +#define __constraint_read_2 __constraint_read_1, "r" (arg1)
> +#define __constraint_read_3 __constraint_read_2, "r" (arg2)
> +#define __constraint_read_4 __constraint_read_3, "r" (arg3)
> +#define __constraint_read_5 __constraint_read_4, "r" (arg4)
> +#define __constraint_read_6 __constraint_read_5, "r" (arg5)
> +#define __constraint_read_7 __constraint_read_6, "r" (arg6)
> +#define __constraint_read_8 __constraint_read_7, "r" (arg7)
>
> /*
> * Macro arguments MUST be evaluated before being assigned to a register
> @@ -117,44 +111,43 @@ struct arm_smccc_res {
> * This is manual register scheduling for the asm() statement, and any other
> * logic to evaluate may clobber the already-scheduled registers.
> */
> -#define __declare_arg_0(a0, res) \
> +#define __declare_arg_1(a0) \
> auto __a0 = (uint32_t)(a0); \
> - struct arm_smccc_res *___res = (res); \
> register unsigned long arg0 ASM_REG(0) = __a0
>
> -#define __declare_arg_1(a0, a1, res) \
> +#define __declare_arg_2(a0, a1) \
> auto __a1 = (a1); \
> - __declare_arg_0(a0, res); \
> + __declare_arg_1(a0); \
> register auto arg1 ASM_REG(1) = __a1
>
> -#define __declare_arg_2(a0, a1, a2, res) \
> +#define __declare_arg_3(a0, a1, a2) \
> auto __a2 = (a2); \
> - __declare_arg_1(a0, a1, res); \
> + __declare_arg_2(a0, a1); \
> register auto arg2 ASM_REG(2) = __a2
>
> -#define __declare_arg_3(a0, a1, a2, a3, res) \
> +#define __declare_arg_4(a0, a1, a2, a3) \
> auto __a3 = (a3); \
> - __declare_arg_2(a0, a1, a2, res); \
> + __declare_arg_3(a0, a1, a2); \
> register auto arg3 ASM_REG(3) = __a3
>
> -#define __declare_arg_4(a0, a1, a2, a3, a4, res) \
> +#define __declare_arg_5(a0, a1, a2, a3, a4) \
> auto __a4 = (a4); \
> - __declare_arg_3(a0, a1, a2, a3, res); \
> + __declare_arg_4(a0, a1, a2, a3); \
> register auto arg4 ASM_REG(4) = __a4
>
> -#define __declare_arg_5(a0, a1, a2, a3, a4, a5, res) \
> +#define __declare_arg_6(a0, a1, a2, a3, a4, a5) \
> auto __a5 = (a5); \
> - __declare_arg_4(a0, a1, a2, a3, a4, res); \
> + __declare_arg_5(a0, a1, a2, a3, a4); \
> register auto arg5 ASM_REG(5) = __a5
>
> -#define __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res) \
> - auto __a6 = (a6); \
> - __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
> +#define __declare_arg_7(a0, a1, a2, a3, a4, a5, a6) \
> + auto __a6 = (a6); \
> + __declare_arg_6(a0, a1, a2, a3, a4, a5); \
> register auto arg6 ASM_REG(6) = __a6
>
> -#define __declare_arg_7(a0, a1, a2, a3, a4, a5, a6, a7, res) \
> - auto __a7 = (a7); \
> - __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
> +#define __declare_arg_8(a0, a1, a2, a3, a4, a5, a6, a7) \
> + auto __a7 = (a7); \
> + __declare_arg_7(a0, a1, a2, a3, a4, a5, a6); \
> register auto arg7 ASM_REG(7) = __a7
>
> #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
> @@ -181,21 +174,20 @@ struct arm_smccc_res {
> * makes it stick.
> */
> #define arm_smccc_1_1_smc(...) \
> - do { \
> + ({ \
> register unsigned long r0 ASM_REG(0); \
> register unsigned long r1 ASM_REG(1); \
> register unsigned long r2 ASM_REG(2); \
> register unsigned long r3 ASM_REG(3); \
> - __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
> + __declare_args(count_args(__VA_ARGS__), __VA_ARGS__); \
> asm volatile ( \
> "smc #0" \
> : "=r" (r0), "=r" (r1), "=r" (r2), "=r" (r3) \
> : PASTE(__constraint_read_, \
> - __count_args(__VA_ARGS__)) \
> + count_args(__VA_ARGS__)) \
> : "memory" ); \
> - if ( ___res ) \
> - *___res = (struct arm_smccc_res){ r0, r1, r2, r3 }; \
> - } while ( 0 )
> + (struct arm_smccc_res){ r0, r1, r2, r3 }; \
> + })
>
> /*
> * The calling convention for arm32 is the same for both SMCCC v1.0 and
> @@ -208,8 +200,8 @@ static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
> {
> struct arm_smccc_res res;
>
> - arm_smccc_1_1_smc(regs->r0, regs->r1, regs->r2, regs->r3,
> - regs->r4, regs->r5, regs->r6, regs->r7, &res);
> + res = arm_smccc_1_1_smc(regs->r0, regs->r1, regs->r2, regs->r3,
> + regs->r4, regs->r5, regs->r6, regs->r7);
>
> regs->r0 = res.a0;
> regs->r1 = res.a1;
> @@ -229,7 +221,7 @@ static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
> * so imply list x4 through x17 as clobbered.
> */
> #define arm_smccc_smc(...) \
> - do { \
> + ({ \
> register unsigned long r0 ASM_REG(0); \
> register unsigned long r1 ASM_REG(1); \
> register unsigned long r2 ASM_REG(2); \
> @@ -249,7 +241,7 @@ static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
> register unsigned long c15 ASM_REG(15); \
> register unsigned long c16 ASM_REG(16); \
> register unsigned long c17 ASM_REG(17); \
> - __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
> + __declare_args(count_args(__VA_ARGS__), __VA_ARGS__); \
> asm volatile ( \
> "smc #0" \
> : "=r" (r0), "=r" (r1), "=r" (r2), "=r" (r3), \
> @@ -258,11 +250,10 @@ static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
> "=r" (c12), "=r" (c13), "=r" (c14), "=r" (c15), \
> "=r" (c16), "=r" (c17) \
> : PASTE(__constraint_read_, \
> - __count_args(__VA_ARGS__)) \
> + count_args(__VA_ARGS__)) \
> : "memory" ); \
> - if ( ___res ) \
> - *___res = (struct arm_smccc_res){ r0, r1, r2, r3 }; \
> - } while ( 0 )
> + (struct arm_smccc_res){ r0, r1, r2, r3 }; \
> + })
>
> #define arm_smccc_1_1_smc(...) arm_smccc_smc(__VA_ARGS__)
>
> @@ -271,8 +262,8 @@ static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
> {
> struct arm_smccc_res res;
>
> - arm_smccc_1_1_smc(regs->x0, regs->x1, regs->x2, regs->x3,
> - regs->x4, regs->x5, regs->x6, regs->x7, &res);
> + res = arm_smccc_1_1_smc(regs->x0, regs->x1, regs->x2, regs->x3,
> + regs->x4, regs->x5, regs->x6, regs->x7);
>
> regs->x0 = res.a0;
> regs->x1 = res.a1;
> diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
> index f7c09520675e..f08d50c1fe38 100644
> --- a/xen/arch/arm/platforms/exynos5.c
> +++ b/xen/arch/arm/platforms/exynos5.c
> @@ -249,7 +249,7 @@ static int exynos5_cpu_up(int cpu)
> iounmap(power);
>
> if ( secure_firmware )
> - arm_smccc_smc(SMC_CMD_CPU1BOOT, cpu, NULL);
> + arm_smccc_smc(SMC_CMD_CPU1BOOT, cpu);
>
> return cpu_up_send_sgi(cpu);
> }
> diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c
> index 64cc1868c24b..dfa5cf4265c0 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)
> {
> - arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_RESET, NULL);
> + arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_RESET);
> }
>
> static void seattle_system_off(void)
> {
> - arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_OFF, NULL);
> + arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_OFF);
> }
>
> PLATFORM_START(seattle, "SEATTLE")
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index b6860a776031..634d0d7467cf 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -41,8 +41,8 @@ int call_psci_cpu_on(int cpu)
> {
> struct arm_smccc_res res;
>
> - arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary),
> - &res);
> + res = arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu),
> + __pa(init_secondary));
>
> return PSCI_RET(res);
> }
> @@ -54,7 +54,7 @@ void call_psci_cpu_off(void)
> struct arm_smccc_res res;
>
> /* If successfull the PSCI cpu_off call doesn't return */
> - arm_smccc_smc(PSCI_0_2_FN32_CPU_OFF, &res);
> + res = arm_smccc_smc(PSCI_0_2_FN32_CPU_OFF);
> panic("PSCI cpu off failed for CPU%d err=%d\n", smp_processor_id(),
> PSCI_RET(res));
> }
> @@ -63,13 +63,13 @@ void call_psci_cpu_off(void)
> void call_psci_system_off(void)
> {
> if ( psci_ver > PSCI_VERSION(0, 1) )
> - arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_OFF, NULL);
> + arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_OFF);
> }
>
> void call_psci_system_reset(void)
> {
> if ( psci_ver > PSCI_VERSION(0, 1) )
> - arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_RESET, NULL);
> + arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_RESET);
> }
>
> static int __init psci_features(uint32_t psci_func_id)
> @@ -79,7 +79,7 @@ static int __init psci_features(uint32_t psci_func_id)
> if ( psci_ver < PSCI_VERSION(1, 0) )
> return PSCI_NOT_SUPPORTED;
>
> - arm_smccc_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, &res);
> + res = arm_smccc_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id);
>
> return PSCI_RET(res);
> }
> @@ -116,9 +116,8 @@ static void __init psci_init_smccc(void)
>
> if ( psci_features(ARM_SMCCC_VERSION_FID) != PSCI_NOT_SUPPORTED )
> {
> - struct arm_smccc_res res;
> + struct arm_smccc_res res = arm_smccc_smc(ARM_SMCCC_VERSION_FID);
>
> - arm_smccc_smc(ARM_SMCCC_VERSION_FID, &res);
> if ( PSCI_RET(res) != ARM_SMCCC_NOT_SUPPORTED )
> smccc_ver = PSCI_RET(res);
> }
> @@ -191,7 +190,7 @@ static int __init psci_init_0_2(void)
> }
> }
>
> - arm_smccc_smc(PSCI_0_2_FN32_PSCI_VERSION, &res);
> + res = arm_smccc_smc(PSCI_0_2_FN32_PSCI_VERSION);
> psci_ver = PSCI_RET(res);
>
> /* For the moment, we only support PSCI 0.2 and PSCI 1.x */
> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> index 3d2633237074..e38223d49801 100644
> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -178,7 +178,7 @@ static bool optee_probe(void)
> return false;
>
> /* Check UID */
> - arm_smccc_smc(ARM_SMCCC_CALL_UID_FID(TRUSTED_OS_END), &resp);
> + resp = arm_smccc_smc(ARM_SMCCC_CALL_UID_FID(TRUSTED_OS_END));
>
> if ( (uint32_t)resp.a0 != OPTEE_MSG_UID_0 ||
> (uint32_t)resp.a1 != OPTEE_MSG_UID_1 ||
You missed one conversion just after this (line 190):
arm_smccc_smc(OPTEE_SMC_GET_THREAD_COUNT, &resp);
It also needs to be converted.
> @@ -209,7 +209,7 @@ static bool optee_probe(void)
> * call. It will return OPTEE_SMC_RETURN_UNKNOWN_FUNCTION if
> * OP-TEE have no virtualization support enabled.
> */
> - arm_smccc_smc(OPTEE_SMC_VM_DESTROYED, 0, 0, 0, 0, 0, 0, 0, &resp);
> + resp = arm_smccc_smc(OPTEE_SMC_VM_DESTROYED, 0, 0, 0, 0, 0, 0, 0);
> if ( resp.a0 == OPTEE_SMC_RETURN_UNKNOWN_FUNCTION )
> return false;
>
> @@ -243,8 +243,8 @@ static int optee_domain_init(struct domain *d)
> *
> * a7 should be 0, so we can't skip last 6 parameters of arm_smccc_smc()
> */
> - arm_smccc_smc(OPTEE_SMC_VM_CREATED, OPTEE_CLIENT_ID(d), 0, 0, 0, 0, 0, 0,
> - &resp);
> + resp = arm_smccc_smc(OPTEE_SMC_VM_CREATED, OPTEE_CLIENT_ID(d),
> + 0, 0, 0, 0, 0, 0);
> if ( resp.a0 != OPTEE_SMC_RETURN_OK )
> {
> printk(XENLOG_WARNING "%pd: Unable to create OPTEE client: rc = 0x%X\n",
> @@ -681,8 +681,8 @@ static int optee_relinquish_resources(struct domain *d)
> *
> * a7 should be 0, so we can't skip last 6 parameters of arm_smccc_smc()
> */
> - arm_smccc_smc(OPTEE_SMC_VM_DESTROYED, OPTEE_CLIENT_ID(d), 0, 0, 0, 0, 0, 0,
> - &resp);
> + resp = arm_smccc_smc(OPTEE_SMC_VM_DESTROYED, OPTEE_CLIENT_ID(d),
> + 0, 0, 0, 0, 0, 0);
>
> ASSERT(!spin_is_locked(&ctx->lock));
> ASSERT(!atomic_read(&ctx->call_count));
> @@ -1171,15 +1171,14 @@ static void do_call_with_arg(struct optee_domain *ctx,
> {
> struct arm_smccc_res res;
>
> - arm_smccc_smc(a0, a1, a2, a3, a4, a5, 0, OPTEE_CLIENT_ID(current->domain),
> - &res);
> + res = arm_smccc_smc(a0, a1, a2, a3, a4, a5, 0, OPTEE_CLIENT_ID(current->domain));
This needs wrapping as it is over 80 chars.
>
> if ( OPTEE_SMC_RETURN_IS_RPC(res.a0) )
> {
> while ( handle_rpc_return(ctx, &res, regs, call) == -ERESTART )
> {
> - arm_smccc_smc(res.a0, res.a1, res.a2, res.a3, 0, 0, 0,
> - OPTEE_CLIENT_ID(current->domain), &res);
> + res = arm_smccc_smc(res.a0, res.a1, res.a2, res.a3, 0, 0, 0,
> + OPTEE_CLIENT_ID(current->domain));
>
> if ( !OPTEE_SMC_RETURN_IS_RPC(res.a0) )
> break;
> @@ -1619,8 +1618,8 @@ static void handle_exchange_capabilities(struct cpu_user_regs *regs)
> caps = get_user_reg(regs, 1);
> caps &= OPTEE_KNOWN_NSEC_CAPS;
>
> - arm_smccc_smc(OPTEE_SMC_EXCHANGE_CAPABILITIES, caps, 0, 0, 0, 0, 0,
> - OPTEE_CLIENT_ID(current->domain), &resp);
> + resp = arm_smccc_smc(OPTEE_SMC_EXCHANGE_CAPABILITIES, caps, 0, 0, 0, 0, 0,
> + OPTEE_CLIENT_ID(current->domain));
> if ( resp.a0 != OPTEE_SMC_RETURN_OK ) {
> set_user_reg(regs, 0, resp.a0);
> return;
> @@ -1664,8 +1663,8 @@ static bool optee_handle_call(struct cpu_user_regs *regs)
> return true;
>
> case OPTEE_SMC_CALLS_UID:
> - arm_smccc_smc(OPTEE_SMC_CALLS_UID, 0, 0, 0, 0, 0, 0,
> - OPTEE_CLIENT_ID(current->domain), &resp);
> + resp = arm_smccc_smc(OPTEE_SMC_CALLS_UID, 0, 0, 0, 0, 0, 0,
> + OPTEE_CLIENT_ID(current->domain));
> set_user_reg(regs, 0, resp.a0);
> set_user_reg(regs, 1, resp.a1);
> set_user_reg(regs, 2, resp.a2);
> @@ -1673,15 +1672,15 @@ static bool optee_handle_call(struct cpu_user_regs *regs)
> return true;
>
> case OPTEE_SMC_CALLS_REVISION:
> - arm_smccc_smc(OPTEE_SMC_CALLS_REVISION, 0, 0, 0, 0, 0, 0,
> - OPTEE_CLIENT_ID(current->domain), &resp);
> + resp = arm_smccc_smc(OPTEE_SMC_CALLS_REVISION, 0, 0, 0, 0, 0, 0,
> + OPTEE_CLIENT_ID(current->domain));
> set_user_reg(regs, 0, resp.a0);
> set_user_reg(regs, 1, resp.a1);
> return true;
>
> case OPTEE_SMC_CALL_GET_OS_UUID:
> - arm_smccc_smc(OPTEE_SMC_CALL_GET_OS_UUID, 0, 0, 0, 0, 0, 0,
> - OPTEE_CLIENT_ID(current->domain),&resp);
> + resp = arm_smccc_smc(OPTEE_SMC_CALL_GET_OS_UUID, 0, 0, 0, 0, 0, 0,
> + OPTEE_CLIENT_ID(current->domain));
> set_user_reg(regs, 0, resp.a0);
> set_user_reg(regs, 1, resp.a1);
> set_user_reg(regs, 2, resp.a2);
> @@ -1689,21 +1688,21 @@ static bool optee_handle_call(struct cpu_user_regs *regs)
> return true;
>
> case OPTEE_SMC_CALL_GET_OS_REVISION:
> - arm_smccc_smc(OPTEE_SMC_CALL_GET_OS_REVISION, 0, 0, 0, 0, 0, 0,
> - OPTEE_CLIENT_ID(current->domain), &resp);
> + resp = arm_smccc_smc(OPTEE_SMC_CALL_GET_OS_REVISION, 0, 0, 0, 0, 0, 0,
> + OPTEE_CLIENT_ID(current->domain));
> set_user_reg(regs, 0, resp.a0);
> set_user_reg(regs, 1, resp.a1);
> return true;
>
> case OPTEE_SMC_ENABLE_SHM_CACHE:
> - arm_smccc_smc(OPTEE_SMC_ENABLE_SHM_CACHE, 0, 0, 0, 0, 0, 0,
> - OPTEE_CLIENT_ID(current->domain), &resp);
> + resp = arm_smccc_smc(OPTEE_SMC_ENABLE_SHM_CACHE, 0, 0, 0, 0, 0, 0,
> + OPTEE_CLIENT_ID(current->domain));
> set_user_reg(regs, 0, resp.a0);
> return true;
>
> case OPTEE_SMC_DISABLE_SHM_CACHE:
> - arm_smccc_smc(OPTEE_SMC_DISABLE_SHM_CACHE, 0, 0, 0, 0, 0, 0,
> - OPTEE_CLIENT_ID(current->domain), &resp);
> + resp = arm_smccc_smc(OPTEE_SMC_DISABLE_SHM_CACHE, 0, 0, 0, 0, 0, 0,
> + OPTEE_CLIENT_ID(current->domain));
> set_user_reg(regs, 0, resp.a0);
> if ( resp.a0 == OPTEE_SMC_RETURN_OK ) {
> free_shm_rpc(ctx, regpair_to_uint64(resp.a1, resp.a2));
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 625d229396bb..6a5dcef2aa87 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1991,7 +1991,7 @@ void asmlinkage enter_hypervisor_from_guest_preirq(void)
>
> /* If the guest has disabled the workaround, bring it back on. */
> if ( needs_ssbd_flip(v) )
> - arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL);
> + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1);
> }
>
> /*
> @@ -2334,7 +2334,7 @@ void asmlinkage leave_hypervisor_to_guest(void)
> * If the guest wants it disabled, so be it...
> */
> if ( needs_ssbd_flip(current) )
> - arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 0, NULL);
> + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 0);
> }
>
> /*
> --
> 2.39.5
>
Cheers
Bertrand
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/6] xen/arm: Rewrite arm_smccc_smc() for arm64
2026-09-03 12:16 ` Bertrand Marquis
@ 2026-09-04 10:50 ` Andrew Cooper
0 siblings, 0 replies; 15+ messages in thread
From: Andrew Cooper @ 2026-09-04 10:50 UTC (permalink / raw)
To: Bertrand Marquis
Cc: Andrew Cooper, Xen-devel, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Michal Orzel, Jan Setje-Eilers
On 03/09/2026 1:16 pm, Bertrand Marquis wrote:
> Hi Andrew,
>
>> On 31 Aug 2026, at 14:19, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>
>> PSCI v1.0 says that x4 thru x17 may be clobbered. PSCI v1.1 says they are
>> strictly preserved unless they contain return values.
> I think the PSCI reference here and in the rest of the commit message and the patch
> should not be used and should be SMCCC references instead (which would also be
> more coherent with the fact that this code is not PSCI but SMCCC and is used by
> others than PSCI).
>
> PSCI does not define the preservation rules, this is defined in SMCCC spec and we
> have a way to detect the SMCCC version.
>
> SMCCC defines the following rules:
> - 1.0: x4-x17 have an unpredictable return state.
> - 1.1: x4-x17 must be preserved.
> - 1.2: x4-x17 may additionally be defined as result registers, otherwise they remain preserved.
>
> So the commit message and comments should consistently say SMCCC instead of PSCI.
>
> Other than this the code is right i think.
My apologies, I've clearly got confused somewhere. (This is the first
time I've ever looked at the SMCCC spec.) I'll adjust.
~Andrew
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2026-09-04 10:50 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-31 12:19 [PATCH 0/5] xen/arm: Fixes and improvements to SMCCC Andrew Cooper
2026-08-31 12:19 ` [PATCH 1/6] xen/arm: Fix evaluation of parameters for SMCCC calls Andrew Cooper
2026-09-03 11:46 ` Bertrand Marquis
2026-08-31 12:19 ` [PATCH 2/6] xen/arm: Introduce arm_smccc_guest_smc() Andrew Cooper
2026-09-03 11:49 ` Bertrand Marquis
2026-08-31 12:19 ` [PATCH 3/6] xen/arm: Clean up 32bit arm_smccc_1_1_smc() Andrew Cooper
2026-09-03 11:52 ` Bertrand Marquis
2026-08-31 12:19 ` [PATCH 4/6] xen/arm: Rewrite arm_smccc_smc() for arm64 Andrew Cooper
2026-09-03 12:16 ` Bertrand Marquis
2026-09-04 10:50 ` Andrew Cooper
2026-08-31 12:19 ` [PATCH 5/6] xen/arm: Rewrite arm_smccc_*() to return by value Andrew Cooper
2026-09-03 12:21 ` Bertrand Marquis
2026-09-03 9:20 ` [PATCH 0/5] xen/arm: Fixes and improvements to SMCCC Bertrand Marquis
2026-09-03 9:35 ` Andrew Cooper
2026-09-03 10:26 ` Bertrand Marquis
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.