Re: [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention

[Volodymyr Babchuk <volodymyr_babchuk@epam.com>]

Hi Julien,

On 28.08.18 18:27, Julien Grall wrote:
> Hi Volodymyr,
> 
> On 28/08/18 16:10, Volodymyr Babchuk wrote:
>>
>>
>> On 28.08.18 17:43, Julien Grall wrote:
>> [...]
>>
>>>
>>>>> I have looked at cpus_have_const_cap() and haven't found good way 
>>>>> to optimize it with the current infrastructure in Xen. Feel free to 
>>>>> suggest improvement.
>>>>
>>>> Another thing: maybe it is worth to branch to 1.0 code and leave 1.1 
>>>> in a straight path of execution? This will save you one more 
>>>> instruction for SMCCC 1.1 call.
>>>
>>> I am not sure to understand your suggestion here. Could you expand?
>>
>> +#define arm_smccc_smc(...)                                      \
>> +    do {                                                        \
>> +        if ( cpus_have_const_cap(ARM_SMCCC_1_1) )              \
>> +            arm_smccc_1_1_smc(__VA_ARGS__);                     \
>> +        else                                                    \
>> +            arm_smccc_1_0_smc(__VA_ARGS__);                     \
>> +    } while ( 0 )
>>
>>
>>> However, why SMCCC 1.1 should be in the straight path of execution?
>>
>> It is easier to read - no negation in if(). 
> 
> I can do that. I will also probably add an unlikely in 
> cpus_have_const_cap(...).
That would  be great.

> 
>> Also, I think, branch predictor would be happy. At least, if the 
>> following is true:
>>
>> " If the branch information is not contained in the BTAC, static 
>> branch prediction is used, whereby we assume the branch will be taken 
>> if the branch is a conditional backwards branch or not taken if the 
>> branch is a conditional forwards branch." [1]
> 
> The Arm Arm does not provide any details on the branch predictor 
> implementation. An implementer is free to do whatever he wants and could 
> design a branch predicator doing the opposite as this statement.
Generally speaking - yes. But, AFAIK, most static predictors behave in 
the way described above. Anyways, as you pointed below, better to leave 
hint for the compiler.

> 
> You also can't assume how the compiler will compile the code, it may end 
> up to generate the else branch first because it is predicted to be taken 
> more often. This is why GCC provide __builtin_expect (commonly used as 
> unlikely/likely) to influence the compiler choice for branch prediction.
Yes, this is the good point. So, you can add likely/unlikely not only in 
cpus_have_const_cap(...) but also in #define arm_smccc_smc(...)

-- 
Volodymyr Babchuk

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