| Message ID | 1503347275-13039-8-git-send-email-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@epam.com> > Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > --- > > * 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. > + 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. > + 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, > > /* >
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@epam.com> >> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> --- >> >> * 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, > >> >> /* >> >
On 25/08/17 11:56, Volodymyr Babchuk wrote: > Hi Julien, Hi Volodymyr, > On 24.08.17 19:58, Julien Grall wrote: >>> + 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. You could make fill_uuid() returning a bool. Overall every caller of fill_uuid will do: fill_uuid(....); return true; > >>> + 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? Hmmm, I don't remember :/. Although, we may want to introduce a helper to fill the revision. Those wrapper would simplify the implementation of "generic" function. Cheers,
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 ) + { + 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)); + return true; + } + 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)); + 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; + 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; + 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); + 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); /* * 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); + 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 + * 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__ */ /*