Re: [PATCH v7 2/2] ARM: ftrace: Add MODULE_PLTS support

[Qais Yousef]

Hey Alexander

On 03/24/21 10:04, Alexander Sverdlin wrote:
> Hi Qais,
> 
> On 23/03/2021 23:22, Qais Yousef wrote:
> >>> Yes you're right. I was a bit optimistic on CONFIG_DYNAMIC_FTRACE will imply
> >>> CONFIG_ARM_MODULE_PLTS is enabled too.
> >>>
> >>> It only has an impact on reducing ifdefery when calling
> >>>
> >>> 	ftrace_call_replace_mod(rec->arch.mod, ...)
> >>>
> >>> Should be easy to wrap rec->arch.mod with its own accessor that will return
> >>> NULL if !CONFIG_ARM_MODULE_PLTS or just ifdef the functions.
> >>>
> >>> Up to Alexander to pick what he prefers :-)
> >> well, I of course prefer v7 as-is, because this review is running longer than two
> >> years and I actually hope these patches to be finally merged at some point.
> >> But you are welcome to optimize them with follow up patches :)
> > I appreciate that and thanks a lot for your effort. My attempt to review and
> > test here is to help in getting this merged.
> > 
> > FWIW my main concern is about duplicating the range check in
> > ftrace_call_replace() and using magic values that already exist in
> > __arm_gen_branch_{arm, thumb2}() and better remain encapsulated there.
> 
> could you please check the negative limits? I have an opinion, my limits are
> correct. I could add extra parameter to arm_gen_branch_link(), but for this
> I first need to fix its negative limits, which, I believe, well... Approximate :)

Can you elaborate please?

If you look at Arm ARM [1] the ranges are defined in page 347

	Encoding T1 Even numbers in the range –16777216 to 16777214.
	Encoding T2 Multiples of 4 in the range –16777216 to 16777212.
	Encoding A1 Multiples of 4 in the range –33554432 to 33554428.
	Encoding A2 Even numbers in the range –33554432 to 33554430.

which matches what's in the code (T1 for thumb2 and A1 for arm).

Why do you think it's wrong?

Thanks

--
Qais Yousef

[1] https://developer.arm.com/documentation/ddi0406/latest/