| Message ID | 20180124183445.23705-4-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: > At the moment PSCI function dispatching is done in vsmc.c and the > function implementation in vpsci.c. Some bits of the implementation is > even done in vsmc.c (see PSCI_SYSTEM_RESET). > > This means that it is difficult to follow the implementation and also > requires to export functions for each PSCI functions. > > Therefore move PSCI dispatching in two new functions do_psci_0_1_call > and do_psci_0_2_call. The former will handle PSCI 0.1 call while the latter > 0.2 or later call. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > --- > xen/arch/arm/vpsci.c | 141 ++++++++++++++++++++++++++++++++++++++++----- > xen/arch/arm/vsmc.c | 95 ++---------------------------- > xen/include/asm-arm/psci.h | 21 +------ > 3 files changed, 135 insertions(+), 122 deletions(-) > > diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c > index 979d32ed6d..b3ee193621 100644 > --- a/xen/arch/arm/vpsci.c > +++ b/xen/arch/arm/vpsci.c > @@ -91,12 +91,12 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point, > return PSCI_SUCCESS; > } > > -int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point) > +static int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point) > { > return do_common_cpu_on(vcpuid, entry_point, 0 , PSCI_VERSION(0, 1)); > } > > -int32_t do_psci_cpu_off(uint32_t power_state) > +static int32_t do_psci_cpu_off(uint32_t power_state) > { > struct vcpu *v = current; > if ( !test_and_set_bit(_VPF_down, &v->pause_flags) ) > @@ -104,13 +104,14 @@ int32_t do_psci_cpu_off(uint32_t power_state) > return PSCI_SUCCESS; > } > > -uint32_t do_psci_0_2_version(void) > +static uint32_t do_psci_0_2_version(void) > { > return PSCI_VERSION(0, 2); > } > > -register_t do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point, > - register_t context_id) > +static register_t do_psci_0_2_cpu_suspend(uint32_t power_state, > + register_t entry_point, > + register_t context_id) > { > struct vcpu *v = current; > > @@ -123,13 +124,14 @@ register_t do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point, > return PSCI_SUCCESS; > } > > -int32_t do_psci_0_2_cpu_off(void) > +static int32_t do_psci_0_2_cpu_off(void) > { > return do_psci_cpu_off(0); > } > > -int32_t do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point, > - register_t context_id) > +static int32_t do_psci_0_2_cpu_on(register_t target_cpu, > + register_t entry_point, > + register_t context_id) > { > return do_common_cpu_on(target_cpu, entry_point, context_id, > PSCI_VERSION(0, 2)); > @@ -144,8 +146,8 @@ static const unsigned long target_affinity_mask[] = { > #endif > }; > > -int32_t do_psci_0_2_affinity_info(register_t target_affinity, > - uint32_t lowest_affinity_level) > +static int32_t do_psci_0_2_affinity_info(register_t target_affinity, > + uint32_t lowest_affinity_level) > { > struct domain *d = current->domain; > struct vcpu *v; > @@ -172,23 +174,136 @@ int32_t do_psci_0_2_affinity_info(register_t target_affinity, > return PSCI_0_2_AFFINITY_LEVEL_OFF; > } > > -uint32_t do_psci_0_2_migrate_info_type(void) > +static uint32_t do_psci_0_2_migrate_info_type(void) > { > return PSCI_0_2_TOS_MP_OR_NOT_PRESENT; > } > > -void do_psci_0_2_system_off( void ) > +static void do_psci_0_2_system_off( void ) > { > struct domain *d = current->domain; > domain_shutdown(d,SHUTDOWN_poweroff); > } > > -void do_psci_0_2_system_reset(void) > +static void do_psci_0_2_system_reset(void) > { > struct domain *d = current->domain; > domain_shutdown(d,SHUTDOWN_reboot); > } > > +#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val) > +#define PSCI_ARG(reg, n) get_user_reg(reg, n) > + > +#ifdef CONFIG_ARM_64 > +#define PSCI_ARG32(reg, n) (uint32_t)(get_user_reg(reg, n)) > +#else > +#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n) > +#endif > + > +/* > + * PSCI 0.1 calls. It will return false if the function ID is not > + * handled. > + */ > +bool do_psci_0_1_call(struct cpu_user_regs *regs, uint32_t fid) > +{ > + switch ( (uint32_t)get_user_reg(regs, 0) ) > + { > + case PSCI_cpu_off: > + { > + uint32_t pstate = PSCI_ARG32(regs, 1); > + > + perfc_incr(vpsci_cpu_off); > + PSCI_SET_RESULT(regs, do_psci_cpu_off(pstate)); > + return true; > + } > + case PSCI_cpu_on: > + { > + uint32_t vcpuid = PSCI_ARG32(regs, 1); > + register_t epoint = PSCI_ARG(regs, 2); > + > + perfc_incr(vpsci_cpu_on); > + PSCI_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint)); > + return true; > + } > + default: > + return false; > + } > +} > + > +/* > + * PSCI 0.2 or later calls. It will return false if the function ID is > + * not handled. > + */ > +bool do_psci_0_2_call(struct cpu_user_regs *regs, uint32_t fid) > +{ > + switch ( fid ) > + { > + 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_FN32(CPU_OFF): > + perfc_incr(vpsci_cpu_off); > + PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off()); > + return true; > + > + 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_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_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_FN32(CPU_ON): > + case PSCI_0_2_FN64(CPU_ON): > + { > + register_t vcpuid = PSCI_ARG(regs, 1); > + register_t epoint = PSCI_ARG(regs, 2); > + register_t cid = PSCI_ARG(regs, 3); > + > + perfc_incr(vpsci_cpu_on); > + PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid)); > + return true; > + } > + > + 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); > + register_t cid = PSCI_ARG(regs, 3); > + > + perfc_incr(vpsci_cpu_suspend); > + PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid)); > + return true; > + } > + > + 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); > + > + perfc_incr(vpsci_cpu_affinity_info); > + PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff)); > + return true; > + } > + default: > + return false; > + } > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c > index 7ca2880173..9b48d52896 100644 > --- a/xen/arch/arm/vsmc.c > +++ b/xen/arch/arm/vsmc.c > @@ -100,41 +100,13 @@ static bool handle_hypervisor(struct cpu_user_regs *regs) > } > } > > -#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val) > -#define PSCI_ARG(reg, n) get_user_reg(reg, n) > - > -#ifdef CONFIG_ARM_64 > -#define PSCI_ARG32(reg, n) (uint32_t)(get_user_reg(reg, n)) > -#else > -#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n) > -#endif > - > /* Existing (pre SMCCC) APIs. This includes PSCI 0.1 interface */ > static bool handle_existing_apis(struct cpu_user_regs *regs) > { > /* Only least 32 bits are significant (ARM DEN 0028B, page 12) */ > - switch ( (uint32_t)get_user_reg(regs, 0) ) > - { > - case PSCI_cpu_off: > - { > - uint32_t pstate = PSCI_ARG32(regs, 1); > - > - perfc_incr(vpsci_cpu_off); > - PSCI_SET_RESULT(regs, do_psci_cpu_off(pstate)); > - return true; > - } > - case PSCI_cpu_on: > - { > - uint32_t vcpuid = PSCI_ARG32(regs, 1); > - register_t epoint = PSCI_ARG(regs, 2); > + uint32_t fid = (uint32_t)get_user_reg(regs, 0); > > - perfc_incr(vpsci_cpu_on); > - PSCI_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint)); > - return true; > - } > - default: > - return false; > - } > + return do_psci_0_1_call(regs, fid); > } > > /* PSCI 0.2 interface and other Standard Secure Calls */ > @@ -142,70 +114,11 @@ static bool handle_sssc(struct cpu_user_regs *regs) > { > uint32_t fid = (uint32_t)get_user_reg(regs, 0); > > - switch ( fid ) > - { > - case PSCI_0_2_FN32(PSCI_VERSION): > - perfc_incr(vpsci_version); > - PSCI_SET_RESULT(regs, do_psci_0_2_version()); > + if ( do_psci_0_2_call(regs, fid) ) > return true; > > - 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_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_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_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_FN32(CPU_ON): > - case PSCI_0_2_FN64(CPU_ON): > - { > - register_t vcpuid = PSCI_ARG(regs, 1); > - register_t epoint = PSCI_ARG(regs, 2); > - register_t cid = PSCI_ARG(regs, 3); > - > - perfc_incr(vpsci_cpu_on); > - PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid)); > - return true; > - } > - > - 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); > - register_t cid = PSCI_ARG(regs, 3); > - > - perfc_incr(vpsci_cpu_suspend); > - PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid)); > - return true; > - } > - > - case PSCI_0_2_FN32(AFFINITY_INFO): > - case PSCI_0_2_FN64(AFFINITY_INFO): > + switch ( fid ) > { > - register_t taff = PSCI_ARG(regs, 1); > - uint32_t laff = PSCI_ARG32(regs, 2); > - > - perfc_incr(vpsci_cpu_affinity_info); > - PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff)); > - return true; > - } > - > case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD): > return fill_function_call_count(regs, SSSC_SMCCC_FUNCTION_COUNT); Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c. Maybe it is time to introduce function get_psci_0_2_fn_count() and use it there, what do you think? > > diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h > index f7e2139031..3075c998f3 100644 > --- a/xen/include/asm-arm/psci.h > +++ b/xen/include/asm-arm/psci.h > @@ -22,24 +22,9 @@ int call_psci_cpu_on(int cpu); > void call_psci_system_off(void); > void call_psci_system_reset(void); > > -/* functions to handle guest PSCI requests */ > -int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point); > -int32_t do_psci_cpu_off(uint32_t power_state); > -int32_t do_psci_cpu_suspend(uint32_t power_state, register_t entry_point); > -int32_t do_psci_migrate(uint32_t vcpuid); > - > -/* PSCI 0.2 functions to handle guest PSCI requests */ > -uint32_t do_psci_0_2_version(void); > -register_t do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point, > - register_t context_id); > -int32_t do_psci_0_2_cpu_off(void); > -int32_t do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point, > - register_t context_id); > -int32_t do_psci_0_2_affinity_info(register_t target_affinity, > - uint32_t lowest_affinity_level); > -uint32_t do_psci_0_2_migrate_info_type(void); > -void do_psci_0_2_system_off(void); > -void do_psci_0_2_system_reset(void); > +/* Functions handle PSCI calls from the guests */ > +bool do_psci_0_1_call(struct cpu_user_regs *regs, uint32_t fid); > +bool do_psci_0_2_call(struct cpu_user_regs *regs, uint32_t fid); > > /* PSCI v0.2 interface */ > #define PSCI_0_2_FN32(name) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ >
Hi, On 26/01/18 18:09, Volodymyr Babchuk wrote: > On 24.01.18 20:34, Julien Grall wrote: >> - case PSCI_0_2_FN32(AFFINITY_INFO): >> - case PSCI_0_2_FN64(AFFINITY_INFO): >> + switch ( fid ) >> { >> - register_t taff = PSCI_ARG(regs, 1); >> - uint32_t laff = PSCI_ARG32(regs, 2); >> - >> - perfc_incr(vpsci_cpu_affinity_info); >> - PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff)); >> - return true; >> - } >> - >> case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD): >> return fill_function_call_count(regs, >> SSSC_SMCCC_FUNCTION_COUNT); > Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c. > Maybe it is time to introduce function get_psci_0_2_fn_count() and use > it there, what do you think? Definitely not a function. It is a static number. But I can think of separate the call count. Cheers, > >> diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h >> index f7e2139031..3075c998f3 100644 >> --- a/xen/include/asm-arm/psci.h >> +++ b/xen/include/asm-arm/psci.h >> @@ -22,24 +22,9 @@ int call_psci_cpu_on(int cpu); >> void call_psci_system_off(void); >> void call_psci_system_reset(void); >> -/* functions to handle guest PSCI requests */ >> -int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point); >> -int32_t do_psci_cpu_off(uint32_t power_state); >> -int32_t do_psci_cpu_suspend(uint32_t power_state, register_t >> entry_point); >> -int32_t do_psci_migrate(uint32_t vcpuid); >> - >> -/* PSCI 0.2 functions to handle guest PSCI requests */ >> -uint32_t do_psci_0_2_version(void); >> -register_t do_psci_0_2_cpu_suspend(uint32_t power_state, register_t >> entry_point, >> - register_t context_id); >> -int32_t do_psci_0_2_cpu_off(void); >> -int32_t do_psci_0_2_cpu_on(register_t target_cpu, register_t >> entry_point, >> - register_t context_id); >> -int32_t do_psci_0_2_affinity_info(register_t target_affinity, >> - uint32_t lowest_affinity_level); >> -uint32_t do_psci_0_2_migrate_info_type(void); >> -void do_psci_0_2_system_off(void); >> -void do_psci_0_2_system_reset(void); >> +/* Functions handle PSCI calls from the guests */ >> +bool do_psci_0_1_call(struct cpu_user_regs *regs, uint32_t fid); >> +bool do_psci_0_2_call(struct cpu_user_regs *regs, uint32_t fid); >> /* PSCI v0.2 interface */ >> #define PSCI_0_2_FN32(name) >> ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ >>
Hi, On 26.01.18 20:15, Julien Grall wrote: > Hi, > > On 26/01/18 18:09, Volodymyr Babchuk wrote: >> On 24.01.18 20:34, Julien Grall wrote: >>> - case PSCI_0_2_FN32(AFFINITY_INFO): >>> - case PSCI_0_2_FN64(AFFINITY_INFO): >>> + switch ( fid ) >>> { >>> - register_t taff = PSCI_ARG(regs, 1); >>> - uint32_t laff = PSCI_ARG32(regs, 2); >>> - >>> - perfc_incr(vpsci_cpu_affinity_info); >>> - PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff)); >>> - return true; >>> - } >>> - >>> case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD): >>> return fill_function_call_count(regs, >>> SSSC_SMCCC_FUNCTION_COUNT); >> Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c. >> Maybe it is time to introduce function get_psci_0_2_fn_count() and use >> it there, what do you think? > > Definitely not a function. It is a static number. But I can think of > separate the call count. Yep, separate call count for vPSCI and for SSSC itself would be a good thing.
On 26/01/18 18:27, Volodymyr Babchuk wrote: > Hi, Hi Volodymyr, > On 26.01.18 20:15, Julien Grall wrote: >> Hi, >> >> On 26/01/18 18:09, Volodymyr Babchuk wrote: >>> On 24.01.18 20:34, Julien Grall wrote: >>>> - case PSCI_0_2_FN32(AFFINITY_INFO): >>>> - case PSCI_0_2_FN64(AFFINITY_INFO): >>>> + switch ( fid ) >>>> { >>>> - register_t taff = PSCI_ARG(regs, 1); >>>> - uint32_t laff = PSCI_ARG32(regs, 2); >>>> - >>>> - perfc_incr(vpsci_cpu_affinity_info); >>>> - PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff)); >>>> - return true; >>>> - } >>>> - >>>> case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD): >>>> return fill_function_call_count(regs, >>>> SSSC_SMCCC_FUNCTION_COUNT); >>> Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c. >>> Maybe it is time to introduce function get_psci_0_2_fn_count() and >>> use it there, what do you think? >> >> Definitely not a function. It is a static number. But I can think of >> separate the call count. > Yep, separate call count for vPSCI and for SSSC itself would be a good > thing. Looking a bit more into it, this will not make a real improvement. This will be equally difficult to remember to update the call count. Also, based on the recent SMCCC update (2.1.5 [1]): "The General Service Queries for SMCCC call ranges are described in SMCCC section 6.2. These functions are not always well suited to firmware that is integrated with multiple sub-services being combined into one service range. For example, PSCI and SDEI in the Standard Service range. In particular, the ‘call count’ and ‘revision’ functions do not provide useful information to the caller when multiple functions are provided. As a result, these are not widely used to identify firmware services." So I would not worry that much if the function count is not sync in the future. BTW, it is already wrong in Xen 4.10. For standard service, we don't implement 13 but 52. Cheers, [1] https://developer.arm.com/-/media/developer/pdf/ARM%20DEN%200070A%20Firmware%20interfaces%20for%20mitigating%20CVE-2017-5715_V1.0.pdf?revision=d20d1476-b770-4739-aebe-2d3147788e3f&la=en I don't expect PSCI to be
Hi Julien, On 30.01.18 20:01, Julien Grall wrote: > > > On 26/01/18 18:27, Volodymyr Babchuk wrote: >> Hi, > > Hi Volodymyr, > >> On 26.01.18 20:15, Julien Grall wrote: >>> Hi, >>> >>> On 26/01/18 18:09, Volodymyr Babchuk wrote: >>>> On 24.01.18 20:34, Julien Grall wrote: >>>>> - case PSCI_0_2_FN32(AFFINITY_INFO): >>>>> - case PSCI_0_2_FN64(AFFINITY_INFO): >>>>> + switch ( fid ) >>>>> { >>>>> - register_t taff = PSCI_ARG(regs, 1); >>>>> - uint32_t laff = PSCI_ARG32(regs, 2); >>>>> - >>>>> - perfc_incr(vpsci_cpu_affinity_info); >>>>> - PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff)); >>>>> - return true; >>>>> - } >>>>> - >>>>> case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD): >>>>> return fill_function_call_count(regs, >>>>> SSSC_SMCCC_FUNCTION_COUNT); >>>> Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c. >>>> Maybe it is time to introduce function get_psci_0_2_fn_count() and >>>> use it there, what do you think? >>> >>> Definitely not a function. It is a static number. But I can think of >>> separate the call count. >> Yep, separate call count for vPSCI and for SSSC itself would be a good >> thing. > > Looking a bit more into it, this will not make a real improvement. This > will be equally difficult to remember to update the call count. Nevertheless, I think that it is right thing to hold call count in the same file, where calls are implemented. This increases chances that call count will be held in sync. > Also, based on the recent SMCCC update (2.1.5 [1]): > "The General Service Queries for SMCCC call ranges are described in > SMCCC section 6.2. These functions are not always well suited to > firmware that is integrated with multiple sub-services being combined > into one service range. For example, PSCI and SDEI in the Standard > Service range. In particular, the ‘call count’ and ‘revision’ functions > do not provide useful information to the caller when multiple functions > are provided. As a result, these are not widely used to identify > firmware services." > > So I would not worry that much if the function count is not sync in the > future. BTW, it is already wrong in Xen 4.10. For standard service, we > don't implement 13 but 52. 2.1.5 deprecates general service queries in SMCCC 1.1. So, either we upgradeSMCCC in Xen to 1.1 or we should be conform with SMCCC 1.0 and should return right number of functions, including changes in Xen 4.10 that you mentioned. BTW, personally I'm not happy with call UID and call version deprecation. This makes impossible dynamic discovery of TEE, which was used in my TEE mediator patch series. > Cheers, > > [1] > https://developer.arm.com/-/media/developer/pdf/ARM%20DEN%200070A%20Firmware%20interfaces%20for%20mitigating%20CVE-2017-5715_V1.0.pdf?revision=d20d1476-b770-4739-aebe-2d3147788e3f&la=en > > > I don't expect PSCI to be > There is some text missing. Are you wanted to add something?
On 30/01/18 18:28, Volodymyr Babchuk wrote: > Hi Julien, > > On 30.01.18 20:01, Julien Grall wrote: >> >> >> On 26/01/18 18:27, Volodymyr Babchuk wrote: >>> Hi, >> >> Hi Volodymyr, >> >>> On 26.01.18 20:15, Julien Grall wrote: >>>> Hi, >>>> >>>> On 26/01/18 18:09, Volodymyr Babchuk wrote: >>>>> On 24.01.18 20:34, Julien Grall wrote: >>>>>> - case PSCI_0_2_FN32(AFFINITY_INFO): >>>>>> - case PSCI_0_2_FN64(AFFINITY_INFO): >>>>>> + switch ( fid ) >>>>>> { >>>>>> - register_t taff = PSCI_ARG(regs, 1); >>>>>> - uint32_t laff = PSCI_ARG32(regs, 2); >>>>>> - >>>>>> - perfc_incr(vpsci_cpu_affinity_info); >>>>>> - PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, >>>>>> laff)); >>>>>> - return true; >>>>>> - } >>>>>> - >>>>>> case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD): >>>>>> return fill_function_call_count(regs, >>>>>> SSSC_SMCCC_FUNCTION_COUNT); >>>>> Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c. >>>>> Maybe it is time to introduce function get_psci_0_2_fn_count() and >>>>> use it there, what do you think? >>>> >>>> Definitely not a function. It is a static number. But I can think of >>>> separate the call count. >>> Yep, separate call count for vPSCI and for SSSC itself would be a >>> good thing. >> >> Looking a bit more into it, this will not make a real improvement. >> This will be equally difficult to remember to update the call count. > Nevertheless, I think that it is right thing to hold call count in the > same file, where calls are implemented. This increases chances that call > count will be held in sync. So you are suggesting to implement a function? If so, that's a no-go from my side. > >> Also, based on the recent SMCCC update (2.1.5 [1]): >> "The General Service Queries for SMCCC call ranges are described in >> SMCCC section 6.2. These functions are not always well suited to >> firmware that is integrated with multiple sub-services being combined >> into one service range. For example, PSCI and SDEI in the Standard >> Service range. In particular, the ‘call count’ and ‘revision’ >> functions do not provide useful information to the caller when >> multiple functions are provided. As a result, these are not widely >> used to identify firmware services." >> >> So I would not worry that much if the function count is not sync in >> the future. BTW, it is already wrong in Xen 4.10. For standard >> service, we don't implement 13 but 52. > 2.1.5 deprecates general service queries in SMCCC 1.1. So, either we > upgradeSMCCC in Xen to 1.1 or we should be conform with SMCCC 1.0 and > should return right number of functions, including changes in Xen 4.10 > that you mentioned. > > BTW, personally I'm not happy with call UID and call version > deprecation. This makes impossible dynamic discovery of TEE, which was > used in my TEE mediator patch series. 2.1.5 only deprecates for Architecture service which is not implemented for Xen at moment. Nothing is been said for the rest of them OP-TEE. The documentation only acknowledge the fact it is difficult for some service (such as SSSC) to infer information from that. I looked at some implementation of SMCCC and those calls are either not handled or the number are not correct. Anyway, I have a SMCCC 1.1 patch series coming up soon. Cheers,
On 30.01.18 20:44, Julien Grall wrote: > > > On 30/01/18 18:28, Volodymyr Babchuk wrote: >> Hi Julien, >> >> On 30.01.18 20:01, Julien Grall wrote: >>> >>> >>> On 26/01/18 18:27, Volodymyr Babchuk wrote: >>>> Hi, >>> >>> Hi Volodymyr, >>> >>>> On 26.01.18 20:15, Julien Grall wrote: >>>>> Hi, >>>>> >>>>> On 26/01/18 18:09, Volodymyr Babchuk wrote: >>>>>> On 24.01.18 20:34, Julien Grall wrote: >>>>>>> - case PSCI_0_2_FN32(AFFINITY_INFO): >>>>>>> - case PSCI_0_2_FN64(AFFINITY_INFO): >>>>>>> + switch ( fid ) >>>>>>> { >>>>>>> - register_t taff = PSCI_ARG(regs, 1); >>>>>>> - uint32_t laff = PSCI_ARG32(regs, 2); >>>>>>> - >>>>>>> - perfc_incr(vpsci_cpu_affinity_info); >>>>>>> - PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, >>>>>>> laff)); >>>>>>> - return true; >>>>>>> - } >>>>>>> - >>>>>>> case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD): >>>>>>> return fill_function_call_count(regs, >>>>>>> SSSC_SMCCC_FUNCTION_COUNT); >>>>>> Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c. >>>>>> Maybe it is time to introduce function get_psci_0_2_fn_count() and >>>>>> use it there, what do you think? >>>>> >>>>> Definitely not a function. It is a static number. But I can think >>>>> of separate the call count. >>>> Yep, separate call count for vPSCI and for SSSC itself would be a >>>> good thing. >>> >>> Looking a bit more into it, this will not make a real improvement. >>> This will be equally difficult to remember to update the call count. >> Nevertheless, I think that it is right thing to hold call count in the >> same file, where calls are implemented. This increases chances that >> call count will be held in sync. > > So you are suggesting to implement a function? If so, that's a no-go > from my side. I'm not insist on function if you can propose alternative. But why you are against getter function in the first place? I wanted to propose another approach: define this call count in vpsci.h, but there is no vpsci.h, instead you use psci.h, which is confusing in itself. >> >>> Also, based on the recent SMCCC update (2.1.5 [1]): >>> "The General Service Queries for SMCCC call ranges are described in >>> SMCCC section 6.2. These functions are not always well suited to >>> firmware that is integrated with multiple sub-services being combined >>> into one service range. For example, PSCI and SDEI in the Standard >>> Service range. In particular, the ‘call count’ and ‘revision’ >>> functions do not provide useful information to the caller when >>> multiple functions are provided. As a result, these are not widely >>> used to identify firmware services." >>> >>> So I would not worry that much if the function count is not sync in >>> the future. BTW, it is already wrong in Xen 4.10. For standard >>> service, we don't implement 13 but 52. >> 2.1.5 deprecates general service queries in SMCCC 1.1. So, either we >> upgradeSMCCC in Xen to 1.1 or we should be conform with SMCCC 1.0 and >> should return right number of functions, including changes in Xen 4.10 >> that you mentioned. >> >> BTW, personally I'm not happy with call UID and call version >> deprecation. This makes impossible dynamic discovery of TEE, which was >> used in my TEE mediator patch series. > > 2.1.5 only deprecates for Architecture service which is not implemented > for Xen at moment. Nothing is been said for the rest of them OP-TEE. The > documentation only acknowledge the fact it is difficult for some service > (such as SSSC) to infer information from that. Ah, sorry. I missed that point. But, anyways, if it is deprecated only for Architecture service, then all other services still should provide right responses to standard queries. > > I looked at some implementation of SMCCC and those calls are either not > handled or the number are not correct. I agree that *some* implementations can not conform to SMCCC.But, I thought you want Xen to follow standards as close as possible... > Anyway, I have a SMCCC 1.1 patch series coming up soon. That would be great.
On 30 January 2018 at 19:35, Volodymyr Babchuk <volodymyr_babchuk@epam.com> wrote: > > > On 30.01.18 20:44, Julien Grall wrote: >> >> >> >> On 30/01/18 18:28, Volodymyr Babchuk wrote: >>> >>> Hi Julien, >>> >>> On 30.01.18 20:01, Julien Grall wrote: >>>> >>>> >>>> >>>> On 26/01/18 18:27, Volodymyr Babchuk wrote: >>>>> >>>>> Hi, >>>> >>>> >>>> Hi Volodymyr, >>>> >>>>> On 26.01.18 20:15, Julien Grall wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> On 26/01/18 18:09, Volodymyr Babchuk wrote: >>>>>>> >>>>>>> On 24.01.18 20:34, Julien Grall wrote: >>>>>>>> >>>>>>>> - case PSCI_0_2_FN32(AFFINITY_INFO): >>>>>>>> - case PSCI_0_2_FN64(AFFINITY_INFO): >>>>>>>> + switch ( fid ) >>>>>>>> { >>>>>>>> - register_t taff = PSCI_ARG(regs, 1); >>>>>>>> - uint32_t laff = PSCI_ARG32(regs, 2); >>>>>>>> - >>>>>>>> - perfc_incr(vpsci_cpu_affinity_info); >>>>>>>> - PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, >>>>>>>> laff)); >>>>>>>> - return true; >>>>>>>> - } >>>>>>>> - >>>>>>>> case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD): >>>>>>>> return fill_function_call_count(regs, >>>>>>>> SSSC_SMCCC_FUNCTION_COUNT); >>>>>>> >>>>>>> Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c. >>>>>>> Maybe it is time to introduce function get_psci_0_2_fn_count() and >>>>>>> use it there, what do you think? >>>>>> >>>>>> >>>>>> Definitely not a function. It is a static number. But I can think of >>>>>> separate the call count. >>>>> >>>>> Yep, separate call count for vPSCI and for SSSC itself would be a good >>>>> thing. >>>> >>>> >>>> Looking a bit more into it, this will not make a real improvement. This >>>> will be equally difficult to remember to update the call count. >>> >>> Nevertheless, I think that it is right thing to hold call count in the >>> same file, where calls are implemented. This increases chances that call >>> count will be held in sync. >> >> >> So you are suggesting to implement a function? If so, that's a no-go from >> my side. > > I'm not insist on function if you can propose alternative. > But why you are against getter function in the first place? Because a function returning a const value is pretty dumb. > > I wanted to propose another approach: define this call count in > vpsci.h, but there is no vpsci.h, instead you use psci.h, which is confusing > in itself. I thought about vpsci.h, but basically you will have only 2 functions in it and the number of PSCI calls. That's it. So it is not going to help to update because the header will unlikely need to change when adding new PSCI call. [...] >> >> I looked at some implementation of SMCCC and those calls are either not >> handled or the number are not correct. > > I agree that *some* implementations can not conform to SMCCC.But, I thought > you want Xen to follow standards as close as possible... It is not about cannot conform but only implements partially SMCCC. I could name ATF for that (yes). So it makes me more confident the call count is not something people seem to care. Cheers,
Hi Julien, On 31.01.18 00:06, Julien Grall wrote: > On 30 January 2018 at 19:35, Volodymyr Babchuk > <volodymyr_babchuk@epam.com> wrote: >> >> >> On 30.01.18 20:44, Julien Grall wrote: >>> >>> >>> >>> On 30/01/18 18:28, Volodymyr Babchuk wrote: >>>> >>>> Hi Julien, >>>> >>>> On 30.01.18 20:01, Julien Grall wrote: >>>>> >>>>> >>>>> >>>>> On 26/01/18 18:27, Volodymyr Babchuk wrote: >>>>>> >>>>>> Hi, >>>>> >>>>> >>>>> Hi Volodymyr, >>>>> >>>>>> On 26.01.18 20:15, Julien Grall wrote: >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> On 26/01/18 18:09, Volodymyr Babchuk wrote: >>>>>>>> >>>>>>>> On 24.01.18 20:34, Julien Grall wrote: >>>>>>>>> >>>>>>>>> - case PSCI_0_2_FN32(AFFINITY_INFO): >>>>>>>>> - case PSCI_0_2_FN64(AFFINITY_INFO): >>>>>>>>> + switch ( fid ) >>>>>>>>> { >>>>>>>>> - register_t taff = PSCI_ARG(regs, 1); >>>>>>>>> - uint32_t laff = PSCI_ARG32(regs, 2); >>>>>>>>> - >>>>>>>>> - perfc_incr(vpsci_cpu_affinity_info); >>>>>>>>> - PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, >>>>>>>>> laff)); >>>>>>>>> - return true; >>>>>>>>> - } >>>>>>>>> - >>>>>>>>> case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD): >>>>>>>>> return fill_function_call_count(regs, >>>>>>>>> SSSC_SMCCC_FUNCTION_COUNT); >>>>>>>> >>>>>>>> Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c. >>>>>>>> Maybe it is time to introduce function get_psci_0_2_fn_count() and >>>>>>>> use it there, what do you think? >>>>>>> >>>>>>> >>>>>>> Definitely not a function. It is a static number. But I can think of >>>>>>> separate the call count. >>>>>> >>>>>> Yep, separate call count for vPSCI and for SSSC itself would be a good >>>>>> thing. >>>>> >>>>> >>>>> Looking a bit more into it, this will not make a real improvement. This >>>>> will be equally difficult to remember to update the call count. >>>> >>>> Nevertheless, I think that it is right thing to hold call count in the >>>> same file, where calls are implemented. This increases chances that call >>>> count will be held in sync. >>> >>> >>> So you are suggesting to implement a function? If so, that's a no-go from >>> my side. >> >> I'm not insist on function if you can propose alternative. >> But why you are against getter function in the first place? > > Because a function returning a const value is pretty dumb. I can't agree with you there. In my opinion - if it is needed for clarity - then it is fine. Nevertheless, you have my opinion. It is up to you what to do with it. >> >> I wanted to propose another approach: define this call count in >> vpsci.h, but there is no vpsci.h, instead you use psci.h, which is confusing >> in itself. > > I thought about vpsci.h, but basically you will have only 2 functions in it and > the number of PSCI calls. That's it. Is this really a problem? It is quite natural to find declarations for something.c in something.h. By moving declaration into different file, you are hiding it from anyone who does not carry sacred knowledge (or use grep/cscope, yes). And then, when people decide to extend something.c they continue to put declarations into inappropriate.h. Just look at processor.h as a good example. All functions it define are implemented either in traps.c or domain.c. But functions from processor.c are defined in procinfo.h. I can tell for sure, that this confuses newbies. > So it is not going to help to update because the header will unlikely need to > change when adding new PSCI call. Yes. But at least we can put comment above switch(fid): /* Please don't forget to update call count in (v)psci.h */ > [...] > >>> >>> I looked at some implementation of SMCCC and those calls are either not >>> handled or the number are not correct. >> >> I agree that *some* implementations can not conform to SMCCC.But, I thought >> you want Xen to follow standards as close as possible... > > It is not about cannot conform but only implements partially SMCCC. I could name > ATF for that (yes). > > So it makes me more confident the call count is not something people seem to > care. So, you propose to fix this only when something will trip over this?
Hi, On 31/01/18 11:32, Volodymyr Babchuk wrote: >> I thought about vpsci.h, but basically you will have only 2 functions >> in it and >> the number of PSCI calls. That's it. > > Is this really a problem? It is quite natural to find declarations for > something.c in something.h. By moving declaration into different file, > you are hiding it from anyone who does not carry sacred knowledge (or > use grep/cscope, yes). > And then, when people decide to extend something.c they continue to put > declarations into inappropriate.h. Just look at processor.h as a good > example. All functions it define are implemented either in traps.c or > domain.c. But functions from processor.c are defined in procinfo.h. > I can tell for sure, that this confuses newbies. > > >> So it is not going to help to update because the header will unlikely >> need to >> change when adding new PSCI call. > > Yes. But at least we can put comment above switch(fid): > > /* Please don't forget to update call count in (v)psci.h */ See my answer on my own e-mail I sent few minutes ago. I said I will create a the vpsci.h. > > >> [...] >> >>>> >>>> I looked at some implementation of SMCCC and those calls are either not >>>> handled or the number are not correct. >>> >>> I agree that *some* implementations can not conform to SMCCC.But, I >>> thought >>> you want Xen to follow standards as close as possible... >> >> It is not about cannot conform but only implements partially SMCCC. I >> could name >> ATF for that (yes). >> >> So it makes me more confident the call count is not something people >> seem to >> care. > > So, you propose to fix this only when something will trip over this? No, I am just saying that it is not that important if we get it wrong. That number does not tell you anything. You can have 2 functions and still does not know where they are (it is not always possible to just probe a function ID because they may have side-effect). Cheers,
Hi, On 31.01.18 13:36, Julien Grall wrote: > Hi, > > On 31/01/18 11:32, Volodymyr Babchuk wrote: >>> I thought about vpsci.h, but basically you will have only 2 functions >>> in it and >>> the number of PSCI calls. That's it. >> >> Is this really a problem? It is quite natural to find declarations >> for something.c in something.h. By moving declaration into different >> file, you are hiding it from anyone who does not carry sacred >> knowledge (or use grep/cscope, yes). >> And then, when people decide to extend something.c they continue to >> put declarations into inappropriate.h. Just look at processor.h as a >> good example. All functions it define are implemented either in >> traps.c or domain.c. But functions from processor.c are defined in >> procinfo.h. >> I can tell for sure, that this confuses newbies. >> >> >>> So it is not going to help to update because the header will unlikely >>> need to >>> change when adding new PSCI call. >> >> Yes. But at least we can put comment above switch(fid): >> >> /* Please don't forget to update call count in (v)psci.h */ > > See my answer on my own e-mail I sent few minutes ago. I said I will > create a the vpsci.h. > I can't find it anywhere. I even tried Markmail. Would you please point it to me? [...]
On 31/01/18 14:29, Volodymyr Babchuk wrote: > Hi, Hi Volodymyr, > On 31.01.18 13:36, Julien Grall wrote: >> Hi, >> >> On 31/01/18 11:32, Volodymyr Babchuk wrote: >>>> I thought about vpsci.h, but basically you will have only 2 >>>> functions in it and >>>> the number of PSCI calls. That's it. >>> >>> Is this really a problem? It is quite natural to find declarations >>> for something.c in something.h. By moving declaration into different >>> file, you are hiding it from anyone who does not carry sacred >>> knowledge (or use grep/cscope, yes). >>> And then, when people decide to extend something.c they continue to >>> put declarations into inappropriate.h. Just look at processor.h as a >>> good example. All functions it define are implemented either in >>> traps.c or domain.c. But functions from processor.c are defined in >>> procinfo.h. >>> I can tell for sure, that this confuses newbies. >>> >>> >>>> So it is not going to help to update because the header will >>>> unlikely need to >>>> change when adding new PSCI call. >>> >>> Yes. But at least we can put comment above switch(fid): >>> >>> /* Please don't forget to update call count in (v)psci.h */ >> >> See my answer on my own e-mail I sent few minutes ago. I said I will >> create a the vpsci.h. >> > > I can't find it anywhere. I even tried Markmail. Would you please point > it to me? I wrote it somewhere and thought I sent but I can't find it :/. Sorry for that. I was saying after some thought, I will create a header vpsic.h with number in it. I will also add a comment as you suggested. Cheers,
On 31.01.18 16:54, Julien Grall wrote: >>> On 31/01/18 11:32, Volodymyr Babchuk wrote: >>>>> I thought about vpsci.h, but basically you will have only 2 >>>>> functions in it and >>>>> the number of PSCI calls. That's it. >>>> >>>> Is this really a problem? It is quite natural to find declarations >>>> for something.c in something.h. By moving declaration into different >>>> file, you are hiding it from anyone who does not carry sacred >>>> knowledge (or use grep/cscope, yes). >>>> And then, when people decide to extend something.c they continue to >>>> put declarations into inappropriate.h. Just look at processor.h as a >>>> good example. All functions it define are implemented either in >>>> traps.c or domain.c. But functions from processor.c are defined in >>>> procinfo.h. >>>> I can tell for sure, that this confuses newbies. >>>> >>>> >>>>> So it is not going to help to update because the header will >>>>> unlikely need to >>>>> change when adding new PSCI call. >>>> >>>> Yes. But at least we can put comment above switch(fid): >>>> >>>> /* Please don't forget to update call count in (v)psci.h */ >>> >>> See my answer on my own e-mail I sent few minutes ago. I said I will >>> create a the vpsci.h. >>> >> >> I can't find it anywhere. I even tried Markmail. Would you please >> point it to me? > > I wrote it somewhere and thought I sent but I can't find it :/. Sorry > for that. > Ah, I see. It is okay. > I was saying after some thought, I will create a header vpsic.h with > number in it. > > I will also add a comment as you suggested. Thanks. I'm fine with this.
diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c index 979d32ed6d..b3ee193621 100644 --- a/xen/arch/arm/vpsci.c +++ b/xen/arch/arm/vpsci.c @@ -91,12 +91,12 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point, return PSCI_SUCCESS; } -int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point) +static int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point) { return do_common_cpu_on(vcpuid, entry_point, 0 , PSCI_VERSION(0, 1)); } -int32_t do_psci_cpu_off(uint32_t power_state) +static int32_t do_psci_cpu_off(uint32_t power_state) { struct vcpu *v = current; if ( !test_and_set_bit(_VPF_down, &v->pause_flags) ) @@ -104,13 +104,14 @@ int32_t do_psci_cpu_off(uint32_t power_state) return PSCI_SUCCESS; } -uint32_t do_psci_0_2_version(void) +static uint32_t do_psci_0_2_version(void) { return PSCI_VERSION(0, 2); } -register_t do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point, - register_t context_id) +static register_t do_psci_0_2_cpu_suspend(uint32_t power_state, + register_t entry_point, + register_t context_id) { struct vcpu *v = current; @@ -123,13 +124,14 @@ register_t do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point, return PSCI_SUCCESS; } -int32_t do_psci_0_2_cpu_off(void) +static int32_t do_psci_0_2_cpu_off(void) { return do_psci_cpu_off(0); } -int32_t do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point, - register_t context_id) +static int32_t do_psci_0_2_cpu_on(register_t target_cpu, + register_t entry_point, + register_t context_id) { return do_common_cpu_on(target_cpu, entry_point, context_id, PSCI_VERSION(0, 2)); @@ -144,8 +146,8 @@ static const unsigned long target_affinity_mask[] = { #endif }; -int32_t do_psci_0_2_affinity_info(register_t target_affinity, - uint32_t lowest_affinity_level) +static int32_t do_psci_0_2_affinity_info(register_t target_affinity, + uint32_t lowest_affinity_level) { struct domain *d = current->domain; struct vcpu *v; @@ -172,23 +174,136 @@ int32_t do_psci_0_2_affinity_info(register_t target_affinity, return PSCI_0_2_AFFINITY_LEVEL_OFF; } -uint32_t do_psci_0_2_migrate_info_type(void) +static uint32_t do_psci_0_2_migrate_info_type(void) { return PSCI_0_2_TOS_MP_OR_NOT_PRESENT; } -void do_psci_0_2_system_off( void ) +static void do_psci_0_2_system_off( void ) { struct domain *d = current->domain; domain_shutdown(d,SHUTDOWN_poweroff); } -void do_psci_0_2_system_reset(void) +static void do_psci_0_2_system_reset(void) { struct domain *d = current->domain; domain_shutdown(d,SHUTDOWN_reboot); } +#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val) +#define PSCI_ARG(reg, n) get_user_reg(reg, n) + +#ifdef CONFIG_ARM_64 +#define PSCI_ARG32(reg, n) (uint32_t)(get_user_reg(reg, n)) +#else +#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n) +#endif + +/* + * PSCI 0.1 calls. It will return false if the function ID is not + * handled. + */ +bool do_psci_0_1_call(struct cpu_user_regs *regs, uint32_t fid) +{ + switch ( (uint32_t)get_user_reg(regs, 0) ) + { + case PSCI_cpu_off: + { + uint32_t pstate = PSCI_ARG32(regs, 1); + + perfc_incr(vpsci_cpu_off); + PSCI_SET_RESULT(regs, do_psci_cpu_off(pstate)); + return true; + } + case PSCI_cpu_on: + { + uint32_t vcpuid = PSCI_ARG32(regs, 1); + register_t epoint = PSCI_ARG(regs, 2); + + perfc_incr(vpsci_cpu_on); + PSCI_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint)); + return true; + } + default: + return false; + } +} + +/* + * PSCI 0.2 or later calls. It will return false if the function ID is + * not handled. + */ +bool do_psci_0_2_call(struct cpu_user_regs *regs, uint32_t fid) +{ + switch ( fid ) + { + 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_FN32(CPU_OFF): + perfc_incr(vpsci_cpu_off); + PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off()); + return true; + + 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_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_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_FN32(CPU_ON): + case PSCI_0_2_FN64(CPU_ON): + { + register_t vcpuid = PSCI_ARG(regs, 1); + register_t epoint = PSCI_ARG(regs, 2); + register_t cid = PSCI_ARG(regs, 3); + + perfc_incr(vpsci_cpu_on); + PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid)); + return true; + } + + 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); + register_t cid = PSCI_ARG(regs, 3); + + perfc_incr(vpsci_cpu_suspend); + PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid)); + return true; + } + + 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); + + perfc_incr(vpsci_cpu_affinity_info); + PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff)); + return true; + } + default: + return false; + } +} + /* * Local variables: * mode: C diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c index 7ca2880173..9b48d52896 100644 --- a/xen/arch/arm/vsmc.c +++ b/xen/arch/arm/vsmc.c @@ -100,41 +100,13 @@ static bool handle_hypervisor(struct cpu_user_regs *regs) } } -#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val) -#define PSCI_ARG(reg, n) get_user_reg(reg, n) - -#ifdef CONFIG_ARM_64 -#define PSCI_ARG32(reg, n) (uint32_t)(get_user_reg(reg, n)) -#else -#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n) -#endif - /* Existing (pre SMCCC) APIs. This includes PSCI 0.1 interface */ static bool handle_existing_apis(struct cpu_user_regs *regs) { /* Only least 32 bits are significant (ARM DEN 0028B, page 12) */ - switch ( (uint32_t)get_user_reg(regs, 0) ) - { - case PSCI_cpu_off: - { - uint32_t pstate = PSCI_ARG32(regs, 1); - - perfc_incr(vpsci_cpu_off); - PSCI_SET_RESULT(regs, do_psci_cpu_off(pstate)); - return true; - } - case PSCI_cpu_on: - { - uint32_t vcpuid = PSCI_ARG32(regs, 1); - register_t epoint = PSCI_ARG(regs, 2); + uint32_t fid = (uint32_t)get_user_reg(regs, 0); - perfc_incr(vpsci_cpu_on); - PSCI_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint)); - return true; - } - default: - return false; - } + return do_psci_0_1_call(regs, fid); } /* PSCI 0.2 interface and other Standard Secure Calls */ @@ -142,70 +114,11 @@ static bool handle_sssc(struct cpu_user_regs *regs) { uint32_t fid = (uint32_t)get_user_reg(regs, 0); - switch ( fid ) - { - case PSCI_0_2_FN32(PSCI_VERSION): - perfc_incr(vpsci_version); - PSCI_SET_RESULT(regs, do_psci_0_2_version()); + if ( do_psci_0_2_call(regs, fid) ) return true; - 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_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_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_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_FN32(CPU_ON): - case PSCI_0_2_FN64(CPU_ON): - { - register_t vcpuid = PSCI_ARG(regs, 1); - register_t epoint = PSCI_ARG(regs, 2); - register_t cid = PSCI_ARG(regs, 3); - - perfc_incr(vpsci_cpu_on); - PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid)); - return true; - } - - 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); - register_t cid = PSCI_ARG(regs, 3); - - perfc_incr(vpsci_cpu_suspend); - PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid)); - return true; - } - - case PSCI_0_2_FN32(AFFINITY_INFO): - case PSCI_0_2_FN64(AFFINITY_INFO): + switch ( fid ) { - register_t taff = PSCI_ARG(regs, 1); - uint32_t laff = PSCI_ARG32(regs, 2); - - perfc_incr(vpsci_cpu_affinity_info); - PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff)); - return true; - } - case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD): return fill_function_call_count(regs, SSSC_SMCCC_FUNCTION_COUNT); diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h index f7e2139031..3075c998f3 100644 --- a/xen/include/asm-arm/psci.h +++ b/xen/include/asm-arm/psci.h @@ -22,24 +22,9 @@ int call_psci_cpu_on(int cpu); void call_psci_system_off(void); void call_psci_system_reset(void); -/* functions to handle guest PSCI requests */ -int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point); -int32_t do_psci_cpu_off(uint32_t power_state); -int32_t do_psci_cpu_suspend(uint32_t power_state, register_t entry_point); -int32_t do_psci_migrate(uint32_t vcpuid); - -/* PSCI 0.2 functions to handle guest PSCI requests */ -uint32_t do_psci_0_2_version(void); -register_t do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point, - register_t context_id); -int32_t do_psci_0_2_cpu_off(void); -int32_t do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point, - register_t context_id); -int32_t do_psci_0_2_affinity_info(register_t target_affinity, - uint32_t lowest_affinity_level); -uint32_t do_psci_0_2_migrate_info_type(void); -void do_psci_0_2_system_off(void); -void do_psci_0_2_system_reset(void); +/* Functions handle PSCI calls from the guests */ +bool do_psci_0_1_call(struct cpu_user_regs *regs, uint32_t fid); +bool do_psci_0_2_call(struct cpu_user_regs *regs, uint32_t fid); /* PSCI v0.2 interface */ #define PSCI_0_2_FN32(name) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
At the moment PSCI function dispatching is done in vsmc.c and the function implementation in vpsci.c. Some bits of the implementation is even done in vsmc.c (see PSCI_SYSTEM_RESET). This means that it is difficult to follow the implementation and also requires to export functions for each PSCI functions. Therefore move PSCI dispatching in two new functions do_psci_0_1_call and do_psci_0_2_call. The former will handle PSCI 0.1 call while the latter 0.2 or later call. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/vpsci.c | 141 ++++++++++++++++++++++++++++++++++++++++----- xen/arch/arm/vsmc.c | 95 ++---------------------------- xen/include/asm-arm/psci.h | 21 +------ 3 files changed, 135 insertions(+), 122 deletions(-)