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

From: Julien Grall <julien.grall@arm.com>
To: Volodymyr Babchuk <volodymyr_babchuk@epam.com>, xen-devel@lists.xen.org
Cc: sstabellini@kernel.org
Subject: Re: [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention
Date: Tue, 28 Aug 2018 16:57:03 +0100	[thread overview]
Message-ID: <18febf9b-7ad5-2238-e0ad-9d27e48e016b@arm.com> (raw)
In-Reply-To: <76a634d3-b0b6-76fe-c265-bbe414b9b51a@epam.com>

On 28/08/18 16:50, Volodymyr Babchuk wrote:
> Hi Julien,

Hi,

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

Spectre would not have existed if the branch predictor was so easy ;).

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

There are no need to have likely/unlikely in arm_smccc_smc if it is 
already present in cpus_have_const_cap.

Cheers,

-- 
Julien Grall

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