Re: [Xen-devel] [PATCH v2] xen/arm : emulation of arm's PSCI v0.2 standard

[Parth Dixit <parth.dixit@xxxxxxxxxx>]

Hi

On 27 June 2014 22:13, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 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@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.

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel