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 17:05, Julien Grall wrote:
> Hi Volodymyr,
> 
> On 27/08/18 17:50, Volodymyr Babchuk wrote:
>> On 27.08.18 18:29, Julien Grall wrote:
>>> On 27/08/2018 15:15, Volodymyr Babchuk wrote:
>>>> On 24.08.18 19:58, Julien Grall wrote:
>>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>> This is indeed increasing the size of the function, but with a better 
>>> performance when you perform an SMC with 1.1.
>>>
>>> The main goal of SMCCC 1.1 is to limit the number of registers saved 
>>> when calling an SMC. So if you provide provide a wrapper using a 
>>> function, then you are better off sticking with SMCCC 1.0.
>>>
>>> The idea of this code is to provide a faster call on the presence of 
>>> SMCCC 1.1 while providing a fallback for other case. The function 
>>> cpus_have_const_cap is implemented using an ALTERNATIVE (similar to 
>>> static key) that will make if close to a NOP. I am saying close 
>>> because this is not quite a static key as we don't have it in Xen. 
>>> Instead, you replace a memory load by a constant.
>> Ah, yes, true.
>>
>> I checked how static keys are working. Seems, they use asm goto 
>> feature. Now I think: can we optimize this more? Something like that:
>>
>> #define arm_smccc_smc(...)
>>      do {
>>
>>              asm goto (ALTERNATIVE(nop",
>>                                    "b %l[smccc_1_1]",
>>                                    ARM_SMCCC_1_1));
> 
> You would need to list the label in GotoLabels (see 6.45.2.7 [1]).
Yes, but as you said, we can use __LINE__ there.

>>              arm_smccc_1_0_smc(__VA_ARGS__);
>>              break;
>> smccc_1_1:
> The label will get redefined if you have multiple SMC call in the same 
> function. We could possibly generate label based on the file/line.
> 
>>              arm_smccc_1_1_smc(__VA_ARGS__);
>>     } while ( 0 )
>>
>> This will save use additional conditional branch.
> The code with this patch looks like:
> 
> mov x0, #0        mov x0, #1
> cbz w0, label        cbz w0, label
> 
> The mov + conditional branch is likely to be negligible over the cost of 
> switching to EL3. Overall, I am not really convinced that it would be 
> worth the cost of open-coding it. I would prefer if we keep the call to 
> cpus_have_const_cap() and see if it can be optimized.
Okay. I'm fine with this. I just saw one more way to optimize and wanted
to share it with you. Anyways:

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

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

-- 
Volodymyr Babchuk

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