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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.