|
[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
Hi,
On 30 June 2014 15:45, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 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?
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |