| Message ID | 20180723150756.11108-1-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 06d0bbc6d0f56dacac3a79900e9a9a0d5972d818 |
| Headers | show |
| Series | [1/5] powerpc/asm: Add a patch_site macro & helpers for patching instructions | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | fail | Test checkpatch on branch next |
On Mon, 2018-07-23 at 15:07:52 UTC, Michael Ellerman wrote: > Add a macro and some helper C functions for patching single asm > instructions. > > The gas macro means we can do something like: > > 1: nop > patch_site 1b, patch__foo > > Which is less visually distracting than defining a GLOBAL symbol at 1, > and also doesn't pollute the symbol table which can confuse eg. perf. > > These are obviously similar to our existing feature sections, but are > not automatically patched based on CPU/MMU features, rather they are > designed to be manually patched by C code at some arbitrary point. > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> Series applied to powerpc next. https://git.kernel.org/powerpc/c/06d0bbc6d0f56dacac3a79900e9a9a cheers
Le 23/07/2018 à 17:07, Michael Ellerman a écrit : > Add a macro and some helper C functions for patching single asm > instructions. > > The gas macro means we can do something like: > > 1: nop > patch_site 1b, patch__foo > > Which is less visually distracting than defining a GLOBAL symbol at 1, > and also doesn't pollute the symbol table which can confuse eg. perf. > > These are obviously similar to our existing feature sections, but are > not automatically patched based on CPU/MMU features, rather they are > designed to be manually patched by C code at some arbitrary point. > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > --- > arch/powerpc/include/asm/code-patching-asm.h | 18 ++++++++++++++++++ > arch/powerpc/include/asm/code-patching.h | 2 ++ > arch/powerpc/lib/code-patching.c | 16 ++++++++++++++++ > 3 files changed, 36 insertions(+) > create mode 100644 arch/powerpc/include/asm/code-patching-asm.h > > diff --git a/arch/powerpc/include/asm/code-patching-asm.h b/arch/powerpc/include/asm/code-patching-asm.h > new file mode 100644 > index 000000000000..ed7b1448493a > --- /dev/null > +++ b/arch/powerpc/include/asm/code-patching-asm.h > @@ -0,0 +1,18 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > +/* > + * Copyright 2018, Michael Ellerman, IBM Corporation. > + */ > +#ifndef _ASM_POWERPC_CODE_PATCHING_ASM_H > +#define _ASM_POWERPC_CODE_PATCHING_ASM_H > + > +/* Define a "site" that can be patched */ > +.macro patch_site label name > + .pushsection ".rodata" > + .balign 4 > + .global \name > +\name: > + .4byte \label - . > + .popsection > +.endm > + > +#endif /* _ASM_POWERPC_CODE_PATCHING_ASM_H */ > diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h > index 812535f40124..b2051234ada8 100644 > --- a/arch/powerpc/include/asm/code-patching.h > +++ b/arch/powerpc/include/asm/code-patching.h > @@ -32,6 +32,8 @@ unsigned int create_cond_branch(const unsigned int *addr, > int patch_branch(unsigned int *addr, unsigned long target, int flags); > int patch_instruction(unsigned int *addr, unsigned int instr); > int raw_patch_instruction(unsigned int *addr, unsigned int instr); > +int patch_instruction_site(s32 *addr, unsigned int instr); > +int patch_branch_site(s32 *site, unsigned long target, int flags); Why use s32* instead of unsigned int* as usual for pointer to code ? Christophe > > int instr_is_relative_branch(unsigned int instr); > int instr_is_relative_link_branch(unsigned int instr); > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c > index e0d881ab304e..850f3b8f4da5 100644 > --- a/arch/powerpc/lib/code-patching.c > +++ b/arch/powerpc/lib/code-patching.c > @@ -195,6 +195,22 @@ int patch_branch(unsigned int *addr, unsigned long target, int flags) > return patch_instruction(addr, create_branch(addr, target, flags)); > } > > +int patch_branch_site(s32 *site, unsigned long target, int flags) > +{ > + unsigned int *addr; > + > + addr = (unsigned int *)((unsigned long)site + *site); > + return patch_instruction(addr, create_branch(addr, target, flags)); > +} > + > +int patch_instruction_site(s32 *site, unsigned int instr) > +{ > + unsigned int *addr; > + > + addr = (unsigned int *)((unsigned long)site + *site); > + return patch_instruction(addr, instr); > +} > + > bool is_offset_in_branch_range(long offset) > { > /* >
Le 08/08/2018 à 18:30, Christophe LEROY a écrit : > > > Le 23/07/2018 à 17:07, Michael Ellerman a écrit : >> Add a macro and some helper C functions for patching single asm >> instructions. >> >> The gas macro means we can do something like: >> >> 1: nop >> patch_site 1b, patch__foo >> >> Which is less visually distracting than defining a GLOBAL symbol at 1, >> and also doesn't pollute the symbol table which can confuse eg. perf. >> >> These are obviously similar to our existing feature sections, but are >> not automatically patched based on CPU/MMU features, rather they are >> designed to be manually patched by C code at some arbitrary point. >> >> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> >> --- >> arch/powerpc/include/asm/code-patching-asm.h | 18 ++++++++++++++++++ >> arch/powerpc/include/asm/code-patching.h | 2 ++ >> arch/powerpc/lib/code-patching.c | 16 ++++++++++++++++ >> 3 files changed, 36 insertions(+) >> create mode 100644 arch/powerpc/include/asm/code-patching-asm.h >> >> diff --git a/arch/powerpc/include/asm/code-patching-asm.h >> b/arch/powerpc/include/asm/code-patching-asm.h >> new file mode 100644 >> index 000000000000..ed7b1448493a >> --- /dev/null >> +++ b/arch/powerpc/include/asm/code-patching-asm.h >> @@ -0,0 +1,18 @@ >> +/* SPDX-License-Identifier: GPL-2.0+ */ >> +/* >> + * Copyright 2018, Michael Ellerman, IBM Corporation. >> + */ >> +#ifndef _ASM_POWERPC_CODE_PATCHING_ASM_H >> +#define _ASM_POWERPC_CODE_PATCHING_ASM_H >> + >> +/* Define a "site" that can be patched */ >> +.macro patch_site label name >> + .pushsection ".rodata" >> + .balign 4 >> + .global \name >> +\name: >> + .4byte \label - . >> + .popsection >> +.endm >> + >> +#endif /* _ASM_POWERPC_CODE_PATCHING_ASM_H */ >> diff --git a/arch/powerpc/include/asm/code-patching.h >> b/arch/powerpc/include/asm/code-patching.h >> index 812535f40124..b2051234ada8 100644 >> --- a/arch/powerpc/include/asm/code-patching.h >> +++ b/arch/powerpc/include/asm/code-patching.h >> @@ -32,6 +32,8 @@ unsigned int create_cond_branch(const unsigned int >> *addr, >> int patch_branch(unsigned int *addr, unsigned long target, int flags); >> int patch_instruction(unsigned int *addr, unsigned int instr); >> int raw_patch_instruction(unsigned int *addr, unsigned int instr); >> +int patch_instruction_site(s32 *addr, unsigned int instr); >> +int patch_branch_site(s32 *site, unsigned long target, int flags); > > Why use s32* instead of unsigned int* as usual for pointer to code ? Forget my stupid question, I didn't see it was a relative address and not an absolute one. Christophe > > Christophe > >> int instr_is_relative_branch(unsigned int instr); >> int instr_is_relative_link_branch(unsigned int instr); >> diff --git a/arch/powerpc/lib/code-patching.c >> b/arch/powerpc/lib/code-patching.c >> index e0d881ab304e..850f3b8f4da5 100644 >> --- a/arch/powerpc/lib/code-patching.c >> +++ b/arch/powerpc/lib/code-patching.c >> @@ -195,6 +195,22 @@ int patch_branch(unsigned int *addr, unsigned >> long target, int flags) >> return patch_instruction(addr, create_branch(addr, target, flags)); >> } >> +int patch_branch_site(s32 *site, unsigned long target, int flags) >> +{ >> + unsigned int *addr; >> + >> + addr = (unsigned int *)((unsigned long)site + *site); >> + return patch_instruction(addr, create_branch(addr, target, flags)); >> +} >> + >> +int patch_instruction_site(s32 *site, unsigned int instr) >> +{ >> + unsigned int *addr; >> + >> + addr = (unsigned int *)((unsigned long)site + *site); >> + return patch_instruction(addr, instr); >> +} >> + >> bool is_offset_in_branch_range(long offset) >> { >> /* >>
Christophe LEROY <christophe.leroy@c-s.fr> writes: > Le 08/08/2018 à 18:30, Christophe LEROY a écrit : >> Le 23/07/2018 à 17:07, Michael Ellerman a écrit : ... >>> diff --git a/arch/powerpc/include/asm/code-patching.h >>> b/arch/powerpc/include/asm/code-patching.h >>> index 812535f40124..b2051234ada8 100644 >>> --- a/arch/powerpc/include/asm/code-patching.h >>> +++ b/arch/powerpc/include/asm/code-patching.h >>> @@ -32,6 +32,8 @@ unsigned int create_cond_branch(const unsigned int >>> *addr, >>> int patch_branch(unsigned int *addr, unsigned long target, int flags); >>> int patch_instruction(unsigned int *addr, unsigned int instr); >>> int raw_patch_instruction(unsigned int *addr, unsigned int instr); >>> +int patch_instruction_site(s32 *addr, unsigned int instr); >>> +int patch_branch_site(s32 *site, unsigned long target, int flags); >> >> Why use s32* instead of unsigned int* as usual for pointer to code ? > > Forget my stupid question, I didn't see it was a relative address and > not an absolute one. No worries. It is a bit non-obvious at first glance, it looks like the s32 * points to the instruction. But it points to the s32 that holds the relative offset from itself, of the instruction. We could add a typedef to try and make that more obvious, but I generally don't like typedefs that hide pointerness. cheers
diff --git a/arch/powerpc/include/asm/code-patching-asm.h b/arch/powerpc/include/asm/code-patching-asm.h new file mode 100644 index 000000000000..ed7b1448493a --- /dev/null +++ b/arch/powerpc/include/asm/code-patching-asm.h @@ -0,0 +1,18 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Copyright 2018, Michael Ellerman, IBM Corporation. + */ +#ifndef _ASM_POWERPC_CODE_PATCHING_ASM_H +#define _ASM_POWERPC_CODE_PATCHING_ASM_H + +/* Define a "site" that can be patched */ +.macro patch_site label name + .pushsection ".rodata" + .balign 4 + .global \name +\name: + .4byte \label - . + .popsection +.endm + +#endif /* _ASM_POWERPC_CODE_PATCHING_ASM_H */ diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h index 812535f40124..b2051234ada8 100644 --- a/arch/powerpc/include/asm/code-patching.h +++ b/arch/powerpc/include/asm/code-patching.h @@ -32,6 +32,8 @@ unsigned int create_cond_branch(const unsigned int *addr, int patch_branch(unsigned int *addr, unsigned long target, int flags); int patch_instruction(unsigned int *addr, unsigned int instr); int raw_patch_instruction(unsigned int *addr, unsigned int instr); +int patch_instruction_site(s32 *addr, unsigned int instr); +int patch_branch_site(s32 *site, unsigned long target, int flags); int instr_is_relative_branch(unsigned int instr); int instr_is_relative_link_branch(unsigned int instr); diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c index e0d881ab304e..850f3b8f4da5 100644 --- a/arch/powerpc/lib/code-patching.c +++ b/arch/powerpc/lib/code-patching.c @@ -195,6 +195,22 @@ int patch_branch(unsigned int *addr, unsigned long target, int flags) return patch_instruction(addr, create_branch(addr, target, flags)); } +int patch_branch_site(s32 *site, unsigned long target, int flags) +{ + unsigned int *addr; + + addr = (unsigned int *)((unsigned long)site + *site); + return patch_instruction(addr, create_branch(addr, target, flags)); +} + +int patch_instruction_site(s32 *site, unsigned int instr) +{ + unsigned int *addr; + + addr = (unsigned int *)((unsigned long)site + *site); + return patch_instruction(addr, instr); +} + bool is_offset_in_branch_range(long offset) { /*
Add a macro and some helper C functions for patching single asm instructions. The gas macro means we can do something like: 1: nop patch_site 1b, patch__foo Which is less visually distracting than defining a GLOBAL symbol at 1, and also doesn't pollute the symbol table which can confuse eg. perf. These are obviously similar to our existing feature sections, but are not automatically patched based on CPU/MMU features, rather they are designed to be manually patched by C code at some arbitrary point. Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> --- arch/powerpc/include/asm/code-patching-asm.h | 18 ++++++++++++++++++ arch/powerpc/include/asm/code-patching.h | 2 ++ arch/powerpc/lib/code-patching.c | 16 ++++++++++++++++ 3 files changed, 36 insertions(+) create mode 100644 arch/powerpc/include/asm/code-patching-asm.h