| Message ID | 1403760382-1863-1-git-send-email-parth.dixit@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Parth, Thank you for the patch. On 06/26/2014 06:26 AM, Parth Dixit wrote: > +void vcpu_block_event(struct vcpu *v) > +{ > + vcpu_block(); > + /* The ARM spec declares that even if local irqs are masked in > + * the CPSR register, an irq should wake up a cpu from WFI anyway. > + * For this reason we need to check for irqs that need delivery, > + * ignoring the CPSR register, *after* calling SCHEDOP_block to > + * avoid races with vgic_vcpu_inject_irq. > + */ The comment is out of date here and you should move it on top of the function. Also, you've introduce this function but didn't replace the code in WFI trap. Furthermore, I would move this change in a separate patch. It will be more clear. > + if ( local_events_need_delivery_nomask() ) > + vcpu_unblock(current); > +} > + [..] > +int do_psci_0_2_cpu_off(void) > +{ > + uint32_t power_state = 0 ; Coding style require a newline between the declarations. Also, defining power_state looks pointless here. > + return do_psci_cpu_off(power_state); > +} > + > +int do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point, > + register_t context_id) This function is 95% the same code. I would merge as much as possible with do_psci_cpu_on to avoid code duplication. [..] > + > +int do_psci_0_2_migrate(uint32_t target_cpu) > +{ > + return PSCI_ENOSYS; > +} > + > + Only one newline. > +int do_psci_0_2_migrate_info_type(void) > +{ > + return PSCI_0_2_TOS_MP; > +} > + > + Same here. > +register_t do_psci_0_2_migrate_info_up_cpu(void) > +{ > + return PSCI_ENOSYS; > +} > + > + Same here. > +void do_psci_0_2_system_off( void ) > +{ > + struct domain *d = current->domain; > + domain_shutdown(d,SHUTDOWN_poweroff); > +} > + > + Same here. > +void do_psci_0_2_system_reset(void) > +{ > + struct domain *d = current->domain; > + domain_shutdown(d,SHUTDOWN_reboot); > +} > + [..] > static inline int vcpu_event_delivery_is_enabled(struct vcpu *v) > { > @@ -55,6 +56,7 @@ static inline int arch_virq_is_global(int virq) > return 1; > } > > + Spurious change. > diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h > index 189964b..3487380 100644 > --- a/xen/include/asm-arm/psci.h > +++ b/xen/include/asm-arm/psci.h > @@ -1,11 +1,6 @@ > #ifndef __ASM_PSCI_H__ > #define __ASM_PSCI_H__ > > -#define PSCI_SUCCESS 0 > -#define PSCI_ENOSYS -1 > -#define PSCI_EINVAL -2 > -#define PSCI_DENIED -3 > - Why did you just moved them at the end of the file and change the indentation? > +/* PSCI version */ > +#define XEN_PSCI_V_0_1 1 This doesn't seem to be used, right? > + > +/* PSCI v0.2 interface */ > +#define PSCI_0_2_FN_BASE 0x84000000 > +#define PSCI_0_2_FN(n) (PSCI_0_2_FN_BASE + (n)) > +#define PSCI_0_2_64BIT 0x40000000 In general we don't use hard-tab in Xen coding style. Can you remove them? [..] > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > index 7496556..93803e4 100644 > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -385,6 +385,7 @@ typedef uint64_t xen_callback_t; > #define PSCI_cpu_off 1 > #define PSCI_cpu_on 2 > #define PSCI_migrate 3 > +#define PSCI_0_1_MAX 4 Why do you expose PSCI_0_1_MAX?
Hi Julien, Comments are inline, i will try to send a new patch today/weekend after testing. On 26 June 2014 21:31, Julien Grall <julien.grall@linaro.org> wrote: > Hi Parth, > > Thank you for the patch. > > On 06/26/2014 06:26 AM, Parth Dixit wrote: > > +void vcpu_block_event(struct vcpu *v) > > +{ > > + vcpu_block(); > > + /* The ARM spec declares that even if local irqs are masked in > > + * the CPSR register, an irq should wake up a cpu from WFI anyway. > > + * For this reason we need to check for irqs that need delivery, > > + * ignoring the CPSR register, *after* calling SCHEDOP_block to > > + * avoid races with vgic_vcpu_inject_irq. > > + */ > > The comment is out of date here and you should move it on top of the > function. > > Also, you've introduce this function but didn't replace the code in WFI > trap. > > Furthermore, I would move this change in a separate patch. It will be > more clear. > I will create new patch for this and replace WFI with this function -(AI Parth) > > > + if ( local_events_need_delivery_nomask() ) > > + vcpu_unblock(current); > > +} > > + > > [..] > > > +int do_psci_0_2_cpu_off(void) > > +{ > > + uint32_t power_state = 0 ; > > Coding style require a newline between the declarations. > > Also, defining power_state looks pointless here. Agreed will be taken care of in next patchset - (AI Parth) > > + return do_psci_cpu_off(power_state); > > +} > > + > > +int do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point, > > + register_t context_id) > > > This function is 95% the same code. I would merge as much as possible > with do_psci_cpu_on to avoid code duplication. > > Ok, i will try to merge the functions together (AI Parth) > [..] > > > + > > +int do_psci_0_2_migrate(uint32_t target_cpu) > > +{ > > + return PSCI_ENOSYS; > > +} > > + > > + > > Only one newline. > > > +int do_psci_0_2_migrate_info_type(void) > > +{ > > + return PSCI_0_2_TOS_MP; > > +} > > + > > + > > Same here. > > > +register_t do_psci_0_2_migrate_info_up_cpu(void) > > +{ > > + return PSCI_ENOSYS; > > +} > > + > > + > > Same here. > > > +void do_psci_0_2_system_off( void ) > > +{ > > + struct domain *d = current->domain; > > + domain_shutdown(d,SHUTDOWN_poweroff); > > +} > > + > > + > > Same here. > > Will take care in next patch set (AI Parth) > > +void do_psci_0_2_system_reset(void) > > +{ > > + struct domain *d = current->domain; > > + domain_shutdown(d,SHUTDOWN_reboot); > > +} > > + > > [..] > > > static inline int vcpu_event_delivery_is_enabled(struct vcpu *v) > > { > > @@ -55,6 +56,7 @@ static inline int arch_virq_is_global(int virq) > > return 1; > > } > > > > + > > Spurious change. > > Will take care in next patch set (AI Parth) > > diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h > > index 189964b..3487380 100644 > > --- a/xen/include/asm-arm/psci.h > > +++ b/xen/include/asm-arm/psci.h > > @@ -1,11 +1,6 @@ > > #ifndef __ASM_PSCI_H__ > > #define __ASM_PSCI_H__ > > > > -#define PSCI_SUCCESS 0 > > -#define PSCI_ENOSYS -1 > > -#define PSCI_EINVAL -2 > > -#define PSCI_DENIED -3 > > - > > Why did you just moved them at the end of the file and change the > indentation? > > This was done to have common return functions between PSCI v0.1 and v 0.2 and to define them at one place > > +/* PSCI version */ > > +#define XEN_PSCI_V_0_1 1 > > This doesn't seem to be used, right? > > Yes it is defined for consistency and also for future use in case we return v0.1, i'd prefer to keep it this way what do you think? > > + > > +/* PSCI v0.2 interface */ > > +#define PSCI_0_2_FN_BASE 0x84000000 > > +#define PSCI_0_2_FN(n) (PSCI_0_2_FN_BASE > + (n)) > > +#define PSCI_0_2_64BIT 0x40000000 > > In general we don't use hard-tab in Xen coding style. Can you remove them? > > > Will take care in next patchset (AI Parth) > [..] > > > diff --git a/xen/include/public/arch-arm.h > b/xen/include/public/arch-arm.h > > index 7496556..93803e4 100644 > > --- a/xen/include/public/arch-arm.h > > +++ b/xen/include/public/arch-arm.h > > @@ -385,6 +385,7 @@ typedef uint64_t xen_callback_t; > > #define PSCI_cpu_off 1 > > #define PSCI_cpu_on 2 > > #define PSCI_migrate 3 > > +#define PSCI_0_1_MAX 4 > > Why do you expose PSCI_0_1_MAX You are right i think i can get away by treating PSCI_migrate as max value (AI Parth) > > -- > Julien Grall > Thanks parth
Hi Parth, On 27/06/14 06:18, Parth Dixit wrote: > On 26 June 2014 21:31, Julien Grall <julien.grall@linaro.org > <mailto:julien.grall@linaro.org>> wrote: > > + return do_psci_cpu_off(power_state); > > +} > > + > > +int do_psci_0_2_cpu_on(register_t target_cpu, register_t > entry_point, > > + register_t context_id) > > > This function is 95% the same code. I would merge as much as possible > with do_psci_cpu_on to avoid code duplication. > > Ok, i will try to merge the functions together (AI Parth) FWIW, KVM is using the same PSCI on function on V0.1 and V0.2. I'm wondering if we can assume the same thing here. Of course, you will have to add the few improvement in the function (checking if the VCPU is already online...). > > diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h > > index 189964b..3487380 100644 > > --- a/xen/include/asm-arm/psci.h > > +++ b/xen/include/asm-arm/psci.h > > @@ -1,11 +1,6 @@ > > #ifndef __ASM_PSCI_H__ > > #define __ASM_PSCI_H__ > > > > -#define PSCI_SUCCESS 0 > > -#define PSCI_ENOSYS -1 > > -#define PSCI_EINVAL -2 > > -#define PSCI_DENIED -3 > > - > > Why did you just moved them at the end of the file and change the > indentation? > > This was done to have common return functions between PSCI v0.1 and v > 0.2 and to define them at one place Ok... I still don't understand why you can't add the new return value here. BTW, IIRC there was a copyright int the PSCI header from Linux. I think you have to retain it here. > > +/* PSCI version */ > > +#define XEN_PSCI_V_0_1 1 > > This doesn't seem to be used, right? > > Yes it is defined for consistency and also for future use in case we > return v0.1, i'd prefer to keep it this way what do you think? Ok > [..] > > > diff --git a/xen/include/public/arch-arm.h > b/xen/include/public/arch-arm.h > > index 7496556..93803e4 100644 > > --- a/xen/include/public/arch-arm.h > > +++ b/xen/include/public/arch-arm.h > > @@ -385,6 +385,7 @@ typedef uint64_t xen_callback_t; > > #define PSCI_cpu_off 1 > > #define PSCI_cpu_on 2 > > #define PSCI_migrate 3 > > +#define PSCI_0_1_MAX 4 > > Why do you expose PSCI_0_1_MAX > > You are right i think i can get away by treating PSCI_migrate as max > value (AI Parth) Can't you define 2 table in traps.c: one for PSCI v0.1 and one for PSCI v0.2? This would avoid stupid issue later when we will implement a new function for 0.1. Regards,
Hi Julien On 27 June 2014 16:24, Julien Grall <julien.grall@linaro.org> wrote: > > Hi Parth, > > > On 27/06/14 06:18, Parth Dixit wrote: > >> On 26 June 2014 21:31, Julien Grall <julien.grall@linaro.org >> <mailto:julien.grall@linaro.org>> wrote: >> > + return do_psci_cpu_off(power_state); >> > +} >> > + >> > +int do_psci_0_2_cpu_on(register_t target_cpu, register_t >> entry_point, >> > + register_t context_id) >> >> >> This function is 95% the same code. I would merge as much as possible >> with do_psci_cpu_on to avoid code duplication. >> >> Ok, i will try to merge the functions together (AI Parth) >> > > FWIW, KVM is using the same PSCI on function on V0.1 and V0.2. I'm > wondering if we can assume the same thing here. Of course, you will have to > add the few improvement in the function (checking if the VCPU is already > online...). Implementation of kvm is slightly different, they are setting PSCI version from QEMU through ioctl and then using it to differentiate between the versions at runtime. Are you proposing the same? i.e. setting PSCI version externally using xen tools and then using it throughout ? .(right now i am using function id's to differentiate between the two, i can extend that by introducing some variable in vcpu struct for runtime detection) > > > > diff --git a/xen/include/asm-arm/psci.h >> b/xen/include/asm-arm/psci.h >> > index 189964b..3487380 100644 >> > --- a/xen/include/asm-arm/psci.h >> > +++ b/xen/include/asm-arm/psci.h >> > @@ -1,11 +1,6 @@ >> > #ifndef __ASM_PSCI_H__ >> > #define __ASM_PSCI_H__ >> > >> > -#define PSCI_SUCCESS 0 >> > -#define PSCI_ENOSYS -1 >> > -#define PSCI_EINVAL -2 >> > -#define PSCI_DENIED -3 >> > - >> >> Why did you just moved them at the end of the file and change the >> indentation? >> >> This was done to have common return functions between PSCI v0.1 and v >> 0.2 and to define them at one place >> > > Ok... I still don't understand why you can't add the new return value here. > I can :-), I will change it. sorry for the confusion, i thought you were asking why they have been moved... > BTW, IIRC there was a copyright int the PSCI header from Linux. I think > you have to retain it here. sure will take care in next patchset > > > > +/* PSCI version */ >> > +#define XEN_PSCI_V_0_1 1 >> >> This doesn't seem to be used, right? >> >> Yes it is defined for consistency and also for future use in case we >> return v0.1, i'd prefer to keep it this way what do you think? >> > > Ok > > > [..] >> >> > diff --git a/xen/include/public/arch-arm.h >> b/xen/include/public/arch-arm.h >> > index 7496556..93803e4 100644 >> > --- a/xen/include/public/arch-arm.h >> > +++ b/xen/include/public/arch-arm.h >> > @@ -385,6 +385,7 @@ typedef uint64_t xen_callback_t; >> > #define PSCI_cpu_off 1 >> > #define PSCI_cpu_on 2 >> > #define PSCI_migrate 3 >> > +#define PSCI_0_1_MAX 4 >> >> Why do you expose PSCI_0_1_MAX >> >> You are right i think i can get away by treating PSCI_migrate as max >> value (AI Parth) >> > > Can't you define 2 table in traps.c: one for PSCI v0.1 and one for PSCI > v0.2? This would avoid stupid issue later when we will implement a new > function for 0.1. > Ok, i will create two tables. > > Regards, > > -- > Julien Grall > Thanks parth
On Thu, 2014-06-26 at 10:56 +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. At the moment this will only affect dom0 and not guests because you haven't patched tools/libxl/libxl_arm.c:make_psci_node() to expose the new functionality. Were you intending to do that later (in which case please indicate this is dom0 only in the commit log) or did you just miss the need to do so? I think it should just be a case of adding the new compatibility string, like you've done in xen/arch/arm/domain_build.c. Are you also planning to work on host PSCI or is that someone else (just curious). > Signed-off-by: Parth Dixit <parth.dixit@linaro.org> > --- > xen/arch/arm/domain.c | 13 ++++ > xen/arch/arm/domain_build.c | 5 +- > xen/arch/arm/traps.c | 34 ++++++-- > xen/arch/arm/vpsci.c | 167 ++++++++++++++++++++++++++++++++++++++++ > xen/include/asm-arm/event.h | 2 + > xen/include/asm-arm/processor.h | 6 ++ > xen/include/asm-arm/psci.h | 122 +++++++++++++++++++++++++++-- > xen/include/public/arch-arm.h | 1 + > 8 files changed, 339 insertions(+), 11 deletions(-) > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index 2ae6941..e595a71 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -779,6 +779,19 @@ void vcpu_mark_events_pending(struct vcpu *v) > vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq, 1); > } > > +void vcpu_block_event(struct vcpu *v) > +{ > + vcpu_block(); > + /* The ARM spec declares that even if local irqs are masked in > + * the CPSR register, an irq should wake up a cpu from WFI anyway. > + * For this reason we need to check for irqs that need delivery, > + * ignoring the CPSR register, *after* calling SCHEDOP_block to > + * avoid races with vgic_vcpu_inject_irq. > + */ > + if ( local_events_need_delivery_nomask() ) > + vcpu_unblock(current); > +} You've coped this from the HSR_EC_WFI_WFE handler, I think, but not made that call here. Please can you do that. > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index 03a3da6..e42fb07 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -1030,7 +1030,7 @@ static arm_hypercall_t arm_hypercall_table[] = { > HYPERCALL_ARM(vcpu_op, 3), > }; > > -typedef int (*arm_psci_fn_t)(uint32_t, register_t); > +typedef int (*arm_psci_fn_t)(register_t, register_t, register_t); > > typedef struct { > arm_psci_fn_t fn; > @@ -1046,6 +1046,17 @@ typedef struct { > static arm_psci_t arm_psci_table[] = { > PSCI(cpu_off, 1), > PSCI(cpu_on, 2), > + PSCI(0_2_version,0), > + PSCI(0_2_cpu_suspend,3), > + PSCI(0_2_cpu_off,0), > + PSCI(0_2_cpu_on,3), > + PSCI(0_2_affinity_info,2), > + PSCI(0_2_migrate,1), > + PSCI(0_2_migrate_info_type,0), > + PSCI(0_2_migrate_info_up_cpu,0), > + PSCI(0_2_system_off,0), > + PSCI(0_2_system_reset,0), The v2 ops are 0x84xxxxxx and 0xc4xxxxxx -- doesn't that end up making this table enormous? Also AArch32 and Aarch64 have difference function IDs for the various functions -- I don't see that reflected here. > + > }; > > #ifndef NDEBUG > @@ -1082,24 +1093,37 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code) > #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_ARGS(r) (r)->x1, (r)->x2, (r)->x3 > #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_ARGS(r) (r)->r1, (r)->r2, (r)->r3 > #endif > > static void do_trap_psci(struct cpu_user_regs *regs) > { > arm_psci_fn_t psci_call = NULL; > + unsigned int fn_index ; > + struct domain *d = current->domain; > + > + if ( PSCI_OP_REG(regs) < PSCI_0_1_MAX ) > + fn_index = PSCI_OP_REG(regs); > + else if ( is_64bit_domain(d) ) > + { > + fn_index = ( ( PSCI_OP_REG(regs) - PSCI_0_2_FN_OFFSET ) ); > + if ( fn_index > PSCI_0_2_64BIT ) > + fn_index -= PSCI_0_2_64BIT; > + } > + else > + fn_index = ( PSCI_OP_REG(regs) - PSCI_0_2_FN_OFFSET ) ; I think you should stop 64-bit domains calling 32-bit functions and vice versa, shouldn't you? > +int do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point, > + register_t context_id) This is duplicating a load of do_psci_cpu_on. If you can't call the existing one directly then please refactor it into a common helper. > + > +int do_psci_0_2_affinity_info(register_t target_affinity, > + uint32_t lowest_affinity_level) > +{ > + unsigned long target_affinity_mask; > + unsigned int mpidr; mpidr is 64-bit on aarch64 (use register_t). > + struct vcpu *v = current; > + > + switch ( lowest_affinity_level ) > + { > + case MPIDR_AFF0_SHIFT: > + case MPIDR_AFF1_SHIFT: > + case MPIDR_AFF2_SHIFT: I see you've defined these as 0,1,2 etc. Customarily these would be the bit offsets in the register, i.e. 0,8,16, which makes them useless for your purposes (but using MPIDR_* for that was dodgy to start with). I think you can just case 0: target_affinity_mask = MPIDR_AFF0_MASK; break; case 1: target_affinity_mask = MPIDR_AFF1_MASK; break; etc. (Having defined MPIDR_AFF1_MASK appropriately etc) Or you could alternatively define a const array indexed by lowest_affinity_level with the appropriate values in it. > + target_affinity &= target_affinity_mask; > + mpidr = v->arch.vmpidr; > + if ( ( mpidr & target_affinity_mask ) == target_affinity ) > + return PSCI_0_2_AFFINITY_LEVEL_ON; > + else > + return PSCI_0_2_AFFINITY_LEVEL_OFF; This should return something based upon whether various processors are on or not, not something based on the mpidr of the current vcpu. Or have a I misread the spec? Given our current level of support for affinity in Xen this case pretty much just return ON for AFF1..AFF3 and consult the appropriate vcpus flags for AFF0. > + > +} > + > +int do_psci_0_2_migrate(uint32_t target_cpu) > +{ > + return PSCI_ENOSYS; This isn't a name defined in the spec (I think because our 0.1 implementation was based on a draft). > +} > + > + > +int do_psci_0_2_migrate_info_type(void) > +{ > + return PSCI_0_2_TOS_MP; This isn't a name in the spec either and it's a new one. Please can you use the appropriate names for the new ones, and if you don't mind could you also change the old ones to match the spec too. I don't think you need to version the names either, at least not until PSCI 0.3 defines something contradictory (lets hope not!) OK, I read further and realised that this is not an error code but rather is an unnamed constant from the spec. I'm not sure what "MP" is here. Perhaps a better name would be PSCI_MIGRATE_INFO_TYPE_{CAPABLE,INCAPABLE,UNNECESSARY} or something like that? > diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h > index 9267c1b..bf9d782 100644 > --- a/xen/include/asm-arm/processor.h > +++ b/xen/include/asm-arm/processor.h > @@ -13,9 +13,15 @@ > #define _MPIDR_SMP (31) > #define MPIDR_SMP (_AC(1,U) << _MPIDR_SMP) > #define MPIDR_AFF0_SHIFT (0) > +#define MPIDR_AFF1_SHIFT (1) > +#define MPIDR_AFF2_SHIFT (2) > +#define MPIDR_AFF3_SHIFT (3) As I said before, these should be 8, 16 and 32 respectively. > #define MPIDR_AFF0_MASK (_AC(0xff,U) << MPIDR_AFF0_SHIFT) And you should then copy this pattern for the others (you might need to ifdef CONFIG_ARM_64 AFF3 to avoid breaking the compile due to shift larger than type issues) > #define MPIDR_HWID_MASK _AC(0xffffff,U) > #define MPIDR_INVALID (~MPIDR_HWID_MASK) > +#define MPIDR_AFF_BIT_SHIFT (8) > +#define AFFINITY_MASK(level) (_AC(0xff,U) << ((level)*MPIDR_AFF_BIT_SHIFT)) > + > > /* TTBCR Translation Table Base Control Register */ > #define TTBCR_EAE _AC(0x80000000,U) > diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h > index 189964b..3487380 100644 > --- a/xen/include/asm-arm/psci.h > +++ b/xen/include/asm-arm/psci.h > @@ -1,11 +1,6 @@ > #ifndef __ASM_PSCI_H__ > #define __ASM_PSCI_H__ > > -#define PSCI_SUCCESS 0 > -#define PSCI_ENOSYS -1 > -#define PSCI_EINVAL -2 > -#define PSCI_DENIED -3 > - > /* availability of PSCI on the host for SMP bringup */ > extern bool_t psci_available; > > @@ -18,6 +13,123 @@ int do_psci_cpu_off(uint32_t power_state); > int do_psci_cpu_suspend(uint32_t power_state, register_t entry_point); > int do_psci_migrate(uint32_t vcpuid); > > +/* PSCI 0.2 functions to handle guest PSCI requests */ > +int do_psci_0_2_version(void); > +int do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point, > + register_t context_id); > +int do_psci_0_2_cpu_off(void); > +int do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point, > + register_t context_id); > +int do_psci_0_2_affinity_info(register_t target_affinity, > + uint32_t lowest_affinity_level); > +int do_psci_0_2_migrate(uint32_t target_cpu); > +int do_psci_0_2_migrate_info_type(void); > +register_t do_psci_0_2_migrate_info_up_cpu(void); > +void do_psci_0_2_system_off(void); > +void do_psci_0_2_system_reset(void); > +int do_psci_0_2_fn64_cpu_suspend(uint32_t power_state, > + uint32_t entry_point, register_t context_id); > +int do_psci_0_2_fn64_cpu_on(register_t target_cpu, register_t entry_point, > + register_t context_id); > +int do_psci_0_2_fn64_affinity_info(register_t target_affinity, > + uint32_t lowest_affinity_level); > +int do_psci_0_2_fn64_migrate(uint32_t target_cpu); > +int do_psci_0_2_fn64_migrate_info_up_cpu(void); This is making me thing that we should move do_psci_trap into vpsci.c so we can keep all this stuff internal. Either your functions should take uint32_t/uint64_t as per the spec and be duplicated for 32- vs 64-bit *or* they should take register_t for those parameters which are different for 32- vs 64-bit and not be duplicated. You've used register_t *and* duplicated AFAICT. Ideally I'd prefer not to duplicate, unless the spec makes that hard. (actually, looking back at the implementation you don't actually defined any fn64 -- so maybe half of these are just redundant?) > + > +/* PSCI version */ > +#define XEN_PSCI_V_0_1 1 > +#define XEN_PSCI_V_0_2 2 > + > +/* PSCI v0.2 id's internal to xen */ > + > +#define PSCI_0_2_version 5 > +#define PSCI_0_2_cpu_suspend 6 > +#define PSCI_0_2_cpu_off 7 > +#define PSCI_0_2_cpu_on 8 > +#define PSCI_0_2_affinity_info 9 > +#define PSCI_0_2_migrate 10 > +#define PSCI_0_2_migrate_info_type 11 > +#define PSCI_0_2_migrate_info_up_cpu 12 > +#define PSCI_0_2_system_off 13 > +#define PSCI_0_2_system_reset 14 > + > +#define PSCI_0_2_fn64_cpu_suspend 15 > +#define PSCI_0_2_fn64_cpu_on 16 > +#define PSCI_0_2_fn64_affinity_info 17 > +#define PSCI_0_2_fn64_migrate 18 > +#define PSCI_0_2_fn64_migrate_info_up_cpu 19 > + > +/* PSCI v0.2 xen mapping mask */ > +#define PSCI_0_2_FN_OFFSET 0x83FFFFFB Err? I think you should make the defines be the actual PSCI function numbers and then define three separate function call tables and look up the appropriate one based on the range of the function parameter. TBH given the relatively small number of PSCI calls maybe just switching to a C switch() statement would be best. > +/* PSCI v0.2 multicore support in Trusted OS returned by MIGRATE_INFO_TYPE */ > +#define PSCI_0_2_TOS_UP_MIGRATE 0 > +#define PSCI_0_2_TOS_UP_NO_MIGRATE 1 > +#define PSCI_0_2_TOS_MP 2 The first two names are tolerable, but the last one doesn't seem to match the spec. > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > index 7496556..93803e4 100644 > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -385,6 +385,7 @@ typedef uint64_t xen_callback_t; > #define PSCI_cpu_off 1 > #define PSCI_cpu_on 2 > #define PSCI_migrate 3 > +#define PSCI_0_1_MAX 4 The tools don't need this, so no need to make it public. Ian.
Hi On 27 June 2014 22:13, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Thu, 2014-06-26 at 10:56 +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. > > At the moment this will only affect dom0 and not guests because you > haven't patched tools/libxl/libxl_arm.c:make_psci_node() to expose the > new functionality. > > Were you intending to do that later (in which case please indicate this > is dom0 only in the commit log) or did you just miss the need to do so? > > I think it should just be a case of adding the new compatibility string, > like you've done in xen/arch/arm/domain_build.c. > Thanks, I missed it, i will incorporate it in next patchset. > Are you also planning to work on host PSCI or is that someone else (just > curious). > > PSCI host was scoped out because there was no real hardware where we can test this functionality > > Signed-off-by: Parth Dixit <parth.dixit@linaro.org> > > --- > > xen/arch/arm/domain.c | 13 ++++ > > xen/arch/arm/domain_build.c | 5 +- > > xen/arch/arm/traps.c | 34 ++++++-- > > xen/arch/arm/vpsci.c | 167 > ++++++++++++++++++++++++++++++++++++++++ > > xen/include/asm-arm/event.h | 2 + > > xen/include/asm-arm/processor.h | 6 ++ > > xen/include/asm-arm/psci.h | 122 +++++++++++++++++++++++++++-- > > xen/include/public/arch-arm.h | 1 + > > 8 files changed, 339 insertions(+), 11 deletions(-) > > > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > > index 2ae6941..e595a71 100644 > > --- a/xen/arch/arm/domain.c > > +++ b/xen/arch/arm/domain.c > > @@ -779,6 +779,19 @@ void vcpu_mark_events_pending(struct vcpu *v) > > vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq, 1); > > } > > > > +void vcpu_block_event(struct vcpu *v) > > +{ > > + vcpu_block(); > > + /* The ARM spec declares that even if local irqs are masked in > > + * the CPSR register, an irq should wake up a cpu from WFI anyway. > > + * For this reason we need to check for irqs that need delivery, > > + * ignoring the CPSR register, *after* calling SCHEDOP_block to > > + * avoid races with vgic_vcpu_inject_irq. > > + */ > > + if ( local_events_need_delivery_nomask() ) > > + vcpu_unblock(current); > > +} > > You've coped this from the HSR_EC_WFI_WFE handler, I think, but not made > that call here. Please can you do that. > > sure, i'll add it in next patchset > > + > > /* > > * Local variables: > > * mode: C > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > > index 03a3da6..e42fb07 100644 > > --- a/xen/arch/arm/traps.c > > +++ b/xen/arch/arm/traps.c > > @@ -1030,7 +1030,7 @@ static arm_hypercall_t arm_hypercall_table[] = { > > HYPERCALL_ARM(vcpu_op, 3), > > }; > > > > -typedef int (*arm_psci_fn_t)(uint32_t, register_t); > > +typedef int (*arm_psci_fn_t)(register_t, register_t, register_t); > > > > typedef struct { > > arm_psci_fn_t fn; > > @@ -1046,6 +1046,17 @@ typedef struct { > > static arm_psci_t arm_psci_table[] = { > > PSCI(cpu_off, 1), > > PSCI(cpu_on, 2), > > + PSCI(0_2_version,0), > > + PSCI(0_2_cpu_suspend,3), > > + PSCI(0_2_cpu_off,0), > > + PSCI(0_2_cpu_on,3), > > + PSCI(0_2_affinity_info,2), > > + PSCI(0_2_migrate,1), > > + PSCI(0_2_migrate_info_type,0), > > + PSCI(0_2_migrate_info_up_cpu,0), > > + PSCI(0_2_system_off,0), > > + PSCI(0_2_system_reset,0), > > The v2 ops are 0x84xxxxxx and 0xc4xxxxxx -- doesn't that end up making > this table enormous? > > Also AArch32 and Aarch64 have difference function IDs for the various > functions -- I don't see that reflected here. > > I am not using function id's directly i am using mask to calculate array index based on function id's. 32bit and 64 bit are using common functions. > > > + > > }; > > > > #ifndef NDEBUG > > @@ -1082,24 +1093,37 @@ static void do_debug_trap(struct cpu_user_regs > *regs, unsigned int code) > > #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_ARGS(r) (r)->x1, (r)->x2, (r)->x3 > > #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_ARGS(r) (r)->r1, (r)->r2, (r)->r3 > > #endif > > > > static void do_trap_psci(struct cpu_user_regs *regs) > > { > > arm_psci_fn_t psci_call = NULL; > > + unsigned int fn_index ; > > + struct domain *d = current->domain; > > + > > + if ( PSCI_OP_REG(regs) < PSCI_0_1_MAX ) > > + fn_index = PSCI_OP_REG(regs); > > + else if ( is_64bit_domain(d) ) > > + { > > + fn_index = ( ( PSCI_OP_REG(regs) - PSCI_0_2_FN_OFFSET ) ); > > + if ( fn_index > PSCI_0_2_64BIT ) > > + fn_index -= PSCI_0_2_64BIT; > > + } > > + else > > + fn_index = ( PSCI_OP_REG(regs) - PSCI_0_2_FN_OFFSET ) ; > > I think you should stop 64-bit domains calling 32-bit functions and vice > versa, shouldn't you? > > > +int do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point, > > + register_t context_id) > > This is duplicating a load of do_psci_cpu_on. If you can't call the > existing one directly then please refactor it into a common helper. > > sure.. > > + > > +int do_psci_0_2_affinity_info(register_t target_affinity, > > + uint32_t lowest_affinity_level) > > +{ > > + unsigned long target_affinity_mask; > > + unsigned int mpidr; > > mpidr is 64-bit on aarch64 (use register_t). > > > + struct vcpu *v = current; > > + > > + switch ( lowest_affinity_level ) > > + { > > + case MPIDR_AFF0_SHIFT: > > + case MPIDR_AFF1_SHIFT: > > + case MPIDR_AFF2_SHIFT: > > I see you've defined these as 0,1,2 etc. Customarily these would be the > bit offsets in the register, i.e. 0,8,16, which makes them useless for > your purposes (but using MPIDR_* for that was dodgy to start with). > > I think you can just > case 0: > target_affinity_mask = MPIDR_AFF0_MASK; > break; > case 1: > target_affinity_mask = MPIDR_AFF1_MASK; > break; > > etc. (Having defined MPIDR_AFF1_MASK appropriately etc) > > Or you could alternatively define a const array indexed by > lowest_affinity_level with the appropriate values in it. > > > + target_affinity &= target_affinity_mask; > > + mpidr = v->arch.vmpidr; > > + if ( ( mpidr & target_affinity_mask ) == target_affinity ) > > + return PSCI_0_2_AFFINITY_LEVEL_ON; > > + else > > + return PSCI_0_2_AFFINITY_LEVEL_OFF; > > This should return something based upon whether various processors are > on or not, not something based on the mpidr of the current vcpu. Or have > a I misread the spec? > > Given our current level of support for affinity in Xen this case pretty > much just return ON for AFF1..AFF3 and consult the appropriate vcpus > flags for AFF0. > > > + > > +} > > + > > +int do_psci_0_2_migrate(uint32_t target_cpu) > > +{ > > + return PSCI_ENOSYS; > > This isn't a name defined in the spec (I think because our 0.1 > implementation was based on a draft). > > > +} > > + > > + > > +int do_psci_0_2_migrate_info_type(void) > > +{ > > + return PSCI_0_2_TOS_MP; > > This isn't a name in the spec either and it's a new one. Please can you > use the appropriate names for the new ones, and if you don't mind could > you also change the old ones to match the spec too. > > I don't think you need to version the names either, at least not until > PSCI 0.3 defines something contradictory (lets hope not!) > > OK, I read further and realised that this is not an error code but > rather is an unnamed constant from the spec. I'm not sure what "MP" is > here. Perhaps a better name would be > PSCI_MIGRATE_INFO_TYPE_{CAPABLE,INCAPABLE,UNNECESSARY} or something like > that? > > > Ok,i'll change the names > > diff --git a/xen/include/asm-arm/processor.h > b/xen/include/asm-arm/processor.h > > index 9267c1b..bf9d782 100644 > > --- a/xen/include/asm-arm/processor.h > > +++ b/xen/include/asm-arm/processor.h > > @@ -13,9 +13,15 @@ > > #define _MPIDR_SMP (31) > > #define MPIDR_SMP (_AC(1,U) << _MPIDR_SMP) > > #define MPIDR_AFF0_SHIFT (0) > > +#define MPIDR_AFF1_SHIFT (1) > > +#define MPIDR_AFF2_SHIFT (2) > > +#define MPIDR_AFF3_SHIFT (3) > > As I said before, these should be 8, 16 and 32 respectively. > > > #define MPIDR_AFF0_MASK (_AC(0xff,U) << MPIDR_AFF0_SHIFT) > > And you should then copy this pattern for the others > > (you might need to ifdef CONFIG_ARM_64 AFF3 to avoid breaking the > compile due to shift larger than type issues) > > I will incorporate it in next patchset > > #define MPIDR_HWID_MASK _AC(0xffffff,U) > > #define MPIDR_INVALID (~MPIDR_HWID_MASK) > > +#define MPIDR_AFF_BIT_SHIFT (8) > > +#define AFFINITY_MASK(level) (_AC(0xff,U) << > ((level)*MPIDR_AFF_BIT_SHIFT)) > > + > > > > /* TTBCR Translation Table Base Control Register */ > > #define TTBCR_EAE _AC(0x80000000,U) > > diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h > > index 189964b..3487380 100644 > > --- a/xen/include/asm-arm/psci.h > > +++ b/xen/include/asm-arm/psci.h > > @@ -1,11 +1,6 @@ > > #ifndef __ASM_PSCI_H__ > > #define __ASM_PSCI_H__ > > > > -#define PSCI_SUCCESS 0 > > -#define PSCI_ENOSYS -1 > > -#define PSCI_EINVAL -2 > > -#define PSCI_DENIED -3 > > - > > /* availability of PSCI on the host for SMP bringup */ > > extern bool_t psci_available; > > > > @@ -18,6 +13,123 @@ int do_psci_cpu_off(uint32_t power_state); > > int do_psci_cpu_suspend(uint32_t power_state, register_t entry_point); > > int do_psci_migrate(uint32_t vcpuid); > > > > +/* PSCI 0.2 functions to handle guest PSCI requests */ > > +int do_psci_0_2_version(void); > > +int do_psci_0_2_cpu_suspend(uint32_t power_state, register_t > entry_point, > > + register_t context_id); > > +int do_psci_0_2_cpu_off(void); > > +int do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point, > > + register_t context_id); > > +int do_psci_0_2_affinity_info(register_t target_affinity, > > + uint32_t lowest_affinity_level); > > +int do_psci_0_2_migrate(uint32_t target_cpu); > > +int do_psci_0_2_migrate_info_type(void); > > +register_t do_psci_0_2_migrate_info_up_cpu(void); > > +void do_psci_0_2_system_off(void); > > +void do_psci_0_2_system_reset(void); > > +int do_psci_0_2_fn64_cpu_suspend(uint32_t power_state, > > + uint32_t entry_point, register_t > context_id); > > +int do_psci_0_2_fn64_cpu_on(register_t target_cpu, register_t > entry_point, > > + register_t context_id); > > +int do_psci_0_2_fn64_affinity_info(register_t target_affinity, > > + uint32_t lowest_affinity_level); > > +int do_psci_0_2_fn64_migrate(uint32_t target_cpu); > > +int do_psci_0_2_fn64_migrate_info_up_cpu(void); > > This is making me thing that we should move do_psci_trap into vpsci.c so > we can keep all this stuff internal. > > Either your functions should take uint32_t/uint64_t as per the spec and > be duplicated for 32- vs 64-bit *or* they should take register_t for > those parameters which are different for 32- vs 64-bit and not be > duplicated. You've used register_t *and* duplicated AFAICT. > > Ideally I'd prefer not to duplicate, unless the spec makes that hard. > > (actually, looking back at the implementation you don't actually defined > any fn64 -- so maybe half of these are just redundant?) > > They are there for completion,i am using common functions for 32 bit/64 bit > > + > > +/* PSCI version */ > > +#define XEN_PSCI_V_0_1 1 > > +#define XEN_PSCI_V_0_2 2 > > + > > +/* PSCI v0.2 id's internal to xen */ > > + > > +#define PSCI_0_2_version 5 > > +#define PSCI_0_2_cpu_suspend 6 > > +#define PSCI_0_2_cpu_off 7 > > +#define PSCI_0_2_cpu_on 8 > > +#define PSCI_0_2_affinity_info 9 > > +#define PSCI_0_2_migrate 10 > > +#define PSCI_0_2_migrate_info_type 11 > > +#define PSCI_0_2_migrate_info_up_cpu 12 > > +#define PSCI_0_2_system_off 13 > > +#define PSCI_0_2_system_reset 14 > > + > > +#define PSCI_0_2_fn64_cpu_suspend 15 > > +#define PSCI_0_2_fn64_cpu_on 16 > > +#define PSCI_0_2_fn64_affinity_info 17 > > +#define PSCI_0_2_fn64_migrate 18 > > +#define PSCI_0_2_fn64_migrate_info_up_cpu 19 > > + > > +/* PSCI v0.2 xen mapping mask */ > > +#define PSCI_0_2_FN_OFFSET 0x83FFFFFB > > Err? > > I think you should make the defines be the actual PSCI function numbers > and then define three separate function call tables and look up the > appropriate one based on the range of the function parameter. > > TBH given the relatively small number of PSCI calls maybe just switching > to a C switch() statement would be best. > patchset v1 was based on switch case it was moved to if/else after review comments, should i change it back to switch case? > > > +/* PSCI v0.2 multicore support in Trusted OS returned by > MIGRATE_INFO_TYPE */ > > +#define PSCI_0_2_TOS_UP_MIGRATE 0 > > +#define PSCI_0_2_TOS_UP_NO_MIGRATE 1 > > +#define PSCI_0_2_TOS_MP 2 > > The first two names are tolerable, but the last one doesn't seem to > match the spec. > > > diff --git a/xen/include/public/arch-arm.h > b/xen/include/public/arch-arm.h > > index 7496556..93803e4 100644 > > --- a/xen/include/public/arch-arm.h > > +++ b/xen/include/public/arch-arm.h > > @@ -385,6 +385,7 @@ typedef uint64_t xen_callback_t; > > #define PSCI_cpu_off 1 > > #define PSCI_cpu_on 2 > > #define PSCI_migrate 3 > > +#define PSCI_0_1_MAX 4 > > The tools don't need this, so no need to make it public. > This was taken as it is from previous implementation should i move it to psci.h? > > Ian. > > Thanks parth
On Mon, 2014-06-30 at 10:36 +0530, Parth Dixit wrote: Can you please try and configure your Linaro mail to send plain text mails and to use the normal ">" quoting style rather than HTML based indentation, which can get unreadable after a few round trips. > Are you also planning to work on host PSCI or is that someone > else (just > curious). > > > PSCI host was scoped out because there was no real hardware where we > can test this functionality OK. > > + > > /* > > * Local variables: > > * mode: C > > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > > index 03a3da6..e42fb07 100644 > > --- a/xen/arch/arm/traps.c > > +++ b/xen/arch/arm/traps.c > > @@ -1030,7 +1030,7 @@ static arm_hypercall_t > arm_hypercall_table[] = { > > HYPERCALL_ARM(vcpu_op, 3), > > }; > > > > -typedef int (*arm_psci_fn_t)(uint32_t, register_t); > > +typedef int (*arm_psci_fn_t)(register_t, register_t, > register_t); > > > > typedef struct { > > arm_psci_fn_t fn; > > @@ -1046,6 +1046,17 @@ typedef struct { > > static arm_psci_t arm_psci_table[] = { > > PSCI(cpu_off, 1), > > PSCI(cpu_on, 2), > > + PSCI(0_2_version,0), > > + PSCI(0_2_cpu_suspend,3), > > + PSCI(0_2_cpu_off,0), > > + PSCI(0_2_cpu_on,3), > > + PSCI(0_2_affinity_info,2), > > + PSCI(0_2_migrate,1), > > + PSCI(0_2_migrate_info_type,0), > > + PSCI(0_2_migrate_info_up_cpu,0), > > + PSCI(0_2_system_off,0), > > + PSCI(0_2_system_reset,0), > > > The v2 ops are 0x84xxxxxx and 0xc4xxxxxx -- doesn't that end > up making > this table enormous? > > Also AArch32 and Aarch64 have difference function IDs for the > various > functions -- I don't see that reflected here. > > > I am not using function id's directly i am using mask to calculate > array index based on function id's. > 32bit and 64 bit are using common functions. Oh, OK. I'm not mad keen on this (see below). > > > #define MPIDR_HWID_MASK _AC(0xffffff,U) > > #define MPIDR_INVALID (~MPIDR_HWID_MASK) > > +#define MPIDR_AFF_BIT_SHIFT (8) > > +#define AFFINITY_MASK(level) (_AC(0xff,U) << > ((level)*MPIDR_AFF_BIT_SHIFT)) > > + > > > > /* TTBCR Translation Table Base Control Register */ > > #define TTBCR_EAE _AC(0x80000000,U) > > diff --git a/xen/include/asm-arm/psci.h > b/xen/include/asm-arm/psci.h > > index 189964b..3487380 100644 > > --- a/xen/include/asm-arm/psci.h > > +++ b/xen/include/asm-arm/psci.h > > @@ -1,11 +1,6 @@ > > #ifndef __ASM_PSCI_H__ > > #define __ASM_PSCI_H__ > > > > -#define PSCI_SUCCESS 0 > > -#define PSCI_ENOSYS -1 > > -#define PSCI_EINVAL -2 > > -#define PSCI_DENIED -3 > > - > > /* availability of PSCI on the host for SMP bringup */ > > extern bool_t psci_available; > > > > @@ -18,6 +13,123 @@ int do_psci_cpu_off(uint32_t > power_state); > > int do_psci_cpu_suspend(uint32_t power_state, register_t > entry_point); > > int do_psci_migrate(uint32_t vcpuid); > > > > +/* PSCI 0.2 functions to handle guest PSCI requests */ > > +int do_psci_0_2_version(void); > > +int do_psci_0_2_cpu_suspend(uint32_t power_state, > register_t entry_point, > > + register_t context_id); > > +int do_psci_0_2_cpu_off(void); > > +int do_psci_0_2_cpu_on(register_t target_cpu, register_t > entry_point, > > + register_t context_id); > > +int do_psci_0_2_affinity_info(register_t target_affinity, > > + uint32_t > lowest_affinity_level); > > +int do_psci_0_2_migrate(uint32_t target_cpu); > > +int do_psci_0_2_migrate_info_type(void); > > +register_t do_psci_0_2_migrate_info_up_cpu(void); > > +void do_psci_0_2_system_off(void); > > +void do_psci_0_2_system_reset(void); > > +int do_psci_0_2_fn64_cpu_suspend(uint32_t power_state, > > + uint32_t entry_point, > register_t context_id); > > +int do_psci_0_2_fn64_cpu_on(register_t target_cpu, > register_t entry_point, > > + register_t context_id); > > +int do_psci_0_2_fn64_affinity_info(register_t > target_affinity, > > + uint32_t > lowest_affinity_level); > > +int do_psci_0_2_fn64_migrate(uint32_t target_cpu); > > +int do_psci_0_2_fn64_migrate_info_up_cpu(void); > > > This is making me thing that we should move do_psci_trap into > vpsci.c so > we can keep all this stuff internal. > > Either your functions should take uint32_t/uint64_t as per the > spec and > be duplicated for 32- vs 64-bit *or* they should take > register_t for > those parameters which are different for 32- vs 64-bit and not > be > duplicated. You've used register_t *and* duplicated AFAICT. > > Ideally I'd prefer not to duplicate, unless the spec makes > that hard. > > (actually, looking back at the implementation you don't > actually defined > any fn64 -- so maybe half of these are just redundant?) > > > They are there for completion,i am using common functions for 32 > bit/64 bit What do you mean by "completion"? If those prototypes do not correspond to some real function which is actually being used then please remove them. > > + > > +/* PSCI version */ > > +#define XEN_PSCI_V_0_1 1 > > +#define XEN_PSCI_V_0_2 2 > > + > > +/* PSCI v0.2 id's internal to xen */ > > + > > +#define PSCI_0_2_version 5 > > +#define PSCI_0_2_cpu_suspend 6 > > +#define PSCI_0_2_cpu_off 7 > > +#define PSCI_0_2_cpu_on 8 > > +#define PSCI_0_2_affinity_info 9 > > +#define PSCI_0_2_migrate 10 > > +#define PSCI_0_2_migrate_info_type 11 > > +#define PSCI_0_2_migrate_info_up_cpu 12 > > +#define PSCI_0_2_system_off 13 > > +#define PSCI_0_2_system_reset 14 > > + > > +#define PSCI_0_2_fn64_cpu_suspend 15 > > +#define PSCI_0_2_fn64_cpu_on 16 > > +#define PSCI_0_2_fn64_affinity_info 17 > > +#define PSCI_0_2_fn64_migrate 18 > > +#define PSCI_0_2_fn64_migrate_info_up_cpu 19 > > + > > +/* PSCI v0.2 xen mapping mask */ > > +#define PSCI_0_2_FN_OFFSET 0x83FFFFFB > > > Err? > > I think you should make the defines be the actual PSCI > function numbers > and then define three separate function call tables and look > up the > appropriate one based on the range of the function parameter. > > TBH given the relatively small number of PSCI calls maybe just > switching > to a C switch() statement would be best. > patchset v1 was based on switch case it was moved to if/else after > review comments, should i change it back to switch case? Sorry for not getting to this for v1. v1 had a switch statement to lookup fn_index, which it then looked up again in the op table. I think that approach was the worst of both worlds. What I was suggesting was a switch statement which actually dispatched the call to the appropriate handler directly, rather than having a second indirection via a table. The main thing I don't like is macro hoops you are jumping through in order to turn the spec mandated sparse function space into a compact one suitable for placing in an array. Another viable alternative would be to have separate tables for PSCI 0.1 and PSCSI 0.2 32- and 64-bit and look through each in turn. > > +/* PSCI v0.2 multicore support in Trusted OS returned by > MIGRATE_INFO_TYPE */ > > +#define PSCI_0_2_TOS_UP_MIGRATE 0 > > +#define PSCI_0_2_TOS_UP_NO_MIGRATE 1 > > +#define PSCI_0_2_TOS_MP 2 > > > The first two names are tolerable, but the last one doesn't > seem to > match the spec. > > > diff --git a/xen/include/public/arch-arm.h > b/xen/include/public/arch-arm.h > > index 7496556..93803e4 100644 > > --- a/xen/include/public/arch-arm.h > > +++ b/xen/include/public/arch-arm.h > > @@ -385,6 +385,7 @@ typedef uint64_t xen_callback_t; > > #define PSCI_cpu_off 1 > > #define PSCI_cpu_on 2 > > #define PSCI_migrate 3 > > +#define PSCI_0_1_MAX 4 > > > The tools don't need this, so no need to make it public. > This was taken as it is from previous implementation should i move it > to psci.h? What previous implementation? I don't see anything like that in the tree right now. Ian.
Hi, On 30 June 2014 15:45, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Mon, 2014-06-30 at 10:36 +0530, Parth Dixit wrote: > > Can you please try and configure your Linaro mail to send plain text > mails and to use the normal ">" quoting style rather than HTML based > indentation, which can get unreadable after a few round trips. > >> Are you also planning to work on host PSCI or is that someone >> else (just >> curious). >> >> >> PSCI host was scoped out because there was no real hardware where we >> can test this functionality > > OK. > >> > + >> > /* >> > * Local variables: >> > * mode: C >> >> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c >> > index 03a3da6..e42fb07 100644 >> > --- a/xen/arch/arm/traps.c >> > +++ b/xen/arch/arm/traps.c >> > @@ -1030,7 +1030,7 @@ static arm_hypercall_t >> arm_hypercall_table[] = { >> > HYPERCALL_ARM(vcpu_op, 3), >> > }; >> > >> > -typedef int (*arm_psci_fn_t)(uint32_t, register_t); >> > +typedef int (*arm_psci_fn_t)(register_t, register_t, >> register_t); >> > >> > typedef struct { >> > arm_psci_fn_t fn; >> > @@ -1046,6 +1046,17 @@ typedef struct { >> > static arm_psci_t arm_psci_table[] = { >> > PSCI(cpu_off, 1), >> > PSCI(cpu_on, 2), >> > + PSCI(0_2_version,0), >> > + PSCI(0_2_cpu_suspend,3), >> > + PSCI(0_2_cpu_off,0), >> > + PSCI(0_2_cpu_on,3), >> > + PSCI(0_2_affinity_info,2), >> > + PSCI(0_2_migrate,1), >> > + PSCI(0_2_migrate_info_type,0), >> > + PSCI(0_2_migrate_info_up_cpu,0), >> > + PSCI(0_2_system_off,0), >> > + PSCI(0_2_system_reset,0), >> >> >> The v2 ops are 0x84xxxxxx and 0xc4xxxxxx -- doesn't that end >> up making >> this table enormous? >> >> Also AArch32 and Aarch64 have difference function IDs for the >> various >> functions -- I don't see that reflected here. >> >> >> I am not using function id's directly i am using mask to calculate >> array index based on function id's. >> 32bit and 64 bit are using common functions. > > Oh, OK. I'm not mad keen on this (see below). > >> >> > #define MPIDR_HWID_MASK _AC(0xffffff,U) >> > #define MPIDR_INVALID (~MPIDR_HWID_MASK) >> > +#define MPIDR_AFF_BIT_SHIFT (8) >> > +#define AFFINITY_MASK(level) (_AC(0xff,U) << >> ((level)*MPIDR_AFF_BIT_SHIFT)) >> > + >> > >> > /* TTBCR Translation Table Base Control Register */ >> > #define TTBCR_EAE _AC(0x80000000,U) >> > diff --git a/xen/include/asm-arm/psci.h >> b/xen/include/asm-arm/psci.h >> > index 189964b..3487380 100644 >> > --- a/xen/include/asm-arm/psci.h >> > +++ b/xen/include/asm-arm/psci.h >> > @@ -1,11 +1,6 @@ >> > #ifndef __ASM_PSCI_H__ >> > #define __ASM_PSCI_H__ >> > >> > -#define PSCI_SUCCESS 0 >> > -#define PSCI_ENOSYS -1 >> > -#define PSCI_EINVAL -2 >> > -#define PSCI_DENIED -3 >> > - >> > /* availability of PSCI on the host for SMP bringup */ >> > extern bool_t psci_available; >> > >> > @@ -18,6 +13,123 @@ int do_psci_cpu_off(uint32_t >> power_state); >> > int do_psci_cpu_suspend(uint32_t power_state, register_t >> entry_point); >> > int do_psci_migrate(uint32_t vcpuid); >> > >> > +/* PSCI 0.2 functions to handle guest PSCI requests */ >> > +int do_psci_0_2_version(void); >> > +int do_psci_0_2_cpu_suspend(uint32_t power_state, >> register_t entry_point, >> > + register_t context_id); >> > +int do_psci_0_2_cpu_off(void); >> > +int do_psci_0_2_cpu_on(register_t target_cpu, register_t >> entry_point, >> > + register_t context_id); >> > +int do_psci_0_2_affinity_info(register_t target_affinity, >> > + uint32_t >> lowest_affinity_level); >> > +int do_psci_0_2_migrate(uint32_t target_cpu); >> > +int do_psci_0_2_migrate_info_type(void); >> > +register_t do_psci_0_2_migrate_info_up_cpu(void); >> > +void do_psci_0_2_system_off(void); >> > +void do_psci_0_2_system_reset(void); >> > +int do_psci_0_2_fn64_cpu_suspend(uint32_t power_state, >> > + uint32_t entry_point, >> register_t context_id); >> > +int do_psci_0_2_fn64_cpu_on(register_t target_cpu, >> register_t entry_point, >> > + register_t context_id); >> > +int do_psci_0_2_fn64_affinity_info(register_t >> target_affinity, >> > + uint32_t >> lowest_affinity_level); >> > +int do_psci_0_2_fn64_migrate(uint32_t target_cpu); >> > +int do_psci_0_2_fn64_migrate_info_up_cpu(void); >> >> >> This is making me thing that we should move do_psci_trap into >> vpsci.c so >> we can keep all this stuff internal. >> >> Either your functions should take uint32_t/uint64_t as per the >> spec and >> be duplicated for 32- vs 64-bit *or* they should take >> register_t for >> those parameters which are different for 32- vs 64-bit and not >> be >> duplicated. You've used register_t *and* duplicated AFAICT. >> >> Ideally I'd prefer not to duplicate, unless the spec makes >> that hard. >> >> (actually, looking back at the implementation you don't >> actually defined >> any fn64 -- so maybe half of these are just redundant?) >> >> >> They are there for completion,i am using common functions for 32 >> bit/64 bit > > What do you mean by "completion"? > > If those prototypes do not correspond to some real function which is > actually being used then please remove them PSCI v0.1 had two functions implemented and two others were declared but not implemented, i was following the same style. i.e. declaring all the functions in the spec but implementing only the ones that are mandated/used I will remove the the unused functions. > >> > + >> > +/* PSCI version */ >> > +#define XEN_PSCI_V_0_1 1 >> > +#define XEN_PSCI_V_0_2 2 >> > + >> > +/* PSCI v0.2 id's internal to xen */ >> > + >> > +#define PSCI_0_2_version 5 >> > +#define PSCI_0_2_cpu_suspend 6 >> > +#define PSCI_0_2_cpu_off 7 >> > +#define PSCI_0_2_cpu_on 8 >> > +#define PSCI_0_2_affinity_info 9 >> > +#define PSCI_0_2_migrate 10 >> > +#define PSCI_0_2_migrate_info_type 11 >> > +#define PSCI_0_2_migrate_info_up_cpu 12 >> > +#define PSCI_0_2_system_off 13 >> > +#define PSCI_0_2_system_reset 14 >> > + >> > +#define PSCI_0_2_fn64_cpu_suspend 15 >> > +#define PSCI_0_2_fn64_cpu_on 16 >> > +#define PSCI_0_2_fn64_affinity_info 17 >> > +#define PSCI_0_2_fn64_migrate 18 >> > +#define PSCI_0_2_fn64_migrate_info_up_cpu 19 >> > + >> > +/* PSCI v0.2 xen mapping mask */ >> > +#define PSCI_0_2_FN_OFFSET 0x83FFFFFB >> >> >> Err? >> >> I think you should make the defines be the actual PSCI >> function numbers >> and then define three separate function call tables and look >> up the >> appropriate one based on the range of the function parameter. >> >> TBH given the relatively small number of PSCI calls maybe just >> switching >> to a C switch() statement would be best. >> patchset v1 was based on switch case it was moved to if/else after >> review comments, should i change it back to switch case? > > Sorry for not getting to this for v1. > > v1 had a switch statement to lookup fn_index, which it then looked up > again in the op table. I think that approach was the worst of both > worlds. > > What I was suggesting was a switch statement which actually dispatched > the call to the appropriate handler directly, rather than having a > second indirection via a table. > > The main thing I don't like is macro hoops you are jumping through in > order to turn the spec mandated sparse function space into a compact one > suitable for placing in an array. > > Another viable alternative would be to have separate tables for PSCI 0.1 > and PSCSI 0.2 32- and 64-bit and look through each in turn. Sure, let me try the two table approach. > >> > +/* PSCI v0.2 multicore support in Trusted OS returned by >> MIGRATE_INFO_TYPE */ >> > +#define PSCI_0_2_TOS_UP_MIGRATE 0 >> > +#define PSCI_0_2_TOS_UP_NO_MIGRATE 1 >> > +#define PSCI_0_2_TOS_MP 2 >> >> >> The first two names are tolerable, but the last one doesn't >> seem to >> match the spec. >> >> > diff --git a/xen/include/public/arch-arm.h >> b/xen/include/public/arch-arm.h >> > index 7496556..93803e4 100644 >> > --- a/xen/include/public/arch-arm.h >> > +++ b/xen/include/public/arch-arm.h >> > @@ -385,6 +385,7 @@ typedef uint64_t xen_callback_t; >> > #define PSCI_cpu_off 1 >> > #define PSCI_cpu_on 2 >> > #define PSCI_migrate 3 >> > +#define PSCI_0_1_MAX 4 >> >> >> The tools don't need this, so no need to make it public. >> This was taken as it is from previous implementation should i move it >> to psci.h? > > What previous implementation? I don't see anything like that in the tree > right now. > > Ian. > > hmmm, just to be sure you are talking about these constants "PSCI_cpu_off,PSCI_cpu_on ..etc." in xen/include/public/arch-arm.h ? i can see them in xen/master branch...or is it something else?
On Mon, 2014-06-30 at 16:36 +0530, Parth Dixit wrote: > >> They are there for completion,i am using common functions for 32 > >> bit/64 bit > > > > What do you mean by "completion"? > > > > If those prototypes do not correspond to some real function which is > > actually being used then please remove them > > PSCI v0.1 had two functions implemented and two others were declared > but not implemented, i was following the same style. > i.e. declaring all the functions in the spec but implementing only the > ones that are mandated/used Oh, I think those v0.1 ones are there in error. > I will remove the the unused functions. Thanks. > > > >> > + > >> > +/* PSCI version */ > >> > +#define XEN_PSCI_V_0_1 1 > >> > +#define XEN_PSCI_V_0_2 2 > >> > + > >> > +/* PSCI v0.2 id's internal to xen */ > >> > + > >> > +#define PSCI_0_2_version 5 > >> > +#define PSCI_0_2_cpu_suspend 6 > >> > +#define PSCI_0_2_cpu_off 7 > >> > +#define PSCI_0_2_cpu_on 8 > >> > +#define PSCI_0_2_affinity_info 9 > >> > +#define PSCI_0_2_migrate 10 > >> > +#define PSCI_0_2_migrate_info_type 11 > >> > +#define PSCI_0_2_migrate_info_up_cpu 12 > >> > +#define PSCI_0_2_system_off 13 > >> > +#define PSCI_0_2_system_reset 14 > >> > + > >> > +#define PSCI_0_2_fn64_cpu_suspend 15 > >> > +#define PSCI_0_2_fn64_cpu_on 16 > >> > +#define PSCI_0_2_fn64_affinity_info 17 > >> > +#define PSCI_0_2_fn64_migrate 18 > >> > +#define PSCI_0_2_fn64_migrate_info_up_cpu 19 > >> > + > >> > +/* PSCI v0.2 xen mapping mask */ > >> > +#define PSCI_0_2_FN_OFFSET 0x83FFFFFB > >> > >> > >> Err? > >> > >> I think you should make the defines be the actual PSCI > >> function numbers > >> and then define three separate function call tables and look > >> up the > >> appropriate one based on the range of the function parameter. > >> > >> TBH given the relatively small number of PSCI calls maybe just > >> switching > >> to a C switch() statement would be best. > >> patchset v1 was based on switch case it was moved to if/else after > >> review comments, should i change it back to switch case? > > > > Sorry for not getting to this for v1. > > > > v1 had a switch statement to lookup fn_index, which it then looked up > > again in the op table. I think that approach was the worst of both > > worlds. > > > > What I was suggesting was a switch statement which actually dispatched > > the call to the appropriate handler directly, rather than having a > > second indirection via a table. > > > > The main thing I don't like is macro hoops you are jumping through in > > order to turn the spec mandated sparse function space into a compact one > > suitable for placing in an array. > > > > Another viable alternative would be to have separate tables for PSCI 0.1 > > and PSCSI 0.2 32- and 64-bit and look through each in turn. > > Sure, let me try the two table approach. NB: Three tables (0.1, 0.2-32bit, 0.2-64bit), unless you are planning something clever with multiple fn ids per table entry. > > > >> > +/* PSCI v0.2 multicore support in Trusted OS returned by > >> MIGRATE_INFO_TYPE */ > >> > +#define PSCI_0_2_TOS_UP_MIGRATE 0 > >> > +#define PSCI_0_2_TOS_UP_NO_MIGRATE 1 > >> > +#define PSCI_0_2_TOS_MP 2 > >> > >> > >> The first two names are tolerable, but the last one doesn't > >> seem to > >> match the spec. > >> > >> > diff --git a/xen/include/public/arch-arm.h > >> b/xen/include/public/arch-arm.h > >> > index 7496556..93803e4 100644 > >> > --- a/xen/include/public/arch-arm.h > >> > +++ b/xen/include/public/arch-arm.h > >> > @@ -385,6 +385,7 @@ typedef uint64_t xen_callback_t; > >> > #define PSCI_cpu_off 1 > >> > #define PSCI_cpu_on 2 > >> > #define PSCI_migrate 3 > >> > +#define PSCI_0_1_MAX 4 > >> > >> > >> The tools don't need this, so no need to make it public. > >> This was taken as it is from previous implementation should i move it > >> to psci.h? > > > > What previous implementation? I don't see anything like that in the tree > > right now. > > > > Ian. > > > > > hmmm, just to be sure you are talking about these constants > "PSCI_cpu_off,PSCI_cpu_on ..etc." in xen/include/public/arch-arm.h ? > i can see them in xen/master branch...or is it something else? I was talking only about PSCI_0_1_MAX, not the other ones. (PSCI_migrate isn't used either, it probably shouldn't exist as a #define) Ian.
Hi, On 30 June 2014 16:41, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Mon, 2014-06-30 at 16:36 +0530, Parth Dixit wrote: >> >> They are there for completion,i am using common functions for 32 >> >> bit/64 bit >> > >> > What do you mean by "completion"? >> > >> > If those prototypes do not correspond to some real function which is >> > actually being used then please remove them >> >> PSCI v0.1 had two functions implemented and two others were declared >> but not implemented, i was following the same style. >> i.e. declaring all the functions in the spec but implementing only the >> ones that are mandated/used > > Oh, I think those v0.1 ones are there in error. > >> I will remove the the unused functions. > > Thanks. > >> > >> >> > + >> >> > +/* PSCI version */ >> >> > +#define XEN_PSCI_V_0_1 1 >> >> > +#define XEN_PSCI_V_0_2 2 >> >> > + >> >> > +/* PSCI v0.2 id's internal to xen */ >> >> > + >> >> > +#define PSCI_0_2_version 5 >> >> > +#define PSCI_0_2_cpu_suspend 6 >> >> > +#define PSCI_0_2_cpu_off 7 >> >> > +#define PSCI_0_2_cpu_on 8 >> >> > +#define PSCI_0_2_affinity_info 9 >> >> > +#define PSCI_0_2_migrate 10 >> >> > +#define PSCI_0_2_migrate_info_type 11 >> >> > +#define PSCI_0_2_migrate_info_up_cpu 12 >> >> > +#define PSCI_0_2_system_off 13 >> >> > +#define PSCI_0_2_system_reset 14 >> >> > + >> >> > +#define PSCI_0_2_fn64_cpu_suspend 15 >> >> > +#define PSCI_0_2_fn64_cpu_on 16 >> >> > +#define PSCI_0_2_fn64_affinity_info 17 >> >> > +#define PSCI_0_2_fn64_migrate 18 >> >> > +#define PSCI_0_2_fn64_migrate_info_up_cpu 19 >> >> > + >> >> > +/* PSCI v0.2 xen mapping mask */ >> >> > +#define PSCI_0_2_FN_OFFSET 0x83FFFFFB >> >> >> >> >> >> Err? >> >> >> >> I think you should make the defines be the actual PSCI >> >> function numbers >> >> and then define three separate function call tables and look >> >> up the >> >> appropriate one based on the range of the function parameter. >> >> >> >> TBH given the relatively small number of PSCI calls maybe just >> >> switching >> >> to a C switch() statement would be best. >> >> patchset v1 was based on switch case it was moved to if/else after >> >> review comments, should i change it back to switch case? >> > >> > Sorry for not getting to this for v1. >> > >> > v1 had a switch statement to lookup fn_index, which it then looked up >> > again in the op table. I think that approach was the worst of both >> > worlds. >> > >> > What I was suggesting was a switch statement which actually dispatched >> > the call to the appropriate handler directly, rather than having a >> > second indirection via a table. >> > >> > The main thing I don't like is macro hoops you are jumping through in >> > order to turn the spec mandated sparse function space into a compact one >> > suitable for placing in an array. >> > >> > Another viable alternative would be to have separate tables for PSCI 0.1 >> > and PSCSI 0.2 32- and 64-bit and look through each in turn. >> >> Sure, let me try the two table approach. > > NB: Three tables (0.1, 0.2-32bit, 0.2-64bit), unless you are planning > something clever with multiple fn ids per table entry. As i am using common functions for 32bit as well as 64 bit two tables are sufficient 0.2, 64 bit table for 0.2 would be redundant. On second thought's i can also use the current table with single mask something like psci_call = arm_psci_table[PSCI_OP_REG(regs) & VERSION_MASK].fn; what do you think? i'll wait for ur opinion > >> > >> >> > +/* PSCI v0.2 multicore support in Trusted OS returned by >> >> MIGRATE_INFO_TYPE */ >> >> > +#define PSCI_0_2_TOS_UP_MIGRATE 0 >> >> > +#define PSCI_0_2_TOS_UP_NO_MIGRATE 1 >> >> > +#define PSCI_0_2_TOS_MP 2 >> >> >> >> >> >> The first two names are tolerable, but the last one doesn't >> >> seem to >> >> match the spec. >> >> >> >> > diff --git a/xen/include/public/arch-arm.h >> >> b/xen/include/public/arch-arm.h >> >> > index 7496556..93803e4 100644 >> >> > --- a/xen/include/public/arch-arm.h >> >> > +++ b/xen/include/public/arch-arm.h >> >> > @@ -385,6 +385,7 @@ typedef uint64_t xen_callback_t; >> >> > #define PSCI_cpu_off 1 >> >> > #define PSCI_cpu_on 2 >> >> > #define PSCI_migrate 3 >> >> > +#define PSCI_0_1_MAX 4 >> >> >> >> >> >> The tools don't need this, so no need to make it public. >> >> This was taken as it is from previous implementation should i move it >> >> to psci.h? >> > >> > What previous implementation? I don't see anything like that in the tree >> > right now. >> > >> > Ian. >> > >> > >> hmmm, just to be sure you are talking about these constants >> "PSCI_cpu_off,PSCI_cpu_on ..etc." in xen/include/public/arch-arm.h ? >> i can see them in xen/master branch...or is it something else? > > I was talking only about PSCI_0_1_MAX, not the other ones. > I will remove it. > (PSCI_migrate isn't used either, it probably shouldn't exist as a > #define) > > Ian. > Parth
On Tue, 2014-07-01 at 14:11 +0530, Parth Dixit wrote: > Hi, > > On 30 June 2014 16:41, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Mon, 2014-06-30 at 16:36 +0530, Parth Dixit wrote: > >> >> TBH given the relatively small number of PSCI calls maybe just > >> >> switching > >> >> to a C switch() statement would be best. > >> >> patchset v1 was based on switch case it was moved to if/else after > >> >> review comments, should i change it back to switch case? > >> > > >> > Sorry for not getting to this for v1. > >> > > >> > v1 had a switch statement to lookup fn_index, which it then looked up > >> > again in the op table. I think that approach was the worst of both > >> > worlds. > >> > > >> > What I was suggesting was a switch statement which actually dispatched > >> > the call to the appropriate handler directly, rather than having a > >> > second indirection via a table. > >> > > >> > The main thing I don't like is macro hoops you are jumping through in > >> > order to turn the spec mandated sparse function space into a compact one > >> > suitable for placing in an array. > >> > > >> > Another viable alternative would be to have separate tables for PSCI 0.1 > >> > and PSCSI 0.2 32- and 64-bit and look through each in turn. > >> > >> Sure, let me try the two table approach. > > > > NB: Three tables (0.1, 0.2-32bit, 0.2-64bit), unless you are planning > > something clever with multiple fn ids per table entry. > As i am using common functions for 32bit as well as 64 bit two tables > are sufficient 0.2, 64 bit table for 0.2 would be redundant. Oh, because the per-funciton offsets from the 32- vs. 64-bit base are all consistent. Yes I think that could work. > On second thought's i can also use the current table with single mask > something like > psci_call = arm_psci_table[PSCI_OP_REG(regs) & VERSION_MASK].fn; > what do you think? i'll wait for ur opinion Are the 0.1 and 0.2 fn numbers not overlapping? I suppose you control the 0.1 numbers so you could make it so if it isn't already. You would also need to validate that PSCI_OP_REG(regs) & ~VERSION_MASK was one of the acceptable base addresses. Ian
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 2ae6941..e595a71 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -779,6 +779,19 @@ void vcpu_mark_events_pending(struct vcpu *v) vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq, 1); } +void vcpu_block_event(struct vcpu *v) +{ + vcpu_block(); + /* The ARM spec declares that even if local irqs are masked in + * the CPSR register, an irq should wake up a cpu from WFI anyway. + * For this reason we need to check for irqs that need delivery, + * ignoring the CPSR register, *after* calling SCHEDOP_block to + * avoid races with vgic_vcpu_inject_irq. + */ + if ( local_events_need_delivery_nomask() ) + vcpu_unblock(current); +} + /* * Local variables: * mode: C 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 03a3da6..e42fb07 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -1030,7 +1030,7 @@ static arm_hypercall_t arm_hypercall_table[] = { HYPERCALL_ARM(vcpu_op, 3), }; -typedef int (*arm_psci_fn_t)(uint32_t, register_t); +typedef int (*arm_psci_fn_t)(register_t, register_t, register_t); typedef struct { arm_psci_fn_t fn; @@ -1046,6 +1046,17 @@ typedef struct { static arm_psci_t arm_psci_table[] = { PSCI(cpu_off, 1), PSCI(cpu_on, 2), + PSCI(0_2_version,0), + PSCI(0_2_cpu_suspend,3), + PSCI(0_2_cpu_off,0), + PSCI(0_2_cpu_on,3), + PSCI(0_2_affinity_info,2), + PSCI(0_2_migrate,1), + PSCI(0_2_migrate_info_type,0), + PSCI(0_2_migrate_info_up_cpu,0), + PSCI(0_2_system_off,0), + PSCI(0_2_system_reset,0), + }; #ifndef NDEBUG @@ -1082,24 +1093,37 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code) #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_ARGS(r) (r)->x1, (r)->x2, (r)->x3 #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_ARGS(r) (r)->r1, (r)->r2, (r)->r3 #endif static void do_trap_psci(struct cpu_user_regs *regs) { arm_psci_fn_t psci_call = NULL; + unsigned int fn_index ; + struct domain *d = current->domain; + + if ( PSCI_OP_REG(regs) < PSCI_0_1_MAX ) + fn_index = PSCI_OP_REG(regs); + else if ( is_64bit_domain(d) ) + { + fn_index = ( ( PSCI_OP_REG(regs) - PSCI_0_2_FN_OFFSET ) ); + if ( fn_index > PSCI_0_2_64BIT ) + fn_index -= PSCI_0_2_64BIT; + } + else + fn_index = ( PSCI_OP_REG(regs) - PSCI_0_2_FN_OFFSET ) ; - if ( PSCI_OP_REG(regs) >= ARRAY_SIZE(arm_psci_table) ) + if ( fn_index >= ARRAY_SIZE(arm_psci_table) ) { domain_crash_synchronous(); return; } - psci_call = arm_psci_table[PSCI_OP_REG(regs)].fn; + psci_call = arm_psci_table[fn_index].fn; if ( psci_call == NULL ) { domain_crash_synchronous(); diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c index 1ceb8cb..d29c097 100644 --- a/xen/arch/arm/vpsci.c +++ b/xen/arch/arm/vpsci.c @@ -17,6 +17,8 @@ #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) { @@ -83,6 +85,171 @@ int do_psci_cpu_off(uint32_t power_state) return PSCI_SUCCESS; } +int do_psci_0_2_version(void) +{ + return XEN_PSCI_V_0_2; +} + +int do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point, + register_t context_id) +{ + struct vcpu *v = current; + struct domain *d = v->domain; + struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs; + + if ( is_32bit_domain(d) ) + { + regs->pc32 = entry_point; + regs->r0 = context_id; + } +#ifdef CONFIG_ARM_64 + else + { + regs->pc = entry_point; + regs->x0 = context_id; + } +#endif + vcpu_block_event(v); + return PSCI_SUCCESS; +} + +int do_psci_0_2_cpu_off(void) +{ + uint32_t power_state = 0 ; + return do_psci_cpu_off(power_state); +} + +int do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point, + register_t context_id) +{ + struct vcpu *v; + struct domain *d = current->domain; + struct vcpu_guest_context *ctxt; + int rc; + int is_thumb = entry_point & 1; + uint32_t vcpuid = target_cpu & MPIDR_HWID_MASK; + + if ( (vcpuid < 0) || (vcpuid >= MAX_VIRT_CPUS) ) + return PSCI_EINVAL; + + if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL ) + return PSCI_EINVAL; + + /* THUMB set is not allowed with 64-bit domain */ + if ( is_64bit_domain(d) && is_thumb ) + return PSCI_EINVAL; + + if ( !test_bit(_VPF_down, &v->pause_flags) ) + return PSCI_ALREADY_ON; + + if ( (ctxt = alloc_vcpu_guest_context()) == NULL ) + return PSCI_DENIED; + + vgic_clear_pending_irqs(v); + + memset(ctxt, 0, sizeof(*ctxt)); + ctxt->user_regs.pc64 = (u64) entry_point; + ctxt->sctlr = SCTLR_GUEST_INIT; + ctxt->ttbr0 = 0; + ctxt->ttbr1 = 0; + ctxt->ttbcr = 0; /* Defined Reset Value */ + if ( is_32bit_domain(d) ) + { + ctxt->user_regs.cpsr = PSR_GUEST32_INIT; + ctxt->user_regs.r0_usr = context_id; + } +#ifdef CONFIG_ARM_64 + else + { + ctxt->user_regs.cpsr = PSR_GUEST64_INIT; + ctxt->user_regs.x0 = context_id; + } +#endif + + /* Start the VCPU with THUMB set if it's requested by the kernel */ + if ( is_thumb ) + ctxt->user_regs.cpsr |= PSR_THUMB; + ctxt->flags = VGCF_online; + + domain_lock(d); + rc = arch_set_info_guest(v, ctxt); + free_vcpu_guest_context(ctxt); + + if ( rc < 0 ) + { + domain_unlock(d); + return PSCI_DENIED; + } + domain_unlock(d); + + vcpu_wake(v); + + return PSCI_SUCCESS; +} + +int do_psci_0_2_affinity_info(register_t target_affinity, + uint32_t lowest_affinity_level) +{ + unsigned long target_affinity_mask; + unsigned int mpidr; + struct vcpu *v = current; + + switch ( lowest_affinity_level ) + { + case MPIDR_AFF0_SHIFT: + case MPIDR_AFF1_SHIFT: + case MPIDR_AFF2_SHIFT: + target_affinity_mask + = ( MPIDR_HWID_MASK & AFFINITY_MASK( lowest_affinity_level ) ); + break; + case MPIDR_AFF3_SHIFT: + target_affinity_mask + = ( MPIDR_HWID_MASK & AFFINITY_MASK(lowest_affinity_level+1) ); + break; + default: + return PSCI_EINVAL; + } + + target_affinity &= target_affinity_mask; + mpidr = v->arch.vmpidr; + if ( ( mpidr & target_affinity_mask ) == target_affinity ) + return PSCI_0_2_AFFINITY_LEVEL_ON; + else + return PSCI_0_2_AFFINITY_LEVEL_OFF; + +} + +int do_psci_0_2_migrate(uint32_t target_cpu) +{ + return PSCI_ENOSYS; +} + + +int do_psci_0_2_migrate_info_type(void) +{ + return PSCI_0_2_TOS_MP; +} + + +register_t do_psci_0_2_migrate_info_up_cpu(void) +{ + return PSCI_ENOSYS; +} + + +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) +{ + struct domain *d = current->domain; + domain_shutdown(d,SHUTDOWN_reboot); +} + /* * Local variables: * mode: C diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h index dd3ad13..6aba939 100644 --- a/xen/include/asm-arm/event.h +++ b/xen/include/asm-arm/event.h @@ -6,6 +6,7 @@ void vcpu_kick(struct vcpu *v); void vcpu_mark_events_pending(struct vcpu *v); +void vcpu_block_event(struct vcpu *v); static inline int vcpu_event_delivery_is_enabled(struct vcpu *v) { @@ -55,6 +56,7 @@ static inline int arch_virq_is_global(int virq) return 1; } + #endif /* * Local variables: diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h index 9267c1b..bf9d782 100644 --- a/xen/include/asm-arm/processor.h +++ b/xen/include/asm-arm/processor.h @@ -13,9 +13,15 @@ #define _MPIDR_SMP (31) #define MPIDR_SMP (_AC(1,U) << _MPIDR_SMP) #define MPIDR_AFF0_SHIFT (0) +#define MPIDR_AFF1_SHIFT (1) +#define MPIDR_AFF2_SHIFT (2) +#define MPIDR_AFF3_SHIFT (3) #define MPIDR_AFF0_MASK (_AC(0xff,U) << MPIDR_AFF0_SHIFT) #define MPIDR_HWID_MASK _AC(0xffffff,U) #define MPIDR_INVALID (~MPIDR_HWID_MASK) +#define MPIDR_AFF_BIT_SHIFT (8) +#define AFFINITY_MASK(level) (_AC(0xff,U) << ((level)*MPIDR_AFF_BIT_SHIFT)) + /* TTBCR Translation Table Base Control Register */ #define TTBCR_EAE _AC(0x80000000,U) diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h index 189964b..3487380 100644 --- a/xen/include/asm-arm/psci.h +++ b/xen/include/asm-arm/psci.h @@ -1,11 +1,6 @@ #ifndef __ASM_PSCI_H__ #define __ASM_PSCI_H__ -#define PSCI_SUCCESS 0 -#define PSCI_ENOSYS -1 -#define PSCI_EINVAL -2 -#define PSCI_DENIED -3 - /* availability of PSCI on the host for SMP bringup */ extern bool_t psci_available; @@ -18,6 +13,123 @@ int do_psci_cpu_off(uint32_t power_state); int do_psci_cpu_suspend(uint32_t power_state, register_t entry_point); int do_psci_migrate(uint32_t vcpuid); +/* PSCI 0.2 functions to handle guest PSCI requests */ +int do_psci_0_2_version(void); +int do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point, + register_t context_id); +int do_psci_0_2_cpu_off(void); +int do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point, + register_t context_id); +int do_psci_0_2_affinity_info(register_t target_affinity, + uint32_t lowest_affinity_level); +int do_psci_0_2_migrate(uint32_t target_cpu); +int do_psci_0_2_migrate_info_type(void); +register_t do_psci_0_2_migrate_info_up_cpu(void); +void do_psci_0_2_system_off(void); +void do_psci_0_2_system_reset(void); +int do_psci_0_2_fn64_cpu_suspend(uint32_t power_state, + uint32_t entry_point, register_t context_id); +int do_psci_0_2_fn64_cpu_on(register_t target_cpu, register_t entry_point, + register_t context_id); +int do_psci_0_2_fn64_affinity_info(register_t target_affinity, + uint32_t lowest_affinity_level); +int do_psci_0_2_fn64_migrate(uint32_t target_cpu); +int do_psci_0_2_fn64_migrate_info_up_cpu(void); + +/* PSCI version */ +#define XEN_PSCI_V_0_1 1 +#define XEN_PSCI_V_0_2 2 + +/* PSCI v0.2 id's internal to xen */ + +#define PSCI_0_2_version 5 +#define PSCI_0_2_cpu_suspend 6 +#define PSCI_0_2_cpu_off 7 +#define PSCI_0_2_cpu_on 8 +#define PSCI_0_2_affinity_info 9 +#define PSCI_0_2_migrate 10 +#define PSCI_0_2_migrate_info_type 11 +#define PSCI_0_2_migrate_info_up_cpu 12 +#define PSCI_0_2_system_off 13 +#define PSCI_0_2_system_reset 14 + +#define PSCI_0_2_fn64_cpu_suspend 15 +#define PSCI_0_2_fn64_cpu_on 16 +#define PSCI_0_2_fn64_affinity_info 17 +#define PSCI_0_2_fn64_migrate 18 +#define PSCI_0_2_fn64_migrate_info_up_cpu 19 + +/* PSCI v0.2 xen mapping mask */ +#define PSCI_0_2_FN_OFFSET 0x83FFFFFB + +/* PSCI v0.2 interface */ +#define PSCI_0_2_FN_BASE 0x84000000 +#define PSCI_0_2_FN(n) (PSCI_0_2_FN_BASE + (n)) +#define PSCI_0_2_64BIT 0x40000000 +#define PSCI_0_2_FN64_BASE \ + (PSCI_0_2_FN_BASE + PSCI_0_2_64BIT) +#define PSCI_0_2_FN64(n) (PSCI_0_2_FN64_BASE + (n)) + +#define PSCI_0_2_FN_PSCI_VERSION PSCI_0_2_FN(0) +#define PSCI_0_2_FN_CPU_SUSPEND PSCI_0_2_FN(1) +#define PSCI_0_2_FN_CPU_OFF PSCI_0_2_FN(2) +#define PSCI_0_2_FN_CPU_ON PSCI_0_2_FN(3) +#define PSCI_0_2_FN_AFFINITY_INFO PSCI_0_2_FN(4) +#define PSCI_0_2_FN_MIGRATE PSCI_0_2_FN(5) +#define PSCI_0_2_FN_MIGRATE_INFO_TYPE PSCI_0_2_FN(6) +#define PSCI_0_2_FN_MIGRATE_INFO_UP_CPU PSCI_0_2_FN(7) +#define PSCI_0_2_FN_SYSTEM_OFF PSCI_0_2_FN(8) +#define PSCI_0_2_FN_SYSTEM_RESET PSCI_0_2_FN(9) + +#define PSCI_0_2_FN64_CPU_SUSPEND PSCI_0_2_FN64(1) +#define PSCI_0_2_FN64_CPU_ON PSCI_0_2_FN64(3) +#define PSCI_0_2_FN64_AFFINITY_INFO PSCI_0_2_FN64(4) +#define PSCI_0_2_FN64_MIGRATE PSCI_0_2_FN64(5) +#define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU PSCI_0_2_FN64(7) + +/* PSCI v0.2 power state encoding for CPU_SUSPEND function */ +#define PSCI_0_2_POWER_STATE_ID_MASK 0xffff +#define PSCI_0_2_POWER_STATE_ID_SHIFT 0 +#define PSCI_0_2_POWER_STATE_TYPE_SHIFT 16 +#define PSCI_0_2_POWER_STATE_TYPE_MASK \ + (0x1 << PSCI_0_2_POWER_STATE_TYPE_SHIFT) +#define PSCI_0_2_POWER_STATE_AFFL_SHIFT 24 +#define PSCI_0_2_POWER_STATE_AFFL_MASK \ + (0x3 << PSCI_0_2_POWER_STATE_AFFL_SHIFT) + +/* PSCI v0.2 affinity level state returned by AFFINITY_INFO */ +#define PSCI_0_2_AFFINITY_LEVEL_ON 0 +#define PSCI_0_2_AFFINITY_LEVEL_OFF 1 +#define PSCI_0_2_AFFINITY_LEVEL_ON_PENDING 2 + +/* PSCI v0.2 multicore support in Trusted OS returned by MIGRATE_INFO_TYPE */ +#define PSCI_0_2_TOS_UP_MIGRATE 0 +#define PSCI_0_2_TOS_UP_NO_MIGRATE 1 +#define PSCI_0_2_TOS_MP 2 + +/* PSCI version decoding (independent of PSCI version) */ +#define PSCI_VERSION_MAJOR_SHIFT 16 +#define PSCI_VERSION_MINOR_MASK \ + ((1U << PSCI_VERSION_MAJOR_SHIFT) - 1) +#define PSCI_VERSION_MAJOR_MASK ~PSCI_VERSION_MINOR_MASK +#define PSCI_VERSION_MAJOR(ver) \ + (((ver) & PSCI_VERSION_MAJOR_MASK) >> PSCI_VERSION_MAJOR_SHIFT) +#define PSCI_VERSION_MINOR(ver) \ + ((ver) & PSCI_VERSION_MINOR_MASK) + + +/* PSCI return values (inclusive of all PSCI versions) */ +#define PSCI_SUCCESS 0 +#define PSCI_ENOSYS -1 +#define PSCI_EINVAL -2 +#define PSCI_DENIED -3 +#define PSCI_ALREADY_ON -4 +#define PSCI_ON_PENDING -5 +#define PSCI_INTERNAL_FAILURE -6 +#define PSCI_NOT_PRESENT -7 +#define PSCI_DISABLED -8 + + #endif /* __ASM_PSCI_H__ */ /* diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h index 7496556..93803e4 100644 --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -385,6 +385,7 @@ typedef uint64_t xen_callback_t; #define PSCI_cpu_off 1 #define PSCI_cpu_on 2 #define PSCI_migrate 3 +#define PSCI_0_1_MAX 4 #endif
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. - implemented WFI helper function for cpu_suspend - modified arm_psci_fn_t to take three arguments - implemented psci_cpu_on with additional error conditions - removed switch-case in do_trap_psci function - added PSCI v0.2 macros in psci.h from linux kernel Signed-off-by: Parth Dixit <parth.dixit@linaro.org> --- xen/arch/arm/domain.c | 13 ++++ xen/arch/arm/domain_build.c | 5 +- xen/arch/arm/traps.c | 34 ++++++-- xen/arch/arm/vpsci.c | 167 ++++++++++++++++++++++++++++++++++++++++ xen/include/asm-arm/event.h | 2 + xen/include/asm-arm/processor.h | 6 ++ xen/include/asm-arm/psci.h | 122 +++++++++++++++++++++++++++-- xen/include/public/arch-arm.h | 1 + 8 files changed, 339 insertions(+), 11 deletions(-)