| Message ID | 20180208192203.9556-10-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm: PSCI 1.1 and SMCCC-1.1 support and XSA-254 variant 2 update | expand |
Julien, On 08.02.18 21:21, Julien Grall wrote: > PSCI 1.0 and later allows the SMCCC version to be (indirectly) probed > via PSCI_FEATURES. If the PSCI_FEATURES does not exist (PSCI 0.2 or > earlier) and the function return an error, then we considered SMCCC 1.0 > is implemented. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > --- > Changes in v2: > - Patch added > --- > xen/arch/arm/psci.c | 34 +++++++++++++++++++++++++++++++++- > xen/include/asm-arm/smccc.h | 5 ++++- > 2 files changed, 37 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c I find it strange to determine SMCCC version in PSCI code. psci.c is not the first place, where I will look for SMCCC version discovery. I think it is better to add smccc.c, where such functions can reside. > index 5dda35cd7c..bc7b2260e8 100644 > --- a/xen/arch/arm/psci.c > +++ b/xen/arch/arm/psci.c > @@ -37,6 +37,7 @@ > #endif > > uint32_t psci_ver; > +uint32_t smccc_ver; And this variable actually is not related to PSCI. > > static uint32_t psci_cpu_on_nr; > > @@ -57,6 +58,14 @@ void call_psci_system_reset(void) > call_smc(PSCI_0_2_FN32_SYSTEM_RESET, 0, 0, 0); > } > > +static int __init psci_features(uint32_t psci_func_id) > +{ > + if ( psci_ver < PSCI_VERSION(1, 0) ) > + return PSCI_NOT_SUPPORTED; > + > + return call_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, 0, 0); > +} > + > int __init psci_is_smc_method(const struct dt_device_node *psci) > { > int ret; > @@ -82,6 +91,24 @@ int __init psci_is_smc_method(const struct dt_device_node *psci) > return 0; > } > > +static void __init psci_init_smccc(void) > +{ > + /* PSCI is using at least SMCC 1.0 calling convention. */ > + smccc_ver = ARM_SMCCC_VERSION_1_0; > + > + if ( psci_features(ARM_SMCCC_VERSION_FID) != PSCI_NOT_SUPPORTED ) > + { > + uint32_t ret; > + > + ret = call_smc(ARM_SMCCC_VERSION_FID, 0, 0, 0); > + if ( ret != ARM_SMCCC_NOT_SUPPORTED ) > + smccc_ver = ret; > + } > + > + printk(XENLOG_INFO "Using SMC Calling Convention v%u.%u\n", > + SMCCC_VERSION_MAJOR(smccc_ver), SMCCC_VERSION_MINOR(smccc_ver)); > +} > + > int __init psci_init_0_1(void) > { > int ret; > @@ -173,7 +200,12 @@ int __init psci_init(void) > if ( ret ) > ret = psci_init_0_1(); > > - return ret; > + if ( ret ) > + return ret; > + > + psci_init_smccc(); > + > + return 0; > } > > /* > diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h > index caa2c9cc1b..bc067892c7 100644 > --- a/xen/include/asm-arm/smccc.h > +++ b/xen/include/asm-arm/smccc.h > @@ -52,6 +52,8 @@ > > #ifndef __ASSEMBLY__ > > +extern uint32_t smccc_ver; > + > /* Check if this is fast call. */ > static inline bool smccc_is_fast_call(register_t funcid) > { > @@ -137,8 +139,9 @@ static inline uint32_t smccc_get_owner(register_t funcid) > ARM_SMCCC_OWNER_ARCH, \ > 0x8000) > > -/* Only one error code defined in SMCCC */ > +/* SMCCC error codes */ > #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION (-1) > +#define ARM_SMCCC_NOT_SUPPORTED (-1) In patch "xen/arm: vsmc: Implement SMCCC 1.1" you return plain -1 in static bool handle_arch(struct cpu_user_regs *regs) Could you please move definition of ARM_SMCCC_NOT_SUPPORTED into that patch and use it in mentioned function or add new patch that changes -1 to ARM_SMCCC_NOT_SUPPORTED ? > > /* SMCCC function identifier range which is reserved for existing APIs */ > #define ARM_SMCCC_RESERVED_RANGE_START 0x0 >
On 02/09/2018 05:04 PM, Volodymyr Babchuk wrote: > Julien, > > > On 08.02.18 21:21, Julien Grall wrote: >> PSCI 1.0 and later allows the SMCCC version to be (indirectly) probed >> via PSCI_FEATURES. If the PSCI_FEATURES does not exist (PSCI 0.2 or >> earlier) and the function return an error, then we considered SMCCC 1.0 >> is implemented. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> --- >> Changes in v2: >> - Patch added >> --- >> xen/arch/arm/psci.c | 34 +++++++++++++++++++++++++++++++++- >> xen/include/asm-arm/smccc.h | 5 ++++- >> 2 files changed, 37 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c > > I find it strange to determine SMCCC version in PSCI code. psci.c is not > the first place, where I will look for SMCCC version discovery. > I think it is better to add smccc.c, where such functions can reside. SMCCC version discovery is based on PSCI, hence it is in the PSCI code. I can't see a good reason to create a file with 3 lines at the moment. > >> index 5dda35cd7c..bc7b2260e8 100644 >> --- a/xen/arch/arm/psci.c >> +++ b/xen/arch/arm/psci.c >> @@ -37,6 +37,7 @@ >> #endif >> uint32_t psci_ver; >> +uint32_t smccc_ver; > > And this variable actually is not related to PSCI. See my comment above. I am not going to create a file just for 3 lines. >> static uint32_t psci_cpu_on_nr; >> @@ -57,6 +58,14 @@ void call_psci_system_reset(void) >> call_smc(PSCI_0_2_FN32_SYSTEM_RESET, 0, 0, 0); >> } >> +static int __init psci_features(uint32_t psci_func_id) >> +{ >> + if ( psci_ver < PSCI_VERSION(1, 0) ) >> + return PSCI_NOT_SUPPORTED; >> + >> + return call_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, 0, 0); >> +} >> + >> int __init psci_is_smc_method(const struct dt_device_node *psci) >> { >> int ret; >> @@ -82,6 +91,24 @@ int __init psci_is_smc_method(const struct >> dt_device_node *psci) >> return 0; >> } >> +static void __init psci_init_smccc(void) >> +{ >> + /* PSCI is using at least SMCC 1.0 calling convention. */ >> + smccc_ver = ARM_SMCCC_VERSION_1_0; >> + >> + if ( psci_features(ARM_SMCCC_VERSION_FID) != PSCI_NOT_SUPPORTED ) >> + { >> + uint32_t ret; >> + >> + ret = call_smc(ARM_SMCCC_VERSION_FID, 0, 0, 0); >> + if ( ret != ARM_SMCCC_NOT_SUPPORTED ) >> + smccc_ver = ret; >> + } >> + >> + printk(XENLOG_INFO "Using SMC Calling Convention v%u.%u\n", >> + SMCCC_VERSION_MAJOR(smccc_ver), >> SMCCC_VERSION_MINOR(smccc_ver)); >> +} >> + >> int __init psci_init_0_1(void) >> { >> int ret; >> @@ -173,7 +200,12 @@ int __init psci_init(void) >> if ( ret ) >> ret = psci_init_0_1(); >> - return ret; >> + if ( ret ) >> + return ret; >> + >> + psci_init_smccc(); >> + >> + return 0; >> } >> /* >> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h >> index caa2c9cc1b..bc067892c7 100644 >> --- a/xen/include/asm-arm/smccc.h >> +++ b/xen/include/asm-arm/smccc.h >> @@ -52,6 +52,8 @@ >> #ifndef __ASSEMBLY__ >> +extern uint32_t smccc_ver; >> + >> /* Check if this is fast call. */ >> static inline bool smccc_is_fast_call(register_t funcid) >> { >> @@ -137,8 +139,9 @@ static inline uint32_t smccc_get_owner(register_t >> funcid) >> ARM_SMCCC_OWNER_ARCH, \ >> 0x8000) >> -/* Only one error code defined in SMCCC */ >> +/* SMCCC error codes */ >> #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION (-1) >> +#define ARM_SMCCC_NOT_SUPPORTED (-1) > > In patch "xen/arm: vsmc: Implement SMCCC 1.1" you return plain -1 in > static bool handle_arch(struct cpu_user_regs *regs) > > Could you please move definition of ARM_SMCCC_NOT_SUPPORTED into that > patch and use it in mentioned function or add new patch that changes -1 > to ARM_SMCCC_NOT_SUPPORTED ? Will do. > >> /* SMCCC function identifier range which is reserved for existing >> APIs */ >> #define ARM_SMCCC_RESERVED_RANGE_START 0x0 >> > Cheers,
Hi Julien, On 09.02.18 19:09, Julien Grall wrote: > > > On 02/09/2018 05:04 PM, Volodymyr Babchuk wrote: >> Julien, >> >> >> On 08.02.18 21:21, Julien Grall wrote: >>> PSCI 1.0 and later allows the SMCCC version to be (indirectly) probed >>> via PSCI_FEATURES. If the PSCI_FEATURES does not exist (PSCI 0.2 or >>> earlier) and the function return an error, then we considered SMCCC 1.0 >>> is implemented. >>> >>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>> --- >>> Changes in v2: >>> - Patch added >>> --- >>> xen/arch/arm/psci.c | 34 +++++++++++++++++++++++++++++++++- >>> xen/include/asm-arm/smccc.h | 5 ++++- >>> 2 files changed, 37 insertions(+), 2 deletions(-) >>> >>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c >> >> I find it strange to determine SMCCC version in PSCI code. psci.c is >> not the first place, where I will look for SMCCC version discovery. >> I think it is better to add smccc.c, where such functions can reside. > > SMCCC version discovery is based on PSCI, hence it is in the PSCI code. > I can't see a good reason to create a file with 3 lines at the moment. SMCCC version discovery is a Arm Architecture Service function. PSCI used to discover if this function is supported at all. Dubious architectural solution from my point of view. But it is already done... We had similar discussions about introducing new files earlier, so you know my point. I would like to see clean codebase where one can navigate without grep/cscope. I see no point, why function that calls Arm architecture service to identify SMCCC version should reside in PSCI code. Besides, that file will have more than 3 lines at the moment. Your current psci_init_smccc is longer right now :) >> >>> index 5dda35cd7c..bc7b2260e8 100644 >>> --- a/xen/arch/arm/psci.c >>> +++ b/xen/arch/arm/psci.c >>> @@ -37,6 +37,7 @@ >>> #endif >>> uint32_t psci_ver; >>> +uint32_t smccc_ver; >> >> And this variable actually is not related to PSCI. > > See my comment above. I am not going to create a file just for 3 lines. > See my comments above :)
On 12/02/18 14:43, Volodymyr Babchuk wrote: > Hi Julien, > > On 09.02.18 19:09, Julien Grall wrote: >> >> >> On 02/09/2018 05:04 PM, Volodymyr Babchuk wrote: >>> Julien, >>> >>> >>> On 08.02.18 21:21, Julien Grall wrote: >>>> PSCI 1.0 and later allows the SMCCC version to be (indirectly) probed >>>> via PSCI_FEATURES. If the PSCI_FEATURES does not exist (PSCI 0.2 or >>>> earlier) and the function return an error, then we considered SMCCC 1.0 >>>> is implemented. >>>> >>>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>>> --- >>>> Changes in v2: >>>> - Patch added >>>> --- >>>> xen/arch/arm/psci.c | 34 +++++++++++++++++++++++++++++++++- >>>> xen/include/asm-arm/smccc.h | 5 ++++- >>>> 2 files changed, 37 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c >>> >>> I find it strange to determine SMCCC version in PSCI code. psci.c is >>> not the first place, where I will look for SMCCC version discovery. >>> I think it is better to add smccc.c, where such functions can reside. >> >> SMCCC version discovery is based on PSCI, hence it is in the PSCI >> code. I can't see a good reason to create a file with 3 lines at the >> moment. > > SMCCC version discovery is a Arm Architecture Service function. PSCI > used to discover if this function is supported at all. Dubious > architectural solution from my point of view. But it is already done... > > We had similar discussions about introducing new files earlier, so you > know my point. I would like to see clean codebase where one can > navigate without grep/cscope. I see no point, why function that calls > Arm architecture service to identify SMCCC version should reside in PSCI > code. Good luck for navigating without grep/cscope in a such a big code base. > > Besides, that file will have more than 3 lines at the moment. Your > current psci_init_smccc is longer right now :) You got my point with "3 lines"... It is one function in one file. There limited reason to create a file for that, more that I clearly don't want to expose psci_features() outside of psci.c for now. We can revisit this in the future if we need to discover SMCCC in a different way. Cheers,
diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c index 5dda35cd7c..bc7b2260e8 100644 --- a/xen/arch/arm/psci.c +++ b/xen/arch/arm/psci.c @@ -37,6 +37,7 @@ #endif uint32_t psci_ver; +uint32_t smccc_ver; static uint32_t psci_cpu_on_nr; @@ -57,6 +58,14 @@ void call_psci_system_reset(void) call_smc(PSCI_0_2_FN32_SYSTEM_RESET, 0, 0, 0); } +static int __init psci_features(uint32_t psci_func_id) +{ + if ( psci_ver < PSCI_VERSION(1, 0) ) + return PSCI_NOT_SUPPORTED; + + return call_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, 0, 0); +} + int __init psci_is_smc_method(const struct dt_device_node *psci) { int ret; @@ -82,6 +91,24 @@ int __init psci_is_smc_method(const struct dt_device_node *psci) return 0; } +static void __init psci_init_smccc(void) +{ + /* PSCI is using at least SMCC 1.0 calling convention. */ + smccc_ver = ARM_SMCCC_VERSION_1_0; + + if ( psci_features(ARM_SMCCC_VERSION_FID) != PSCI_NOT_SUPPORTED ) + { + uint32_t ret; + + ret = call_smc(ARM_SMCCC_VERSION_FID, 0, 0, 0); + if ( ret != ARM_SMCCC_NOT_SUPPORTED ) + smccc_ver = ret; + } + + printk(XENLOG_INFO "Using SMC Calling Convention v%u.%u\n", + SMCCC_VERSION_MAJOR(smccc_ver), SMCCC_VERSION_MINOR(smccc_ver)); +} + int __init psci_init_0_1(void) { int ret; @@ -173,7 +200,12 @@ int __init psci_init(void) if ( ret ) ret = psci_init_0_1(); - return ret; + if ( ret ) + return ret; + + psci_init_smccc(); + + return 0; } /* diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h index caa2c9cc1b..bc067892c7 100644 --- a/xen/include/asm-arm/smccc.h +++ b/xen/include/asm-arm/smccc.h @@ -52,6 +52,8 @@ #ifndef __ASSEMBLY__ +extern uint32_t smccc_ver; + /* Check if this is fast call. */ static inline bool smccc_is_fast_call(register_t funcid) { @@ -137,8 +139,9 @@ static inline uint32_t smccc_get_owner(register_t funcid) ARM_SMCCC_OWNER_ARCH, \ 0x8000) -/* Only one error code defined in SMCCC */ +/* SMCCC error codes */ #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION (-1) +#define ARM_SMCCC_NOT_SUPPORTED (-1) /* SMCCC function identifier range which is reserved for existing APIs */ #define ARM_SMCCC_RESERVED_RANGE_START 0x0
PSCI 1.0 and later allows the SMCCC version to be (indirectly) probed via PSCI_FEATURES. If the PSCI_FEATURES does not exist (PSCI 0.2 or earlier) and the function return an error, then we considered SMCCC 1.0 is implemented. Signed-off-by: Julien Grall <julien.grall@arm.com> --- Changes in v2: - Patch added --- xen/arch/arm/psci.c | 34 +++++++++++++++++++++++++++++++++- xen/include/asm-arm/smccc.h | 5 ++++- 2 files changed, 37 insertions(+), 2 deletions(-)