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

[Julien Grall <julien.grall@arm.com>]

On 14/02/18 19:14, Mirela Simonovic wrote:
> Hi Julien,

Hi,

> On 02/13/2018 12:44 AM, Julien Grall wrote:
>>
>>
>> On 12/02/2018 23:16, Mirela Simonovic wrote:
>>> Hi Julien,
>>
>> Hi,
>>
>>> 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).
>>
>> Looking at the spec:
>>
>> "2 Trusted OS is either not present or does not require migration. A 
>> system of this type does not require the caller to use the MIGRATE 
>> function. MIGRATE function calls return NOT_SUPPORTED."
>>
>> So returning 2 in our case seems to be valid.
>>
>>>
>>> 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...
>> I disagree, not all PSCI_INVALID_PARAMETERS should be replaced by 
>> PSCI_INVALID_ADDRESS. They have two distinct meaning. However, I am 
>> not sure where we would need to use it in Xen. The error is described 
>> as "INVALID_ADDRESS is returned when the entry point address is known 
>> by the implementation to be invalid, because it is in a range that is 
>> known not to be available to the caller."
>>
>> The only potential one would be the check on is_thumb, but even there 
>> it does not match the description. The range is still available to the 
>> guest. I think that check should just be dropped.
> 
> To be more specific, I was thinking that in xen/arch/arm/vpsci.c line 41 
> for psci version other than 0.1 the PSCI_INVALID_ADDRESS error should be 
> returned instead PSCI_INVALID_PARAMETERS.

This is exactly the place I was speaking in my previous e-mail. I am not 
entirely convinced we should keep the check or even switch the return to 
PSCI_INVALID_PARAMETERS as the usage does not entirely match the error 
description.

Cheers,

-- 
Julien Grall

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