Re: [PATCH v4 07/11] arm: traps: handle PSCI calls inside `vsmc.c`

[Julien Grall <julien.grall@arm.com>]

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 *)&regs->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, &regs->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,

>
>  /*
>

-- 
Julien Grall

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