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

[Julien Grall <julien.grall@xxxxxxx>]

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