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

[Parth Dixit <parth.dixit@xxxxxxxxxx>]

forgot to add xen-devel list...

---------- Forwarded message ----------
From: Parth Dixit <parth.dixit@xxxxxxxxxx>
Date: 25 July 2014 11:05
Subject: Re: [PATCH v5] xen/arm : emulation of arm's PSCI v0.2 standard
To: Ian Campbell <Ian.Campbell@xxxxxxxxxx>


On 24 July 2014 19:36, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> 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 will fix in patchset v6
>
> 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?
It was done because rest of the functions were using default preloaded
value with return type int32 , functions with else case are functions
that have return type other than int32(uint32 or register_t)  but as
you have commented further if PSCI* result codes do not require
casting, this will go away.
To summarize i will remove the casting of result codes this will also
do away the need of else statements in patchset v6.
> 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.
>
sure, will take care in patchset v6
>> +
>> +    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)
valid point, i am just wondering why kvm implementation ignored these values,
i will implement it in next patchset
> 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?
I think it is better to put a comment explicitly stating that we have
ignored the value because affinities > 0 are not valid for xen or we
can check for affinity and return if it is greater than zero.
I think i'll go with check, will take care in next patchset
> 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.
>
Thanks
parth

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