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
