[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] Fwd: [PATCH v5] xen/arm : emulation of arm's PSCI v0.2 standard
forgot to add xen-devel list... ---------- Forwarded message ---------- From: Parth Dixit <parth.dixit@xxxxxxxxxx> Date: 25 July 2014 11:05 Subject: Re: [PATCH v5] xen/arm : emulation of arm's PSCI v0.2 standard To: Ian Campbell <Ian.Campbell@xxxxxxxxxx> On 24 July 2014 19:36, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote: > On Thu, 2014-07-17 at 22:11 +0530, Parth Dixit wrote: >> Changelog v5 >> - removed PSCI_ARGS macro >> - preload error value for calls that do not return >> - added new macro for 32 bit arguments >> - casting return calls to appropriate types >> - added new function psci_mode_check >> - added error checks for PSCI v0.2 function id's > > Looking good. I've a couple of questions which I hope you can answer. > >> +static void do_trap_psci(struct cpu_user_regs *regs) >> +{ >> + register_t fid = PSCI_ARG(regs,0); >> >> - psci_call = arm_psci_table[PSCI_OP_REG(regs)].fn; >> - if ( psci_call == NULL ) >> + /* preloading in case psci_mode_check fails */ >> + PSCI_RESULT_REG(regs) = (int32_t)PSCI_INVALID_PARAMETERS; >> [...] >> + case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU: >> + if ( psci_mode_check(current->domain, fid) ) >> + PSCI_RESULT_REG(regs) = >> + (uint32_t) do_psci_0_2_migrate_info_up_cpu(); >> + else >> + PSCI_RESULT_REG(regs) = (uint32_t)PSCI_INVALID_PARAMETERS; >> + break; > [...] >> + case PSCI_0_2_FN_CPU_ON: >> + case PSCI_0_2_FN64_CPU_ON: >> + if ( psci_mode_check(current->domain, fid) ) >> + { >> + register_t vcpuid = PSCI_ARG(regs,1); >> + register_t epoint = PSCI_ARG(regs,2); >> + register_t cid = PSCI_ARG(regs,3); >> + PSCI_RESULT_REG(regs) = >> + (int32_t) do_psci_0_2_cpu_on(vcpuid, epoint, cid); > > Indentation here is broken. i will fix in patchset v6 > > I notice that sometimes you have an else case setting > PSCI_INVALID_PARAMETERS and other times you don't. Is that just an > oversight or is there a reason why they should differ? It was done because rest of the functions were using default preloaded value with return type int32 , functions with else case are functions that have return type other than int32(uint32 or register_t) but as you have commented further if PSCI* result codes do not require casting, this will go away. To summarize i will remove the casting of result codes this will also do away the need of else statements in patchset v6. > I don't think you need to cast the PSCI_* result codes BTW. > > [...] >> +int32_t do_psci_0_2_cpu_suspend(uint32_t power_state, register_t >> entry_point, >> + register_t context_id) >> +{ >> + struct vcpu *v = current; >> + struct domain *d = v->domain; >> + struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs; > > I think it would make sense to pass this as an argument, but if not then > guest_cpu_user_regs() is the accessor to use. > sure, will take care in patchset v6 >> + >> + if ( is_32bit_domain(d) ) >> + { >> + regs->pc32 = entry_point; >> + regs->r0 = context_id; >> + } >> +#ifdef CONFIG_ARM_64 >> + else >> + { >> + regs->pc = entry_point; >> + regs->x0 = context_id; >> + } >> +#endif >> + vcpu_block_unless_event_pending(v); >> + return PSCI_SUCCESS; > > This only seems to be handling the case where the requested power state > is power down and not the standby state. > > By my reading when bit 16 if the power_state argument is 0 (standby) > then entry_point and context_id are ignored and the PSCI call is > expected to return success to the place it was called from, not jump to > entry_point. > > If bit 16 is one you are supposed to return to entry_point with x0/r0 > set to context_id, but because you return PSCI_SUCCESS that will get > written to x0/r0 by the generic handler clobbering your write of x0/r0 > above. You should probably return the desired value (with an explanatory > comment) valid point, i am just wondering why kvm implementation ignored these values, i will implement it in next patchset > The upper bits of power_state also describe an affinity level to put to > sleep, which you also don't handle AFAICT. Perhaps since we don't > actually have affinities > 0 what you've done is OK, what do you think? I think it is better to put a comment explicitly stating that we have ignored the value because affinities > 0 are not valid for xen or we can check for affinity and return if it is greater than zero. I think i'll go with check, will take care in next patchset > The lower bits are supposed to be some sort of per platform ID which we > are supposed to have exposed via tables. I'm not sure there are any DTB > bindings for that, so perhaps we can ignore it. > > The handling of bit 16 is the only bit which really concerns me here, > the rest seem like things we can tolerate. > > Ian. > Thanks parth _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |