[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 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |