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

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel