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

[Mirela Simonovic <mirela.simonovic@aggios.com>]

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel