| Message ID | 20180202114150.23817-3-julien.grall@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: SMCCC fixes and PSCI clean-up | expand |
On 02.02.18 13:41, 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, call 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. > > At the same time, the new macros for call count, call uid, revision are > renamed to better suit the spec. > > Lastly, update SSSC_SMCCC_FUNCTION_COUNT to match the correct number of > funtions. Note that version is not updated because the number has always > been wrong, and nobody could properly use it. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> `Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>` > --- > 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. > > Changes in v2: > - Rename the call count, call uid, revision macros > - Update SSSC_SMCCC_FUNCTION_COUNT > --- > xen/arch/arm/vsmc.c | 39 ++++++++++++++++++++++----------------- > xen/include/asm-arm/smccc.h | 20 +++++++++++++++++--- > 2 files changed, 39 insertions(+), 20 deletions(-) > > diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c > index 997f2e0ebc..3d8cbcc808 100644 > --- a/xen/arch/arm/vsmc.c > +++ b/xen/arch/arm/vsmc.c > @@ -28,7 +28,7 @@ > #define XEN_SMCCC_FUNCTION_COUNT 3 > > /* Number of functions currently supported by Standard Service Service Calls. */ > -#define SSSC_SMCCC_FUNCTION_COUNT 11 > +#define SSSC_SMCCC_FUNCTION_COUNT 14 > > static bool fill_uid(struct cpu_user_regs *regs, xen_uuid_t uuid) > { > @@ -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_CALL_COUNT_FID(HYPERVISOR): > return fill_function_call_count(regs, XEN_SMCCC_FUNCTION_COUNT); > - case ARM_SMCCC_FUNC_CALL_UID: > + case ARM_SMCCC_CALL_UID_FID(HYPERVISOR): > return fill_uid(regs, XEN_SMCCC_UID); > - case ARM_SMCCC_FUNC_CALL_REVISION: > + case ARM_SMCCC_REVISION_FID(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_CALL_COUNT_FID(STANDARD): > return fill_function_call_count(regs, SSSC_SMCCC_FUNCTION_COUNT); > > - case ARM_SMCCC_FUNC_CALL_UID: > + case ARM_SMCCC_CALL_UID_FID(STANDARD): > return fill_uid(regs, SSSC_SMCCC_UID); > > - case ARM_SMCCC_FUNC_CALL_REVISION: > + case ARM_SMCCC_REVISION_FID(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..62b3a8cdf5 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_CALL_COUNT_FID(owner) \ > + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ > + ARM_SMCCC_CONV_32, \ > + ARM_SMCCC_OWNER_##owner, \ > + 0xFF00) > + > +#define ARM_SMCCC_CALL_UID_FID(owner) \ > + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ > + ARM_SMCCC_CONV_32, \ > + ARM_SMCCC_OWNER_##owner, \ > + 0xFF01) > + > +#define ARM_SMCCC_REVISION_FID(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) >
Hi Volodymyr, On 02/02/2018 01:46 PM, Volodymyr Babchuk wrote: > > > On 02.02.18 13:41, 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, call 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. >> >> At the same time, the new macros for call count, call uid, revision are >> renamed to better suit the spec. >> >> Lastly, update SSSC_SMCCC_FUNCTION_COUNT to match the correct number of >> funtions. Note that version is not updated because the number has always >> been wrong, and nobody could properly use it. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > > `Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>` Thank you for the reviewed-by. I noticed that you put ` at the beginning and end of the line in your reviewed-by tag so far. Is there any reason for that? Cheers,
Hi Julien, On 06.02.18 16:53, Julien Grall wrote: > Hi Volodymyr, > > On 02/02/2018 01:46 PM, Volodymyr Babchuk wrote: >> >> >> On 02.02.18 13:41, 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, call 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. >>> >>> At the same time, the new macros for call count, call uid, revision are >>> renamed to better suit the spec. >>> >>> Lastly, update SSSC_SMCCC_FUNCTION_COUNT to match the correct number of >>> funtions. Note that version is not updated because the number has always >>> been wrong, and nobody could properly use it. >>> >>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> >> `Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>` > > Thank you for the reviewed-by. I noticed that you put ` at the beginning > and end of the line in your reviewed-by tag so far. Is there any reason > for that? It is out of habit. You see, this is preferred way to add R-b tag at GitHub (or at least in OP-TEE repos). Github uses Markdown, and text in ` becomes formatted as a code. I do code review mostly at github, so it become a habit to add ` around R-b tags. You can ignore this, of course. I'll try to omit ` in the future.
On 02/06/2018 03:15 PM, Volodymyr Babchuk wrote: > Hi Julien, Hi, > On 06.02.18 16:53, Julien Grall wrote: >> On 02/02/2018 01:46 PM, Volodymyr Babchuk wrote: >>> >>> >>> On 02.02.18 13:41, 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, call 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. >>>> >>>> At the same time, the new macros for call count, call uid, revision are >>>> renamed to better suit the spec. >>>> >>>> Lastly, update SSSC_SMCCC_FUNCTION_COUNT to match the correct number of >>>> funtions. Note that version is not updated because the number has >>>> always >>>> been wrong, and nobody could properly use it. >>>> >>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>> >>> `Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>` >> >> Thank you for the reviewed-by. I noticed that you put ` at the >> beginning and end of the line in your reviewed-by tag so far. Is there >> any reason for that? > > It is out of habit. You see, this is preferred way to add R-b tag at > GitHub (or at least in OP-TEE repos). Github uses Markdown, and text in > ` becomes formatted as a code. I do code review mostly at github, > so it become a habit to add ` around R-b tags. You can ignore this, of > course. I'll try to omit ` in the future. Oh, good to know. I am usually copying the full line and my editor was not happy at all with the `. For Xen, we tend to only have the Reviewed-by tag on a line. Anyway, thank you for reviewing the series :). I will resend a version soon. If you have time, you might want to have a look at "xen/arm: PSCI 1.1 and SMCCC-1.1 support and XSA-254 variant 2 update" [1]. Cheers, [1] https://lists.xen.org/archives/html/xen-devel/2018-02/msg00285.html
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c index 997f2e0ebc..3d8cbcc808 100644 --- a/xen/arch/arm/vsmc.c +++ b/xen/arch/arm/vsmc.c @@ -28,7 +28,7 @@ #define XEN_SMCCC_FUNCTION_COUNT 3 /* Number of functions currently supported by Standard Service Service Calls. */ -#define SSSC_SMCCC_FUNCTION_COUNT 11 +#define SSSC_SMCCC_FUNCTION_COUNT 14 static bool fill_uid(struct cpu_user_regs *regs, xen_uuid_t uuid) { @@ -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_CALL_COUNT_FID(HYPERVISOR): return fill_function_call_count(regs, XEN_SMCCC_FUNCTION_COUNT); - case ARM_SMCCC_FUNC_CALL_UID: + case ARM_SMCCC_CALL_UID_FID(HYPERVISOR): return fill_uid(regs, XEN_SMCCC_UID); - case ARM_SMCCC_FUNC_CALL_REVISION: + case ARM_SMCCC_REVISION_FID(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_CALL_COUNT_FID(STANDARD): return fill_function_call_count(regs, SSSC_SMCCC_FUNCTION_COUNT); - case ARM_SMCCC_FUNC_CALL_UID: + case ARM_SMCCC_CALL_UID_FID(STANDARD): return fill_uid(regs, SSSC_SMCCC_UID); - case ARM_SMCCC_FUNC_CALL_REVISION: + case ARM_SMCCC_REVISION_FID(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..62b3a8cdf5 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_CALL_COUNT_FID(owner) \ + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ + ARM_SMCCC_CONV_32, \ + ARM_SMCCC_OWNER_##owner, \ + 0xFF00) + +#define ARM_SMCCC_CALL_UID_FID(owner) \ + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ + ARM_SMCCC_CONV_32, \ + ARM_SMCCC_OWNER_##owner, \ + 0xFF01) + +#define ARM_SMCCC_REVISION_FID(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, call 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. At the same time, the new macros for call count, call uid, revision are renamed to better suit the spec. Lastly, update SSSC_SMCCC_FUNCTION_COUNT to match the correct number of funtions. Note that version is not updated because the number has always been wrong, and nobody could properly use it. 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. Changes in v2: - Rename the call count, call uid, revision macros - Update SSSC_SMCCC_FUNCTION_COUNT --- xen/arch/arm/vsmc.c | 39 ++++++++++++++++++++++----------------- xen/include/asm-arm/smccc.h | 20 +++++++++++++++++--- 2 files changed, 39 insertions(+), 20 deletions(-)