Re: [Xen-devel] [PATCH v5] xen/arm : emulation of arm's PSCI v0.2 standard

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-07-17 at 22:11 +0530, Parth Dixit wrote:
> Changelog v5
> - removed PSCI_ARGS macro
> - preload error value for calls that do not return
> - added new macro for 32 bit arguments
> - casting return calls to appropriate types
> - added new function psci_mode_check
> - added error checks for PSCI v0.2 function id's

Looking good. I've a couple of questions which I hope you can answer.

> +static void do_trap_psci(struct cpu_user_regs *regs)
> +{
> +    register_t fid = PSCI_ARG(regs,0);
>  
> -    psci_call = arm_psci_table[PSCI_OP_REG(regs)].fn;
> -    if ( psci_call == NULL )
> +    /* preloading in case psci_mode_check fails */
> +    PSCI_RESULT_REG(regs) = (int32_t)PSCI_INVALID_PARAMETERS;
> [...]
> +    case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU:
> +        if ( psci_mode_check(current->domain, fid) )
> +            PSCI_RESULT_REG(regs) =
> +               (uint32_t) do_psci_0_2_migrate_info_up_cpu();
> +        else
> +            PSCI_RESULT_REG(regs) = (uint32_t)PSCI_INVALID_PARAMETERS;
> +        break;
[...]
> +    case PSCI_0_2_FN_CPU_ON:
> +    case PSCI_0_2_FN64_CPU_ON:
> +        if ( psci_mode_check(current->domain, fid) )
> +        {
> +        register_t vcpuid = PSCI_ARG(regs,1);
> +        register_t epoint = PSCI_ARG(regs,2);
> +        register_t cid = PSCI_ARG(regs,3);
> +        PSCI_RESULT_REG(regs) =
> +            (int32_t) do_psci_0_2_cpu_on(vcpuid, epoint, cid);

Indentation here is broken.

I notice that sometimes you have an else case setting
PSCI_INVALID_PARAMETERS and other times you don't. Is that just an
oversight or is there a reason why they should differ?

I don't think you need to cast the PSCI_* result codes BTW.

[...]
> +int32_t do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point,
> +                            register_t context_id)
> +{
> +    struct vcpu *v = current;
> +    struct domain *d = v->domain;
> +    struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs;

I think it would make sense to pass this as an argument, but if not then
guest_cpu_user_regs() is the accessor to use.

> +
> +    if ( is_32bit_domain(d) )
> +    {
> +        regs->pc32 = entry_point;
> +        regs->r0 = context_id;
> +    }
> +#ifdef CONFIG_ARM_64
> +    else
> +    {
> +        regs->pc = entry_point;
> +        regs->x0 = context_id;
> +    }
> +#endif
> +    vcpu_block_unless_event_pending(v);
> +    return PSCI_SUCCESS;

This only seems to be handling the case where the requested power state
is power down and not the standby state.

By my reading when bit 16 if the power_state argument is 0 (standby)
then entry_point and context_id are ignored and the PSCI call is
expected to return success to the place it was called from, not jump to
entry_point.

If bit 16 is one you are supposed to return to entry_point with x0/r0
set to context_id, but because you return PSCI_SUCCESS that will get
written to x0/r0 by the generic handler clobbering your write of x0/r0
above. You should probably return the desired value (with an explanatory
comment)

The upper bits of power_state also describe an affinity level to put to
sleep, which you also don't handle AFAICT. Perhaps since we don't
actually have affinities > 0 what you've done is OK, what do you think?

The lower bits are supposed to be some sort of per platform ID which we
are supposed to have exposed via tables. I'm not sure there are any DTB
bindings for that, so perhaps we can ignore it.

The handling of bit 16 is the only bit which really concerns me here,
the rest seem like things we can tolerate.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel