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

[Julien Grall <julien.grall@xxxxxxx>]

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.
Cheers,

--
Julien Grall

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