| Message ID | 1503347275-13039-9-git-send-email-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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,
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, >
On 25/08/17 12:00, Volodymyr Babchuk wrote: > 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 But now you have inconsistent naming which is not better. I would prefer much prefer to keep the same name as before. >>> 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 If you remove the code in this series, then there are no need to review it. You can just mention in the commit message why you didn't clean-up that function. Cheers,
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> 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); } /* 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 */ -#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) +#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) +#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
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. This change also affects vsmc.c and seattle.c, because they both use PSCI function numbers. 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(-)