Re: [PATCH v4 08/11] arm: PSCI: use definitions provided by asm/smccc.h

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

Hi Volodymyr,

On 21/08/17 21:27, Volodymyr Babchuk wrote:
> smccc.h provides definitions to construct SMC call function number according
> to SMCCC. We don't need multiple definitions for one thing, and definitions
> in smccc.h are more generic than ones used in psci.h.
>
> So psci.h will only provide function codes, while whole SMC function
> identifier will be constructed using generic macros from smccc.h.
>
> PSCI_0_2_FN_xxx was deliberately renamed to PSCI_0_2_FUNC_xxx, because this
> is a new entity. It can lead to problems, if we'll just change value of
> PSCI_0_2_FN_xxx without renaming it.

I don't think "new entity" is a good reason to rename them. And the 
previous naming was kind of nice to read. More that you still use 
PSCI_0_2_FN{32,64}. We should definitely stay consistent in naming.

So what is the exact problem? Is it because you are worry to miss some 
of them?

>
> This change also affects vsmc.c and seattle.c, because they both use PSCI
> function numbers.

This paragraph could be dropped.

>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>
>  * Reworked definitions to minimize their lenght
>  * Described why I replaced PSCI_0_2_FN_xxx with PSCI_0_2_FUNC_xxx
>
> ---
>  xen/arch/arm/platforms/seattle.c |  5 +++--
>  xen/arch/arm/psci.c              | 10 ++++-----
>  xen/arch/arm/vsmc.c              | 24 +++++++++++-----------
>  xen/include/asm-arm/psci.h       | 44 ++++++++++++++++++----------------------
>  4 files changed, 40 insertions(+), 43 deletions(-)
>
> diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c
> index 86dce91..fb2ad13 100644
> --- a/xen/arch/arm/platforms/seattle.c
> +++ b/xen/arch/arm/platforms/seattle.c
> @@ -19,6 +19,7 @@
>
>  #include <asm/platform.h>
>  #include <asm/psci.h>
> +#include <asm/vsmc.h>

Again, vsmc.h stands for "virtual SMC". Here you use for the "physical 
SMC". So please don't include vsmc.h here.

>
>  static const char * const seattle_dt_compat[] __initconst =
>  {
> @@ -33,12 +34,12 @@ static const char * const seattle_dt_compat[] __initconst =
>   */
>  static void seattle_system_reset(void)
>  {
> -    call_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
> +    call_smc(PSCI_0_2_FN32(SYSTEM_RESET), 0, 0, 0);
>  }
>
>  static void seattle_system_off(void)
>  {
> -    call_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> +    call_smc(PSCI_0_2_FN32(SYSTEM_OFF), 0, 0, 0);
>  }
>
>  PLATFORM_START(seattle, "SEATTLE")
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index 34ee97e..645fe58 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -31,9 +31,9 @@
>   * (native-width) function ID.
>   */
>  #ifdef CONFIG_ARM_64
> -#define PSCI_0_2_FN_NATIVE(name)	PSCI_0_2_FN64_##name
> +#define PSCI_0_2_FN_NATIVE(name)   PSCI_0_2_FN64(name)
>  #else
> -#define PSCI_0_2_FN_NATIVE(name)	PSCI_0_2_FN_##name
> +#define PSCI_0_2_FN_NATIVE(name)   PSCI_0_2_FN32(name)
>  #endif
>
>  uint32_t psci_ver;
> @@ -48,13 +48,13 @@ int call_psci_cpu_on(int cpu)
>  void call_psci_system_off(void)
>  {
>      if ( psci_ver > PSCI_VERSION(0, 1) )
> -        call_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> +        call_smc(PSCI_0_2_FN32(SYSTEM_OFF), 0, 0, 0);
>  }
>
>  void call_psci_system_reset(void)
>  {
>      if ( psci_ver > PSCI_VERSION(0, 1) )
> -        call_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
> +        call_smc(PSCI_0_2_FN32(SYSTEM_RESET), 0, 0, 0);
>  }
>
>  int __init psci_is_smc_method(const struct dt_device_node *psci)
> @@ -144,7 +144,7 @@ int __init psci_init_0_2(void)
>          }
>      }
>
> -    psci_ver = call_smc(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
> +    psci_ver = call_smc(PSCI_0_2_FN32(PSCI_VERSION), 0, 0, 0);
>
>      /* For the moment, we only support PSCI 0.2 and PSCI 1.x */
>      if ( psci_ver != PSCI_VERSION(0, 2) && PSCI_VERSION_MAJOR(psci_ver) != 1 )
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index 956d4ef..46a2fde 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -102,7 +102,7 @@ static bool handle_psci_0_x(struct cpu_user_regs *regs)
>  /* 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 ) );
> +    return is_64bit_domain(d) || !ARM_SMCCC_IS_64(fid);

I don't see any reason to do the renaming in psci_mode_check given that 
you will remove this function in the next patch.

>  }
>
>  /* PSCI 0.2 interface and other Standard Secure Calls */
> @@ -112,34 +112,34 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>
>      switch ( ARM_SMCCC_FUNC_NUM(fid) )
>      {
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_PSCI_VERSION):
> +    case PSCI_0_2_FUNC_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):
> +    case PSCI_0_2_FUNC_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):
> +    case PSCI_0_2_FUNC_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):
> +    case PSCI_0_2_FUNC_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):
> +    case PSCI_0_2_FUNC_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):
> +       return true;
> +    case PSCI_0_2_FUNC_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):
> +    case PSCI_0_2_FUNC_CPU_ON:
>          perfc_incr(vpsci_cpu_on);
>          if ( psci_mode_check(current->domain, fid) )
>          {
> @@ -149,7 +149,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>              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):
> +    case PSCI_0_2_FUNC_CPU_SUSPEND:
>          perfc_incr(vpsci_cpu_suspend);
>          if ( psci_mode_check(current->domain, fid) )
>          {
> @@ -159,7 +159,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>              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):
> +    case PSCI_0_2_FUNC_AFFINITY_INFO:
>          perfc_incr(vpsci_cpu_affinity_info);
>          if ( psci_mode_check(current->domain, fid) )
>          {
> @@ -168,7 +168,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>              PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
>          }
>          return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE):
> +    case PSCI_0_2_FUNC_MIGRATE:
>          perfc_incr(vpsci_cpu_migrate);
>          if ( psci_mode_check(current->domain, fid) )
>          {
> diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
> index be2458a..14fb98f 100644
> --- a/xen/include/asm-arm/psci.h
> +++ b/xen/include/asm-arm/psci.h
> @@ -1,6 +1,8 @@
>  #ifndef __ASM_PSCI_H__
>  #define __ASM_PSCI_H__
>
> +#include <asm/smccc.h>
> +
>  /* PSCI return values (inclusive of all PSCI versions) */
>  #define PSCI_SUCCESS                 0
>  #define PSCI_NOT_SUPPORTED          -1
> @@ -41,30 +43,24 @@ register_t do_psci_0_2_migrate_info_up_cpu(void);
>  void do_psci_0_2_system_off(void);
>  void do_psci_0_2_system_reset(void);
>
> -/* PSCI v0.2 interface */

Why did you drop this comment?

> -#define PSCI_0_2_FN_BASE        0x84000000
> -#define PSCI_0_2_FN(n)          (PSCI_0_2_FN_BASE + (n))
> -#define PSCI_0_2_64BIT          0x40000000
> -#define PSCI_0_2_FN64_BASE      \
> -                        (PSCI_0_2_FN_BASE + PSCI_0_2_64BIT)
> -#define PSCI_0_2_FN64(n)        (PSCI_0_2_FN64_BASE + (n))
> -
> -#define PSCI_0_2_FN_PSCI_VERSION        PSCI_0_2_FN(0)
> -#define PSCI_0_2_FN_CPU_SUSPEND         PSCI_0_2_FN(1)
> -#define PSCI_0_2_FN_CPU_OFF             PSCI_0_2_FN(2)
> -#define PSCI_0_2_FN_CPU_ON              PSCI_0_2_FN(3)
> -#define PSCI_0_2_FN_AFFINITY_INFO       PSCI_0_2_FN(4)
> -#define PSCI_0_2_FN_MIGRATE             PSCI_0_2_FN(5)
> -#define PSCI_0_2_FN_MIGRATE_INFO_TYPE   PSCI_0_2_FN(6)
> -#define PSCI_0_2_FN_MIGRATE_INFO_UP_CPU PSCI_0_2_FN(7)
> -#define PSCI_0_2_FN_SYSTEM_OFF          PSCI_0_2_FN(8)
> -#define PSCI_0_2_FN_SYSTEM_RESET        PSCI_0_2_FN(9)
> -
> -#define PSCI_0_2_FN64_CPU_SUSPEND       PSCI_0_2_FN64(1)
> -#define PSCI_0_2_FN64_CPU_ON            PSCI_0_2_FN64(3)
> -#define PSCI_0_2_FN64_AFFINITY_INFO     PSCI_0_2_FN64(4)
> -#define PSCI_0_2_FN64_MIGRATE           PSCI_0_2_FN64(5)
> -#define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU   PSCI_0_2_FN64(7)
> +#define PSCI_0_2_FN32(name) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,             \
> +                                            ARM_SMCCC_SMC_32,                   \
> +                                            ARM_SMCCC_OWNER_STANDARD,           \
> +                                            PSCI_0_2_FUNC_##name)

The indentation looks wrong here.

> +#define PSCI_0_2_FN64(name) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,             \
> +                                            ARM_SMCCC_SMC_64,                   \
> +                                            ARM_SMCCC_OWNER_STANDARD,           \
> +                                            PSCI_0_2_FUNC_##name)


Ditto.

> +#define PSCI_0_2_FUNC_PSCI_VERSION        0
> +#define PSCI_0_2_FUNC_CPU_SUSPEND         1
> +#define PSCI_0_2_FUNC_CPU_OFF             2
> +#define PSCI_0_2_FUNC_CPU_ON              3
> +#define PSCI_0_2_FUNC_AFFINITY_INFO       4
> +#define PSCI_0_2_FUNC_MIGRATE             5
> +#define PSCI_0_2_FUNC_MIGRATE_INFO_TYPE   6
> +#define PSCI_0_2_FUNC_MIGRATE_INFO_UP_CPU 7
> +#define PSCI_0_2_FUNC_SYSTEM_OFF          8
> +#define PSCI_0_2_FUNC_SYSTEM_RESET        9
>
>  /* PSCI v0.2 affinity level state returned by AFFINITY_INFO */
>  #define PSCI_0_2_AFFINITY_LEVEL_ON      0
>

Cheers,

-- 
Julien Grall

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