|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 3/4] arm: traps: handle PSCI calls inside `vsmc.c`
On Thu, 22 Jun 2017, 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`.
>
> PSCI is considered as two different "services" in terms of SMCCC.
> Older PSCI 1.0 is treated as "architecture service", while never
> PSCI 2.0 is defined as "standard secure service".
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> ---
> Splitted this patch into two. Now this patch does not change the way,
> how PSCI code accesses the arguments.
> ---
> xen/arch/arm/traps.c | 124 ++++------------------------------
> xen/arch/arm/vsmc.c | 136
> ++++++++++++++++++++++++++++++++++++++
> xen/include/public/arch-arm/smc.h | 5 ++
> 3 files changed, 153 insertions(+), 112 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 66242e5..e806474 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -39,7 +39,6 @@
> #include <asm/event.h>
> #include <asm/regs.h>
> #include <asm/cpregs.h>
> -#include <asm/psci.h>
> #include <asm/mmio.h>
> #include <asm/cpufeature.h>
> #include <asm/flushtlb.h>
> @@ -1450,113 +1449,6 @@ static void do_debug_trap(struct cpu_user_regs *regs,
> unsigned int code)
> }
> #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 = get_user_reg(regs,0);
> -
> - /* preloading in case psci_mode_check fails */
> - set_user_reg(regs, 0, PSCI_INVALID_PARAMETERS);
> - switch( fid )
> - {
> - case PSCI_cpu_off:
> - {
> - uint32_t pstate = get_user_reg(regs, 1);
> - perfc_incr(vpsci_cpu_off);
> - set_user_reg(regs, 0, do_psci_cpu_off(pstate));
> - }
> - break;
> - case PSCI_cpu_on:
> - {
> - uint32_t vcpuid = get_user_reg(regs, 1);
> - register_t epoint = get_user_reg(regs, 2);
> - perfc_incr(vpsci_cpu_on);
> - set_user_reg(regs, 0, do_psci_cpu_on(vcpuid, epoint));
> - }
> - break;
> - case PSCI_0_2_FN_PSCI_VERSION:
> - perfc_incr(vpsci_version);
> - set_user_reg(regs, 0, do_psci_0_2_version());
> - break;
> - case PSCI_0_2_FN_CPU_OFF:
> - perfc_incr(vpsci_cpu_off);
> - set_user_reg(regs, 0, do_psci_0_2_cpu_off());
> - break;
> - case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
> - perfc_incr(vpsci_migrate_info_type);
> - set_user_reg(regs, 0, 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) )
> - set_user_reg(regs, 0, 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();
> - set_user_reg(regs, 0, PSCI_INTERNAL_FAILURE);
> - break;
> - case PSCI_0_2_FN_SYSTEM_RESET:
> - perfc_incr(vpsci_system_reset);
> - do_psci_0_2_system_reset();
> - set_user_reg(regs, 0, 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 = get_user_reg(regs, 1);
> - register_t epoint = get_user_reg(regs, 2);
> - register_t cid = get_user_reg(regs, 3);
> - set_user_reg(regs, 0,
> - 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 = get_user_reg(regs, 1);
> - register_t epoint = get_user_reg(regs, 2);
> - register_t cid = get_user_reg(regs, 3);
> - set_user_reg(regs, 0,
> - 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 = get_user_reg(regs, 1);
> - uint32_t laff = get_user_reg(regs, 2);
> - set_user_reg(regs, 0,
> - 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 = get_user_reg(regs, 1);
> - set_user_reg(regs, 0, 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
> @@ -2888,8 +2780,12 @@ 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);
> - do_trap_hypercall(regs, (register_t *)®s->r12, hsr.iss);
> + {
> + if ( !vsmc_handle_call(regs) )
> + domain_crash_synchronous();
> + }
> + else
> + do_trap_hypercall(regs, (register_t *)®s->r12, hsr.iss);
> break;
> #ifdef CONFIG_ARM_64
> case HSR_EC_HVC64:
> @@ -2900,8 +2796,12 @@ 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);
> - do_trap_hypercall(regs, ®s->x16, hsr.iss);
> + {
> + if ( !vsmc_handle_call(regs) )
> + domain_crash_synchronous();
> + }
> + else
> + 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 10c4acd..5f10fd1 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -22,6 +22,7 @@
> #include <xen/stdbool.h>
> #include <xen/types.h>
> #include <public/arch-arm/smc.h>
> +#include <asm/psci.h>
> #include <asm/vsmc.h>
> #include <asm/regs.h>
>
> @@ -43,6 +44,14 @@
> /* Number of functions currently supported by Hypervisor Service. */
> #define XEN_SMCCC_FUNCTION_COUNT 3
>
> +/* Standard Service version. Check comment for Hypervisor Service for rules
> */
> +#define SSC_SMCCC_MAJOR_REVISION 0
> +#define SSC_SMCCC_MINOR_REVISION 1
> +
> +/* Number of functions currently supported by Standard Service Service. */
> +#define SSC_SMCCC_FUNCTION_COUNT 13
> +
> +
> /* SMCCC interface for hypervisor. Tell about itself. */
> static bool handle_hypervisor(struct cpu_user_regs *regs)
> {
> @@ -65,6 +74,127 @@ static bool handle_hypervisor(struct cpu_user_regs *regs)
> return false;
> }
>
> +/* old (arvm7) PSCI interface */
> +static bool handle_arch(struct cpu_user_regs *regs)
> +{
> + switch ( get_user_reg(regs,0) & 0xFFFFFFFF )
> + {
> + case PSCI_cpu_off:
> + {
> + uint32_t pstate = get_user_reg(regs, 1);
> + perfc_incr(vpsci_cpu_off);
> + set_user_reg(regs, 0, do_psci_cpu_off(pstate));
> + }
> + return true;
Don't you want the `return true' to be within the { } block? It looks
weird this way.
> + case PSCI_cpu_on:
> + {
> + uint32_t vcpuid = get_user_reg(regs, 1);
> + register_t epoint = get_user_reg(regs, 2);
> + perfc_incr(vpsci_cpu_on);
> + set_user_reg(regs, 0, do_psci_cpu_on(vcpuid, epoint));
> + }
> + return true;
Same here.
> + }
> + 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 2.0 interface */
> +static bool handle_ssc(struct cpu_user_regs *regs)
> +{
> + register_t fid = get_user_reg(regs, 0);
> +
> + switch ( ARM_SMCCC_FUNC_NUM(fid) )
> + {
> + case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_PSCI_VERSION):
As we are not using the PSCI_0_2_FN64_* definitions anymore, it would
make sense to add a comment in psci.h on top of them to explain why they
are not used (the function number is the same as the 32-bit
counterpart).
> + perfc_incr(vpsci_version);
> + set_user_reg(regs, 0, do_psci_0_2_version());
> + return true;
> + case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_OFF):
> + perfc_incr(vpsci_cpu_off);
> + set_user_reg(regs, 0, 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);
> + set_user_reg(regs, 0, 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) )
> + set_user_reg(regs, 0, 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();
> + set_user_reg(regs, 0, 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();
> + set_user_reg(regs, 0, 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 = get_user_reg(regs, 1);
> + register_t epoint = get_user_reg(regs, 2);
> + register_t cid = get_user_reg(regs, 3);
> + set_user_reg(regs, 0,
> + 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 = get_user_reg(regs, 1);
> + register_t epoint = get_user_reg(regs, 2);
> + register_t cid = get_user_reg(regs, 3);
> + set_user_reg(regs, 0,
> + 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 = get_user_reg(regs, 1);
> + uint32_t laff = get_user_reg(regs,2);
> + set_user_reg(regs, 0,
> + 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 = get_user_reg(regs, 1);
> + set_user_reg(regs, 0, do_psci_0_2_migrate(tcpu));
> + }
> + return true;
> + case ARM_SMCCC_FUNC_CALL_COUNT:
> + set_user_reg(regs, 0, SSC_SMCCC_FUNCTION_COUNT);
> + return true;
> + case ARM_SMCCC_FUNC_CALL_UID:
> + set_user_reg(regs, 0, SSC_SMCCC_UID.a[0]);
> + set_user_reg(regs, 1, SSC_SMCCC_UID.a[1]);
> + set_user_reg(regs, 2, SSC_SMCCC_UID.a[2]);
> + set_user_reg(regs, 3, SSC_SMCCC_UID.a[3]);
> + return true;
> + case ARM_SMCCC_FUNC_CALL_REVISION:
> + set_user_reg(regs, 0, SSC_SMCCC_MAJOR_REVISION);
> + set_user_reg(regs, 1, SSC_SMCCC_MINOR_REVISION);
> + return true;
> + }
> + return false;
> +}
> +
> /**
> * vsmc_handle_call() - handle SMC/HVC call according to ARM SMCCC
> */
> @@ -105,6 +235,12 @@ int vsmc_handle_call(struct cpu_user_regs *regs)
> case ARM_SMCCC_OWNER_HYPERVISOR:
> handled = handle_hypervisor(regs);
> break;
> + case ARM_SMCCC_OWNER_ARCH:
> + handled = handle_arch(regs);
> + break;
> + case ARM_SMCCC_OWNER_STANDARD:
> + handled = handle_ssc(regs);
> + break;
> }
>
NIT: Remove double space in the patch where it was introduced
> if ( !handled )
> diff --git a/xen/include/public/arch-arm/smc.h
> b/xen/include/public/arch-arm/smc.h
> index aac292a..6a5a556 100644
> --- a/xen/include/public/arch-arm/smc.h
> +++ b/xen/include/public/arch-arm/smc.h
> @@ -42,4 +42,9 @@ typedef struct {
> 0x9a, 0xcf, 0x79, 0xd1, \
> 0x8d, 0xde, 0xe6, 0x67)
>
NIT: Remove double space in the patch where it was introduced
> +/* Standard Service Call UID. Randomly generated with 3rd party tool */
> +#define SSC_SMCCC_UID ARM_SMCCC_UID(0xf863386f, 0x4b39, 0x4cbd, \
> + 0x92, 0x20, 0xce, 0x16, \
> + 0x41, 0xe5, 0x9f, 0x6f)
NIT: please align the \
These are only minor issues. The patch looks pretty good.
> #endif /* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */
> --
> 2.7.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |