|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2] xen/arm : emulation of arm's PSCI v0.2 standard
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@xxxxxxxxxx>
> ---
> 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.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |