[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v4 07/11] arm: traps: handle PSCI calls inside `vsmc.c`
Hi Julien,
Thanks for the review.
On 24.08.17 19:58, Julien Grall wrote:
Hi Volodymyr,
On 21/08/17 21:27, Volodymyr Babchuk wrote:
PSCI is part of HVC/SMC interface, so it should be handled in
appropriate place: `vsmc.c`. This patch just moves PSCI
handler calls from `traps.c` to `vsmc.c`.
Older PSCI 0.1 uses SMC function identifiers in range that is
resereved for exisings APIs (ARM DEN 0028B, page 16), while newer
s/resereved/reserved/
s/exisings/existing/
PSCI 0.2 is defined as "standard secure service" with own ranges
0.2 and later
"with its own ranges" I think.
(ARM DEN 0028B, page 18).
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
---
* Fixed mistakes about non-existant PSCI 2.0
* Added special SMC function number handling for PSCI 0.1
* Fixed coding style in handle_psci_0_1()
* Changed how return do_trap_hvc_smccc() is called from traps.c
* Renamed SSC to SSSC (Standard Secure Service Calls)
---
xen/arch/arm/traps.c | 117 +------------------------
xen/arch/arm/vsmc.c | 175
++++++++++++++++++++++++++++++++++++--
xen/include/asm-arm/smccc.h | 4 +
xen/include/asm-arm/vsmc.h | 1 +
xen/include/public/arch-arm/smc.h | 8 ++
5 files changed, 183 insertions(+), 122 deletions(-)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 4141a89..517e013 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1451,119 +1451,6 @@ static void do_debug_trap(struct cpu_user_regs
*regs, unsigned int code)
}
#endif
-#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
-#define PSCI_ARG(reg, n) get_user_reg(reg, n)
-
-#ifdef CONFIG_ARM_64
-#define PSCI_ARG32(reg, n) (uint32_t)(get_user_reg(reg, n) & 0xFFFFFFFF)
-#else
-#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
-#endif
-
-/* helper function for checking arm mode 32/64 bit */
-static inline int psci_mode_check(struct domain *d, register_t fid)
-{
- return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
-}
-
-static void do_trap_psci(struct cpu_user_regs *regs)
-{
- register_t fid = PSCI_ARG(regs,0);
-
- /* preloading in case psci_mode_check fails */
- PSCI_SET_RESULT(regs, PSCI_INVALID_PARAMETERS);
- switch( fid )
- {
- case PSCI_cpu_off:
- {
- uint32_t pstate = PSCI_ARG32(regs,1);
- perfc_incr(vpsci_cpu_off);
- PSCI_SET_RESULT(regs, do_psci_cpu_off(pstate));
- }
- break;
- case PSCI_cpu_on:
- {
- uint32_t vcpuid = PSCI_ARG32(regs,1);
- register_t epoint = PSCI_ARG(regs,2);
- perfc_incr(vpsci_cpu_on);
- PSCI_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint));
- }
- break;
- case PSCI_0_2_FN_PSCI_VERSION:
- perfc_incr(vpsci_version);
- PSCI_SET_RESULT(regs, do_psci_0_2_version());
- break;
- case PSCI_0_2_FN_CPU_OFF:
- perfc_incr(vpsci_cpu_off);
- PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
- break;
- case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
- perfc_incr(vpsci_migrate_info_type);
- PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type());
- break;
- case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU:
- case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
- perfc_incr(vpsci_migrate_info_up_cpu);
- if ( psci_mode_check(current->domain, fid) )
- PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
- break;
- case PSCI_0_2_FN_SYSTEM_OFF:
- perfc_incr(vpsci_system_off);
- do_psci_0_2_system_off();
- PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
- break;
- case PSCI_0_2_FN_SYSTEM_RESET:
- perfc_incr(vpsci_system_reset);
- do_psci_0_2_system_reset();
- PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
- break;
- case PSCI_0_2_FN_CPU_ON:
- case PSCI_0_2_FN64_CPU_ON:
- perfc_incr(vpsci_cpu_on);
- if ( psci_mode_check(current->domain, fid) )
- {
- register_t vcpuid = PSCI_ARG(regs,1);
- register_t epoint = PSCI_ARG(regs,2);
- register_t cid = PSCI_ARG(regs,3);
- PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint,
cid));
- }
- break;
- case PSCI_0_2_FN_CPU_SUSPEND:
- case PSCI_0_2_FN64_CPU_SUSPEND:
- perfc_incr(vpsci_cpu_suspend);
- if ( psci_mode_check(current->domain, fid) )
- {
- uint32_t pstate = PSCI_ARG32(regs,1);
- register_t epoint = PSCI_ARG(regs,2);
- register_t cid = PSCI_ARG(regs,3);
- PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate,
epoint, cid));
- }
- break;
- case PSCI_0_2_FN_AFFINITY_INFO:
- case PSCI_0_2_FN64_AFFINITY_INFO:
- perfc_incr(vpsci_cpu_affinity_info);
- if ( psci_mode_check(current->domain, fid) )
- {
- register_t taff = PSCI_ARG(regs,1);
- uint32_t laff = PSCI_ARG32(regs,2);
- PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff,
laff));
- }
- break;
- case PSCI_0_2_FN_MIGRATE:
- case PSCI_0_2_FN64_MIGRATE:
- perfc_incr(vpsci_cpu_migrate);
- if ( psci_mode_check(current->domain, fid) )
- {
- uint32_t tcpu = PSCI_ARG32(regs,1);
- PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
- }
- break;
- default:
- domain_crash_synchronous();
- return;
- }
-}
-
#ifdef CONFIG_ARM_64
#define HYPERCALL_RESULT_REG(r) (r)->x0
#define HYPERCALL_ARG1(r) (r)->x0
@@ -2252,7 +2139,7 @@ asmlinkage void do_trap_guest_sync(struct
cpu_user_regs *regs)
return do_debug_trap(regs, hsr.iss & 0x00ff);
#endif
if ( hsr.iss == 0 )
- return do_trap_psci(regs);
+ return do_trap_hvc_smccc(regs);
do_trap_hypercall(regs, (register_t *)®s->r12, hsr.iss);
break;
#ifdef CONFIG_ARM_64
@@ -2264,7 +2151,7 @@ asmlinkage void do_trap_guest_sync(struct
cpu_user_regs *regs)
return do_debug_trap(regs, hsr.iss & 0x00ff);
#endif
if ( hsr.iss == 0 )
- return do_trap_psci(regs);
+ return do_trap_hvc_smccc(regs);
do_trap_hypercall(regs, ®s->x16, hsr.iss);
break;
case HSR_EC_SMC64:
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 0a81294..956d4ef 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -19,6 +19,7 @@
#include <xen/types.h>
#include <public/arch-arm/smc.h>
#include <asm/monitor.h>
+#include <asm/psci.h>
#include <asm/regs.h>
#include <asm/smccc.h>
#include <asm/traps.h>
@@ -27,6 +28,9 @@
/* Number of functions currently supported by Hypervisor Service. */
#define XEN_SMCCC_FUNCTION_COUNT 3
+/* Number of functions currently supported by Standard Service
Service Calls. */
+#define SSSC_SMCCC_FUNCTION_COUNT 13
+
static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t u)
{
#define FILL_UUID(n) \
@@ -62,6 +66,133 @@ static bool handle_hypervisor(struct cpu_user_regs
*regs)
return false;
}
+#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
+#define PSCI_ARG(reg, n) get_user_reg(reg, n)
+
+#ifdef CONFIG_ARM_64
+#define PSCI_ARG32(reg, n) (uint32_t)(get_user_reg(reg, n) & 0xFFFFFFFF)
+#else
+#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
+#endif
+
+/* PSCI 0.1 interface */
+static bool handle_psci_0_1(struct cpu_user_regs *regs)
+{
+ switch ( get_user_reg(regs,0) & 0xFFFFFFFF )
I am not sure to understand the mask here.
+ {
+ case PSCI_cpu_off:
+ {
+ uint32_t pstate = PSCI_ARG32(regs, 1);
Missing newline here. I am ok with you do the clean-up in this patch
rather separately.
+ perfc_incr(vpsci_cpu_off);
+ PSCI_SET_RESULT(regs, do_psci_cpu_off(pstate));
+ return true;
+ }
+ case PSCI_cpu_on:
+ {
+ uint32_t vcpuid = PSCI_ARG32(regs, 1);
+ register_t epoint = PSCI_ARG(regs, 2);
Ditto.
+ perfc_incr(vpsci_cpu_on);
+ PSCI_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint));
+ return true;
+ }
+ }
+ return false;
+}
+
+/* helper function for checking arm mode 32/64 bit */
+static inline int psci_mode_check(struct domain *d, register_t fid)
+{
+ return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
+}
+
+/* PSCI 0.2 interface and other Standard Secure Calls */
+static bool handle_sssc(struct cpu_user_regs *regs)
+{
+ register_t fid = PSCI_ARG(regs, 0);
+
+ switch ( ARM_SMCCC_FUNC_NUM(fid) )
+ {
+ case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_PSCI_VERSION):
+ perfc_incr(vpsci_version);
+ PSCI_SET_RESULT(regs, do_psci_0_2_version());
+ return true;
It is a bit hard to read. Some newline between each "case" would help
for clarity I think.
+ case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_OFF):
+ perfc_incr(vpsci_cpu_off);
+ PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
+ return true;
+ case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_TYPE):
+ perfc_incr(vpsci_migrate_info_type);
+ PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type());
+ return true;
+ case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
+ perfc_incr(vpsci_migrate_info_up_cpu);
+ if ( psci_mode_check(current->domain, fid) )
+ PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
+ return true;
+ case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
+ perfc_incr(vpsci_system_off);
+ do_psci_0_2_system_off();
+ PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
+ return true;
+ case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_RESET):
+ perfc_incr(vpsci_system_reset);
+ do_psci_0_2_system_reset();
+ PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
+ return true;
+ case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_ON):
+ perfc_incr(vpsci_cpu_on);
+ if ( psci_mode_check(current->domain, fid) )
+ {
+ register_t vcpuid = PSCI_ARG(regs, 1);
+ register_t epoint = PSCI_ARG(regs, 2);
+ register_t cid = PSCI_ARG(regs, 3);
+ PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint,
cid));
+ }
+ return true;
+ case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_SUSPEND):
+ perfc_incr(vpsci_cpu_suspend);
+ if ( psci_mode_check(current->domain, fid) )
+ {
+ uint32_t pstate = PSCI_ARG32(regs, 1);
+ register_t epoint = PSCI_ARG(regs, 2);
+ register_t cid = PSCI_ARG(regs, 3);
+ PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate,
epoint, cid));
+ }
+ return true;
+ case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_AFFINITY_INFO):
+ perfc_incr(vpsci_cpu_affinity_info);
+ if ( psci_mode_check(current->domain, fid) )
+ {
+ register_t taff = PSCI_ARG(regs, 1);
+ uint32_t laff = PSCI_ARG32(regs, 2);
+ PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff,
laff));
+ }
+ return true;
+ case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE):
+ perfc_incr(vpsci_cpu_migrate);
+ if ( psci_mode_check(current->domain, fid) )
+ {
+ uint32_t tcpu = PSCI_ARG32(regs, 1);
+ PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
+ }
+ return true;
+ case ARM_SMCCC_FUNC_CALL_COUNT:
+ set_user_reg(regs, 0, SSSC_SMCCC_FUNCTION_COUNT);
+ return true;
+ case ARM_SMCCC_FUNC_CALL_UID:
+ {
+ static const xen_uuid_t psci_uuid = SSSC_SMCCC_UID;
Newline here please. But can't we just do:
return fill_uuid(regs, SSC_SMCCC_UID);
This would make the code simpler.
I'm not sure that I got this. fill_uuid() returns void, while
handle_sssc() returns bool.
+ fill_uuid(regs, psci_uuid);
+ return true;
+ }
+ case ARM_SMCCC_FUNC_CALL_REVISION:
+ set_user_reg(regs, 0, SSSC_SMCCC_MAJOR_REVISION);
+ set_user_reg(regs, 1, SSSC_SMCCC_MINOR_REVISION);
Same here.
Under "same" you meant newline, correct?
+ return true;
+ }
+ return false;
+}
+
/*
* vsmccc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
* returns true if that was valid SMCCC call (even if function number
@@ -71,6 +202,7 @@ static bool vsmccc_handle_call(struct cpu_user_regs
*regs)
{
bool handled = false;
const union hsr hsr = { .bits = regs->hsr };
+ register_t func_id = get_user_reg(regs, 0);
Please introduce func_id in patch #6 rather than doing this rework here.
/*
* Check immediate value for HVC32, HVC64 and SMC64.
@@ -94,24 +226,38 @@ static bool vsmccc_handle_call(struct
cpu_user_regs *regs)
}
/* 64 bit calls are allowed only from 64 bit domains. */
- if ( ARM_SMCCC_IS_64(get_user_reg(regs, 0)) &&
+ if ( ARM_SMCCC_IS_64(func_id) &&
is_32bit_domain(current->domain) )
{
set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
return true;
}
- switch ( ARM_SMCCC_OWNER_NUM(get_user_reg(regs, 0)) )
+ /*
+ * Special case: identifier range for existing APIs.
+ * This range is described in SMCCC (ARM DEN 0028B, page 16),
+ * but it does not conforms to standard function identifier
+ * encoding.
+ */
+ if ( func_id >= ARM_SMCCC_RESERVED_RANGE_START &&
+ func_id <= ARM_SMCCC_RESERVED_RANGE_END )
+ handled = handle_psci_0_1(regs);
The region "reserved for existing APIs" is not only for psci 0.1. You
should make it clear in the name.
+ else
{
- case ARM_SMCCC_OWNER_HYPERVISOR:
- handled = handle_hypervisor(regs);
- break;
+ switch ( ARM_SMCCC_OWNER_NUM(func_id) )
+ {
+ case ARM_SMCCC_OWNER_HYPERVISOR:
+ handled = handle_hypervisor(regs);
+ break;
+ case ARM_SMCCC_OWNER_STANDARD:
+ handled = handle_sssc(regs);
+ break;
+ }
}
if ( !handled )
{
- gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n",
- get_user_reg(regs, 0));
+ gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n",
func_id);
/* Inform caller that function is not supported. */
set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
}
@@ -150,6 +296,21 @@ void do_trap_smc(struct cpu_user_regs *regs,
const union hsr hsr)
inject_undef_exception(regs, hsr);
}
+/* This function will be called from traps.c */
+void do_trap_hvc_smccc(struct cpu_user_regs *regs)
+{
+ const union hsr hsr = { .bits = regs->hsr };
+
+ /*
+ * vsmccc_handle_call() will return false if this call is not
+ * SMCCC compatbile (i.e. immediate value != 0). As it is not
s/compatbile/compatible/. And you want to use e.g instead of i.e because
"immediate value != 0" is not the only way to return false.
+ * compatible, we can't be sure that guest will understand
+ * ARM_SMCCC_ERR_UNKNOWN_FUNCTION.
+ */
+ if ( !vsmccc_handle_call(regs) )
+ inject_undef_exception(regs, hsr);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index 67da3fb..7c0003c 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -80,6 +80,10 @@
/* Only one error code defined in SMCCC */
#define ARM_SMCCC_ERR_UNKNOWN_FUNCTION (-1)
+/* SMCCC function identifier range which is reserved for existing
APIs */
+#define ARM_SMCCC_RESERVED_RANGE_START 0x0
+#define ARM_SMCCC_RESERVED_RANGE_END 0x0100FFFF
+
#endif /* __ASM_ARM_SMCCC_H__ */
/*
diff --git a/xen/include/asm-arm/vsmc.h b/xen/include/asm-arm/vsmc.h
index 31aaa55..90ff610 100644
--- a/xen/include/asm-arm/vsmc.h
+++ b/xen/include/asm-arm/vsmc.h
@@ -17,6 +17,7 @@
#include <xen/types.h>
void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr);
+void do_trap_hvc_smccc(struct cpu_user_regs *regs);
#endif /* __ASM_ARM_VSMC_H__ */
diff --git a/xen/include/public/arch-arm/smc.h
b/xen/include/public/arch-arm/smc.h
index 3d3cd90..c5327e3 100644
--- a/xen/include/public/arch-arm/smc.h
+++ b/xen/include/public/arch-arm/smc.h
@@ -46,6 +46,14 @@
#define XEN_SMCCC_UID XEN_DEFINE_UUID(0xa71812dc, 0xc698, 0x4369,
0x9acf, \
0x79, 0xd1, 0x8d, 0xde, 0xe6, 0x67)
+/* Standard Service Service Call version. */
+#define SSSC_SMCCC_MAJOR_REVISION 0
+#define SSSC_SMCCC_MINOR_REVISION 1
+
+/* Standard Service Call UID. Randomly generated with uuidgen. */
+#define SSSC_SMCCC_UID XEN_DEFINE_UUID(0xf863386f, 0x4b39, 0x4cbd,
0x9220,\
+ 0xce, 0x16, 0x41, 0xe5, 0x9f, 0x6f)
+
#endif /* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */
Cheers,
/*
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
