|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v8] xen/arm : emulation of arm's PSCI v0.2 standard
Hi,
On 31 July 2014 15:44, Christoffer Dall <christoffer.dall@xxxxxxxxxx> wrote:
> On Thu, Jul 31, 2014 at 12:30:23PM +0530, Parth Dixit wrote:
>> Arm based virtual machines dom0/guest will request power related
>> functionality
>> from xen through PSCI interface. This patch implements version 0.2 of
>> PSCI standard specified by arm for 64bit and 32 bit arm machines.
>>
>> - removed arm_psci_fn_t
>> - implemented psci_cpu_on with additional error conditions
>> - added switch-case in do_trap_psci function
>> - added PSCI v0.2 macros in psci.h
>> - removed tables for PSCI v0.1 and v0.2
>> - implemented affinity_info
>> - moved do_common_cpu up in vpsci.c removed declaration
>> - removed PSCI_ARGS macro
>> - added new macro for 32 bit arguments
>> - added new function psci_mode_check
>> - modified cpu_suspend to return context_id
>> - added macros for checking powe_state
>>
>> Signed-off-by: Parth Dixit <parth.dixit@xxxxxxxxxx>
>> ---
>> Changelog v8
>> - fixed error in cpu_suspend
>> Changelog v7
>> - removed casting from functions in do_psci_trap
>> - modified cpu_suspend to return success in case the call returns
>>
>> Changelog v6
>> - fixed indentation
>> - removed casting from error codes
>> - modified cpu_suspend to check power_state
>> - ignoring context_id in case of standby
>> - added comments for ignoring affinity
>> - added macro for checking power_state
>>
>> Changelog v5
>> - removed PSCI_ARGS macro
>> - preload error value for calls that do not return
>> - added new macro for 32 bit arguments
>> - casting return calls to appropriate types
>> - added new function psci_mode_check
>> - added error checks for PSCI v0.2 function id's
>>
>> Changelog v4
>> - added target_affinity_mask[] to local variable
>> - removed AFF3 masking for 64 bit cpu
>> - added do_common_cpu_on for PSCI v0.1 and v0.2
>> - moved cpu_on decision based on entry point
>> - removed vpsci_ver introduced in v3
>> - removed psci tables
>> - added new macro for selecting arguments
>> - do_trap_psci is now switch case based instead of tables
>>
>> Changelog v3
>> - moved wfi helper to seperate patch
>> - replaced new wfi function in traps.c
>> - removed defining power_state variable using value directly
>> - added new line between declaration
>> - merged PSCI v0.1 and v0.2 cpu_on function to avoid duplication
>> - removed spurious change
>> - renamed PSCI return values to reflect v0.2 moved them to top
>> - removed PSCI v0.1 version declaration for now, will introduce it when
>> needed
>> - removed hard tabs in the code lifted from linux
>> - removed PSCI_0_1_MAX
>> - did not retained copyright header from linux as most of functions are
>> removed
>> - added two function tables for PSCI v0.1 and PSCI v0.2
>> - added compatibility string to libxl_arm to expose new functionality
>> - refactored affinity_info code
>> - added table for masking function
>> - removed definitions of unused PSCI v0.2 functions
>> - removed function id's of unused PSCI v0.2 functions
>> - renamed constant PSCI_0_2_TOS_MP ( multicore aware) as per spec
>>
>> tools/libxl/libxl_arm.c | 2 +-
>> xen/arch/arm/domain_build.c | 5 +-
>> xen/arch/arm/traps.c | 123 +++++++++++++++++++++++++----------
>> xen/arch/arm/vpsci.c | 140
>> ++++++++++++++++++++++++++++++++++++++--
>> xen/include/asm-arm/processor.h | 3 +
>> xen/include/asm-arm/psci.h | 83 +++++++++++++++++++++---
>> 6 files changed, 308 insertions(+), 48 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>> index 4f0f0e2..e8bcd05 100644
>> --- a/tools/libxl/libxl_arm.c
>> +++ b/tools/libxl/libxl_arm.c
>> @@ -237,7 +237,7 @@ static int make_psci_node(libxl__gc *gc, void *fdt)
>> res = fdt_begin_node(fdt, "psci");
>> if (res) return res;
>>
>> - res = fdt_property_compat(gc, fdt, 1, "arm,psci");
>> + res = fdt_property_compat(gc, fdt, 2, "arm,psci-0.2","arm,psci");
>> if (res) return res;
>>
>> res = fdt_property_string(fdt, "method", "hvc");
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index c424793..ebd4170 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -388,6 +388,9 @@ static int make_hypervisor_node(struct domain *d,
>> static int make_psci_node(void *fdt, const struct dt_device_node *parent)
>> {
>> int res;
>> + const char compat[] =
>> + "arm,psci-0.2""\0"
>> + "arm,psci";
>>
>> DPRINT("Create PSCI node\n");
>>
>> @@ -396,7 +399,7 @@ static int make_psci_node(void *fdt, const struct
>> dt_device_node *parent)
>> if ( res )
>> return res;
>>
>> - res = fdt_property_string(fdt, "compatible", "arm,psci");
>> + res = fdt_property(fdt, "compatible", compat, sizeof(compat));
>> if ( res )
>> return res;
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 8102540..844da9d 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1030,24 +1030,6 @@ static arm_hypercall_t arm_hypercall_table[] = {
>> HYPERCALL_ARM(vcpu_op, 3),
>> };
>>
>> -typedef int (*arm_psci_fn_t)(uint32_t, register_t);
>> -
>> -typedef struct {
>> - arm_psci_fn_t fn;
>> - int nr_args;
>> -} arm_psci_t;
>> -
>> -#define PSCI(_name, _nr_args) \
>> - [ PSCI_ ## _name ] = { \
>> - .fn = (arm_psci_fn_t) &do_psci_ ## _name, \
>> - .nr_args = _nr_args, \
>> - }
>> -
>> -static arm_psci_t arm_psci_table[] = {
>> - PSCI(cpu_off, 1),
>> - PSCI(cpu_on, 2),
>> -};
>> -
>> #ifndef NDEBUG
>> static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>> {
>> @@ -1080,33 +1062,108 @@ static void do_debug_trap(struct cpu_user_regs
>> *regs, unsigned int code)
>> #endif
>>
>> #ifdef CONFIG_ARM_64
>> -#define PSCI_OP_REG(r) (r)->x0
>> #define PSCI_RESULT_REG(r) (r)->x0
>> -#define PSCI_ARGS(r) (r)->x1, (r)->x2
>> +#define PSCI_ARG(r,n) (r)->x##n
>> +#define PSCI_ARG32(r,n) (uint32_t)( (r)->x##n & 0x00000000FFFFFFFF )
>> #else
>> -#define PSCI_OP_REG(r) (r)->r0
>> #define PSCI_RESULT_REG(r) (r)->r0
>> -#define PSCI_ARGS(r) (r)->r1, (r)->r2
>> +#define PSCI_ARG(r,n) (r)->r##n
>> +#define PSCI_ARG32(r,n) PSCI_ARG(r,n)
>> #endif
>>
>> -static void do_trap_psci(struct cpu_user_regs *regs)
>> +/* helper function for checking arm mode 32/64 bit */
>> +static inline int psci_mode_check(struct domain *d, register_t fid)
>> {
>> - arm_psci_fn_t psci_call = NULL;
>> + return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
>> +}
>>
>> - if ( PSCI_OP_REG(regs) >= ARRAY_SIZE(arm_psci_table) )
>> - {
>> - domain_crash_synchronous();
>> - return;
>> - }
>> +static void do_trap_psci(struct cpu_user_regs *regs)
>> +{
>> + register_t fid = PSCI_ARG(regs,0);
>>
>> - psci_call = arm_psci_table[PSCI_OP_REG(regs)].fn;
>> - if ( psci_call == NULL )
>> + /* preloading in case psci_mode_check fails */
>> + PSCI_RESULT_REG(regs) = PSCI_INVALID_PARAMETERS;
>> + switch( fid )
>> {
>> + case PSCI_cpu_off:
>> + {
>> + uint32_t pstate = PSCI_ARG32(regs,1);
>> + PSCI_RESULT_REG(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);
>> + PSCI_RESULT_REG(regs) = do_psci_cpu_on(vcpuid, epoint);
>> + }
>> + break;
>> + case PSCI_0_2_FN_PSCI_VERSION:
>> + PSCI_RESULT_REG(regs) = do_psci_0_2_version();
>> + break;
>> + case PSCI_0_2_FN_CPU_OFF:
>> + PSCI_RESULT_REG(regs) = do_psci_0_2_cpu_off();
>> + break;
>> + case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
>> + PSCI_RESULT_REG(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:
>> + if ( psci_mode_check(current->domain, fid) )
>> + PSCI_RESULT_REG(regs) = do_psci_0_2_migrate_info_up_cpu();
>> + break;
>> + case PSCI_0_2_FN_SYSTEM_OFF:
>> + do_psci_0_2_system_off();
>> + PSCI_RESULT_REG(regs) = PSCI_INTERNAL_FAILURE;
>> + break;
>> + case PSCI_0_2_FN_SYSTEM_RESET:
>> + do_psci_0_2_system_reset();
>> + PSCI_RESULT_REG(regs) = PSCI_INTERNAL_FAILURE;
>> + break;
>> + case PSCI_0_2_FN_CPU_ON:
>> + case PSCI_0_2_FN64_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_RESULT_REG(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:
>> + 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_RESULT_REG(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:
>> + if ( psci_mode_check(current->domain, fid) )
>> + {
>> + register_t taff = PSCI_ARG(regs,1);
>> + uint32_t laff = PSCI_ARG32(regs,2);
>> + PSCI_RESULT_REG(regs) =
>> + do_psci_0_2_affinity_info(taff, laff);
>> + }
>> + break;
>> + case PSCI_0_2_FN_MIGRATE:
>> + case PSCI_0_2_FN64_MIGRATE:
>> + if ( psci_mode_check(current->domain, fid) )
>> + {
>> + uint32_t tcpu = PSCI_ARG32(regs,1);
>> + PSCI_RESULT_REG(regs) = do_psci_0_2_migrate(tcpu);
>> + }
>> + break;
>> + default:
>> domain_crash_synchronous();
>> return;
>> }
>> -
>> - PSCI_RESULT_REG(regs) = psci_call(PSCI_ARGS(regs));
>> }
>>
>> #ifdef CONFIG_ARM_64
>> diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
>> index 1ceb8cb..c6c1bdc 100644
>> --- a/xen/arch/arm/vpsci.c
>> +++ b/xen/arch/arm/vpsci.c
>> @@ -17,24 +17,37 @@
>> #include <asm/current.h>
>> #include <asm/gic.h>
>> #include <asm/psci.h>
>> +#include <public/sched.h>
>> +#include <asm-arm/event.h>
>>
>> -int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point)
>> +static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
>> + register_t context_id,int ver)
>> {
>> struct vcpu *v;
>> struct domain *d = current->domain;
>> struct vcpu_guest_context *ctxt;
>> int rc;
>> int is_thumb = entry_point & 1;
>> + register_t vcpuid;
>> +
>> + if( ver == XEN_PSCI_V_0_2 )
>> + vcpuid = (target_cpu & MPIDR_HWID_MASK);
>> + else
>> + vcpuid = target_cpu;
>>
>> if ( (vcpuid < 0) || (vcpuid >= MAX_VIRT_CPUS) )
>> - return PSCI_EINVAL;
>> + return PSCI_INVALID_PARAMETERS;
>>
>> if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
>> - return PSCI_EINVAL;
>> + return PSCI_INVALID_PARAMETERS;
>>
>> /* THUMB set is not allowed with 64-bit domain */
>> if ( is_64bit_domain(d) && is_thumb )
>> - return PSCI_EINVAL;
>> + return PSCI_INVALID_PARAMETERS;
>> +
>> + if( ( ver == XEN_PSCI_V_0_2 ) &&
>> + ( !test_bit(_VPF_down, &v->pause_flags) ) )
>> + return PSCI_ALREADY_ON;
>>
>> if ( (ctxt = alloc_vcpu_guest_context()) == NULL )
>> return PSCI_DENIED;
>> @@ -48,10 +61,18 @@ int do_psci_cpu_on(uint32_t vcpuid, register_t
>> entry_point)
>> ctxt->ttbr1 = 0;
>> ctxt->ttbcr = 0; /* Defined Reset Value */
>> if ( is_32bit_domain(d) )
>> + {
>> ctxt->user_regs.cpsr = PSR_GUEST32_INIT;
>> + if( ver == XEN_PSCI_V_0_2 )
>> + ctxt->user_regs.r0_usr = context_id;
>> + }
>> #ifdef CONFIG_ARM_64
>> else
>> + {
>> ctxt->user_regs.cpsr = PSR_GUEST64_INIT;
>> + if( ver == XEN_PSCI_V_0_2 )
>> + ctxt->user_regs.x0 = context_id;
>> + }
>> #endif
>>
>> /* Start the VCPU with THUMB set if it's requested by the kernel */
>> @@ -75,7 +96,12 @@ int do_psci_cpu_on(uint32_t vcpuid, register_t
>> entry_point)
>> return PSCI_SUCCESS;
>> }
>>
>> -int do_psci_cpu_off(uint32_t power_state)
>> +int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point)
>> +{
>> + return do_common_cpu_on(vcpuid,entry_point,0,XEN_PSCI_V_0_1);
>> +}
>> +
>> +int32_t do_psci_cpu_off(uint32_t power_state)
>> {
>> struct vcpu *v = current;
>> if ( !test_and_set_bit(_VPF_down, &v->pause_flags) )
>> @@ -83,6 +109,110 @@ int do_psci_cpu_off(uint32_t power_state)
>> return PSCI_SUCCESS;
>> }
>>
>> +uint32_t do_psci_0_2_version(void)
>> +{
>> + return XEN_PSCI_V_0_2;
>> +}
>> +
>> +register_t do_psci_0_2_cpu_suspend(uint32_t power_state, register_t
>> entry_point,
>> + register_t context_id)
>> +{
>> + struct vcpu *v = current;
>> + struct cpu_user_regs *regs = guest_cpu_user_regs();
>> +
>
> Not sure you want to keep this comment around (as I would comment on the
> fact that we always treat power off requests as only performing standby
> as it simplifies the Xen implementation (as Ian points out in his
> comments), but if you do want to keep the comment block:
>
>> + /* affinity values are ignored in this implementation as
>
> Not sure of the Xen common practice here, but why not start this with
> upper case?
>
>> + * at present xen does not supports affinity level greater
>
> does not support affinity levels greater
>
>> + * than 0, for all affinity values passed we power down/ standby
>
> ^ extra white space missing space ^
>
>> + * the current core */
>
> only the current core. (?)
> [...]
>
> -Christoffer
Sure, I will change the comment to make it clear that we are
performing standby and delete the existing comment.
Thanks
parth
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |