Re: [PATCH v2 3/4] arm: traps: handle PSCI calls inside `vsmc.c`

[Stefano Stabellini <sstabellini@kernel.org>]

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@epam.com>
> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
> 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 *)&regs->r12, hsr.iss);
> +        {
> +            if ( !vsmc_handle_call(regs) )
> +                domain_crash_synchronous();
> +        }
> +        else
> +            do_trap_hypercall(regs, (register_t *)&regs->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, &regs->x16, hsr.iss);
> +        {
> +            if ( !vsmc_handle_call(regs) )
> +                domain_crash_synchronous();
> +        }
> +        else
> +            do_trap_hypercall(regs, &regs->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@lists.xen.org
> https://lists.xen.org/xen-devel
> 

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