Re: [PATCH 6/6] livepatch v5: clean up usage of the old and new addresses

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Wed, Dec 10, 2014 at 02:50:54PM +0100, Petr Mladek wrote:
> On Tue 2014-12-09 13:20:09, Josh Poimboeuf wrote:
> > On Tue, Dec 09, 2014 at 07:05:07PM +0100, Petr Mladek wrote:
> > > The situation with variables storing the address of the old and new code
> > > is not ideal. One is called "func" and the other is called "addr".
> > > The one is pointer and the other is unsigned long. It makes sense
> > > from the point of how the values are defined. But it might make problems
> > > to understand the code when the values are used.
> > >
> > > This patch introduces two new variables "old_ip" and "new_ip".
> > > They have the same type and the name is symmetric.
> > 
> > I agree that the naming of addr vs func is a little weird, but I find
> > that this patch makes the code more confusing.  Now we have "func",
> > "addr" and "ip", all different words meaning "function address".
> >
> > Adding to the confusion is the existence of four variables instead of
> > two.
> 
> Yup, I am not super happy about it as well. This is why I made this
> change in the last patch, so that it is easier to ignore or rework.
> 
> Another solution would be to rename either new_func -> new_addr or
> old_addr -> old_func. In this case, they should have the same type.
> 
> "unsigned long" is used on most locations, so I would prefer this
> type. And it better fits with the *_addr names.
> 
> The problem is that it would mean to cast the pointer to the new
> function in hand-made patches. But we could hide this under some macro
> that would be handy anyway.
> 
> Ot we could leave it as is for now. I do not have strong opinion
> about it.

Ok, I'd rather leave it as it is for now.

> > > They are supposed to carry the address of the mcount call-site that
> > > ftrace framework operates with. The value is the same as the function
> > > address on x86 but it might be different on another architectures.
> >
> > I didn't know the mcount call site address can differ from the function
> > address for some architectures.  Can you give more details about this?
> 
> Only fentry is put at the beginning of the functions. The classic
> mcount call is put after the function prologue.
> 
> AFAIK, fentry is supported only on x86_64. There is another usable
> feature on s390x that can be enabled by -mhotpatch gcc option but
> we do not use it with kGraft. Vojtech/JiriK told me that the important
> code is at the beginning of the function on PPC. But I am pretty
> sure that there will be architectures where the code won't be at
> the beginning.
> 
> The current implementation supports only x86_64. s390 and PPC seems
> to be solvable without the shifted address. I am not sure if we
> want to make it ready for other architectures or keep is simple for
> now.

Yeah, I'd say let's keep the code simple for now until we need the added
complexity.

[...]
> > > +	func->new_ip = ftrace_location((unsigned long)func->new_func);
> > > +	if (!func->old_ip) {
> > > +		pr_err("function at the address '%p' can not be used for patching\n",
> > > +		       func->new_func);
> > > +		return -EINVAL;
> > > +	}
> > 
> > Why does the new function need to have an mcount call site?  And why do
> > we want to use that address rather than the real function address?

I think you missed this question, but it doesn't matter since we can
skip this patch for now anyway.

Thanks,
Josh