Re: [PATCH v5 09/21] powerpc: Use a datatype for instructions
On Tue, Apr 7, 2020 at 8:30 PM Balamuruhan S wrote:
>
> On Mon, 2020-04-06 at 18:09 +1000, Jordan Niethe wrote:
> > Currently unsigned ints are used to represent instructions on powerpc.
> > This has worked well as instructions have always been 4 byte words.
> > However, a future ISA version will introduce some changes to
> > instructions that mean this scheme will no longer work as well. This
> > change is Prefixed Instructions. A prefixed instruction is made up of a
> > word prefix followed by a word suffix to make an 8 byte double word
> > instruction. No matter the endianess of the system the prefix always
> > comes first. Prefixed instructions are only planned for powerpc64.
> >
> > Introduce a ppc_inst type to represent both prefixed and word
> > instructions on powerpc64 while keeping it possible to exclusively have
> > word instructions on powerpc32, A latter patch will expand the type to
> > include prefixed instructions but for now just typedef it to a u32.
> >
> > Later patches will introduce helper functions and macros for
> > manipulating the instructions so that powerpc64 and powerpc32 might
> > maintain separate type definitions.
> >
> > Signed-off-by: Jordan Niethe
> > ---
> > v4: New to series
> > v5: Add to epapr_paravirt.c, kgdb.c
> > ---
> > arch/powerpc/include/asm/code-patching.h | 32 -
> > arch/powerpc/include/asm/inst.h | 20 +++---
> > arch/powerpc/include/asm/sstep.h | 5 +-
> > arch/powerpc/include/asm/uprobes.h | 5 +-
> > arch/powerpc/kernel/align.c | 4 +-
> > arch/powerpc/kernel/epapr_paravirt.c | 4 +-
> > arch/powerpc/kernel/hw_breakpoint.c | 4 +-
> > arch/powerpc/kernel/jump_label.c | 2 +-
> > arch/powerpc/kernel/kgdb.c | 4 +-
> > arch/powerpc/kernel/kprobes.c| 8 +--
> > arch/powerpc/kernel/mce_power.c | 5 +-
> > arch/powerpc/kernel/optprobes.c | 40 ++--
> > arch/powerpc/kernel/setup_32.c | 2 +-
> > arch/powerpc/kernel/trace/ftrace.c | 83
> > arch/powerpc/kernel/vecemu.c | 5 +-
> > arch/powerpc/lib/code-patching.c | 69 ++--
> > arch/powerpc/lib/feature-fixups.c| 48 +++---
> > arch/powerpc/lib/sstep.c | 4 +-
> > arch/powerpc/lib/test_emulate_step.c | 9 +--
> > arch/powerpc/perf/core-book3s.c | 4 +-
> > arch/powerpc/xmon/xmon.c | 24 +++
> > 21 files changed, 196 insertions(+), 185 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/code-patching.h
> > b/arch/powerpc/include/asm/code-patching.h
> > index 48e021957ee5..eacc9102c251 100644
> > --- a/arch/powerpc/include/asm/code-patching.h
> > +++ b/arch/powerpc/include/asm/code-patching.h
> > @@ -23,33 +23,33 @@
> > #define BRANCH_ABSOLUTE 0x2
> >
> > bool is_offset_in_branch_range(long offset);
> > -int create_branch(unsigned int *instr, const unsigned int *addr,
> > +int create_branch(struct ppc_inst *instr, const struct ppc_inst *addr,
> > unsigned long target, int flags);
> > -int create_cond_branch(unsigned int *instr, const unsigned int *addr,
> > +int create_cond_branch(struct ppc_inst *instr, const struct ppc_inst *addr,
> > unsigned long target, int flags);
> > -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_branch(struct ppc_inst *addr, unsigned long target, int flags);
> > +int patch_instruction(struct ppc_inst *addr, struct ppc_inst instr);
> > +int raw_patch_instruction(struct ppc_inst *addr, struct ppc_inst instr);
> >
> > static inline unsigned long patch_site_addr(s32 *site)
> > {
> > return (unsigned long)site + *site;
> > }
> >
> > -static inline int patch_instruction_site(s32 *site, unsigned int instr)
> > +static inline int patch_instruction_site(s32 *site, struct ppc_inst instr)
> > {
> > - return patch_instruction((unsigned int *)patch_site_addr(site),
> > instr);
> > + return patch_instruction((struct ppc_inst *)patch_site_addr(site),
> > instr);
> > }
> >
> > static inline int patch_branch_site(s32 *site, unsigned long target, int
> > flags)
> > {
> > - return patch_branch((unsigned int *)patch_site_addr(site), target,
> > flags);
> > + return patch_branch((struct ppc_inst *)patch_site_addr(site), target,
> > flags);
> > }
> >
> > static inline int modify_instruction(unsigned int *addr, unsigned int clr,
> >unsigned int set)
> > {
> > - return patch_instruction(addr, ppc_inst((*addr & ~clr) | set));
> > + return patch_instruction((struct ppc_inst *)addr, ppc_inst((*addr &
> > ~clr) | set));
> > }
> >
> > static inline int modify_instruction_site(s32 *site, unsigned int clr,
> > unsigned int set)
> > @@ -57,13 +57,13
Re: [PATCH v5 09/21] powerpc: Use a datatype for instructions
On Mon, 2020-04-06 at 18:09 +1000, Jordan Niethe wrote:
> Currently unsigned ints are used to represent instructions on powerpc.
> This has worked well as instructions have always been 4 byte words.
> However, a future ISA version will introduce some changes to
> instructions that mean this scheme will no longer work as well. This
> change is Prefixed Instructions. A prefixed instruction is made up of a
> word prefix followed by a word suffix to make an 8 byte double word
> instruction. No matter the endianess of the system the prefix always
> comes first. Prefixed instructions are only planned for powerpc64.
>
> Introduce a ppc_inst type to represent both prefixed and word
> instructions on powerpc64 while keeping it possible to exclusively have
> word instructions on powerpc32, A latter patch will expand the type to
> include prefixed instructions but for now just typedef it to a u32.
>
> Later patches will introduce helper functions and macros for
> manipulating the instructions so that powerpc64 and powerpc32 might
> maintain separate type definitions.
>
> Signed-off-by: Jordan Niethe
> ---
> v4: New to series
> v5: Add to epapr_paravirt.c, kgdb.c
> ---
> arch/powerpc/include/asm/code-patching.h | 32 -
> arch/powerpc/include/asm/inst.h | 20 +++---
> arch/powerpc/include/asm/sstep.h | 5 +-
> arch/powerpc/include/asm/uprobes.h | 5 +-
> arch/powerpc/kernel/align.c | 4 +-
> arch/powerpc/kernel/epapr_paravirt.c | 4 +-
> arch/powerpc/kernel/hw_breakpoint.c | 4 +-
> arch/powerpc/kernel/jump_label.c | 2 +-
> arch/powerpc/kernel/kgdb.c | 4 +-
> arch/powerpc/kernel/kprobes.c| 8 +--
> arch/powerpc/kernel/mce_power.c | 5 +-
> arch/powerpc/kernel/optprobes.c | 40 ++--
> arch/powerpc/kernel/setup_32.c | 2 +-
> arch/powerpc/kernel/trace/ftrace.c | 83
> arch/powerpc/kernel/vecemu.c | 5 +-
> arch/powerpc/lib/code-patching.c | 69 ++--
> arch/powerpc/lib/feature-fixups.c| 48 +++---
> arch/powerpc/lib/sstep.c | 4 +-
> arch/powerpc/lib/test_emulate_step.c | 9 +--
> arch/powerpc/perf/core-book3s.c | 4 +-
> arch/powerpc/xmon/xmon.c | 24 +++
> 21 files changed, 196 insertions(+), 185 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/code-patching.h
> b/arch/powerpc/include/asm/code-patching.h
> index 48e021957ee5..eacc9102c251 100644
> --- a/arch/powerpc/include/asm/code-patching.h
> +++ b/arch/powerpc/include/asm/code-patching.h
> @@ -23,33 +23,33 @@
> #define BRANCH_ABSOLUTE 0x2
>
> bool is_offset_in_branch_range(long offset);
> -int create_branch(unsigned int *instr, const unsigned int *addr,
> +int create_branch(struct ppc_inst *instr, const struct ppc_inst *addr,
> unsigned long target, int flags);
> -int create_cond_branch(unsigned int *instr, const unsigned int *addr,
> +int create_cond_branch(struct ppc_inst *instr, const struct ppc_inst *addr,
> unsigned long target, int flags);
> -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_branch(struct ppc_inst *addr, unsigned long target, int flags);
> +int patch_instruction(struct ppc_inst *addr, struct ppc_inst instr);
> +int raw_patch_instruction(struct ppc_inst *addr, struct ppc_inst instr);
>
> static inline unsigned long patch_site_addr(s32 *site)
> {
> return (unsigned long)site + *site;
> }
>
> -static inline int patch_instruction_site(s32 *site, unsigned int instr)
> +static inline int patch_instruction_site(s32 *site, struct ppc_inst instr)
> {
> - return patch_instruction((unsigned int *)patch_site_addr(site), instr);
> + return patch_instruction((struct ppc_inst *)patch_site_addr(site),
> instr);
> }
>
> static inline int patch_branch_site(s32 *site, unsigned long target, int
> flags)
> {
> - return patch_branch((unsigned int *)patch_site_addr(site), target,
> flags);
> + return patch_branch((struct ppc_inst *)patch_site_addr(site), target,
> flags);
> }
>
> static inline int modify_instruction(unsigned int *addr, unsigned int clr,
>unsigned int set)
> {
> - return patch_instruction(addr, ppc_inst((*addr & ~clr) | set));
> + return patch_instruction((struct ppc_inst *)addr, ppc_inst((*addr &
> ~clr) | set));
> }
>
> static inline int modify_instruction_site(s32 *site, unsigned int clr,
> unsigned int set)
> @@ -57,13 +57,13 @@ static inline int modify_instruction_site(s32 *site,
> unsigned int clr, unsigned
> return modify_instruction((unsigned int *)patch_site_addr(site), clr,
> set);
> }
>
> -int instr_is_relative_branch(unsigned int instr);
> -int
Re: [PATCH v5 09/21] powerpc: Use a datatype for instructions
Hi Jordan, Thank you for the patch! Yet something to improve: [auto build test ERROR on v5.6] [cannot apply to powerpc/next kvm-ppc/kvm-ppc-next scottwood/next next-20200406] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Jordan-Niethe/Initial-Prefixed-Instruction-support/20200406-165215 base:7111951b8d4973bda27ff663f2cf18b663d15b48 config: powerpc-allnoconfig (attached as .config) compiler: powerpc-linux-gcc (GCC) 9.3.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=9.3.0 make.cross ARCH=powerpc If you fix the issue, kindly add following tag as appropriate Reported-by: kbuild test robot All error/warnings (new ones prefixed by >>): In file included from arch/powerpc/include/asm/asm-compat.h:6, from arch/powerpc/include/asm/bitops.h:42, from include/linux/bitops.h:29, from include/linux/kernel.h:12, from include/linux/list.h:9, from include/linux/module.h:12, from arch/powerpc/kernel/setup_32.c:6: arch/powerpc/kernel/setup_32.c: In function 'machine_init': >> arch/powerpc/include/asm/ppc-opcode.h:234:24: error: incompatible type for >> argument 2 of 'patch_instruction_site' 234 | #define PPC_INST_NOP 0x6000 |^~ || |int >> arch/powerpc/kernel/setup_32.c:89:49: note: in expansion of macro >> 'PPC_INST_NOP' 89 | patch_instruction_site(__memcpy_nocache, PPC_INST_NOP); | ^~~~ In file included from arch/powerpc/kernel/setup_32.c:42: arch/powerpc/include/asm/code-patching.h:39:69: note: expected 'struct ppc_inst' but argument is of type 'int' 39 | static inline int patch_instruction_site(s32 *site, struct ppc_inst instr) | ^ >> arch/powerpc/kernel/setup_32.c:91:48: error: passing argument 1 of >> 'branch_target' from incompatible pointer type >> [-Werror=incompatible-pointer-types] 91 | create_cond_branch(, addr, branch_target(addr), 0x82); |^~~~ || |unsigned int * In file included from arch/powerpc/kernel/setup_32.c:42: arch/powerpc/include/asm/code-patching.h:63:52: note: expected 'const struct ppc_inst *' but argument is of type 'unsigned int *' 63 | unsigned long branch_target(const struct ppc_inst *instr); | ~~~^ arch/powerpc/kernel/setup_32.c:91:28: error: passing argument 2 of 'create_cond_branch' from incompatible pointer type [-Werror=incompatible-pointer-types] 91 | create_cond_branch(, addr, branch_target(addr), 0x82); |^~~~ || |unsigned int * In file included from arch/powerpc/kernel/setup_32.c:42: arch/powerpc/include/asm/code-patching.h:28:71: note: expected 'const struct ppc_inst *' but argument is of type 'unsigned int *' 28 | int create_cond_branch(struct ppc_inst *instr, const struct ppc_inst *addr, | ~~~^~~~ >> arch/powerpc/kernel/setup_32.c:92:20: error: passing argument 1 of >> 'patch_instruction' from incompatible pointer type >> [-Werror=incompatible-pointer-types] 92 | patch_instruction(addr, insn); /* replace b by bne cr0 */ |^~~~ || |unsigned int * In file included from arch/powerpc/kernel/setup_32.c:42: arch/powerpc/include/asm/code-patching.h:31:40: note: expected 'struct ppc_inst *' but argument is of type 'unsigned int *' 31 | int patch_instruction(struct ppc_inst *addr, struct ppc_inst instr); | ~^~~~ cc1: all warnings being treated as errors vim +/patch_instruction_site +234 arch/powerpc/include/asm/ppc-opcode.h 9123c5ed45a583 Hongtao Jia 2013-04-28 193 16c57b3620d77e Kumar Gala 2009-02-10 194 /* sorted alphabetically */ 95213959aefc94 Anshuman Khandual2013-04-22 195 #define PPC_INST_BHRBE 0x7c00025c 95213959aefc94 Anshuman Khandual2013-04-22 196 #define
Re: [PATCH v5 09/21] powerpc: Use a datatype for instructions
Hi Jordan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on v5.6]
[cannot apply to powerpc/next kvm-ppc/kvm-ppc-next scottwood/next next-20200406]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Jordan-Niethe/Initial-Prefixed-Instruction-support/20200406-165215
base:7111951b8d4973bda27ff663f2cf18b663d15b48
config: powerpc-rhel-kconfig (attached as .config)
compiler: powerpc64le-linux-gcc (GCC) 9.3.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=9.3.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot
All errors (new ones prefixed by >>):
arch/powerpc/lib/feature-fixups.c: In function 'do_final_fixups':
>> arch/powerpc/lib/feature-fixups.c:404:25: error: passing argument 1 of
>> 'raw_patch_instruction' from incompatible pointer type
>> [-Werror=incompatible-pointer-types]
404 | raw_patch_instruction(dest, ppc_inst(*src));
| ^~~~
| |
| int *
In file included from arch/powerpc/lib/feature-fixups.c:18:
arch/powerpc/include/asm/code-patching.h:32:44: note: expected 'struct
ppc_inst *' but argument is of type 'int *'
32 | int raw_patch_instruction(struct ppc_inst *addr, struct ppc_inst
instr);
| ~^~~~
cc1: some warnings being treated as errors
vim +/raw_patch_instruction +404 arch/powerpc/lib/feature-fixups.c
2d1b2027626d51 Kumar Gala 2008-07-02 389
9402c684613163 Benjamin Herrenschmidt 2016-07-05 390 static void
do_final_fixups(void)
d715e433b7ad19 Anton Blanchard2011-11-14 391 {
d715e433b7ad19 Anton Blanchard2011-11-14 392 #if
defined(CONFIG_PPC64) && defined(CONFIG_RELOCATABLE)
d715e433b7ad19 Anton Blanchard2011-11-14 393 int *src, *dest;
d715e433b7ad19 Anton Blanchard2011-11-14 394 unsigned long length;
d715e433b7ad19 Anton Blanchard2011-11-14 395
d715e433b7ad19 Anton Blanchard2011-11-14 396 if (PHYSICAL_START == 0)
d715e433b7ad19 Anton Blanchard2011-11-14 397 return;
d715e433b7ad19 Anton Blanchard2011-11-14 398
d715e433b7ad19 Anton Blanchard2011-11-14 399 src = (int
*)(KERNELBASE + PHYSICAL_START);
d715e433b7ad19 Anton Blanchard2011-11-14 400 dest = (int
*)KERNELBASE;
d715e433b7ad19 Anton Blanchard2011-11-14 401 length =
(__end_interrupts - _stext) / sizeof(int);
d715e433b7ad19 Anton Blanchard2011-11-14 402
d715e433b7ad19 Anton Blanchard2011-11-14 403 while (length--) {
16f7ae823ee707 Jordan Niethe 2020-04-06 @404
raw_patch_instruction(dest, ppc_inst(*src));
d715e433b7ad19 Anton Blanchard2011-11-14 405 src++;
d715e433b7ad19 Anton Blanchard2011-11-14 406 dest++;
d715e433b7ad19 Anton Blanchard2011-11-14 407 }
d715e433b7ad19 Anton Blanchard2011-11-14 408 #endif
d715e433b7ad19 Anton Blanchard2011-11-14 409 }
d715e433b7ad19 Anton Blanchard2011-11-14 410
:: The code at line 404 was first introduced by commit
:: 16f7ae823ee70796c5ba2cc321b2c02f3dcfb816 powerpc: Use a macro for
creating instructions from u32s
:: TO: Jordan Niethe
:: CC: 0day robot
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
[PATCH v5 09/21] powerpc: Use a datatype for instructions
Currently unsigned ints are used to represent instructions on powerpc.
This has worked well as instructions have always been 4 byte words.
However, a future ISA version will introduce some changes to
instructions that mean this scheme will no longer work as well. This
change is Prefixed Instructions. A prefixed instruction is made up of a
word prefix followed by a word suffix to make an 8 byte double word
instruction. No matter the endianess of the system the prefix always
comes first. Prefixed instructions are only planned for powerpc64.
Introduce a ppc_inst type to represent both prefixed and word
instructions on powerpc64 while keeping it possible to exclusively have
word instructions on powerpc32, A latter patch will expand the type to
include prefixed instructions but for now just typedef it to a u32.
Later patches will introduce helper functions and macros for
manipulating the instructions so that powerpc64 and powerpc32 might
maintain separate type definitions.
Signed-off-by: Jordan Niethe
---
v4: New to series
v5: Add to epapr_paravirt.c, kgdb.c
---
arch/powerpc/include/asm/code-patching.h | 32 -
arch/powerpc/include/asm/inst.h | 20 +++---
arch/powerpc/include/asm/sstep.h | 5 +-
arch/powerpc/include/asm/uprobes.h | 5 +-
arch/powerpc/kernel/align.c | 4 +-
arch/powerpc/kernel/epapr_paravirt.c | 4 +-
arch/powerpc/kernel/hw_breakpoint.c | 4 +-
arch/powerpc/kernel/jump_label.c | 2 +-
arch/powerpc/kernel/kgdb.c | 4 +-
arch/powerpc/kernel/kprobes.c| 8 +--
arch/powerpc/kernel/mce_power.c | 5 +-
arch/powerpc/kernel/optprobes.c | 40 ++--
arch/powerpc/kernel/setup_32.c | 2 +-
arch/powerpc/kernel/trace/ftrace.c | 83
arch/powerpc/kernel/vecemu.c | 5 +-
arch/powerpc/lib/code-patching.c | 69 ++--
arch/powerpc/lib/feature-fixups.c| 48 +++---
arch/powerpc/lib/sstep.c | 4 +-
arch/powerpc/lib/test_emulate_step.c | 9 +--
arch/powerpc/perf/core-book3s.c | 4 +-
arch/powerpc/xmon/xmon.c | 24 +++
21 files changed, 196 insertions(+), 185 deletions(-)
diff --git a/arch/powerpc/include/asm/code-patching.h
b/arch/powerpc/include/asm/code-patching.h
index 48e021957ee5..eacc9102c251 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -23,33 +23,33 @@
#define BRANCH_ABSOLUTE0x2
bool is_offset_in_branch_range(long offset);
-int create_branch(unsigned int *instr, const unsigned int *addr,
+int create_branch(struct ppc_inst *instr, const struct ppc_inst *addr,
unsigned long target, int flags);
-int create_cond_branch(unsigned int *instr, const unsigned int *addr,
+int create_cond_branch(struct ppc_inst *instr, const struct ppc_inst *addr,
unsigned long target, int flags);
-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_branch(struct ppc_inst *addr, unsigned long target, int flags);
+int patch_instruction(struct ppc_inst *addr, struct ppc_inst instr);
+int raw_patch_instruction(struct ppc_inst *addr, struct ppc_inst instr);
static inline unsigned long patch_site_addr(s32 *site)
{
return (unsigned long)site + *site;
}
-static inline int patch_instruction_site(s32 *site, unsigned int instr)
+static inline int patch_instruction_site(s32 *site, struct ppc_inst instr)
{
- return patch_instruction((unsigned int *)patch_site_addr(site), instr);
+ return patch_instruction((struct ppc_inst *)patch_site_addr(site),
instr);
}
static inline int patch_branch_site(s32 *site, unsigned long target, int flags)
{
- return patch_branch((unsigned int *)patch_site_addr(site), target,
flags);
+ return patch_branch((struct ppc_inst *)patch_site_addr(site), target,
flags);
}
static inline int modify_instruction(unsigned int *addr, unsigned int clr,
unsigned int set)
{
- return patch_instruction(addr, ppc_inst((*addr & ~clr) | set));
+ return patch_instruction((struct ppc_inst *)addr, ppc_inst((*addr &
~clr) | set));
}
static inline int modify_instruction_site(s32 *site, unsigned int clr,
unsigned int set)
@@ -57,13 +57,13 @@ static inline int modify_instruction_site(s32 *site,
unsigned int clr, unsigned
return modify_instruction((unsigned int *)patch_site_addr(site), clr,
set);
}
-int instr_is_relative_branch(unsigned int instr);
-int instr_is_relative_link_branch(unsigned int instr);
-int instr_is_branch_to_addr(const unsigned int *instr, unsigned long addr);
-unsigned long branch_target(const unsigned int *instr);
-int translate_branch(unsigned int *instr, const unsigned int
