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

--
Julien Grall

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