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

From: Julien Grall <julien.grall@arm.com>
To: Mirela Simonovic <mirela.simonovic@aggios.com>, xen-devel@lists.xen.org
Cc: sstabellini@kernel.org, Wei Liu <wei.liu2@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	andre.przywara@linaro.org, nd@arm.com,
	volodymyr_babchuk@epam.com
Subject: Re: [PATCH v2 02/15] xen/arm: vpsci: Add support for PSCI 1.1
Date: Mon, 12 Feb 2018 21:41:48 +0000	[thread overview]
Message-ID: <7606535b-543c-f09b-dc67-b57f063aac9c@arm.com> (raw)
In-Reply-To: <447c3870-2bf7-cc5d-1d60-7ce5f4052eb6@aggios.com>

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