|
[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 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.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |