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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.

> >
> >>         > +/* 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.

(PSCI_migrate isn't used either, it probably shouldn't exist as a
#define)

Ian.


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