Re: [Xen-devel] [PATCH v2 02/15] xen/arm: vpsci: Add support for PSCI 1.1

[Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>]

Hi Julien,


On 02/12/2018 10:41 PM, Julien Grall wrote:
> On 12/02/2018 20:12, Mirela Simonovic wrote:
> > Hi Julien,
> Hi Mirela,
>
> Thank you for the review.
>
> > I've done pretty much the same work in parallel, but there are few 
> > additional minor changes I've made. Briefly, the difference is in 
> > return values that some already implemented functions should return 
> > starting from v1.0 (and even v0.2 errata). Please let me know whether 
> > you omitted that intentionally.
> Could you give a bit more details here? From a brief look we don't 
> seem to implement correctly:
>     - CPU_OFF: PSCI_DENY should be return on failure (though it should 
> never fail in Xen case) and the check on the vCPU state is pointless.
I believe CPU_OFF is fine today, it never returns.

>     - MIGRATE_INFO_TYPE: should technically return int32_t instead of 
> uint32_t. That not really matter for now.
> If you speak about denying SMC64 call from AArch32, then this is 
> already done in vsmccc.c (see vsmccc_call).
Agreed on above, there are 2 more:

1. MIGRATE_INFO_TYPE should return PSCI_NOT_SUPPORTED instead 
PSCI_0_2_TOS_MP_OR_NOT_PRESENT. The function is effectively not 
implemented, but in v0.2 it was mandatory, so it couldn't return 
PSCI_NOT_SUPPORTED (I guess this was some kind of a workaround). Since 
v0.2 errata and v1.0 release the function is made optional and it should 
return "not supported" error - just removing the function should be fine 
(and mismatching return type issue would be gone).
2. A new error code has been introduced in PSCI v1.0: 
PSCI_INVALID_ADDRESS. This error should be returned by PSCI functions 
which receive an address as the argument when the provided address is 
incorrect. In implementation in Xen this affects CPU_ON and CPU_SUSPEND. 
CPU_ON today returns invalid parameter error and that needs to be 
replaced with invalid address error. I'm not sure for CPU_SUSPEND since 
its implementation doesn't use/check any of the arguments today...
Thanks,
Mirela

> > I can submit these patches if you want. Currently I have few - one 
> > for each fix, easier to review. I guess all of them should be 
> > squashed with the patch you submitted.
> > One more note - starting from v1.0, PSCI_NOT_SUPPORTED error should 
> > be returned for all optional functions that are not implemented. Is 
> > that the case? I.e. when there is no case for a particular function 
> > ID in do_vpsci_0_2_call the PSCI_NOT_SUPPORTED error will be returned?
> This is done by vmsccc_handle_call().
> See set_user_regs(regs, 0, ARM_SMCC_ERR_UNKNOWN_FUNCTION) which is 
> equivalent to PSCI_NOT_SUPPORTED.
> [...]
>
> > > +static int32_t do_psci_1_0_features(uint32_t psci_func_id)
> > > +{
> > > +    /* /!\ Ordered by function ID and not name */
> > > +    switch ( psci_func_id )
> > > +    {
> > > +    case PSCI_0_2_FN32_PSCI_VERSION:
> > > +    case PSCI_0_2_FN32_CPU_SUSPEND:
> > > +    case PSCI_0_2_FN64_CPU_SUSPEND:
> > Just a note here - PSCI_FEATURES should return additional information 
> > just for CPU_SUSPEND (supported power state and mode). AFAIU, that 
> > value is also 0, so the return code should be fine.
> I think so, from what I understood this is inline with CPU_SUSPEND 
> only supports 0.2 format.
> Cheers,

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel