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

[Volodymyr Babchuk <volodymyr_babchuk@epam.com>]

Hi,

On 24.08.17 20:22, Julien Grall wrote:
> 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?
Actually yes. That helped me to find references to PSCI code in seattle.c

>>
>> 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.
Yep, but in comments for the previous series you asked me to make this code
easier to read :)
PSCI_0_2_64BIT is dropped now, so I had to use long definitions from smccc.h

>>  }
>>
>>  /* 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,
> 

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