From: Julien Grall <julien.grall@arm.com>
To: Volodymyr Babchuk <volodymyr_babchuk@epam.com>, xen-devel@lists.xen.org
Cc: "Edgar E . Iglesias" <edgar.iglesias@xilinx.com>,
Stefano Stabellini <sstabellini@kernel.org>,
Wei Liu <wei.liu2@citrix.com>,
George Dunlap <George.Dunlap@eu.citrix.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Ian Jackson <ian.jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
Jan Beulich <jbeulich@suse.com>
Subject: Re: [PATCH v5 07/10] arm: traps: handle PSCI calls inside `vsmc.c`
Date: Wed, 13 Sep 2017 12:53:16 +0100 [thread overview]
Message-ID: <16e3f6c0-0167-890b-cbd5-531807152bbc@arm.com> (raw)
In-Reply-To: <1504210172-27234-8-git-send-email-volodymyr_babchuk@epam.com>
Hi Volodymyr,
On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:
> PSCI is part of HVC/SMC interface, so it should be handled in
> appropriate place: `vsmc.c`. This patch moves PSCI handler
> calls from `traps.c` to `vsmc.c`. Also it corrects coding
> style of the PSCI handler functions.
>
> Older PSCI 0.1 uses SMC function identifiers in range that is
> reserved for existing APIs (ARM DEN 0028B, page 16), while newer
> PSCI 0.2 and later is defined as "standard secure service" with its
> own ranges (ARM DEN 0028B, page 18).
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>
> * handle_psci_0_x() renamed to handle_existing_apis()
> * spelling fixes
> * fixed coding style for moved PSCI code
> * previously introduced `funcid` moved to previous patch
>
> ---
> xen/arch/arm/traps.c | 117 +---------------------
> xen/arch/arm/vsmc.c | 189 +++++++++++++++++++++++++++++++++++-
> xen/include/asm-arm/traps.h | 1 +
> xen/include/public/arch-arm/smccc.h | 8 ++
> 4 files changed, 196 insertions(+), 119 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c,
> index f3b64b4..d00ff36 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1450,119 +1450,6 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
> }
> #endif
>
> -#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
> -
> -/* helper function for checking arm mode 32/64 bit */
> -static inline int psci_mode_check(struct domain *d, register_t fid)
> -{
> - return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
> -}
> -
> -static void do_trap_psci(struct cpu_user_regs *regs)
> -{
> - register_t fid = PSCI_ARG(regs,0);
> -
> - /* preloading in case psci_mode_check fails */
> - PSCI_SET_RESULT(regs, PSCI_INVALID_PARAMETERS);
> - switch( fid )
> - {
> - 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));
> - }
> - break;
> - 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));
> - }
> - break;
> - case PSCI_0_2_FN_PSCI_VERSION:
> - perfc_incr(vpsci_version);
> - PSCI_SET_RESULT(regs, do_psci_0_2_version());
> - break;
> - case PSCI_0_2_FN_CPU_OFF:
> - perfc_incr(vpsci_cpu_off);
> - PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
> - break;
> - case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
> - perfc_incr(vpsci_migrate_info_type);
> - PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type());
> - break;
> - case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU:
> - case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
> - perfc_incr(vpsci_migrate_info_up_cpu);
> - if ( psci_mode_check(current->domain, fid) )
> - PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
> - break;
> - case PSCI_0_2_FN_SYSTEM_OFF:
> - perfc_incr(vpsci_system_off);
> - do_psci_0_2_system_off();
> - PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
> - break;
> - case PSCI_0_2_FN_SYSTEM_RESET:
> - perfc_incr(vpsci_system_reset);
> - do_psci_0_2_system_reset();
> - PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
> - break;
> - case PSCI_0_2_FN_CPU_ON:
> - case PSCI_0_2_FN64_CPU_ON:
> - perfc_incr(vpsci_cpu_on);
> - if ( psci_mode_check(current->domain, fid) )
> - {
> - register_t vcpuid = PSCI_ARG(regs,1);
> - register_t epoint = PSCI_ARG(regs,2);
> - register_t cid = PSCI_ARG(regs,3);
> - PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
> - }
> - break;
> - case PSCI_0_2_FN_CPU_SUSPEND:
> - case PSCI_0_2_FN64_CPU_SUSPEND:
> - perfc_incr(vpsci_cpu_suspend);
> - if ( psci_mode_check(current->domain, fid) )
> - {
> - uint32_t pstate = PSCI_ARG32(regs,1);
> - register_t epoint = PSCI_ARG(regs,2);
> - register_t cid = PSCI_ARG(regs,3);
> - PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid));
> - }
> - break;
> - case PSCI_0_2_FN_AFFINITY_INFO:
> - case PSCI_0_2_FN64_AFFINITY_INFO:
> - perfc_incr(vpsci_cpu_affinity_info);
> - if ( psci_mode_check(current->domain, fid) )
> - {
> - register_t taff = PSCI_ARG(regs,1);
> - uint32_t laff = PSCI_ARG32(regs,2);
> - PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
> - }
> - break;
> - case PSCI_0_2_FN_MIGRATE:
> - case PSCI_0_2_FN64_MIGRATE:
> - perfc_incr(vpsci_cpu_migrate);
> - if ( psci_mode_check(current->domain, fid) )
> - {
> - uint32_t tcpu = PSCI_ARG32(regs,1);
> - PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
> - }
> - break;
> - default:
> - domain_crash_synchronous();
> - return;
> - }
> -}
> -
> #ifdef CONFIG_ARM_64
> #define HYPERCALL_RESULT_REG(r) (r)->x0
> #define HYPERCALL_ARG1(r) (r)->x0
> @@ -2251,7 +2138,7 @@ asmlinkage void do_trap_guest_sync(struct cpu_user_regs *regs)
> return do_debug_trap(regs, hsr.iss & 0x00ff);
> #endif
> if ( hsr.iss == 0 )
> - return do_trap_psci(regs);
> + return do_trap_hvc_smccc(regs);
> do_trap_hypercall(regs, (register_t *)®s->r12, hsr.iss);
> break;
> #ifdef CONFIG_ARM_64
> @@ -2263,7 +2150,7 @@ asmlinkage void do_trap_guest_sync(struct cpu_user_regs *regs)
> return do_debug_trap(regs, hsr.iss & 0x00ff);
> #endif
> if ( hsr.iss == 0 )
> - return do_trap_psci(regs);
> + return do_trap_hvc_smccc(regs);
> do_trap_hypercall(regs, ®s->x16, hsr.iss);
> break;
> case HSR_EC_SMC64:
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index 97a6be3..d3120a5 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -19,6 +19,7 @@
> #include <xen/types.h>
> #include <public/arch-arm/smccc.h>
> #include <asm/monitor.h>
> +#include <asm/psci.h>
> #include <asm/regs.h>
> #include <asm/smccc.h>
> #include <asm/traps.h>
> @@ -26,6 +27,9 @@
> /* Number of functions currently supported by Hypervisor Service. */
> #define XEN_SMCCC_FUNCTION_COUNT 3
>
> +/* Number of functions currently supported by Standard Service Service Calls. */
> +#define SSSC_SMCCC_FUNCTION_COUNT 13
> +
> static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t *u)
> {
> int n;
> @@ -71,6 +75,154 @@ 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)
> +{
> + switch ( PSCI_ARG32(regs, 0) )
It is a bit odd to use PSCI_ARG32 here for a function called
"handle_existing_apis". What are you trying to achieve and why the
32-bit is necessary here and not in other places (such as handle_sssc)?
> + {
> + 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;
> + }
> +}
> +
> +/* helper function for checking arm mode 32/64 bit */
> +static inline int psci_mode_check(struct domain *d, register_t fid)
> +{
> + return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
> +}
> +
> +/* PSCI 0.2 interface and other Standard Secure Calls */
> +static bool handle_sssc(struct cpu_user_regs *regs)
> +{
> + register_t fid = PSCI_ARG(regs, 0);
> +
> + switch ( smccc_get_fn(fid) )
> + {
> + case smccc_get_fn(PSCI_0_2_FN_PSCI_VERSION):
> + perfc_incr(vpsci_version);
> + PSCI_SET_RESULT(regs, do_psci_0_2_version());
> + return true;
> +
> + case smccc_get_fn(PSCI_0_2_FN_CPU_OFF):
> + perfc_incr(vpsci_cpu_off);
> + PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
> + return true;
> +
> + case smccc_get_fn(PSCI_0_2_FN_MIGRATE_INFO_TYPE):
> + perfc_incr(vpsci_migrate_info_type);
> + PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type());
> + return true;
> +
> + case smccc_get_fn(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
> + perfc_incr(vpsci_migrate_info_up_cpu);
> + if ( psci_mode_check(current->domain, fid) )
> + PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
> + return true;
> +
> + case smccc_get_fn(PSCI_0_2_FN_SYSTEM_OFF):
> + perfc_incr(vpsci_system_off);
> + do_psci_0_2_system_off();
> + PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
> + return true;
> +
> + case smccc_get_fn(PSCI_0_2_FN_SYSTEM_RESET):
> + perfc_incr(vpsci_system_reset);
> + do_psci_0_2_system_reset();
> + PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
> + return true;
> +
> + case smccc_get_fn(PSCI_0_2_FN_CPU_ON):
> + perfc_incr(vpsci_cpu_on);
> + if ( psci_mode_check(current->domain, fid) )
> + {
> + register_t vcpuid = PSCI_ARG(regs, 1);
> + register_t epoint = PSCI_ARG(regs, 2);
> + register_t cid = PSCI_ARG(regs, 3);
> +
> + PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
> + }
> + return true;
> +
> + case smccc_get_fn(PSCI_0_2_FN_CPU_SUSPEND):
> + perfc_incr(vpsci_cpu_suspend);
> + if ( psci_mode_check(current->domain, fid) )
> + {
> + uint32_t pstate = PSCI_ARG32(regs, 1);
> + register_t epoint = PSCI_ARG(regs, 2);
> + register_t cid = PSCI_ARG(regs, 3);
> +
> + PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid));
> + }
> + return true;
> +
> + case smccc_get_fn(PSCI_0_2_FN_AFFINITY_INFO):
> + perfc_incr(vpsci_cpu_affinity_info);
> + if ( psci_mode_check(current->domain, fid) )
> + {
> + register_t taff = PSCI_ARG(regs, 1);
> + uint32_t laff = PSCI_ARG32(regs, 2);
> + PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
> + }
> + return true;
> +
> + case smccc_get_fn(PSCI_0_2_FN_MIGRATE):
> + perfc_incr(vpsci_cpu_migrate);
> + if ( psci_mode_check(current->domain, fid) )
> + {
> + uint32_t tcpu = PSCI_ARG32(regs, 1);
> +
> + PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
> + }
> + return true;
> +
> + case ARM_SMCCC_FUNC_CALL_COUNT:
> + set_user_reg(regs, 0, SSSC_SMCCC_FUNCTION_COUNT);
> + return true;
> +
> + case ARM_SMCCC_FUNC_CALL_UID:
> + {
> + static const xen_uuid_t psci_uuid = SSSC_SMCCC_UID;
> +
> + fill_uuid(regs, &psci_uuid);
> + return true;
See my comment on patch #6.
> + }
> +
> + case ARM_SMCCC_FUNC_CALL_REVISION:
> + set_user_reg(regs, 0, SSSC_SMCCC_MAJOR_REVISION);
> + set_user_reg(regs, 1, SSSC_SMCCC_MINOR_REVISION);
> + return true;
As suggest on v4, you could introduce a helper to do that for you and
return true. This would make the implementation of
ARM_SMCCC_FUNC_CALL_REVISION easier to read.
> +
> + default:
> + return false;
> + }
> +}
> +
> /*
> * vsmccc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
> * returns true if that was valid SMCCC call (even if function number
> @@ -110,11 +262,26 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
> return true;
> }
>
> - switch ( smccc_get_owner(funcid) )
> + /*
> + * Special case: identifier range for existing APIs.
> + * This range is described in SMCCC (ARM DEN 0028B, page 16),
> + * but it does not conforms to standard function identifier
> + * encoding.
> + */
> + if ( funcid >= ARM_SMCCC_RESERVED_RANGE_START &&
> + funcid <= ARM_SMCCC_RESERVED_RANGE_END )
> + handled = handle_existing_apis(regs);
> + else
> {
> - case ARM_SMCCC_OWNER_HYPERVISOR:
> - handled = handle_hypervisor(regs);
> - break;
> + switch ( smccc_get_owner(funcid) )
> + {
> + case ARM_SMCCC_OWNER_HYPERVISOR:
> + handled = handle_hypervisor(regs);
> + break;
> + case ARM_SMCCC_OWNER_STANDARD:
> + handled = handle_sssc(regs);
> + break;
> + }
> }
>
> if ( !handled )
> @@ -158,6 +325,20 @@ void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
> inject_undef_exception(regs, hsr);
> }
>
> +void do_trap_hvc_smccc(struct cpu_user_regs *regs)
> +{
> + const union hsr hsr = { .bits = regs->hsr };
> +
> + /*
> + * vsmccc_handle_call() will return false if this call is not
> + * SMCCC compatible (e.g. immediate value != 0). As it is not
> + * compatible, we can't be sure that guest will understand
> + * ARM_SMCCC_ERR_UNKNOWN_FUNCTION.
> + */
> + if ( !vsmccc_handle_call(regs) )
> + inject_undef_exception(regs, hsr);
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/include/asm-arm/traps.h b/xen/include/asm-arm/traps.h
> index 6efd1c5..0b91aa7 100644
> --- a/xen/include/asm-arm/traps.h
> +++ b/xen/include/asm-arm/traps.h
> @@ -33,6 +33,7 @@ void do_cp(struct cpu_user_regs *regs, const union hsr hsr);
>
> /* SMCCC handling */
> void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr);
> +void do_trap_hvc_smccc(struct cpu_user_regs *regs);
>
> #endif /* __ASM_ARM_TRAPS__ */
> /*
> diff --git a/xen/include/public/arch-arm/smccc.h b/xen/include/public/arch-arm/smccc.h
> index a1d00ae..caead6e 100644
> --- a/xen/include/public/arch-arm/smccc.h
> +++ b/xen/include/public/arch-arm/smccc.h
> @@ -46,6 +46,14 @@
> #define XEN_SMCCC_UID XEN_DEFINE_UUID(0xa71812dc, 0xc698, 0x4369, 0x9acf, \
> 0x79, 0xd1, 0x8d, 0xde, 0xe6, 0x67)
>
> +/* Standard Service Service Call version. */
> +#define SSSC_SMCCC_MAJOR_REVISION 0
> +#define SSSC_SMCCC_MINOR_REVISION 1
> +
> +/* Standard Service Call UID. Randomly generated with uuidgen. */
> +#define SSSC_SMCCC_UID XEN_DEFINE_UUID(0xf863386f, 0x4b39, 0x4cbd, 0x9220,\
> + 0xce, 0x16, 0x41, 0xe5, 0x9f, 0x6f)
> +
> #endif /* __XEN_PUBLIC_ARCH_ARM_SMCCC_H__ */
>
> /*
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
next prev parent reply other threads:[~2017-09-13 11:53 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-31 20:09 [PATCH v5 00/10] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
2017-08-31 20:09 ` [PATCH v5 01/10] arm: traps: use generic register accessors in the PSCI code Volodymyr Babchuk
2017-09-13 9:59 ` Julien Grall
2017-08-31 20:09 ` [PATCH v5 02/10] arm: traps: check if SMC was conditional before handling it Volodymyr Babchuk
2017-08-31 20:09 ` [PATCH v5 03/10] public: xen.h: add definitions for UUID handling Volodymyr Babchuk
2017-09-01 9:42 ` Ian Jackson
2017-08-31 20:09 ` [PATCH v5 04/10] arm: processor.h: add definition for immediate value mask Volodymyr Babchuk
2017-09-13 10:02 ` Julien Grall
2017-08-31 20:09 ` [PATCH v5 05/10] arm: add SMCCC protocol definitions Volodymyr Babchuk
2017-09-13 10:07 ` Julien Grall
2017-08-31 20:09 ` [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC Volodymyr Babchuk
2017-09-13 10:17 ` Julien Grall
2017-09-13 11:11 ` Julien Grall
2017-09-19 21:44 ` Volodymyr Babchuk
2017-09-20 17:21 ` Julien Grall
2017-09-20 18:11 ` Volodymyr Babchuk
2017-09-20 20:02 ` Julien Grall
2017-09-20 20:26 ` Volodymyr Babchuk
2017-09-21 14:48 ` Julien Grall
2017-08-31 20:09 ` [PATCH v5 07/10] arm: traps: handle PSCI calls inside `vsmc.c` Volodymyr Babchuk
2017-09-13 11:53 ` Julien Grall [this message]
2017-08-31 20:09 ` [PATCH v5 08/10] arm: PSCI: use definitions provided by asm/smccc.h Volodymyr Babchuk
2017-09-13 11:58 ` Julien Grall
2017-09-21 18:28 ` Volodymyr Babchuk
2017-08-31 20:09 ` [PATCH v5 09/10] arm: vsmc: remove 64 bit mode check in PSCI handler Volodymyr Babchuk
2017-08-31 20:09 ` [PATCH v5 10/10] public: add and enable XENFEAT_ARM_SMCCC_supported feature Volodymyr Babchuk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=16e3f6c0-0167-890b-cbd5-531807152bbc@arm.com \
--to=julien.grall@arm.com \
--cc=George.Dunlap@eu.citrix.com \
--cc=andrew.cooper3@citrix.com \
--cc=edgar.iglesias@xilinx.com \
--cc=ian.jackson@eu.citrix.com \
--cc=jbeulich@suse.com \
--cc=sstabellini@kernel.org \
--cc=tim@xen.org \
--cc=volodymyr_babchuk@epam.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).