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

[Parth Dixit <parth.dixit@xxxxxxxxxx>]

Hi,

On 30 June 2014 16:41, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 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

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