[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi Julien,


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

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

Thanks,
Mirela


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,



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.