| Message ID | 20180124183445.23705-3-julien.grall@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: SMCCC fixes and PSCI clean-up | expand |
On 24.01.18 20:34, Julien Grall wrote: > The current implementation of SMCCC relies on the fact only function > number (bits [15:0]) is enough to identify what to implement. > > However, PSCI call are only available in the range 0x84000000-0x8400001F > and 0xC4000000-0xC400001F. Furthermore, not all SMC32 functions have > equivalent in the SMC64. This is the case of: > * PSCI_VERSION > * CPU_OFF > * MIGRATE_INFO_TYPE > * SYSTEM_OFF > * SYSTEM_RESET > > Similarly call count, uid, revision can only be query using smc32/hvc32 > fast calls (See 6.2 in ARM DEN 0028B). > > Xen should only implement identifier existing in the specification in > order to avoid potential clash with later revision. Therefore rework the > vsmc code to use the whole function identifier rather than only the > function number. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > --- > This should be backported to Xen 4.10 as we should not implement > functions identifier that does not exist toprevent clash with a > later revision. > --- > xen/arch/arm/vsmc.c | 37 +++++++++++++++++++++---------------- > xen/include/asm-arm/smccc.h | 20 +++++++++++++++++--- > 2 files changed, 38 insertions(+), 19 deletions(-) > > diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c > index 563c2d8dda..7ca2880173 100644 > --- a/xen/arch/arm/vsmc.c > +++ b/xen/arch/arm/vsmc.c > @@ -84,13 +84,15 @@ static bool fill_function_call_count(struct cpu_user_regs *regs, uint32_t cnt) > /* SMCCC interface for hypervisor. Tell about itself. */ > static bool handle_hypervisor(struct cpu_user_regs *regs) > { > - switch ( smccc_get_fn(get_user_reg(regs, 0)) ) > + uint32_t fid = (uint32_t)get_user_reg(regs, 0); > + > + switch ( fid ) > { > - case ARM_SMCCC_FUNC_CALL_COUNT: > + case ARM_SMCCC_FUNC_CALL_COUNT(HYPERVISOR): I think, in this case better to rename ARM_SMCCC_FUNC_CALL_COUNT to something like ARM_SMCCC_CALL_COUNT. Or ARM_SMCCC_FID_CALL_COUNT. I proposing this to distinguish function number from function identifier (as per ARM SMCCC). > return fill_function_call_count(regs, XEN_SMCCC_FUNCTION_COUNT); > - case ARM_SMCCC_FUNC_CALL_UID: > + case ARM_SMCCC_FUNC_CALL_UID(HYPERVISOR): > return fill_uid(regs, XEN_SMCCC_UID); > - case ARM_SMCCC_FUNC_CALL_REVISION: > + case ARM_SMCCC_FUNC_CALL_REVISION(HYPERVISOR): > return fill_revision(regs, XEN_SMCCC_MAJOR_REVISION, > XEN_SMCCC_MINOR_REVISION); > default: > @@ -140,36 +142,37 @@ static bool handle_sssc(struct cpu_user_regs *regs) > { > uint32_t fid = (uint32_t)get_user_reg(regs, 0); > > - switch ( smccc_get_fn(fid) ) > + switch ( fid ) > { > - case PSCI_0_2_FN_PSCI_VERSION: > + case PSCI_0_2_FN32(PSCI_VERSION): > perfc_incr(vpsci_version); > PSCI_SET_RESULT(regs, do_psci_0_2_version()); > return true; > > - case PSCI_0_2_FN_CPU_OFF: > + case PSCI_0_2_FN32(CPU_OFF): > perfc_incr(vpsci_cpu_off); > PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off()); > return true; > > - case PSCI_0_2_FN_MIGRATE_INFO_TYPE: > + case PSCI_0_2_FN32(MIGRATE_INFO_TYPE): > perfc_incr(vpsci_migrate_info_type); > PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type()); > return true; > > - case PSCI_0_2_FN_SYSTEM_OFF: > + case PSCI_0_2_FN32(SYSTEM_OFF): > perfc_incr(vpsci_system_off); > do_psci_0_2_system_off(); > PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE); > return true; > > - case PSCI_0_2_FN_SYSTEM_RESET: > + case PSCI_0_2_FN32(SYSTEM_RESET): > perfc_incr(vpsci_system_reset); > do_psci_0_2_system_reset(); > PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE); > return true; > > - case PSCI_0_2_FN_CPU_ON: > + case PSCI_0_2_FN32(CPU_ON): > + case PSCI_0_2_FN64(CPU_ON): > { > register_t vcpuid = PSCI_ARG(regs, 1); > register_t epoint = PSCI_ARG(regs, 2); > @@ -180,7 +183,8 @@ static bool handle_sssc(struct cpu_user_regs *regs) > return true; > } > > - case PSCI_0_2_FN_CPU_SUSPEND: > + case PSCI_0_2_FN32(CPU_SUSPEND): > + case PSCI_0_2_FN64(CPU_SUSPEND): > { > uint32_t pstate = PSCI_ARG32(regs, 1); > register_t epoint = PSCI_ARG(regs, 2); > @@ -191,7 +195,8 @@ static bool handle_sssc(struct cpu_user_regs *regs) > return true; > } > > - case PSCI_0_2_FN_AFFINITY_INFO: > + case PSCI_0_2_FN32(AFFINITY_INFO): > + case PSCI_0_2_FN64(AFFINITY_INFO): > { > register_t taff = PSCI_ARG(regs, 1); > uint32_t laff = PSCI_ARG32(regs, 2); > @@ -201,13 +206,13 @@ static bool handle_sssc(struct cpu_user_regs *regs) > return true; > } > > - case ARM_SMCCC_FUNC_CALL_COUNT: > + case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD): > return fill_function_call_count(regs, SSSC_SMCCC_FUNCTION_COUNT); > > - case ARM_SMCCC_FUNC_CALL_UID: > + case ARM_SMCCC_FUNC_CALL_UID(STANDARD): > return fill_uid(regs, SSSC_SMCCC_UID); > > - case ARM_SMCCC_FUNC_CALL_REVISION: > + case ARM_SMCCC_FUNC_CALL_REVISION(STANDARD): > return fill_revision(regs, SSSC_SMCCC_MAJOR_REVISION, > SSSC_SMCCC_MINOR_REVISION); > > diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h > index f543dea0bb..303517459f 100644 > --- a/xen/include/asm-arm/smccc.h > +++ b/xen/include/asm-arm/smccc.h > @@ -82,9 +82,23 @@ static inline uint32_t smccc_get_owner(register_t funcid) > #define ARM_SMCCC_OWNER_TRUSTED_OS_END 63 > > /* List of generic function numbers */ > -#define ARM_SMCCC_FUNC_CALL_COUNT 0xFF00 > -#define ARM_SMCCC_FUNC_CALL_UID 0xFF01 > -#define ARM_SMCCC_FUNC_CALL_REVISION 0xFF03 So, I thing it is bette to leave #define ARM_SMCCC_FUNC_CALL_* and introduce +#define ARM_SMCCC_FID_CALL_COUNT(owner) \ + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ + ARM_SMCCC_CONV_32, \ + ARM_SMCCC_OWNER_##owner, \ + ARM_SMCCC_FUNC_CALL_COUNT) > +#define ARM_SMCCC_FUNC_CALL_COUNT(owner) \ > + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ > + ARM_SMCCC_CONV_32, \ > + ARM_SMCCC_OWNER_##owner, \ > + 0xFF00) > + > +#define ARM_SMCCC_FUNC_CALL_UID(owner) \ > + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ > + ARM_SMCCC_CONV_32, \ > + ARM_SMCCC_OWNER_##owner, \ > + 0xFF01) > + > +#define ARM_SMCCC_FUNC_CALL_REVISION(owner) \ > + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ > + ARM_SMCCC_CONV_32, \ > + ARM_SMCCC_OWNER_##owner, \ > + 0xFF03) > > /* Only one error code defined in SMCCC */ > #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION (-1) >
On 26/01/18 18:03, Volodymyr Babchuk wrote: >> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h >> index f543dea0bb..303517459f 100644 >> --- a/xen/include/asm-arm/smccc.h >> +++ b/xen/include/asm-arm/smccc.h >> @@ -82,9 +82,23 @@ static inline uint32_t smccc_get_owner(register_t >> funcid) >> #define ARM_SMCCC_OWNER_TRUSTED_OS_END 63 >> /* List of generic function numbers */ >> -#define ARM_SMCCC_FUNC_CALL_COUNT 0xFF00 >> -#define ARM_SMCCC_FUNC_CALL_UID 0xFF01 >> -#define ARM_SMCCC_FUNC_CALL_REVISION 0xFF03 > So, I thing it is bette to leave > #define ARM_SMCCC_FUNC_CALL_* > and introduce I am ok to use FID, but I don't see any reason to keep ARM_SMCCC_FUNC_* one around. No-one can use them directly in Xen and this would add confusion. Cheers,
On 26.01.18 20:07, Julien Grall wrote: > > > On 26/01/18 18:03, Volodymyr Babchuk wrote: >>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h >>> index f543dea0bb..303517459f 100644 >>> --- a/xen/include/asm-arm/smccc.h >>> +++ b/xen/include/asm-arm/smccc.h >>> @@ -82,9 +82,23 @@ static inline uint32_t smccc_get_owner(register_t >>> funcid) >>> #define ARM_SMCCC_OWNER_TRUSTED_OS_END 63 >>> /* List of generic function numbers */ >>> -#define ARM_SMCCC_FUNC_CALL_COUNT 0xFF00 >>> -#define ARM_SMCCC_FUNC_CALL_UID 0xFF01 >>> -#define ARM_SMCCC_FUNC_CALL_REVISION 0xFF03 >> So, I thing it is bette to leave >> #define ARM_SMCCC_FUNC_CALL_* >> and introduce > > I am ok to use FID, but I don't see any reason to keep ARM_SMCCC_FUNC_* > one around. No-one can use them directly in Xen and this would add > confusion. Right. So you can nuke them. Another idea is to leave only one definition and use it like this: ARM_SMCCC_FID_CALL(COUNT, HYPERVISOR) This will reduce number of similar macro definitions. What do you think? I'm okay with any variant.
Hi, On 26/01/18 18:12, Volodymyr Babchuk wrote: > > > On 26.01.18 20:07, Julien Grall wrote: >> >> >> On 26/01/18 18:03, Volodymyr Babchuk wrote: >>>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h >>>> index f543dea0bb..303517459f 100644 >>>> --- a/xen/include/asm-arm/smccc.h >>>> +++ b/xen/include/asm-arm/smccc.h >>>> @@ -82,9 +82,23 @@ static inline uint32_t smccc_get_owner(register_t >>>> funcid) >>>> #define ARM_SMCCC_OWNER_TRUSTED_OS_END 63 >>>> /* List of generic function numbers */ >>>> -#define ARM_SMCCC_FUNC_CALL_COUNT 0xFF00 >>>> -#define ARM_SMCCC_FUNC_CALL_UID 0xFF01 >>>> -#define ARM_SMCCC_FUNC_CALL_REVISION 0xFF03 >>> So, I thing it is bette to leave >>> #define ARM_SMCCC_FUNC_CALL_* >>> and introduce >> >> I am ok to use FID, but I don't see any reason to keep >> ARM_SMCCC_FUNC_* one around. No-one can use them directly in Xen and >> this would add confusion. > > Right. So you can nuke them. > Another idea is to leave only one definition and use it like this: > > ARM_SMCCC_FID_CALL(COUNT, HYPERVISOR) > > This will reduce number of similar macro definitions. What do you think? Well technically the function names are "Call Count", "Call UID", "Revision". Therefore the name is already wrong for revision. I also don't see a good way to use this macro more than 3 times. Indeed fastcall, 32-conv is pretty specific. So here the benefits seems really limited. > > I'm okay with any variant. I will stick with the one suggested in the patch I sent. Cheers,
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c index 563c2d8dda..7ca2880173 100644 --- a/xen/arch/arm/vsmc.c +++ b/xen/arch/arm/vsmc.c @@ -84,13 +84,15 @@ static bool fill_function_call_count(struct cpu_user_regs *regs, uint32_t cnt) /* SMCCC interface for hypervisor. Tell about itself. */ static bool handle_hypervisor(struct cpu_user_regs *regs) { - switch ( smccc_get_fn(get_user_reg(regs, 0)) ) + uint32_t fid = (uint32_t)get_user_reg(regs, 0); + + switch ( fid ) { - case ARM_SMCCC_FUNC_CALL_COUNT: + case ARM_SMCCC_FUNC_CALL_COUNT(HYPERVISOR): return fill_function_call_count(regs, XEN_SMCCC_FUNCTION_COUNT); - case ARM_SMCCC_FUNC_CALL_UID: + case ARM_SMCCC_FUNC_CALL_UID(HYPERVISOR): return fill_uid(regs, XEN_SMCCC_UID); - case ARM_SMCCC_FUNC_CALL_REVISION: + case ARM_SMCCC_FUNC_CALL_REVISION(HYPERVISOR): return fill_revision(regs, XEN_SMCCC_MAJOR_REVISION, XEN_SMCCC_MINOR_REVISION); default: @@ -140,36 +142,37 @@ static bool handle_sssc(struct cpu_user_regs *regs) { uint32_t fid = (uint32_t)get_user_reg(regs, 0); - switch ( smccc_get_fn(fid) ) + switch ( fid ) { - case PSCI_0_2_FN_PSCI_VERSION: + case PSCI_0_2_FN32(PSCI_VERSION): perfc_incr(vpsci_version); PSCI_SET_RESULT(regs, do_psci_0_2_version()); return true; - case PSCI_0_2_FN_CPU_OFF: + case PSCI_0_2_FN32(CPU_OFF): perfc_incr(vpsci_cpu_off); PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off()); return true; - case PSCI_0_2_FN_MIGRATE_INFO_TYPE: + case PSCI_0_2_FN32(MIGRATE_INFO_TYPE): perfc_incr(vpsci_migrate_info_type); PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type()); return true; - case PSCI_0_2_FN_SYSTEM_OFF: + case PSCI_0_2_FN32(SYSTEM_OFF): perfc_incr(vpsci_system_off); do_psci_0_2_system_off(); PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE); return true; - case PSCI_0_2_FN_SYSTEM_RESET: + case PSCI_0_2_FN32(SYSTEM_RESET): perfc_incr(vpsci_system_reset); do_psci_0_2_system_reset(); PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE); return true; - case PSCI_0_2_FN_CPU_ON: + case PSCI_0_2_FN32(CPU_ON): + case PSCI_0_2_FN64(CPU_ON): { register_t vcpuid = PSCI_ARG(regs, 1); register_t epoint = PSCI_ARG(regs, 2); @@ -180,7 +183,8 @@ static bool handle_sssc(struct cpu_user_regs *regs) return true; } - case PSCI_0_2_FN_CPU_SUSPEND: + case PSCI_0_2_FN32(CPU_SUSPEND): + case PSCI_0_2_FN64(CPU_SUSPEND): { uint32_t pstate = PSCI_ARG32(regs, 1); register_t epoint = PSCI_ARG(regs, 2); @@ -191,7 +195,8 @@ static bool handle_sssc(struct cpu_user_regs *regs) return true; } - case PSCI_0_2_FN_AFFINITY_INFO: + case PSCI_0_2_FN32(AFFINITY_INFO): + case PSCI_0_2_FN64(AFFINITY_INFO): { register_t taff = PSCI_ARG(regs, 1); uint32_t laff = PSCI_ARG32(regs, 2); @@ -201,13 +206,13 @@ static bool handle_sssc(struct cpu_user_regs *regs) return true; } - case ARM_SMCCC_FUNC_CALL_COUNT: + case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD): return fill_function_call_count(regs, SSSC_SMCCC_FUNCTION_COUNT); - case ARM_SMCCC_FUNC_CALL_UID: + case ARM_SMCCC_FUNC_CALL_UID(STANDARD): return fill_uid(regs, SSSC_SMCCC_UID); - case ARM_SMCCC_FUNC_CALL_REVISION: + case ARM_SMCCC_FUNC_CALL_REVISION(STANDARD): return fill_revision(regs, SSSC_SMCCC_MAJOR_REVISION, SSSC_SMCCC_MINOR_REVISION); diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h index f543dea0bb..303517459f 100644 --- a/xen/include/asm-arm/smccc.h +++ b/xen/include/asm-arm/smccc.h @@ -82,9 +82,23 @@ static inline uint32_t smccc_get_owner(register_t funcid) #define ARM_SMCCC_OWNER_TRUSTED_OS_END 63 /* List of generic function numbers */ -#define ARM_SMCCC_FUNC_CALL_COUNT 0xFF00 -#define ARM_SMCCC_FUNC_CALL_UID 0xFF01 -#define ARM_SMCCC_FUNC_CALL_REVISION 0xFF03 +#define ARM_SMCCC_FUNC_CALL_COUNT(owner) \ + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ + ARM_SMCCC_CONV_32, \ + ARM_SMCCC_OWNER_##owner, \ + 0xFF00) + +#define ARM_SMCCC_FUNC_CALL_UID(owner) \ + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ + ARM_SMCCC_CONV_32, \ + ARM_SMCCC_OWNER_##owner, \ + 0xFF01) + +#define ARM_SMCCC_FUNC_CALL_REVISION(owner) \ + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ + ARM_SMCCC_CONV_32, \ + ARM_SMCCC_OWNER_##owner, \ + 0xFF03) /* Only one error code defined in SMCCC */ #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION (-1)
The current implementation of SMCCC relies on the fact only function number (bits [15:0]) is enough to identify what to implement. However, PSCI call are only available in the range 0x84000000-0x8400001F and 0xC4000000-0xC400001F. Furthermore, not all SMC32 functions have equivalent in the SMC64. This is the case of: * PSCI_VERSION * CPU_OFF * MIGRATE_INFO_TYPE * SYSTEM_OFF * SYSTEM_RESET Similarly call count, uid, revision can only be query using smc32/hvc32 fast calls (See 6.2 in ARM DEN 0028B). Xen should only implement identifier existing in the specification in order to avoid potential clash with later revision. Therefore rework the vsmc code to use the whole function identifier rather than only the function number. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- This should be backported to Xen 4.10 as we should not implement functions identifier that does not exist toprevent clash with a later revision. --- xen/arch/arm/vsmc.c | 37 +++++++++++++++++++++---------------- xen/include/asm-arm/smccc.h | 20 +++++++++++++++++--- 2 files changed, 38 insertions(+), 19 deletions(-)