* Re: [PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export
From: Qiujun Huang @ 2020-04-06 10:40 UTC (permalink / raw)
To: Markus Elfring
Cc: LKML, Oliver O'Halloran, Paul Mackerras, Thomas Gleixner,
linuxppc-dev
In-Reply-To: <d8334f5d-403f-941f-abcc-2714d297082b@web.de>
On Mon, Apr 6, 2020 at 6:02 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> >>>> Here needs a NULL check.
> >> quite obvious?
>
> I suggest to consider another fine-tuning for the wording also around
> such “obvious” programming items.
>
>
> >>> I find this change description questionable
> >>> (despite of a reasonable patch subject).
>
> I got further development concerns.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=a10c9c710f9ecea87b9f4bbb837467893b4bef01#n129
>
> * Were changes mixed for different issues according to the diff code?
>
> * I find it safer here to split specific changes into separate update steps
> for a small patch series.
>
> * Will the addition of the desired null pointer check qualify for
> the specification of the tag “Fixes”?
>
>
> >>> Will a patch change log be helpful here?
> >> I realized I should write some change log, and the change log was meaningless.
>
> Will any more adjustments happen for the discussed update suggestion
> after the third patch version?
>
>
> > The changelog is fine IMO. The point of a changelog is to tell a
> > reader doing git archeology why a change happened and this is
> > sufficent for that.
>
> We might stumble on a different understanding for the affected “change logs”.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=a10c9c710f9ecea87b9f4bbb837467893b4bef01#n751
>
> Would you like to follow the patch evolution a bit easier?
>
> Regards,
> Markus
Thanks for the reply.
I should study the documentation first.
BTW, happy new week
^ permalink raw reply
* Re: [PATCH v5 09/21] powerpc: Use a datatype for instructions
From: kbuild test robot @ 2020-04-06 10:35 UTC (permalink / raw)
To: Jordan Niethe
Cc: kbuild-all, Jordan Niethe, npiggin, bala24, alistair,
linuxppc-dev, dja
In-Reply-To: <20200406080936.7180-10-jniethe5@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 11038 bytes --]
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 <lkp@intel.com>
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 0x60000000
| ^~~~~~~~~~
| |
| int
>> arch/powerpc/kernel/setup_32.c:89:49: note: in expansion of macro 'PPC_INST_NOP'
89 | patch_instruction_site(&patch__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(&insn, addr, branch_target(addr), 0x820000);
| ^~~~
| |
| 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(&insn, addr, branch_target(addr), 0x820000);
| ^~~~
| |
| 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 Khandual 2013-04-22 195 #define PPC_INST_BHRBE 0x7c00025c
95213959aefc94 Anshuman Khandual 2013-04-22 196 #define PPC_INST_CLRBHRB 0x7c00035c
07d2a628bc0008 Nicholas Piggin 2017-06-09 197 #define PPC_INST_COPY 0x7c20060c
8a649045e75a4b Chris Smart 2016-04-26 198 #define PPC_INST_CP_ABORT 0x7c00068c
e66ca3db5917f4 Matt Brown 2017-08-04 199 #define PPC_INST_DARN 0x7c0005e6
16c57b3620d77e Kumar Gala 2009-02-10 200 #define PPC_INST_DCBA 0x7c0005ec
16c57b3620d77e Kumar Gala 2009-02-10 201 #define PPC_INST_DCBA_MASK 0xfc0007fe
16c57b3620d77e Kumar Gala 2009-02-10 202 #define PPC_INST_DCBAL 0x7c2005ec
16c57b3620d77e Kumar Gala 2009-02-10 203 #define PPC_INST_DCBZL 0x7c2007ec
1afc149def25ac Tony Breeds 2012-10-02 204 #define PPC_INST_ICBT 0x7c00002c
edc424f8cd84bb Dan Streetman 2015-05-07 205 #define PPC_INST_ICSWX 0x7c00032d
edc424f8cd84bb Dan Streetman 2015-05-07 206 #define PPC_INST_ICSWEPX 0x7c00076d
16c57b3620d77e Kumar Gala 2009-02-10 207 #define PPC_INST_ISEL 0x7c00001e
16c57b3620d77e Kumar Gala 2009-02-10 208 #define PPC_INST_ISEL_MASK 0xfc00003e
864b9e6fd76489 Anton Blanchard 2010-02-10 209 #define PPC_INST_LDARX 0x7c0000a8
156d0e290e969c Naveen N. Rao 2016-06-22 210 #define PPC_INST_STDCX 0x7c0001ad
350779a29f11f8 Paul Mackerras 2017-08-30 211 #define PPC_INST_LQARX 0x7c000228
350779a29f11f8 Paul Mackerras 2017-08-30 212 #define PPC_INST_STQCX 0x7c00016d
16c57b3620d77e Kumar Gala 2009-02-10 213 #define PPC_INST_LSWI 0x7c0004aa
16c57b3620d77e Kumar Gala 2009-02-10 214 #define PPC_INST_LSWX 0x7c00042a
d6ccb1f55ddf51 Kumar Gala 2010-03-10 215 #define PPC_INST_LWARX 0x7c000028
156d0e290e969c Naveen N. Rao 2016-06-22 216 #define PPC_INST_STWCX 0x7c00012d
16c57b3620d77e Kumar Gala 2009-02-10 217 #define PPC_INST_LWSYNC 0x7c2004ac
9863c28a2af90a James Yang 2013-07-03 218 #define PPC_INST_SYNC 0x7c0004ac
9863c28a2af90a James Yang 2013-07-03 219 #define PPC_INST_SYNC_MASK 0xfc0007fe
ddc6cd0d70a6f9 Christophe Leroy 2016-05-17 220 #define PPC_INST_ISYNC 0x4c00012c
dfb432cb960bfc Michael Neuling 2009-04-29 221 #define PPC_INST_LXVD2X 0x7c000698
16c57b3620d77e Kumar Gala 2009-02-10 222 #define PPC_INST_MCRXR 0x7c000400
16c57b3620d77e Kumar Gala 2009-02-10 223 #define PPC_INST_MCRXR_MASK 0xfc0007fe
16c57b3620d77e Kumar Gala 2009-02-10 224 #define PPC_INST_MFSPR_PVR 0x7c1f42a6
178f358208ceb8 Anton Blanchard 2017-01-19 225 #define PPC_INST_MFSPR_PVR_MASK 0xfc1ffffe
e16c8765533a15 Andy Fleming 2011-12-08 226 #define PPC_INST_MFTMR 0x7c0002dc
16c57b3620d77e Kumar Gala 2009-02-10 227 #define PPC_INST_MSGSND 0x7c00019c
755563bc79c764 Paul Mackerras 2015-03-19 228 #define PPC_INST_MSGCLR 0x7c0001dc
6b3edefefa6752 Nicholas Piggin 2017-04-13 229 #define PPC_INST_MSGSYNC 0x7c0006ec
42d02b81f265b7 Ian Munsie 2012-11-14 230 #define PPC_INST_MSGSNDP 0x7c00011c
a9af97aa0a12c3 Nicholas Piggin 2017-06-13 231 #define PPC_INST_MSGCLRP 0x7c00015c
4bb3c7a0208fc1 Paul Mackerras 2018-03-21 232 #define PPC_INST_MTMSRD 0x7c000164
e16c8765533a15 Andy Fleming 2011-12-08 233 #define PPC_INST_MTTMR 0x7c0003dc
16c57b3620d77e Kumar Gala 2009-02-10 @234 #define PPC_INST_NOP 0x60000000
07d2a628bc0008 Nicholas Piggin 2017-06-09 235 #define PPC_INST_PASTE 0x7c20070d
16c57b3620d77e Kumar Gala 2009-02-10 236 #define PPC_INST_POPCNTB 0x7c0000f4
16c57b3620d77e Kumar Gala 2009-02-10 237 #define PPC_INST_POPCNTB_MASK 0xfc0007fe
b5f9b6665b70b4 Anton Blanchard 2010-12-07 238 #define PPC_INST_POPCNTD 0x7c0003f4
b5f9b6665b70b4 Anton Blanchard 2010-12-07 239 #define PPC_INST_POPCNTW 0x7c0002f4
4bb3c7a0208fc1 Paul Mackerras 2018-03-21 240 #define PPC_INST_RFEBB 0x4c000124
16c57b3620d77e Kumar Gala 2009-02-10 241 #define PPC_INST_RFCI 0x4c000066
16c57b3620d77e Kumar Gala 2009-02-10 242 #define PPC_INST_RFDI 0x4c00004e
4bb3c7a0208fc1 Paul Mackerras 2018-03-21 243 #define PPC_INST_RFID 0x4c000024
16c57b3620d77e Kumar Gala 2009-02-10 244 #define PPC_INST_RFMCI 0x4c00004c
cd99ddbea250ee Christophe Leroy 2018-01-12 245 #define PPC_INST_MFSPR 0x7c0002a6
efcac6589a277c Alexey Kardashevskiy 2011-03-02 246 #define PPC_INST_MFSPR_DSCR 0x7c1102a6
178f358208ceb8 Anton Blanchard 2017-01-19 247 #define PPC_INST_MFSPR_DSCR_MASK 0xfc1ffffe
efcac6589a277c Alexey Kardashevskiy 2011-03-02 248 #define PPC_INST_MTSPR_DSCR 0x7c1103a6
178f358208ceb8 Anton Blanchard 2017-01-19 249 #define PPC_INST_MTSPR_DSCR_MASK 0xfc1ffffe
73d2fb758e678c Anton Blanchard 2013-05-01 250 #define PPC_INST_MFSPR_DSCR_USER 0x7c0302a6
178f358208ceb8 Anton Blanchard 2017-01-19 251 #define PPC_INST_MFSPR_DSCR_USER_MASK 0xfc1ffffe
73d2fb758e678c Anton Blanchard 2013-05-01 252 #define PPC_INST_MTSPR_DSCR_USER 0x7c0303a6
178f358208ceb8 Anton Blanchard 2017-01-19 253 #define PPC_INST_MTSPR_DSCR_USER_MASK 0xfc1ffffe
6dd7a82cc54ebd Anton Blanchard 2016-07-01 254 #define PPC_INST_MFVSRD 0x7c000066
6dd7a82cc54ebd Anton Blanchard 2016-07-01 255 #define PPC_INST_MTVSRD 0x7c000166
d16952a629129c Christophe Leroy 2018-11-09 256 #define PPC_INST_SC 0x44000002
697d3899dcb4bc Paul Mackerras 2011-12-12 257 #define PPC_INST_SLBFEE 0x7c0007a7
09cf5bcb0c9355 Aneesh Kumar K.V 2016-07-13 258 #define PPC_INST_SLBIA 0x7c0003e4
16c57b3620d77e Kumar Gala 2009-02-10 259
:::::: The code at line 234 was first introduced by commit
:::::: 16c57b3620d77e0bc981da5ef32beae730512684 powerpc: Unify opcode definitions and support
:::::: TO: Kumar Gala <galak@kernel.crashing.org>
:::::: CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6411 bytes --]
^ permalink raw reply
* Re: [PATCH v5 09/21] powerpc: Use a datatype for instructions
From: kbuild test robot @ 2020-04-06 10:34 UTC (permalink / raw)
To: Jordan Niethe
Cc: kbuild-all, Jordan Niethe, npiggin, bala24, alistair,
linuxppc-dev, dja
In-Reply-To: <20200406080936.7180-10-jniethe5@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3907 bytes --]
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 <lkp@intel.com>
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 Blanchard 2011-11-14 391 {
d715e433b7ad19 Anton Blanchard 2011-11-14 392 #if defined(CONFIG_PPC64) && defined(CONFIG_RELOCATABLE)
d715e433b7ad19 Anton Blanchard 2011-11-14 393 int *src, *dest;
d715e433b7ad19 Anton Blanchard 2011-11-14 394 unsigned long length;
d715e433b7ad19 Anton Blanchard 2011-11-14 395
d715e433b7ad19 Anton Blanchard 2011-11-14 396 if (PHYSICAL_START == 0)
d715e433b7ad19 Anton Blanchard 2011-11-14 397 return;
d715e433b7ad19 Anton Blanchard 2011-11-14 398
d715e433b7ad19 Anton Blanchard 2011-11-14 399 src = (int *)(KERNELBASE + PHYSICAL_START);
d715e433b7ad19 Anton Blanchard 2011-11-14 400 dest = (int *)KERNELBASE;
d715e433b7ad19 Anton Blanchard 2011-11-14 401 length = (__end_interrupts - _stext) / sizeof(int);
d715e433b7ad19 Anton Blanchard 2011-11-14 402
d715e433b7ad19 Anton Blanchard 2011-11-14 403 while (length--) {
16f7ae823ee707 Jordan Niethe 2020-04-06 @404 raw_patch_instruction(dest, ppc_inst(*src));
d715e433b7ad19 Anton Blanchard 2011-11-14 405 src++;
d715e433b7ad19 Anton Blanchard 2011-11-14 406 dest++;
d715e433b7ad19 Anton Blanchard 2011-11-14 407 }
d715e433b7ad19 Anton Blanchard 2011-11-14 408 #endif
d715e433b7ad19 Anton Blanchard 2011-11-14 409 }
d715e433b7ad19 Anton Blanchard 2011-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 <jniethe5@gmail.com>
:::::: CC: 0day robot <lkp@intel.com>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 15331 bytes --]
^ permalink raw reply
* Re: [PATCH v4] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export
From: Michael Ellerman @ 2020-04-06 10:30 UTC (permalink / raw)
To: Qiujun Huang, benh, paulus, oohall
Cc: linux-kernel, Markus.Elfring, tglx, linuxppc-dev, Qiujun Huang
In-Reply-To: <20200406093944.4174-1-hqjagain@gmail.com>
Qiujun Huang <hqjagain@gmail.com> writes:
> Here needs a NULL check as kzalloc may fail returning NULL.
>
> Issue was found by coccinelle.
> Generated by: scripts/coccinelle/null/kmerr.cocci
>
> Signed-off-by: Qiujun Huang <hqjagain@gmail.com>
> Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
>
> ---
Thanks for putting up with all the review comments :)
But I think this should actually be two patches now.
The first patch should change the goto after
of_property_read_u64_array() into a return and drop the redundant
assignments.
Then the second patch can add the NULL check for attr.
cheers
> v3->v4:
> Added the information about coccinelle script.
> Added change log.
> Added Oliver's Reviewed-by.
> v2->v3:
> Removed redundant assignment to 'attr' and 'name'.
> v1->v2:
> Just return -ENOMEM if attr is NULL.
> ---
> arch/powerpc/platforms/powernv/opal.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> index 2b3dfd0b6cdd..908d749bcef5 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -801,16 +801,19 @@ static ssize_t export_attr_read(struct file *fp, struct kobject *kobj,
> static int opal_add_one_export(struct kobject *parent, const char *export_name,
> struct device_node *np, const char *prop_name)
> {
> - struct bin_attribute *attr = NULL;
> - const char *name = NULL;
> + struct bin_attribute *attr;
> + const char *name;
> u64 vals[2];
> int rc;
>
> rc = of_property_read_u64_array(np, prop_name, &vals[0], 2);
> if (rc)
> - goto out;
> + return rc;
>
> attr = kzalloc(sizeof(*attr), GFP_KERNEL);
> + if (!attr)
> + return -ENOMEM;
> +
> name = kstrdup(export_name, GFP_KERNEL);
> if (!name) {
> rc = -ENOMEM;
> --
> 2.17.1
^ permalink raw reply
* Re: [PATCH v5 03/21] powerpc: Change calling convention for create_branch() et. al.
From: kbuild test robot @ 2020-04-06 10:25 UTC (permalink / raw)
To: Jordan Niethe
Cc: kbuild-all, Jordan Niethe, npiggin, bala24, alistair,
linuxppc-dev, dja
In-Reply-To: <20200406080936.7180-4-jniethe5@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3282 bytes --]
Hi Jordan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on v5.6]
[also build test ERROR on next-20200406]
[cannot apply to powerpc/next kvm-ppc/kvm-ppc-next scottwood/next]
[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 <lkp@intel.com>
All errors (new ones prefixed by >>):
arch/powerpc/kernel/setup_32.c: In function 'machine_init':
>> arch/powerpc/kernel/setup_32.c:91:21: error: passing argument 1 of 'create_cond_branch' from incompatible pointer type [-Werror=incompatible-pointer-types]
91 | create_cond_branch(&insn, addr, branch_target(addr), 0x820000);
| ^~~~~
| |
| long unsigned int *
In file included from arch/powerpc/kernel/setup_32.c:42:
arch/powerpc/include/asm/code-patching.h:27:38: note: expected 'unsigned int *' but argument is of type 'long unsigned int *'
27 | int create_cond_branch(unsigned int *instr, const unsigned int *addr,
| ~~~~~~~~~~~~~~^~~~~
cc1: all warnings being treated as errors
vim +/create_cond_branch +91 arch/powerpc/kernel/setup_32.c
67
68 /*
69 * This is run before start_kernel(), the kernel has been relocated
70 * and we are running with enough of the MMU enabled to have our
71 * proper kernel virtual addresses
72 *
73 * We do the initial parsing of the flat device-tree and prepares
74 * for the MMU to be fully initialized.
75 */
76 notrace void __init machine_init(u64 dt_ptr)
77 {
78 unsigned int *addr = (unsigned int *)patch_site_addr(&patch__memset_nocache);
79 unsigned long insn;
80
81 /* Configure static keys first, now that we're relocated. */
82 setup_feature_keys();
83
84 early_ioremap_setup();
85
86 /* Enable early debugging if any specified (see udbg.h) */
87 udbg_early_init();
88
89 patch_instruction_site(&patch__memcpy_nocache, PPC_INST_NOP);
90
> 91 create_cond_branch(&insn, addr, branch_target(addr), 0x820000);
92 patch_instruction(addr, insn); /* replace b by bne cr0 */
93
94 /* Do some early initialization based on the flat device tree */
95 early_init_devtree(__va(dt_ptr));
96
97 early_init_mmu();
98
99 setup_kdump_trampoline();
100 }
101
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6411 bytes --]
^ permalink raw reply
* Re: [PATCH v5 18/21] powerpc64: Add prefixed instructions to instruction data type
From: Christophe Leroy @ 2020-04-06 10:25 UTC (permalink / raw)
To: Alistair Popple, Jordan Niethe; +Cc: bala24, linuxppc-dev, npiggin, dja
In-Reply-To: <7182352.hY56U9iWWN@townsend>
Le 06/04/2020 à 11:52, Alistair Popple a écrit :
[...]
>> @@ -32,14 +76,31 @@ static inline struct ppc_inst ppc_inst_swab(struct
>> ppc_inst x) return ppc_inst(swab32(ppc_inst_val(x)));
>> }
>>
>> +static inline u32 ppc_inst_val(struct ppc_inst x)
>> +{
>> + return x.val;
>> +}
>> +
>> static inline struct ppc_inst ppc_inst_read(const struct ppc_inst *ptr)
>> {
>> return *ptr;
>> }
>>
>> +static inline void ppc_inst_write(struct ppc_inst *ptr, struct ppc_inst x)
>> +{
>> + *ptr = x;
>> +}
>> +
>> +#endif /* __powerpc64__ */
>> +
>> static inline bool ppc_inst_equal(struct ppc_inst x, struct ppc_inst y)
>> {
>> return !memcmp(&x, &y, sizeof(struct ppc_inst));
>> }
>
> Apologies for not picking this up earlier, I was hoping to get to the bottom
> of the issue I was seeing before you sent out v5. However the above definition
> of instruction equality does not seem correct because it does not consider the
> case when an instruction is not prefixed - a non-prefixed instruction should be
> considered equal if the first 32-bit opcode/value is the same. Something like:
>
> if (ppc_inst_prefixed(x) != ppc_inst_prefixed(y))
> return false;
> else if (ppc_inst_prefixed(x))
> return !memcmp(&x, &y, sizeof(struct ppc_inst));
Are we sure memcmp() is a good candidate for the comparison ? Can we do
simpler ? Especially, I understood a prefixed instruction is a 64 bits
properly aligned instruction, can we do a simple u64 compare ? Or is GCC
intelligent enough to do that without calling memcmp() function which is
heavy ?
> else
> return x.val == y.val;
>
> This was causing failures in ftrace_modify_code() as it would falsely detect
> two non-prefixed instructions as being not equal due to differences in the suffix.
>
Christophe
^ permalink raw reply
* Re: [PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export
From: Markus Elfring @ 2020-04-06 10:02 UTC (permalink / raw)
To: Oliver O'Halloran, Qiujun Huang, linuxppc-dev
Cc: Thomas Gleixner, Paul Mackerras, LKML
In-Reply-To: <CAOSf1CGLZ5H1cUsv6atZ7hNpQ+PDyyAjp6dzJjXMwC5XoUOVQQ@mail.gmail.com>
>>>> Here needs a NULL check.
>> quite obvious?
I suggest to consider another fine-tuning for the wording also around
such “obvious” programming items.
>>> I find this change description questionable
>>> (despite of a reasonable patch subject).
I got further development concerns.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=a10c9c710f9ecea87b9f4bbb837467893b4bef01#n129
* Were changes mixed for different issues according to the diff code?
* I find it safer here to split specific changes into separate update steps
for a small patch series.
* Will the addition of the desired null pointer check qualify for
the specification of the tag “Fixes”?
>>> Will a patch change log be helpful here?
>> I realized I should write some change log, and the change log was meaningless.
Will any more adjustments happen for the discussed update suggestion
after the third patch version?
> The changelog is fine IMO. The point of a changelog is to tell a
> reader doing git archeology why a change happened and this is
> sufficent for that.
We might stumble on a different understanding for the affected “change logs”.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=a10c9c710f9ecea87b9f4bbb837467893b4bef01#n751
Would you like to follow the patch evolution a bit easier?
Regards,
Markus
^ permalink raw reply
* Re: Make PowerNV IOMMU group setup saner (and fix it for hotpug)
From: Alexey Kardashevskiy @ 2020-04-06 9:56 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev
In-Reply-To: <20200406030745.24595-1-oohall@gmail.com>
On 06/04/2020 13:07, Oliver O'Halloran wrote:
> Currently on PowerNV the IOMMU group of a device is initialised in
> boot-time fixup which runs after devices are probed. Because this is
> only run at boot time hotplugged devices do not recieve an iommu group
> assignment which prevents them from being passed through to a guest.
>
> This series fixes that by moving the point where IOMMU groups are
> registered to when we configure DMA for a PE, and moves the point where
> we add a device to the PE's IOMMU group into the per-device DMA setup
> callback for IODA phbs (pnv_pci_ioda_dma_dev_setup()). This change means
> that we'll do group setup for hotplugged devices and that we can remove
> the hack we have for VFs which are currently added to their group
> via a bus notifier.
>
> With this change there's no longer any per-device setup that needs to
> run in a fixup for ordinary PCI devices. The exception is, as per usual,
> NVLink devices. For those the GPU and any of it's NVLink devices need
> to be in a "compound" IOMMU group which keeps the DMA address spaces
> of each device in sync with it's attached devices. As a result that
> setup can only be done when both the NVLink devices and the GPU device
> has been probed, so that setup is still done in the fixup. Sucks, but
> it's still an improvement.
>
> Boot tested on a witherspoon with 6xGPUs and it didn't crash so it must
> be good.
Thanks for cleaning this up!
I tried this with IOV on P8 (garrison2) and witherspoon+GPU+NPU
passthrough, works, as before, IOMMU group numbers change but we never
relied on those anyway.
--
Alexey
^ permalink raw reply
* Re: [PATCH 7/7] powerpc/powernv/npu: Move IOMMU group setup into npu-dma.c
From: Alexey Kardashevskiy @ 2020-04-06 9:54 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev
In-Reply-To: <20200406030745.24595-8-oohall@gmail.com>
On 06/04/2020 13:07, Oliver O'Halloran wrote:
> The NVlink IOMMU group setup is only relevant to NVLink devices so move
> it into the NPU containment zone. This let us remove some prototypes in
> pci.h and staticfy some function definitions.
>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/platforms/powernv/npu-dma.c | 54 +++++++++++++++++++-
> arch/powerpc/platforms/powernv/pci-ioda.c | 60 +++--------------------
> arch/powerpc/platforms/powernv/pci.h | 6 +--
> 3 files changed, 60 insertions(+), 60 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
> index df27b8d7e78f..abeaa533b976 100644
> --- a/arch/powerpc/platforms/powernv/npu-dma.c
> +++ b/arch/powerpc/platforms/powernv/npu-dma.c
> @@ -15,6 +15,7 @@
>
> #include <asm/debugfs.h>
> #include <asm/powernv.h>
> +#include <asm/ppc-pci.h>
> #include <asm/opal.h>
>
> #include "pci.h"
> @@ -425,7 +426,8 @@ static void pnv_comp_attach_table_group(struct npu_comp *npucomp,
> ++npucomp->pe_num;
> }
>
> -struct iommu_table_group *pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe)
> +static struct iommu_table_group *
> + pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe)
> {
> struct iommu_table_group *compound_group;
> struct npu_comp *npucomp;
> @@ -491,7 +493,7 @@ struct iommu_table_group *pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe)
> return compound_group;
> }
>
> -struct iommu_table_group *pnv_npu_compound_attach(struct pnv_ioda_pe *pe)
> +static struct iommu_table_group *pnv_npu_compound_attach(struct pnv_ioda_pe *pe)
> {
> struct iommu_table_group *table_group;
> struct npu_comp *npucomp;
> @@ -534,6 +536,54 @@ struct iommu_table_group *pnv_npu_compound_attach(struct pnv_ioda_pe *pe)
>
> return table_group;
> }
> +
> +void pnv_pci_npu_setup_iommu_groups(void)
> +{
> + struct pci_controller *hose;
> + struct pnv_phb *phb;
> + struct pnv_ioda_pe *pe;
> +
> + /*
> + * For non-nvlink devices the IOMMU group is registered when the PE is
> + * configured and devices are added to the group when the per-device
> + * DMA setup is run. That's done in hose->ops.dma_dev_setup() which is
> + * only initialise for "normal" IODA PHBs.
> + *
> + * For NVLink devices we need to ensure the NVLinks and the GPU end up
> + * in the same IOMMU group, so that's handled here.
> + */
> + list_for_each_entry(hose, &hose_list, list_node) {
> + phb = hose->private_data;
> +
> + if (phb->type == PNV_PHB_IODA2)
> + list_for_each_entry(pe, &phb->ioda.pe_list, list)
> + pnv_try_setup_npu_table_group(pe);
> + }
> +
> + /*
> + * Now we have all PHBs discovered, time to add NPU devices to
> + * the corresponding IOMMU groups.
> + */
> + list_for_each_entry(hose, &hose_list, list_node) {
> + unsigned long pgsizes;
> +
> + phb = hose->private_data;
> +
> + if (phb->type != PNV_PHB_NPU_NVLINK)
> + continue;
> +
> + pgsizes = pnv_ioda_parse_tce_sizes(phb);
> + list_for_each_entry(pe, &phb->ioda.pe_list, list) {
> + /*
> + * IODA2 bridges get this set up from
> + * pci_controller_ops::setup_bridge but NPU bridges
> + * do not have this hook defined so we do it here.
> + */
> + pe->table_group.pgsizes = pgsizes;
> + pnv_npu_compound_attach(pe);
> + }
> + }
> +}
> #endif /* CONFIG_IOMMU_API */
>
> int pnv_npu2_init(struct pci_controller *hose)
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index c020ade3a846..dba0c2c09f61 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1288,7 +1288,7 @@ static void pnv_ioda_setup_npu_PEs(struct pci_bus *bus)
> pnv_ioda_setup_npu_PE(pdev);
> }
>
> -static void pnv_pci_ioda_setup_PEs(void)
> +static void pnv_pci_ioda_setup_nvlink(void)
> {
> struct pci_controller *hose;
> struct pnv_phb *phb;
> @@ -1312,6 +1312,11 @@ static void pnv_pci_ioda_setup_PEs(void)
> list_for_each_entry(pe, &phb->ioda.pe_list, list)
> pnv_npu2_map_lpar(pe, MSR_DR | MSR_PR | MSR_HV);
> }
> +
> +#ifdef CONFIG_IOMMU_API
> + /* setup iommu groups so we can do nvlink pass-thru */
> + pnv_pci_npu_setup_iommu_groups();
> +#endif
> }
>
> #ifdef CONFIG_PCI_IOV
> @@ -2584,56 +2589,6 @@ static struct iommu_table_group_ops pnv_pci_ioda2_ops = {
> .take_ownership = pnv_ioda2_take_ownership,
> .release_ownership = pnv_ioda2_release_ownership,
> };
> -
> -static void pnv_pci_ioda_setup_iommu_api(void)
> -{
> - struct pci_controller *hose;
> - struct pnv_phb *phb;
> - struct pnv_ioda_pe *pe;
> -
> - /*
> - * For non-nvlink devices the IOMMU group is registered when the PE is
> - * configured and devices are added to the group when the per-device
> - * DMA setup is run. That's done in hose->ops.dma_dev_setup() which is
> - * only initialise for "normal" IODA PHBs.
> - *
> - * For NVLink devices we need to ensure the NVLinks and the GPU end up
> - * in the same IOMMU group, so that's handled here.
> - */
> - list_for_each_entry(hose, &hose_list, list_node) {
> - phb = hose->private_data;
> -
> - if (phb->type == PNV_PHB_IODA2)
> - list_for_each_entry(pe, &phb->ioda.pe_list, list)
> - pnv_try_setup_npu_table_group(pe);
> - }
> -
> - /*
> - * Now we have all PHBs discovered, time to add NPU devices to
> - * the corresponding IOMMU groups.
> - */
> - list_for_each_entry(hose, &hose_list, list_node) {
> - unsigned long pgsizes;
> -
> - phb = hose->private_data;
> -
> - if (phb->type != PNV_PHB_NPU_NVLINK)
> - continue;
> -
> - pgsizes = pnv_ioda_parse_tce_sizes(phb);
> - list_for_each_entry(pe, &phb->ioda.pe_list, list) {
> - /*
> - * IODA2 bridges get this set up from
> - * pci_controller_ops::setup_bridge but NPU bridges
> - * do not have this hook defined so we do it here.
> - */
> - pe->table_group.pgsizes = pgsizes;
> - pnv_npu_compound_attach(pe);
> - }
> - }
> -}
> -#else /* !CONFIG_IOMMU_API */
> -static void pnv_pci_ioda_setup_iommu_api(void) { };
> #endif
>
> static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
> @@ -3132,8 +3087,7 @@ static void pnv_pci_enable_bridges(void)
>
> static void pnv_pci_ioda_fixup(void)
> {
> - pnv_pci_ioda_setup_PEs();
> - pnv_pci_ioda_setup_iommu_api();
> + pnv_pci_ioda_setup_nvlink();
> pnv_pci_ioda_create_dbgfs();
>
> pnv_pci_enable_bridges();
> diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
> index 0c5845a1f05d..20941ef2706e 100644
> --- a/arch/powerpc/platforms/powernv/pci.h
> +++ b/arch/powerpc/platforms/powernv/pci.h
> @@ -209,11 +209,7 @@ extern void pe_level_printk(const struct pnv_ioda_pe *pe, const char *level,
> /* Nvlink functions */
> extern void pnv_npu_try_dma_set_bypass(struct pci_dev *gpdev, bool bypass);
> extern void pnv_pci_ioda2_tce_invalidate_entire(struct pnv_phb *phb, bool rm);
> -extern struct pnv_ioda_pe *pnv_pci_npu_setup_iommu(struct pnv_ioda_pe *npe);
> -extern struct iommu_table_group *pnv_try_setup_npu_table_group(
> - struct pnv_ioda_pe *pe);
> -extern struct iommu_table_group *pnv_npu_compound_attach(
> - struct pnv_ioda_pe *pe);
> +extern void pnv_pci_npu_setup_iommu_groups(void);
>
> /* pci-ioda-tce.c */
> #define POWERNV_IOMMU_DEFAULT_LEVELS 2
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 6/7] powerpc/powernv/pci: Move tce size parsing to pci-ioda-tce.c
From: Alexey Kardashevskiy @ 2020-04-06 9:53 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev
In-Reply-To: <20200406030745.24595-7-oohall@gmail.com>
On 06/04/2020 13:07, Oliver O'Halloran wrote:
> Move it in with the rest of the TCE wrangling rather than carting around
> a static prototype in pci-ioda.c
>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/platforms/powernv/pci-ioda-tce.c | 28 +++++++++++++++++
> arch/powerpc/platforms/powernv/pci-ioda.c | 30 -------------------
> arch/powerpc/platforms/powernv/pci.h | 2 ++
> 3 files changed, 30 insertions(+), 30 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda-tce.c b/arch/powerpc/platforms/powernv/pci-ioda-tce.c
> index 5dc6847d5f4c..f923359d8afc 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda-tce.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda-tce.c
> @@ -17,6 +17,34 @@
> #include <asm/tce.h>
> #include "pci.h"
>
> +unsigned long pnv_ioda_parse_tce_sizes(struct pnv_phb *phb)
> +{
> + struct pci_controller *hose = phb->hose;
> + struct device_node *dn = hose->dn;
> + unsigned long mask = 0;
> + int i, rc, count;
> + u32 val;
> +
> + count = of_property_count_u32_elems(dn, "ibm,supported-tce-sizes");
> + if (count <= 0) {
> + mask = SZ_4K | SZ_64K;
> + /* Add 16M for POWER8 by default */
> + if (cpu_has_feature(CPU_FTR_ARCH_207S) &&
> + !cpu_has_feature(CPU_FTR_ARCH_300))
> + mask |= SZ_16M | SZ_256M;
> + return mask;
> + }
> +
> + for (i = 0; i < count; i++) {
> + rc = of_property_read_u32_index(dn, "ibm,supported-tce-sizes",
> + i, &val);
> + if (rc == 0)
> + mask |= 1ULL << val;
> + }
> +
> + return mask;
> +}
> +
> void pnv_pci_setup_iommu_table(struct iommu_table *tbl,
> void *tce_mem, u64 tce_size,
> u64 dma_offset, unsigned int page_shift)
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 8b45b8e561e9..c020ade3a846 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -2585,8 +2585,6 @@ static struct iommu_table_group_ops pnv_pci_ioda2_ops = {
> .release_ownership = pnv_ioda2_release_ownership,
> };
>
> -static unsigned long pnv_ioda_parse_tce_sizes(struct pnv_phb *phb);
> -
> static void pnv_pci_ioda_setup_iommu_api(void)
> {
> struct pci_controller *hose;
> @@ -2638,34 +2636,6 @@ static void pnv_pci_ioda_setup_iommu_api(void)
> static void pnv_pci_ioda_setup_iommu_api(void) { };
> #endif
>
> -static unsigned long pnv_ioda_parse_tce_sizes(struct pnv_phb *phb)
> -{
> - struct pci_controller *hose = phb->hose;
> - struct device_node *dn = hose->dn;
> - unsigned long mask = 0;
> - int i, rc, count;
> - u32 val;
> -
> - count = of_property_count_u32_elems(dn, "ibm,supported-tce-sizes");
> - if (count <= 0) {
> - mask = SZ_4K | SZ_64K;
> - /* Add 16M for POWER8 by default */
> - if (cpu_has_feature(CPU_FTR_ARCH_207S) &&
> - !cpu_has_feature(CPU_FTR_ARCH_300))
> - mask |= SZ_16M | SZ_256M;
> - return mask;
> - }
> -
> - for (i = 0; i < count; i++) {
> - rc = of_property_read_u32_index(dn, "ibm,supported-tce-sizes",
> - i, &val);
> - if (rc == 0)
> - mask |= 1ULL << val;
> - }
> -
> - return mask;
> -}
> -
> static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
> struct pnv_ioda_pe *pe)
> {
> diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
> index d3bbdeab3a32..0c5845a1f05d 100644
> --- a/arch/powerpc/platforms/powernv/pci.h
> +++ b/arch/powerpc/platforms/powernv/pci.h
> @@ -244,4 +244,6 @@ extern void pnv_pci_setup_iommu_table(struct iommu_table *tbl,
> void *tce_mem, u64 tce_size,
> u64 dma_offset, unsigned int page_shift);
>
> +extern unsigned long pnv_ioda_parse_tce_sizes(struct pnv_phb *phb);
> +
> #endif /* __POWERNV_PCI_H */
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 5/7] powerpc/powernv/pci: Delete old iommu recursive iommu setup
From: Alexey Kardashevskiy @ 2020-04-06 9:53 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev
In-Reply-To: <20200406030745.24595-6-oohall@gmail.com>
On 06/04/2020 13:07, Oliver O'Halloran wrote:
> No longer used.
>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
Nit: you could fold it into 4/7.
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/platforms/powernv/pci-ioda.c | 32 -----------------------
> 1 file changed, 32 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 9198b7882b57..8b45b8e561e9 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1550,11 +1550,6 @@ void pnv_pci_sriov_disable(struct pci_dev *pdev)
>
> static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
> struct pnv_ioda_pe *pe);
> -#ifdef CONFIG_IOMMU_API
> -static void pnv_ioda_setup_bus_iommu_group(struct pnv_ioda_pe *pe,
> - struct iommu_table_group *table_group, struct pci_bus *bus);
> -
> -#endif
> static void pnv_ioda_setup_vf_PE(struct pci_dev *pdev, u16 num_vfs)
> {
> struct pci_bus *bus;
> @@ -2590,33 +2585,6 @@ static struct iommu_table_group_ops pnv_pci_ioda2_ops = {
> .release_ownership = pnv_ioda2_release_ownership,
> };
>
> -static void pnv_ioda_setup_bus_iommu_group_add_devices(struct pnv_ioda_pe *pe,
> - struct iommu_table_group *table_group,
> - struct pci_bus *bus)
> -{
> - struct pci_dev *dev;
> -
> - list_for_each_entry(dev, &bus->devices, bus_list) {
> - iommu_add_device(table_group, &dev->dev);
> -
> - if ((pe->flags & PNV_IODA_PE_BUS_ALL) && dev->subordinate)
> - pnv_ioda_setup_bus_iommu_group_add_devices(pe,
> - table_group, dev->subordinate);
> - }
> -}
> -
> -static void pnv_ioda_setup_bus_iommu_group(struct pnv_ioda_pe *pe,
> - struct iommu_table_group *table_group, struct pci_bus *bus)
> -{
> -
> - if (pe->flags & PNV_IODA_PE_DEV)
> - iommu_add_device(table_group, &pe->pdev->dev);
> -
> - if ((pe->flags & (PNV_IODA_PE_BUS | PNV_IODA_PE_BUS_ALL)) || bus)
> - pnv_ioda_setup_bus_iommu_group_add_devices(pe, table_group,
> - bus);
> -}
> -
> static unsigned long pnv_ioda_parse_tce_sizes(struct pnv_phb *phb);
>
> static void pnv_pci_ioda_setup_iommu_api(void)
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v5 18/21] powerpc64: Add prefixed instructions to instruction data type
From: Alistair Popple @ 2020-04-06 9:52 UTC (permalink / raw)
To: Jordan Niethe; +Cc: npiggin, bala24, linuxppc-dev, dja
In-Reply-To: <20200406080936.7180-19-jniethe5@gmail.com>
> diff --git a/arch/powerpc/include/asm/inst.h
> b/arch/powerpc/include/asm/inst.h index 70b37a35a91a..7e23e7146c66 100644
> --- a/arch/powerpc/include/asm/inst.h
> +++ b/arch/powerpc/include/asm/inst.h
> @@ -8,23 +8,67 @@
>
> struct ppc_inst {
> u32 val;
> +#ifdef __powerpc64__
> + u32 suffix;
> +#endif /* __powerpc64__ */
> } __packed;
>
> -#define ppc_inst(x) ((struct ppc_inst){ .val = x })
> +static inline int ppc_inst_opcode(struct ppc_inst x)
> +{
> + return x.val >> 26;
> +}
>
> static inline u32 ppc_inst_val(struct ppc_inst x)
> {
> return x.val;
> }
>
> -static inline bool ppc_inst_len(struct ppc_inst x)
> +#ifdef __powerpc64__
> +#define ppc_inst(x) ((struct ppc_inst){ .val = (x), .suffix = 0xff })
> +
> +#define ppc_inst_prefix(x, y) ((struct ppc_inst){ .val = (x), .suffix = (y)
> }) +
> +static inline u32 ppc_inst_suffix(struct ppc_inst x)
> {
> - return sizeof(struct ppc_inst);
> + return x.suffix;
> }
>
> -static inline int ppc_inst_opcode(struct ppc_inst x)
> +static inline bool ppc_inst_prefixed(struct ppc_inst x) {
> + return ((ppc_inst_val(x) >> 26) == 1) && ppc_inst_suffix(x) != 0xff;
> +}
> +
> +static inline struct ppc_inst ppc_inst_swab(struct ppc_inst x)
> {
> - return x.val >> 26;
> + return ppc_inst_prefix(swab32(ppc_inst_val(x)),
> + swab32(ppc_inst_suffix(x)));
> +}
> +
> +static inline struct ppc_inst ppc_inst_read(const struct ppc_inst *ptr)
> +{
> + u32 val, suffix = 0xff;
> + val = *(u32 *)ptr;
> + if ((val >> 26) == 1)
> + suffix = *((u32 *)ptr + 1);
> + return ppc_inst_prefix(val, suffix);
> +}
> +
> +static inline void ppc_inst_write(struct ppc_inst *ptr, struct ppc_inst x)
> +{
> + if (ppc_inst_prefixed(x)) {
> + *(u32 *)ptr = x.val;
> + *((u32 *)ptr + 1) = x.suffix;
> + } else {
> + *(u32 *)ptr = x.val;
> + }
> +}
> +
> +#else
> +
> +#define ppc_inst(x) ((struct ppc_inst){ .val = x })
> +
> +static inline bool ppc_inst_prefixed(ppc_inst x)
> +{
> + return 0;
> }
>
> static inline struct ppc_inst ppc_inst_swab(struct ppc_inst x)
> @@ -32,14 +76,31 @@ static inline struct ppc_inst ppc_inst_swab(struct
> ppc_inst x) return ppc_inst(swab32(ppc_inst_val(x)));
> }
>
> +static inline u32 ppc_inst_val(struct ppc_inst x)
> +{
> + return x.val;
> +}
> +
> static inline struct ppc_inst ppc_inst_read(const struct ppc_inst *ptr)
> {
> return *ptr;
> }
>
> +static inline void ppc_inst_write(struct ppc_inst *ptr, struct ppc_inst x)
> +{
> + *ptr = x;
> +}
> +
> +#endif /* __powerpc64__ */
> +
> static inline bool ppc_inst_equal(struct ppc_inst x, struct ppc_inst y)
> {
> return !memcmp(&x, &y, sizeof(struct ppc_inst));
> }
Apologies for not picking this up earlier, I was hoping to get to the bottom
of the issue I was seeing before you sent out v5. However the above definition
of instruction equality does not seem correct because it does not consider the
case when an instruction is not prefixed - a non-prefixed instruction should be
considered equal if the first 32-bit opcode/value is the same. Something like:
if (ppc_inst_prefixed(x) != ppc_inst_prefixed(y))
return false;
else if (ppc_inst_prefixed(x))
return !memcmp(&x, &y, sizeof(struct ppc_inst));
else
return x.val == y.val;
This was causing failures in ftrace_modify_code() as it would falsely detect
two non-prefixed instructions as being not equal due to differences in the suffix.
- Alistair
> +static inline int ppc_inst_len(struct ppc_inst x)
> +{
> + return (ppc_inst_prefixed(x)) ? 8 : 4;
> +}
> +
> #endif /* _ASM_INST_H */
> diff --git a/arch/powerpc/include/asm/kprobes.h
> b/arch/powerpc/include/asm/kprobes.h index 66b3f2983b22..4fc0e15e23a5
> 100644
> --- a/arch/powerpc/include/asm/kprobes.h
> +++ b/arch/powerpc/include/asm/kprobes.h
> @@ -43,7 +43,7 @@ extern kprobe_opcode_t optprobe_template_ret[];
> extern kprobe_opcode_t optprobe_template_end[];
>
> /* Fixed instruction size for powerpc */
> -#define MAX_INSN_SIZE 1
> +#define MAX_INSN_SIZE 2
> #define MAX_OPTIMIZED_LENGTH sizeof(kprobe_opcode_t) /* 4 bytes */
> #define MAX_OPTINSN_SIZE (optprobe_template_end - optprobe_template_entry)
> #define RELATIVEJUMP_SIZE sizeof(kprobe_opcode_t) /* 4 bytes */
> diff --git a/arch/powerpc/include/asm/uaccess.h
> b/arch/powerpc/include/asm/uaccess.h index c0a35e4586a5..5a3f486ddf02
> 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -105,11 +105,34 @@ static inline int __access_ok(unsigned long addr,
> unsigned long size, #define __put_user_inatomic(x, ptr) \
> __put_user_nosleep((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
>
> -#define __get_user_instr(x, ptr) \
> - __get_user_nocheck((x).val, (u32 *)(ptr), sizeof(u32), true)
> +#define __get_user_instr(x, ptr) \
> +({ \
> + long __gui_ret = 0; \
> + unsigned int prefix, suffix; \
> + __gui_ret = __get_user(prefix, (unsigned int __user *)ptr); \
> + if (!__gui_ret && (prefix >> 26) == 1) { \
> + __gui_ret = __get_user(suffix, (unsigned int __user *)ptr + 1); \
> + (x) = ppc_inst_prefix(prefix, suffix); \
> + } else { \
> + (x) = ppc_inst(prefix); \
> + } \
> + __gui_ret; \
> +})
> +
> +#define __get_user_instr_inatomic(x, ptr) \
> +({ \
> + long __gui_ret = 0; \
> + unsigned int prefix, suffix; \
> + __gui_ret = __get_user_inatomic(prefix, (unsigned int __user *)ptr); \
> + if (!__gui_ret && (prefix >> 26) == 1) { \
> + __gui_ret = __get_user_inatomic(suffix, (unsigned int __user *)ptr +
> 1); \ + (x) = ppc_inst_prefix(prefix, suffix); \
> + } else { \
> + (x) = ppc_inst(prefix); \
> + } \
> + __gui_ret; \
> +})
>
> -#define __get_user_instr_inatomic(x, ptr) \
> - __get_user_nosleep((x).val, (u32 *)(ptr), sizeof(u32))
> extern long __put_user_bad(void);
>
> /*
> diff --git a/arch/powerpc/include/asm/uprobes.h
> b/arch/powerpc/include/asm/uprobes.h index 7e3b329ba2d3..5bf65f5d44a9
> 100644
> --- a/arch/powerpc/include/asm/uprobes.h
> +++ b/arch/powerpc/include/asm/uprobes.h
> @@ -15,7 +15,7 @@
>
> typedef ppc_opcode_t uprobe_opcode_t;
>
> -#define MAX_UINSN_BYTES 4
> +#define MAX_UINSN_BYTES 8
> #define UPROBE_XOL_SLOT_BYTES (MAX_UINSN_BYTES)
>
> /* The following alias is needed for reference from arch-agnostic code */
> diff --git a/arch/powerpc/kernel/optprobes.c
> b/arch/powerpc/kernel/optprobes.c index 684640b8fa2e..689daf430161 100644
> --- a/arch/powerpc/kernel/optprobes.c
> +++ b/arch/powerpc/kernel/optprobes.c
> @@ -159,38 +159,38 @@ void patch_imm32_load_insns(unsigned int val,
> kprobe_opcode_t *addr)
>
> /*
> * Generate instructions to load provided immediate 64-bit value
> - * to register 'r3' and patch these instructions at 'addr'.
> + * to register 'reg' and patch these instructions at 'addr'.
> */
> -void patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr)
> +void patch_imm64_load_insns(unsigned long val, int reg, kprobe_opcode_t
> *addr) {
> - /* lis r3,(op)@highest */
> - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ADDIS |
> ___PPC_RT(3) | + /* lis reg,(op)@highest */
> + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ADDIS |
> ___PPC_RT(reg) | ((val >> 48) & 0xffff)));
> addr++;
>
> - /* ori r3,r3,(op)@higher */
> - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI |
> ___PPC_RA(3) | - ___PPC_RS(3) | ((val >> 32) & 0xffff)));
> + /* ori reg,reg,(op)@higher */
> + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI |
> ___PPC_RA(reg) | + ___PPC_RS(reg) | ((val >> 32) & 0xffff)));
> addr++;
>
> - /* rldicr r3,r3,32,31 */
> - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_RLDICR |
> ___PPC_RA(3) | - ___PPC_RS(3) | __PPC_SH64(32) | __PPC_ME64(31)));
> + /* rldicr reg,reg,32,31 */
> + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_RLDICR |
> ___PPC_RA(reg) | + ___PPC_RS(reg) | __PPC_SH64(32) |
__PPC_ME64(31)));
> addr++;
>
> - /* oris r3,r3,(op)@h */
> - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORIS |
> ___PPC_RA(3) | - ___PPC_RS(3) | ((val >> 16) & 0xffff)));
> + /* oris reg,reg,(op)@h */
> + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORIS |
> ___PPC_RA(reg) | + ___PPC_RS(reg) | ((val >> 16) & 0xffff)));
> addr++;
>
> - /* ori r3,r3,(op)@l */
> - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI |
> ___PPC_RA(3) | - ___PPC_RS(3) | (val & 0xffff)));
> + /* ori reg,reg,(op)@l */
> + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI |
> ___PPC_RA(reg) | + ___PPC_RS(reg) | (val & 0xffff)));
> }
>
> int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct
> kprobe *p) {
> - struct ppc_inst branch_op_callback, branch_emulate_step;
> + struct ppc_inst branch_op_callback, branch_emulate_step, temp;
> kprobe_opcode_t *op_callback_addr, *emulate_step_addr, *buff;
> long b_offset;
> unsigned long nip, size;
> @@ -240,7 +240,7 @@ int arch_prepare_optimized_kprobe(struct
> optimized_kprobe *op, struct kprobe *p) * Fixup the template with
> instructions to:
> * 1. load the address of the actual probepoint
> */
> - patch_imm64_load_insns((unsigned long)op, buff + TMPL_OP_IDX);
> + patch_imm64_load_insns((unsigned long)op, 3, buff + TMPL_OP_IDX);
>
> /*
> * 2. branch to optimized_callback() and emulate_step()
> @@ -271,7 +271,11 @@ int arch_prepare_optimized_kprobe(struct
> optimized_kprobe *op, struct kprobe *p) /*
> * 3. load instruction to be emulated into relevant register, and
> */
> - patch_imm32_load_insns(*p->ainsn.insn, buff + TMPL_INSN_IDX);
> + temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
> + patch_imm64_load_insns(ppc_inst_val(temp) |
> + ((u64)ppc_inst_suffix(temp) << 32),
> + 4,
> + buff + TMPL_INSN_IDX);
>
> /*
> * 4. branch back from trampoline
> diff --git a/arch/powerpc/kernel/optprobes_head.S
> b/arch/powerpc/kernel/optprobes_head.S index cf383520843f..ff8ba4d3824d
> 100644
> --- a/arch/powerpc/kernel/optprobes_head.S
> +++ b/arch/powerpc/kernel/optprobes_head.S
> @@ -94,6 +94,9 @@ optprobe_template_insn:
> /* 2, Pass instruction to be emulated in r4 */
> nop
> nop
> + nop
> + nop
> + nop
>
> .global optprobe_template_call_emulate
> optprobe_template_call_emulate:
> diff --git a/arch/powerpc/kernel/trace/ftrace.c
> b/arch/powerpc/kernel/trace/ftrace.c index e78742613b36..16041a5c86d5
> 100644
> --- a/arch/powerpc/kernel/trace/ftrace.c
> +++ b/arch/powerpc/kernel/trace/ftrace.c
> @@ -41,11 +41,35 @@
> #define NUM_FTRACE_TRAMPS 8
> static unsigned long ftrace_tramps[NUM_FTRACE_TRAMPS];
>
> +#ifdef __powerpc64__
> static long
> probe_kernel_read_inst(struct ppc_inst *inst, const void *src)
> {
> - return probe_kernel_read((void *)inst, src, MCOUNT_INSN_SIZE);
> + u32 val, suffix = 0;
> + long err;
> +
> + err = probe_kernel_read((void *)&val,
> + src, sizeof(val));
> + if (err)
> + return err;
> +
> + if ((val >> 26) == 1)
> + err = probe_kernel_read((void *)&suffix,
> + src + 4, MCOUNT_INSN_SIZE);
> + if (err)
> + return err;
> +
> + *inst = ppc_inst_prefix(val, suffix);
> +
> + return 0;
> }
> +#else
> +static long
> +probe_kernel_read_inst(struct ppc_inst *inst, const void *src)
> +{
> + return probe_kernel_read((void *)inst, src, MCOUNT_INSN_SIZE)
> +}
> +#endif
>
> static struct ppc_inst
> ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
> diff --git a/arch/powerpc/lib/code-patching.c
> b/arch/powerpc/lib/code-patching.c index c329ad657302..b4007e03d8fa 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -24,12 +24,19 @@ static int __patch_instruction(struct ppc_inst
> *exec_addr, struct ppc_inst instr {
> int err = 0;
>
> - __put_user_asm(ppc_inst_val(instr), patch_addr, err, "stw");
> - if (err)
> - return err;
> -
> - asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr),
> - "r" (exec_addr));
> + if (!ppc_inst_prefixed(instr)) {
> + __put_user_asm(ppc_inst_val(instr), patch_addr, err, "stw");
> + if (err)
> + return err;
> + asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr),
> + "r" (exec_addr));
> + } else {
> + __put_user_asm((u64)ppc_inst_suffix(instr) << 32 | ppc_inst_val(instr),
> patch_addr, err, "std"); + if (err)
> + return err;
> + asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr),
> + "r" (exec_addr));
> + }
>
> return 0;
> }
> diff --git a/arch/powerpc/lib/feature-fixups.c
> b/arch/powerpc/lib/feature-fixups.c index f00dd13b1c3c..5519cec83cc8 100644
> --- a/arch/powerpc/lib/feature-fixups.c
> +++ b/arch/powerpc/lib/feature-fixups.c
> @@ -84,12 +84,13 @@ static int patch_feature_section(unsigned long value,
> struct fixup_entry *fcur) src = alt_start;
> dest = start;
>
> - for (; src < alt_end; src++, dest++) {
> + for (; src < alt_end; src = (void *)src +
> ppc_inst_len(ppc_inst_read(src)), + (dest = (void *)dest +
> ppc_inst_len(ppc_inst_read(dest)))) { if (patch_alt_instruction(src, dest,
> alt_start, alt_end))
> return 1;
> }
>
> - for (; dest < end; dest++)
> + for (; dest < end; dest = (void *)dest +
> ppc_inst_len(ppc_inst(PPC_INST_NOP))) raw_patch_instruction(dest,
> ppc_inst(PPC_INST_NOP));
>
> return 0;
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index 52ddd3122dc8..8b285bf11218 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -1169,10 +1169,12 @@ int analyse_instr(struct instruction_op *op, const
> struct pt_regs *regs, unsigned long int imm;
> unsigned long int val, val2;
> unsigned int mb, me, sh;
> - unsigned int word;
> + unsigned int word, suffix;
> long ival;
>
> word = ppc_inst_val(instr);
> + suffix = ppc_inst_suffix(instr);
> +
> op->type = COMPUTE;
>
> opcode = word >> 26;
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index 6f3bcdcfc9c7..b704aebb099a 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -761,8 +761,8 @@ static int xmon_bpt(struct pt_regs *regs)
>
> /* Are we at the trap at bp->instr[1] for some bp? */
> bp = in_breakpoint_table(regs->nip, &offset);
> - if (bp != NULL && offset == 4) {
> - regs->nip = bp->address + 4;
> + if (bp != NULL && (offset == 4 || offset == 8)) {
> + regs->nip = bp->address + offset;
> atomic_dec(&bp->ref_count);
> return 1;
> }
> @@ -863,7 +863,7 @@ static struct bpt *in_breakpoint_table(unsigned long
> nip, unsigned long *offp) if (off >= sizeof(bpt_table))
> return NULL;
> *offp = off % BPT_SIZE;
> - if (*offp != 0 && *offp != 4)
> + if (*offp != 0 && *offp != 4 && *offp != 8)
> return NULL;
> return bpts + (off / BPT_SIZE);
> }
> diff --git a/arch/powerpc/xmon/xmon_bpts.S b/arch/powerpc/xmon/xmon_bpts.S
> index ebb2dbc70ca8..09058eb6abbd 100644
> --- a/arch/powerpc/xmon/xmon_bpts.S
> +++ b/arch/powerpc/xmon/xmon_bpts.S
> @@ -3,6 +3,8 @@
> #include <asm/asm-compat.h>
> #include "xmon_bpts.h"
>
> +/* Prefixed instructions can not cross 64 byte boundaries */
> +.align 6
> .global bpt_table
> bpt_table:
> - .space NBPTS * 8
> + .space NBPTS * 16
^ permalink raw reply
* Re: [PATCH 4/7] powerpc/powernv/pci: Add device to iommu group during dma_dev_setup()
From: Alexey Kardashevskiy @ 2020-04-06 9:51 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev
In-Reply-To: <20200406030745.24595-5-oohall@gmail.com>
On 06/04/2020 13:07, Oliver O'Halloran wrote:
> Historically adding devices to their respective iommu group has been
> handled by the post-init phb fixup for most devices. This was done
> because:
>
> 1) The IOMMU group is tied to the PE (usually) so we can only setup the
> iommu groups after we've done resource allocation since BAR location
> determines the device's PE, and:
> 2) The sysfs directory for the pci_dev needs to be available since
> iommu_add_device() wants to add an attribute for the iommu group.
>
> However, since commit 30d87ef8b38d ("powerpc/pci: Fix
> pcibios_setup_device() ordering") both conditions are met when
> hose->ops->dma_dev_setup() is called so there's no real need to do
> this in the fixup.
>
> Moving the call to iommu_add_device() into pnv_pci_ioda_dma_setup_dev()
> is a nice cleanup since it puts all the per-device IOMMU setup into one
> place. It also results in all (non-nvlink) devices getting their iommu
> group via a common path rather than relying on the bus notifier hack
> in pnv_tce_iommu_bus_notifier() to handle the adding VFs and
> hotplugged devices to their group.
>
> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/platforms/powernv/npu-dma.c | 8 ++++
> arch/powerpc/platforms/powernv/pci-ioda.c | 47 +++++++----------------
> arch/powerpc/platforms/powernv/pci.c | 20 ----------
> 3 files changed, 21 insertions(+), 54 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
> index 4fbbdfa8b327..df27b8d7e78f 100644
> --- a/arch/powerpc/platforms/powernv/npu-dma.c
> +++ b/arch/powerpc/platforms/powernv/npu-dma.c
> @@ -469,6 +469,12 @@ struct iommu_table_group *pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe)
> compound_group->pgsizes = pe->table_group.pgsizes;
> }
>
> + /*
> + * The gpu would have been added to the iommu group that's created
> + * for the PE. Pull it out now.
> + */
> + iommu_del_device(&gpdev->dev);
> +
> /*
> * I'm not sure this is strictly required, but it's probably a good idea
> * since the table_group for the PE is going to be attached to the
> @@ -478,7 +484,9 @@ struct iommu_table_group *pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe)
> */
> iommu_group_put(pe->table_group.group);
>
> + /* now put the GPU into the compound group */
> pnv_comp_attach_table_group(npucomp, pe);
> + iommu_add_device(compound_group, &gpdev->dev);
>
> return compound_group;
> }
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index cf0aaef1b8fa..9198b7882b57 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1774,12 +1774,10 @@ static void pnv_pci_ioda_dma_dev_setup(struct pci_dev *pdev)
> WARN_ON(get_dma_ops(&pdev->dev) != &dma_iommu_ops);
> pdev->dev.archdata.dma_offset = pe->tce_bypass_base;
> set_iommu_table_base(&pdev->dev, pe->table_group.tables[0]);
> - /*
> - * Note: iommu_add_device() will fail here as
> - * for physical PE: the device is already added by now;
> - * for virtual PE: sysfs entries are not ready yet and
> - * tce_iommu_bus_notifier will add the device to a group later.
> - */
> +
> + /* PEs with a DMA weight of zero won't have a group */
> + if (pe->table_group.group)
> + iommu_add_device(&pe->table_group, &pdev->dev);
> }
>
> /*
> @@ -2628,39 +2626,20 @@ static void pnv_pci_ioda_setup_iommu_api(void)
> struct pnv_ioda_pe *pe;
>
> /*
> - * There are 4 types of PEs:
> - * - PNV_IODA_PE_BUS: a downstream port with an adapter,
> - * created from pnv_pci_setup_bridge();
> - * - PNV_IODA_PE_BUS_ALL: a PCI-PCIX bridge with devices behind it,
> - * created from pnv_pci_setup_bridge();
> - * - PNV_IODA_PE_VF: a SRIOV virtual function,
> - * created from pnv_pcibios_sriov_enable();
> - * - PNV_IODA_PE_DEV: an NPU or OCAPI device,
> - * created from pnv_pci_ioda_fixup().
> + * For non-nvlink devices the IOMMU group is registered when the PE is
> + * configured and devices are added to the group when the per-device
> + * DMA setup is run. That's done in hose->ops.dma_dev_setup() which is
> + * only initialise for "normal" IODA PHBs.
> *
> - * Normally a PE is represented by an IOMMU group, however for
> - * devices with side channels the groups need to be more strict.
> + * For NVLink devices we need to ensure the NVLinks and the GPU end up
> + * in the same IOMMU group, so that's handled here.
> */
> list_for_each_entry(hose, &hose_list, list_node) {
> phb = hose->private_data;
>
> - if (phb->type == PNV_PHB_NPU_NVLINK ||
> - phb->type == PNV_PHB_NPU_OCAPI)
> - continue;
> -
> - list_for_each_entry(pe, &phb->ioda.pe_list, list) {
> - struct iommu_table_group *table_group;
> -
> - table_group = pnv_try_setup_npu_table_group(pe);
> - if (!table_group) {
> - if (!pnv_pci_ioda_pe_dma_weight(pe))
> - continue;
> -
> - table_group = &pe->table_group;
> - }
> - pnv_ioda_setup_bus_iommu_group(pe, table_group,
> - pe->pbus);
> - }
> + if (phb->type == PNV_PHB_IODA2)
> + list_for_each_entry(pe, &phb->ioda.pe_list, list)
> + pnv_try_setup_npu_table_group(pe);
> }
>
> /*
> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
> index 5bf818246339..091fe1cf386b 100644
> --- a/arch/powerpc/platforms/powernv/pci.c
> +++ b/arch/powerpc/platforms/powernv/pci.c
> @@ -955,28 +955,8 @@ static int pnv_tce_iommu_bus_notifier(struct notifier_block *nb,
> unsigned long action, void *data)
> {
> struct device *dev = data;
> - struct pci_dev *pdev;
> - struct pci_dn *pdn;
> - struct pnv_ioda_pe *pe;
> - struct pci_controller *hose;
> - struct pnv_phb *phb;
>
> switch (action) {
> - case BUS_NOTIFY_ADD_DEVICE:
> - pdev = to_pci_dev(dev);
> - pdn = pci_get_pdn(pdev);
> - hose = pci_bus_to_host(pdev->bus);
> - phb = hose->private_data;
> -
> - WARN_ON_ONCE(!phb);
> - if (!pdn || pdn->pe_number == IODA_INVALID_PE || !phb)
> - return 0;
> -
> - pe = &phb->ioda.pe_array[pdn->pe_number];
> - if (!pe->table_group.group)
> - return 0;
> - iommu_add_device(&pe->table_group, dev);
> - return 0;
> case BUS_NOTIFY_DEL_DEVICE:
> iommu_del_device(dev);
> return 0;
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 3/7] powerpc/powernv/pci: Register iommu group at PE DMA setup
From: Alexey Kardashevskiy @ 2020-04-06 9:51 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev; +Cc: Alistair Popple, Reza Arbab
In-Reply-To: <20200406030745.24595-4-oohall@gmail.com>
On 06/04/2020 13:07, Oliver O'Halloran wrote:
> Move the registration of IOMMU groups out of the post-phb init fixup and
> into when we configure DMA for a PE. For most devices this doesn't
> result in any functional changes, but for NVLink attached GPUs it
> requires a bit of care. When the GPU is probed an IOMMU group would be
> created for the PE that contains it. We need to ensure that group is
> removed before we add the PE to the compound group that's used to keep
> the translations see by the PCIe and NVLink buses the same.
>
> No functional changes. Probably.
>
> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> Cc: Reza Arbab <arbab@linux.ibm.com>
> Cc: Alistair Popple <alistair@popple.id.au>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
> arch/powerpc/platforms/powernv/npu-dma.c | 9 +++++++++
> arch/powerpc/platforms/powernv/pci-ioda.c | 16 ++++++----------
> 2 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
> index de617549c9a3..4fbbdfa8b327 100644
> --- a/arch/powerpc/platforms/powernv/npu-dma.c
> +++ b/arch/powerpc/platforms/powernv/npu-dma.c
> @@ -469,6 +469,15 @@ struct iommu_table_group *pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe)
> compound_group->pgsizes = pe->table_group.pgsizes;
> }
>
> + /*
> + * I'm not sure this is strictly required, but it's probably a good idea
> + * since the table_group for the PE is going to be attached to the
This seems just a right thing to do so I suggest changing the comment
from "not sure" to "we are going to put GPU to a compound group and
won't need the individual group".
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> + * compound table group. If we leave the PE's iommu group active then
> + * we might have the same table_group being modifiable via two sepeate
> + * iommu groups.
> + */
> + iommu_group_put(pe->table_group.group);
> +
> pnv_comp_attach_table_group(npucomp, pe);
>
> return compound_group;
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 2c340504fa77..cf0aaef1b8fa 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1619,10 +1619,6 @@ static void pnv_ioda_setup_vf_PE(struct pci_dev *pdev, u16 num_vfs)
> }
>
> pnv_pci_ioda2_setup_dma_pe(phb, pe);
> -#ifdef CONFIG_IOMMU_API
> - iommu_register_group(&pe->table_group,
> - pe->phb->hose->global_number, pe->pe_number);
> -#endif
> }
> }
>
> @@ -2661,9 +2657,6 @@ static void pnv_pci_ioda_setup_iommu_api(void)
> continue;
>
> table_group = &pe->table_group;
> - iommu_register_group(&pe->table_group,
> - pe->phb->hose->global_number,
> - pe->pe_number);
> }
> pnv_ioda_setup_bus_iommu_group(pe, table_group,
> pe->pbus);
> @@ -2748,14 +2741,17 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
> IOMMU_TABLE_GROUP_MAX_TABLES;
> pe->table_group.max_levels = POWERNV_IOMMU_MAX_LEVELS;
> pe->table_group.pgsizes = pnv_ioda_parse_tce_sizes(phb);
> -#ifdef CONFIG_IOMMU_API
> - pe->table_group.ops = &pnv_pci_ioda2_ops;
> -#endif
>
> rc = pnv_pci_ioda2_setup_default_config(pe);
> if (rc)
> return;
>
> +#ifdef CONFIG_IOMMU_API
> + pe->table_group.ops = &pnv_pci_ioda2_ops;
> + iommu_register_group(&pe->table_group, phb->hose->global_number,
> + pe->pe_number);
> +#endif
> +
> if (pe->flags & (PNV_IODA_PE_BUS | PNV_IODA_PE_BUS_ALL))
> pnv_ioda_setup_bus_dma(pe, pe->pbus);
> }
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 2/7] powerpc/powernv/iov: Don't add VFs to iommu group during PE config
From: Alexey Kardashevskiy @ 2020-04-06 9:49 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev
In-Reply-To: <20200406030745.24595-3-oohall@gmail.com>
On 06/04/2020 13:07, Oliver O'Halloran wrote:
> In pnv_ioda_setup_vf_PE() we register an iommu group for the VF PE
> then call pnv_ioda_setup_bus_iommu_group() to add devices to that group.
> However, this function is called before the VFs are scanned so there's
> no devices to add.
>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/platforms/powernv/pci-ioda.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 57d3a6af1d52..2c340504fa77 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1622,7 +1622,6 @@ static void pnv_ioda_setup_vf_PE(struct pci_dev *pdev, u16 num_vfs)
> #ifdef CONFIG_IOMMU_API
> iommu_register_group(&pe->table_group,
> pe->phb->hose->global_number, pe->pe_number);
> - pnv_ioda_setup_bus_iommu_group(pe, &pe->table_group, NULL);
> #endif
> }
> }
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 1/7] powerpc/powernv/npu: Clean up compound table group initialisation
From: Alexey Kardashevskiy @ 2020-04-06 9:48 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev
In-Reply-To: <20200406030745.24595-2-oohall@gmail.com>
On 06/04/2020 13:07, Oliver O'Halloran wrote:
> Re-work the control flow a bit so what's going on is a little clearer.
> This also ensures the table_group is only initialised once in the P9
> case. This shouldn't be a functional change since all the GPU PCI
> devices should have the same table_group configuration, but it does
> look strange.
>
> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/platforms/powernv/npu-dma.c | 46 +++++++++++-------------
> 1 file changed, 21 insertions(+), 25 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
> index b95b9e3c4c98..de617549c9a3 100644
> --- a/arch/powerpc/platforms/powernv/npu-dma.c
> +++ b/arch/powerpc/platforms/powernv/npu-dma.c
> @@ -427,7 +427,7 @@ static void pnv_comp_attach_table_group(struct npu_comp *npucomp,
>
> struct iommu_table_group *pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe)
> {
> - struct iommu_table_group *table_group;
> + struct iommu_table_group *compound_group;
> struct npu_comp *npucomp;
> struct pci_dev *gpdev = NULL;
> struct pci_controller *hose;
> @@ -446,36 +446,32 @@ struct iommu_table_group *pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe)
> hose = pci_bus_to_host(npdev->bus);
>
> if (hose->npu) {
> - table_group = &hose->npu->npucomp.table_group;
> -
> - if (!table_group->group) {
> - table_group->ops = &pnv_npu_peers_ops;
> - iommu_register_group(table_group,
> - hose->global_number,
> - pe->pe_number);
> - }
> + /* P9 case: compound group is per-NPU (all gpus, all links) */
> + npucomp = &hose->npu->npucomp;
> } else {
> - /* Create a group for 1 GPU and attached NPUs for POWER8 */
> - pe->npucomp = kzalloc(sizeof(*pe->npucomp), GFP_KERNEL);
> - table_group = &pe->npucomp->table_group;
> - table_group->ops = &pnv_npu_peers_ops;
> - iommu_register_group(table_group, hose->global_number,
> - pe->pe_number);
> + /* P8 case: Compound group is per-GPU (1 gpu, 2 links) */
> + npucomp = pe->npucomp = kzalloc(sizeof(*npucomp), GFP_KERNEL);
> }
>
> - /* Steal capabilities from a GPU PE */
> - table_group->max_dynamic_windows_supported =
> - pe->table_group.max_dynamic_windows_supported;
> - table_group->tce32_start = pe->table_group.tce32_start;
> - table_group->tce32_size = pe->table_group.tce32_size;
> - table_group->max_levels = pe->table_group.max_levels;
> - if (!table_group->pgsizes)
> - table_group->pgsizes = pe->table_group.pgsizes;
> + compound_group = &npucomp->table_group;
> + if (!compound_group->group) {
> + compound_group->ops = &pnv_npu_peers_ops;
> + iommu_register_group(compound_group, hose->global_number,
> + pe->pe_number);
> +
> + /* Steal capabilities from a GPU PE */
> + compound_group->max_dynamic_windows_supported =
> + pe->table_group.max_dynamic_windows_supported;
> + compound_group->tce32_start = pe->table_group.tce32_start;
> + compound_group->tce32_size = pe->table_group.tce32_size;
> + compound_group->max_levels = pe->table_group.max_levels;
> + if (!compound_group->pgsizes)
> + compound_group->pgsizes = pe->table_group.pgsizes;
> + }
>
> - npucomp = container_of(table_group, struct npu_comp, table_group);
> pnv_comp_attach_table_group(npucomp, pe);
>
> - return table_group;
> + return compound_group;
> }
>
> struct iommu_table_group *pnv_npu_compound_attach(struct pnv_ioda_pe *pe)
>
--
Alexey
^ permalink raw reply
* [PATCH v4] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export
From: Qiujun Huang @ 2020-04-06 9:39 UTC (permalink / raw)
To: benh, paulus, mpe, oohall
Cc: linux-kernel, Markus.Elfring, tglx, linuxppc-dev, Qiujun Huang
Here needs a NULL check as kzalloc may fail returning NULL.
Issue was found by coccinelle.
Generated by: scripts/coccinelle/null/kmerr.cocci
Signed-off-by: Qiujun Huang <hqjagain@gmail.com>
Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
---
v3->v4:
Added the information about coccinelle script.
Added change log.
Added Oliver's Reviewed-by.
v2->v3:
Removed redundant assignment to 'attr' and 'name'.
v1->v2:
Just return -ENOMEM if attr is NULL.
---
arch/powerpc/platforms/powernv/opal.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 2b3dfd0b6cdd..908d749bcef5 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -801,16 +801,19 @@ static ssize_t export_attr_read(struct file *fp, struct kobject *kobj,
static int opal_add_one_export(struct kobject *parent, const char *export_name,
struct device_node *np, const char *prop_name)
{
- struct bin_attribute *attr = NULL;
- const char *name = NULL;
+ struct bin_attribute *attr;
+ const char *name;
u64 vals[2];
int rc;
rc = of_property_read_u64_array(np, prop_name, &vals[0], 2);
if (rc)
- goto out;
+ return rc;
attr = kzalloc(sizeof(*attr), GFP_KERNEL);
+ if (!attr)
+ return -ENOMEM;
+
name = kstrdup(export_name, GFP_KERNEL);
if (!name) {
rc = -ENOMEM;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v5 05/21] powerpc: Use a function for getting the instruction op code
From: Jordan Niethe @ 2020-04-06 9:38 UTC (permalink / raw)
To: Christophe Leroy
Cc: Alistair Popple, Nicholas Piggin, Balamuruhan S, linuxppc-dev,
Daniel Axtens
In-Reply-To: <cedd91d6-033a-c503-4b30-9366cac5bc7f@c-s.fr>
On Mon, Apr 6, 2020 at 6:22 PM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>
>
> Le 06/04/2020 à 10:09, Jordan Niethe a écrit :
> > In preparation for using a data type for instructions that can not be
> > directly used with the '>>' operator use a function for getting the op
> > code of an instruction.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v4: New to series
> > ---
> > arch/powerpc/include/asm/inst.h | 5 +++++
> > arch/powerpc/kernel/align.c | 2 +-
> > arch/powerpc/lib/code-patching.c | 4 ++--
>
> What about store_updates_sp() in mm/fault.c ?
True. An early revision of this series used analyse_instr() there,
which ended up causing issues. But it still can use the instruction
data type. I will change that.
>
> Christophe
>
> > 3 files changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
> > index 5298ba33b6e5..93959016fe4b 100644
> > --- a/arch/powerpc/include/asm/inst.h
> > +++ b/arch/powerpc/include/asm/inst.h
> > @@ -8,4 +8,9 @@
> >
> > #define ppc_inst(x) (x)
> >
> > +static inline int ppc_inst_opcode(u32 x)
> > +{
> > + return x >> 26;
> > +}
> > +
> > #endif /* _ASM_INST_H */
> > diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
> > index 86e9bf62f18c..691013aa9f3c 100644
> > --- a/arch/powerpc/kernel/align.c
> > +++ b/arch/powerpc/kernel/align.c
> > @@ -314,7 +314,7 @@ int fix_alignment(struct pt_regs *regs)
> > }
> >
> > #ifdef CONFIG_SPE
> > - if ((instr >> 26) == 0x4) {
> > + if (ppc_inst_opcode(instr) == 0x4) {
> > int reg = (instr >> 21) & 0x1f;
> > PPC_WARN_ALIGNMENT(spe, regs);
> > return emulate_spe(regs, reg, instr);
> > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> > index fdf0d6ea3575..099a515202aa 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -231,7 +231,7 @@ bool is_offset_in_branch_range(long offset)
> > */
> > bool is_conditional_branch(unsigned int instr)
> > {
> > - unsigned int opcode = instr >> 26;
> > + unsigned int opcode = ppc_inst_opcode(instr);
> >
> > if (opcode == 16) /* bc, bca, bcl, bcla */
> > return true;
> > @@ -289,7 +289,7 @@ int create_cond_branch(unsigned int *instr, const unsigned int *addr,
> >
> > static unsigned int branch_opcode(unsigned int instr)
> > {
> > - return (instr >> 26) & 0x3F;
> > + return ppc_inst_opcode(instr) & 0x3F;
> > }
> >
> > static int instr_is_branch_iform(unsigned int instr)
> >
^ permalink raw reply
* Re: [PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export
From: Qiujun Huang @ 2020-04-06 9:23 UTC (permalink / raw)
To: Oliver O'Halloran
Cc: Paul Mackerras, Thomas Gleixner, linuxppc-dev, Markus Elfring,
LKML
In-Reply-To: <CAOSf1CGLZ5H1cUsv6atZ7hNpQ+PDyyAjp6dzJjXMwC5XoUOVQQ@mail.gmail.com>
On Mon, Apr 6, 2020 at 5:01 PM Oliver O'Halloran <oohall@gmail.com> wrote:
>
> On Mon, Apr 6, 2020 at 11:15 AM Qiujun Huang <hqjagain@gmail.com> wrote:
> >
> > On Mon, Apr 6, 2020 at 3:06 AM Markus Elfring <Markus.Elfring@web.de> wrote:
> > >
> > > > Here needs a NULL check.
> > quite obvious?
> > >
> > > I find this change description questionable
> > > (despite of a reasonable patch subject).
> > >
> > >
> > > > Issue found by coccinelle.
> > >
> > > Would an information like “Generated by: scripts/coccinelle/null/kmerr.cocci”
> > > be nicer?
> > Yeah, but I think It was enough.
>
> I didn't know we had that script in the kernel tree so I think it's a
> good to mention that you used it. It might even help idiots like me
> who write this sort of bug.
Yes, I will resend the patch. Thanks :-)
>
> > > Will a patch change log be helpful here?
> > I realized I should write some change log, and the change log was meaningless.
> > So I left it blank.
>
> The changelog is fine IMO. The point of a changelog is to tell a
> reader doing git archeology why a change happened and this is
> sufficent for that.
Get that.
>
> Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
^ permalink raw reply
* Re: [PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export
From: Oliver O'Halloran @ 2020-04-06 9:01 UTC (permalink / raw)
To: Qiujun Huang
Cc: Paul Mackerras, Thomas Gleixner, linuxppc-dev, Markus Elfring,
LKML
In-Reply-To: <CAJRQjofnnhzOp4yis=sL85ESDvNApXHL3dpv6T1NJL-Wh0OOfA@mail.gmail.com>
On Mon, Apr 6, 2020 at 11:15 AM Qiujun Huang <hqjagain@gmail.com> wrote:
>
> On Mon, Apr 6, 2020 at 3:06 AM Markus Elfring <Markus.Elfring@web.de> wrote:
> >
> > > Here needs a NULL check.
> quite obvious?
> >
> > I find this change description questionable
> > (despite of a reasonable patch subject).
> >
> >
> > > Issue found by coccinelle.
> >
> > Would an information like “Generated by: scripts/coccinelle/null/kmerr.cocci”
> > be nicer?
> Yeah, but I think It was enough.
I didn't know we had that script in the kernel tree so I think it's a
good to mention that you used it. It might even help idiots like me
who write this sort of bug.
> > Will a patch change log be helpful here?
> I realized I should write some change log, and the change log was meaningless.
> So I left it blank.
The changelog is fine IMO. The point of a changelog is to tell a
reader doing git archeology why a change happened and this is
sufficent for that.
Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
^ permalink raw reply
* Re: [PATCH v5 05/21] powerpc: Use a function for getting the instruction op code
From: Christophe Leroy @ 2020-04-06 8:22 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: bala24, alistair, dja, npiggin
In-Reply-To: <20200406080936.7180-6-jniethe5@gmail.com>
Le 06/04/2020 à 10:09, Jordan Niethe a écrit :
> In preparation for using a data type for instructions that can not be
> directly used with the '>>' operator use a function for getting the op
> code of an instruction.
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v4: New to series
> ---
> arch/powerpc/include/asm/inst.h | 5 +++++
> arch/powerpc/kernel/align.c | 2 +-
> arch/powerpc/lib/code-patching.c | 4 ++--
What about store_updates_sp() in mm/fault.c ?
Christophe
> 3 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
> index 5298ba33b6e5..93959016fe4b 100644
> --- a/arch/powerpc/include/asm/inst.h
> +++ b/arch/powerpc/include/asm/inst.h
> @@ -8,4 +8,9 @@
>
> #define ppc_inst(x) (x)
>
> +static inline int ppc_inst_opcode(u32 x)
> +{
> + return x >> 26;
> +}
> +
> #endif /* _ASM_INST_H */
> diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
> index 86e9bf62f18c..691013aa9f3c 100644
> --- a/arch/powerpc/kernel/align.c
> +++ b/arch/powerpc/kernel/align.c
> @@ -314,7 +314,7 @@ int fix_alignment(struct pt_regs *regs)
> }
>
> #ifdef CONFIG_SPE
> - if ((instr >> 26) == 0x4) {
> + if (ppc_inst_opcode(instr) == 0x4) {
> int reg = (instr >> 21) & 0x1f;
> PPC_WARN_ALIGNMENT(spe, regs);
> return emulate_spe(regs, reg, instr);
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index fdf0d6ea3575..099a515202aa 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -231,7 +231,7 @@ bool is_offset_in_branch_range(long offset)
> */
> bool is_conditional_branch(unsigned int instr)
> {
> - unsigned int opcode = instr >> 26;
> + unsigned int opcode = ppc_inst_opcode(instr);
>
> if (opcode == 16) /* bc, bca, bcl, bcla */
> return true;
> @@ -289,7 +289,7 @@ int create_cond_branch(unsigned int *instr, const unsigned int *addr,
>
> static unsigned int branch_opcode(unsigned int instr)
> {
> - return (instr >> 26) & 0x3F;
> + return ppc_inst_opcode(instr) & 0x3F;
> }
>
> static int instr_is_branch_iform(unsigned int instr)
>
^ permalink raw reply
* [PATCH v5 21/21] powerpc sstep: Add support for prefixed fixed-point arithmetic
From: Jordan Niethe @ 2020-04-06 8:09 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, npiggin, bala24, Jordan Niethe, dja
In-Reply-To: <20200406080936.7180-1-jniethe5@gmail.com>
This adds emulation support for the following prefixed Fixed-Point
Arithmetic instructions:
* Prefixed Add Immediate (paddi)
Reviewed-by: Balamuruhan S <bala24@linux.ibm.com>
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v3: Since we moved the prefixed loads/stores into the load/store switch
statement it no longer makes sense to have paddi in there, so move it
out.
---
arch/powerpc/lib/sstep.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 8b6aee0ee636..eb24dbfc051a 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1335,6 +1335,26 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
switch (opcode) {
#ifdef __powerpc64__
+ case 1:
+ prefix_r = word & (1ul << 20);
+ ra = (suffix >> 16) & 0x1f;
+ rd = (suffix >> 21) & 0x1f;
+ op->reg = rd;
+ op->val = regs->gpr[rd];
+ suffixopcode = suffix >> 26;
+ prefixtype = (word >> 24) & 0x3;
+ switch (prefixtype) {
+ case 2:
+ if (prefix_r && ra)
+ return 0;
+ switch (suffixopcode) {
+ case 14: /* paddi */
+ op->type = COMPUTE | PREFIXED;
+ op->val = mlsd_8lsd_ea(word, suffix, regs);
+ goto compute_done;
+ }
+ }
+ break;
case 2: /* tdi */
if (rd & trap_compare(regs->gpr[ra], (short) word))
goto trap;
--
2.17.1
^ permalink raw reply related
* [PATCH v5 20/21] powerpc sstep: Add support for prefixed load/stores
From: Jordan Niethe @ 2020-04-06 8:09 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, npiggin, bala24, Jordan Niethe, dja
In-Reply-To: <20200406080936.7180-1-jniethe5@gmail.com>
This adds emulation support for the following prefixed integer
load/stores:
* Prefixed Load Byte and Zero (plbz)
* Prefixed Load Halfword and Zero (plhz)
* Prefixed Load Halfword Algebraic (plha)
* Prefixed Load Word and Zero (plwz)
* Prefixed Load Word Algebraic (plwa)
* Prefixed Load Doubleword (pld)
* Prefixed Store Byte (pstb)
* Prefixed Store Halfword (psth)
* Prefixed Store Word (pstw)
* Prefixed Store Doubleword (pstd)
* Prefixed Load Quadword (plq)
* Prefixed Store Quadword (pstq)
the follow prefixed floating-point load/stores:
* Prefixed Load Floating-Point Single (plfs)
* Prefixed Load Floating-Point Double (plfd)
* Prefixed Store Floating-Point Single (pstfs)
* Prefixed Store Floating-Point Double (pstfd)
and for the following prefixed VSX load/stores:
* Prefixed Load VSX Scalar Doubleword (plxsd)
* Prefixed Load VSX Scalar Single-Precision (plxssp)
* Prefixed Load VSX Vector [0|1] (plxv, plxv0, plxv1)
* Prefixed Store VSX Scalar Doubleword (pstxsd)
* Prefixed Store VSX Scalar Single-Precision (pstxssp)
* Prefixed Store VSX Vector [0|1] (pstxv, pstxv0, pstxv1)
Reviewed-by: Balamuruhan S <bala24@linux.ibm.com>
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: - Combine all load/store patches
- Fix the name of Type 01 instructions
- Remove sign extension flag from pstd/pld
- Rename sufx -> suffix
v3: - Move prefixed loads and stores into the switch statement
---
arch/powerpc/include/asm/sstep.h | 4 +
arch/powerpc/lib/sstep.c | 159 +++++++++++++++++++++++++++++++
2 files changed, 163 insertions(+)
diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
index c3ce903ac488..9b200a5f8794 100644
--- a/arch/powerpc/include/asm/sstep.h
+++ b/arch/powerpc/include/asm/sstep.h
@@ -90,11 +90,15 @@ enum instruction_type {
#define VSX_LDLEFT 4 /* load VSX register from left */
#define VSX_CHECK_VEC 8 /* check MSR_VEC not MSR_VSX for reg >= 32 */
+/* Prefixed flag, ORed in with type */
+#define PREFIXED 0x800
+
/* Size field in type word */
#define SIZE(n) ((n) << 12)
#define GETSIZE(w) ((w) >> 12)
#define GETTYPE(t) ((t) & INSTR_TYPE_MASK)
+#define GETLENGTH(t) (((t) & PREFIXED) ? 8 : 4)
#define MKOP(t, f, s) ((t) | (f) | SIZE(s))
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 8b285bf11218..8b6aee0ee636 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -187,6 +187,44 @@ static nokprobe_inline unsigned long xform_ea(unsigned int instr,
return ea;
}
+/*
+ * Calculate effective address for a MLS:D-form / 8LS:D-form
+ * prefixed instruction
+ */
+static nokprobe_inline unsigned long mlsd_8lsd_ea(unsigned int instr,
+ unsigned int suffix,
+ const struct pt_regs *regs)
+{
+ int ra, prefix_r;
+ unsigned int dd;
+ unsigned long ea, d0, d1, d;
+
+ prefix_r = instr & (1ul << 20);
+ ra = (suffix >> 16) & 0x1f;
+
+ d0 = instr & 0x3ffff;
+ d1 = suffix & 0xffff;
+ d = (d0 << 16) | d1;
+
+ /*
+ * sign extend a 34 bit number
+ */
+ dd = (unsigned int)(d >> 2);
+ ea = (signed int)dd;
+ ea = (ea << 2) | (d & 0x3);
+
+ if (!prefix_r && ra)
+ ea += regs->gpr[ra];
+ else if (!prefix_r && !ra)
+ ; /* Leave ea as is */
+ else if (prefix_r && !ra)
+ ea += regs->nip;
+ else if (prefix_r && ra)
+ ; /* Invalid form. Should already be checked for by caller! */
+
+ return ea;
+}
+
/*
* Return the largest power of 2, not greater than sizeof(unsigned long),
* such that x is a multiple of it.
@@ -1166,6 +1204,7 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
struct ppc_inst instr)
{
unsigned int opcode, ra, rb, rc, rd, spr, u;
+ unsigned int suffixopcode, prefixtype, prefix_r;
unsigned long int imm;
unsigned long int val, val2;
unsigned int mb, me, sh;
@@ -2652,6 +2691,126 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
break;
}
break;
+ case 1: /* Prefixed instructions */
+ prefix_r = word & (1ul << 20);
+ ra = (suffix >> 16) & 0x1f;
+ op->update_reg = ra;
+ rd = (suffix >> 21) & 0x1f;
+ op->reg = rd;
+ op->val = regs->gpr[rd];
+
+ suffixopcode = suffix >> 26;
+ prefixtype = (word >> 24) & 0x3;
+ switch (prefixtype) {
+ case 0: /* Type 00 Eight-Byte Load/Store */
+ if (prefix_r && ra)
+ break;
+ op->ea = mlsd_8lsd_ea(word, suffix, regs);
+ switch (suffixopcode) {
+ case 41: /* plwa */
+ op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 4);
+ break;
+ case 42: /* plxsd */
+ op->reg = rd + 32;
+ op->type = MKOP(LOAD_VSX, PREFIXED, 8);
+ op->element_size = 8;
+ op->vsx_flags = VSX_CHECK_VEC;
+ break;
+ case 43: /* plxssp */
+ op->reg = rd + 32;
+ op->type = MKOP(LOAD_VSX, PREFIXED, 4);
+ op->element_size = 8;
+ op->vsx_flags = VSX_FPCONV | VSX_CHECK_VEC;
+ break;
+ case 46: /* pstxsd */
+ op->reg = rd + 32;
+ op->type = MKOP(STORE_VSX, PREFIXED, 8);
+ op->element_size = 8;
+ op->vsx_flags = VSX_CHECK_VEC;
+ break;
+ case 47: /* pstxssp */
+ op->reg = rd + 32;
+ op->type = MKOP(STORE_VSX, PREFIXED, 4);
+ op->element_size = 8;
+ op->vsx_flags = VSX_FPCONV | VSX_CHECK_VEC;
+ break;
+ case 51: /* plxv1 */
+ op->reg += 32;
+
+ /* fallthru */
+ case 50: /* plxv0 */
+ op->type = MKOP(LOAD_VSX, PREFIXED, 16);
+ op->element_size = 16;
+ op->vsx_flags = VSX_CHECK_VEC;
+ break;
+ case 55: /* pstxv1 */
+ op->reg = rd + 32;
+
+ /* fallthru */
+ case 54: /* pstxv0 */
+ op->type = MKOP(STORE_VSX, PREFIXED, 16);
+ op->element_size = 16;
+ op->vsx_flags = VSX_CHECK_VEC;
+ break;
+ case 56: /* plq */
+ op->type = MKOP(LOAD, PREFIXED, 16);
+ break;
+ case 57: /* pld */
+ op->type = MKOP(LOAD, PREFIXED, 8);
+ break;
+ case 60: /* stq */
+ op->type = MKOP(STORE, PREFIXED, 16);
+ break;
+ case 61: /* pstd */
+ op->type = MKOP(STORE, PREFIXED, 8);
+ break;
+ }
+ break;
+ case 1: /* Type 01 Eight-Byte Register-to-Register */
+ break;
+ case 2: /* Type 10 Modified Load/Store */
+ if (prefix_r && ra)
+ break;
+ op->ea = mlsd_8lsd_ea(word, suffix, regs);
+ switch (suffixopcode) {
+ case 32: /* plwz */
+ op->type = MKOP(LOAD, PREFIXED, 4);
+ break;
+ case 34: /* plbz */
+ op->type = MKOP(LOAD, PREFIXED, 1);
+ break;
+ case 36: /* pstw */
+ op->type = MKOP(STORE, PREFIXED, 4);
+ break;
+ case 38: /* pstb */
+ op->type = MKOP(STORE, PREFIXED, 1);
+ break;
+ case 40: /* plhz */
+ op->type = MKOP(LOAD, PREFIXED, 2);
+ break;
+ case 42: /* plha */
+ op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 2);
+ break;
+ case 44: /* psth */
+ op->type = MKOP(STORE, PREFIXED, 2);
+ break;
+ case 48: /* plfs */
+ op->type = MKOP(LOAD_FP, PREFIXED | FPCONV, 4);
+ break;
+ case 50: /* plfd */
+ op->type = MKOP(LOAD_FP, PREFIXED, 8);
+ break;
+ case 52: /* pstfs */
+ op->type = MKOP(STORE_FP, PREFIXED | FPCONV, 4);
+ break;
+ case 54: /* pstfd */
+ op->type = MKOP(STORE_FP, PREFIXED, 8);
+ break;
+ }
+ break;
+ case 3: /* Type 11 Modified Register-to-Register */
+ break;
+ }
#endif /* __powerpc64__ */
}
--
2.17.1
^ permalink raw reply related
* [PATCH v5 19/21] powerpc: Support prefixed instructions in alignment handler
From: Jordan Niethe @ 2020-04-06 8:09 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, npiggin, bala24, Jordan Niethe, dja
In-Reply-To: <20200406080936.7180-1-jniethe5@gmail.com>
If a prefixed instruction results in an alignment exception, the
SRR1_PREFIXED bit is set. The handler attempts to emulate the
responsible instruction and then increment the NIP past it. Use
SRR1_PREFIXED to determine by how much the NIP should be incremented.
Prefixed instructions are not permitted to cross 64-byte boundaries. If
they do the alignment interrupt is invoked with SRR1 BOUNDARY bit set.
If this occurs send a SIGBUS to the offending process if in user mode.
If in kernel mode call bad_page_fault().
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: - Move __get_user_instr() and __get_user_instr_inatomic() to this
commit (previously in "powerpc sstep: Prepare to support prefixed
instructions").
- Rename sufx to suffix
- Use a macro for calculating instruction length
v3: Move __get_user_{instr(), instr_inatomic()} up with the other
get_user definitions and remove nested if.
v4: Rolled into "Add prefixed instructions to instruction data type"
v5: Only one definition of inst_length()
---
arch/powerpc/kernel/traps.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index a4764b039749..9b97d2e6055a 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -583,6 +583,8 @@ static inline int check_io_access(struct pt_regs *regs)
#define REASON_ILLEGAL (ESR_PIL | ESR_PUO)
#define REASON_PRIVILEGED ESR_PPR
#define REASON_TRAP ESR_PTR
+#define REASON_PREFIXED 0
+#define REASON_BOUNDARY 0
/* single-step stuff */
#define single_stepping(regs) (current->thread.debug.dbcr0 & DBCR0_IC)
@@ -597,12 +599,16 @@ static inline int check_io_access(struct pt_regs *regs)
#define REASON_ILLEGAL SRR1_PROGILL
#define REASON_PRIVILEGED SRR1_PROGPRIV
#define REASON_TRAP SRR1_PROGTRAP
+#define REASON_PREFIXED SRR1_PREFIXED
+#define REASON_BOUNDARY SRR1_BOUNDARY
#define single_stepping(regs) ((regs)->msr & MSR_SE)
#define clear_single_step(regs) ((regs)->msr &= ~MSR_SE)
#define clear_br_trace(regs) ((regs)->msr &= ~MSR_BE)
#endif
+#define inst_length(reason) (((reason) & REASON_PREFIXED) ? 8 : 4)
+
#if defined(CONFIG_E500)
int machine_check_e500mc(struct pt_regs *regs)
{
@@ -1593,11 +1599,20 @@ void alignment_exception(struct pt_regs *regs)
{
enum ctx_state prev_state = exception_enter();
int sig, code, fixed = 0;
+ unsigned long reason;
/* We restore the interrupt state now */
if (!arch_irq_disabled_regs(regs))
local_irq_enable();
+ reason = get_reason(regs);
+
+ if (reason & REASON_BOUNDARY) {
+ sig = SIGBUS;
+ code = BUS_ADRALN;
+ goto bad;
+ }
+
if (tm_abort_check(regs, TM_CAUSE_ALIGNMENT | TM_CAUSE_PERSISTENT))
goto bail;
@@ -1606,7 +1621,8 @@ void alignment_exception(struct pt_regs *regs)
fixed = fix_alignment(regs);
if (fixed == 1) {
- regs->nip += 4; /* skip over emulated instruction */
+ /* skip over emulated instruction */
+ regs->nip += inst_length(reason);
emulate_single_step(regs);
goto bail;
}
@@ -1619,6 +1635,7 @@ void alignment_exception(struct pt_regs *regs)
sig = SIGBUS;
code = BUS_ADRALN;
}
+bad:
if (user_mode(regs))
_exception(sig, regs, code, regs->dar);
else
--
2.17.1
^ permalink raw reply related
* [PATCH v5 18/21] powerpc64: Add prefixed instructions to instruction data type
From: Jordan Niethe @ 2020-04-06 8:09 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, npiggin, bala24, Jordan Niethe, dja
In-Reply-To: <20200406080936.7180-1-jniethe5@gmail.com>
For powerpc64, redefine the ppc_inst type so both word and prefixed
instructions can be represented. On powerpc32 the type will remain the
same. Update places which had assumed instructions to be 4 bytes long.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v4: New to series
v5: - Distinguish normal instructions from prefixed instructions with a
0xff marker for the suffix.
- __patch_instruction() using std for prefixed instructions
---
arch/powerpc/include/asm/inst.h | 71 ++++++++++++++++++++++++++--
arch/powerpc/include/asm/kprobes.h | 2 +-
arch/powerpc/include/asm/uaccess.h | 31 ++++++++++--
arch/powerpc/include/asm/uprobes.h | 2 +-
arch/powerpc/kernel/optprobes.c | 42 ++++++++--------
arch/powerpc/kernel/optprobes_head.S | 3 ++
arch/powerpc/kernel/trace/ftrace.c | 26 +++++++++-
arch/powerpc/lib/code-patching.c | 19 +++++---
arch/powerpc/lib/feature-fixups.c | 5 +-
arch/powerpc/lib/sstep.c | 4 +-
arch/powerpc/xmon/xmon.c | 6 +--
arch/powerpc/xmon/xmon_bpts.S | 4 +-
12 files changed, 171 insertions(+), 44 deletions(-)
diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
index 70b37a35a91a..7e23e7146c66 100644
--- a/arch/powerpc/include/asm/inst.h
+++ b/arch/powerpc/include/asm/inst.h
@@ -8,23 +8,67 @@
struct ppc_inst {
u32 val;
+#ifdef __powerpc64__
+ u32 suffix;
+#endif /* __powerpc64__ */
} __packed;
-#define ppc_inst(x) ((struct ppc_inst){ .val = x })
+static inline int ppc_inst_opcode(struct ppc_inst x)
+{
+ return x.val >> 26;
+}
static inline u32 ppc_inst_val(struct ppc_inst x)
{
return x.val;
}
-static inline bool ppc_inst_len(struct ppc_inst x)
+#ifdef __powerpc64__
+#define ppc_inst(x) ((struct ppc_inst){ .val = (x), .suffix = 0xff })
+
+#define ppc_inst_prefix(x, y) ((struct ppc_inst){ .val = (x), .suffix = (y) })
+
+static inline u32 ppc_inst_suffix(struct ppc_inst x)
{
- return sizeof(struct ppc_inst);
+ return x.suffix;
}
-static inline int ppc_inst_opcode(struct ppc_inst x)
+static inline bool ppc_inst_prefixed(struct ppc_inst x) {
+ return ((ppc_inst_val(x) >> 26) == 1) && ppc_inst_suffix(x) != 0xff;
+}
+
+static inline struct ppc_inst ppc_inst_swab(struct ppc_inst x)
{
- return x.val >> 26;
+ return ppc_inst_prefix(swab32(ppc_inst_val(x)),
+ swab32(ppc_inst_suffix(x)));
+}
+
+static inline struct ppc_inst ppc_inst_read(const struct ppc_inst *ptr)
+{
+ u32 val, suffix = 0xff;
+ val = *(u32 *)ptr;
+ if ((val >> 26) == 1)
+ suffix = *((u32 *)ptr + 1);
+ return ppc_inst_prefix(val, suffix);
+}
+
+static inline void ppc_inst_write(struct ppc_inst *ptr, struct ppc_inst x)
+{
+ if (ppc_inst_prefixed(x)) {
+ *(u32 *)ptr = x.val;
+ *((u32 *)ptr + 1) = x.suffix;
+ } else {
+ *(u32 *)ptr = x.val;
+ }
+}
+
+#else
+
+#define ppc_inst(x) ((struct ppc_inst){ .val = x })
+
+static inline bool ppc_inst_prefixed(ppc_inst x)
+{
+ return 0;
}
static inline struct ppc_inst ppc_inst_swab(struct ppc_inst x)
@@ -32,14 +76,31 @@ static inline struct ppc_inst ppc_inst_swab(struct ppc_inst x)
return ppc_inst(swab32(ppc_inst_val(x)));
}
+static inline u32 ppc_inst_val(struct ppc_inst x)
+{
+ return x.val;
+}
+
static inline struct ppc_inst ppc_inst_read(const struct ppc_inst *ptr)
{
return *ptr;
}
+static inline void ppc_inst_write(struct ppc_inst *ptr, struct ppc_inst x)
+{
+ *ptr = x;
+}
+
+#endif /* __powerpc64__ */
+
static inline bool ppc_inst_equal(struct ppc_inst x, struct ppc_inst y)
{
return !memcmp(&x, &y, sizeof(struct ppc_inst));
}
+static inline int ppc_inst_len(struct ppc_inst x)
+{
+ return (ppc_inst_prefixed(x)) ? 8 : 4;
+}
+
#endif /* _ASM_INST_H */
diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h
index 66b3f2983b22..4fc0e15e23a5 100644
--- a/arch/powerpc/include/asm/kprobes.h
+++ b/arch/powerpc/include/asm/kprobes.h
@@ -43,7 +43,7 @@ extern kprobe_opcode_t optprobe_template_ret[];
extern kprobe_opcode_t optprobe_template_end[];
/* Fixed instruction size for powerpc */
-#define MAX_INSN_SIZE 1
+#define MAX_INSN_SIZE 2
#define MAX_OPTIMIZED_LENGTH sizeof(kprobe_opcode_t) /* 4 bytes */
#define MAX_OPTINSN_SIZE (optprobe_template_end - optprobe_template_entry)
#define RELATIVEJUMP_SIZE sizeof(kprobe_opcode_t) /* 4 bytes */
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index c0a35e4586a5..5a3f486ddf02 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -105,11 +105,34 @@ static inline int __access_ok(unsigned long addr, unsigned long size,
#define __put_user_inatomic(x, ptr) \
__put_user_nosleep((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
-#define __get_user_instr(x, ptr) \
- __get_user_nocheck((x).val, (u32 *)(ptr), sizeof(u32), true)
+#define __get_user_instr(x, ptr) \
+({ \
+ long __gui_ret = 0; \
+ unsigned int prefix, suffix; \
+ __gui_ret = __get_user(prefix, (unsigned int __user *)ptr); \
+ if (!__gui_ret && (prefix >> 26) == 1) { \
+ __gui_ret = __get_user(suffix, (unsigned int __user *)ptr + 1); \
+ (x) = ppc_inst_prefix(prefix, suffix); \
+ } else { \
+ (x) = ppc_inst(prefix); \
+ } \
+ __gui_ret; \
+})
+
+#define __get_user_instr_inatomic(x, ptr) \
+({ \
+ long __gui_ret = 0; \
+ unsigned int prefix, suffix; \
+ __gui_ret = __get_user_inatomic(prefix, (unsigned int __user *)ptr); \
+ if (!__gui_ret && (prefix >> 26) == 1) { \
+ __gui_ret = __get_user_inatomic(suffix, (unsigned int __user *)ptr + 1); \
+ (x) = ppc_inst_prefix(prefix, suffix); \
+ } else { \
+ (x) = ppc_inst(prefix); \
+ } \
+ __gui_ret; \
+})
-#define __get_user_instr_inatomic(x, ptr) \
- __get_user_nosleep((x).val, (u32 *)(ptr), sizeof(u32))
extern long __put_user_bad(void);
/*
diff --git a/arch/powerpc/include/asm/uprobes.h b/arch/powerpc/include/asm/uprobes.h
index 7e3b329ba2d3..5bf65f5d44a9 100644
--- a/arch/powerpc/include/asm/uprobes.h
+++ b/arch/powerpc/include/asm/uprobes.h
@@ -15,7 +15,7 @@
typedef ppc_opcode_t uprobe_opcode_t;
-#define MAX_UINSN_BYTES 4
+#define MAX_UINSN_BYTES 8
#define UPROBE_XOL_SLOT_BYTES (MAX_UINSN_BYTES)
/* The following alias is needed for reference from arch-agnostic code */
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index 684640b8fa2e..689daf430161 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -159,38 +159,38 @@ void patch_imm32_load_insns(unsigned int val, kprobe_opcode_t *addr)
/*
* Generate instructions to load provided immediate 64-bit value
- * to register 'r3' and patch these instructions at 'addr'.
+ * to register 'reg' and patch these instructions at 'addr'.
*/
-void patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr)
+void patch_imm64_load_insns(unsigned long val, int reg, kprobe_opcode_t *addr)
{
- /* lis r3,(op)@highest */
- patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ADDIS | ___PPC_RT(3) |
+ /* lis reg,(op)@highest */
+ patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ADDIS | ___PPC_RT(reg) |
((val >> 48) & 0xffff)));
addr++;
- /* ori r3,r3,(op)@higher */
- patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI | ___PPC_RA(3) |
- ___PPC_RS(3) | ((val >> 32) & 0xffff)));
+ /* ori reg,reg,(op)@higher */
+ patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI | ___PPC_RA(reg) |
+ ___PPC_RS(reg) | ((val >> 32) & 0xffff)));
addr++;
- /* rldicr r3,r3,32,31 */
- patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_RLDICR | ___PPC_RA(3) |
- ___PPC_RS(3) | __PPC_SH64(32) | __PPC_ME64(31)));
+ /* rldicr reg,reg,32,31 */
+ patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_RLDICR | ___PPC_RA(reg) |
+ ___PPC_RS(reg) | __PPC_SH64(32) | __PPC_ME64(31)));
addr++;
- /* oris r3,r3,(op)@h */
- patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORIS | ___PPC_RA(3) |
- ___PPC_RS(3) | ((val >> 16) & 0xffff)));
+ /* oris reg,reg,(op)@h */
+ patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORIS | ___PPC_RA(reg) |
+ ___PPC_RS(reg) | ((val >> 16) & 0xffff)));
addr++;
- /* ori r3,r3,(op)@l */
- patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI | ___PPC_RA(3) |
- ___PPC_RS(3) | (val & 0xffff)));
+ /* ori reg,reg,(op)@l */
+ patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI | ___PPC_RA(reg) |
+ ___PPC_RS(reg) | (val & 0xffff)));
}
int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
{
- struct ppc_inst branch_op_callback, branch_emulate_step;
+ struct ppc_inst branch_op_callback, branch_emulate_step, temp;
kprobe_opcode_t *op_callback_addr, *emulate_step_addr, *buff;
long b_offset;
unsigned long nip, size;
@@ -240,7 +240,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
* Fixup the template with instructions to:
* 1. load the address of the actual probepoint
*/
- patch_imm64_load_insns((unsigned long)op, buff + TMPL_OP_IDX);
+ patch_imm64_load_insns((unsigned long)op, 3, buff + TMPL_OP_IDX);
/*
* 2. branch to optimized_callback() and emulate_step()
@@ -271,7 +271,11 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
/*
* 3. load instruction to be emulated into relevant register, and
*/
- patch_imm32_load_insns(*p->ainsn.insn, buff + TMPL_INSN_IDX);
+ temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
+ patch_imm64_load_insns(ppc_inst_val(temp) |
+ ((u64)ppc_inst_suffix(temp) << 32),
+ 4,
+ buff + TMPL_INSN_IDX);
/*
* 4. branch back from trampoline
diff --git a/arch/powerpc/kernel/optprobes_head.S b/arch/powerpc/kernel/optprobes_head.S
index cf383520843f..ff8ba4d3824d 100644
--- a/arch/powerpc/kernel/optprobes_head.S
+++ b/arch/powerpc/kernel/optprobes_head.S
@@ -94,6 +94,9 @@ optprobe_template_insn:
/* 2, Pass instruction to be emulated in r4 */
nop
nop
+ nop
+ nop
+ nop
.global optprobe_template_call_emulate
optprobe_template_call_emulate:
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index e78742613b36..16041a5c86d5 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -41,11 +41,35 @@
#define NUM_FTRACE_TRAMPS 8
static unsigned long ftrace_tramps[NUM_FTRACE_TRAMPS];
+#ifdef __powerpc64__
static long
probe_kernel_read_inst(struct ppc_inst *inst, const void *src)
{
- return probe_kernel_read((void *)inst, src, MCOUNT_INSN_SIZE);
+ u32 val, suffix = 0;
+ long err;
+
+ err = probe_kernel_read((void *)&val,
+ src, sizeof(val));
+ if (err)
+ return err;
+
+ if ((val >> 26) == 1)
+ err = probe_kernel_read((void *)&suffix,
+ src + 4, MCOUNT_INSN_SIZE);
+ if (err)
+ return err;
+
+ *inst = ppc_inst_prefix(val, suffix);
+
+ return 0;
}
+#else
+static long
+probe_kernel_read_inst(struct ppc_inst *inst, const void *src)
+{
+ return probe_kernel_read((void *)inst, src, MCOUNT_INSN_SIZE)
+}
+#endif
static struct ppc_inst
ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index c329ad657302..b4007e03d8fa 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -24,12 +24,19 @@ static int __patch_instruction(struct ppc_inst *exec_addr, struct ppc_inst instr
{
int err = 0;
- __put_user_asm(ppc_inst_val(instr), patch_addr, err, "stw");
- if (err)
- return err;
-
- asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr),
- "r" (exec_addr));
+ if (!ppc_inst_prefixed(instr)) {
+ __put_user_asm(ppc_inst_val(instr), patch_addr, err, "stw");
+ if (err)
+ return err;
+ asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr),
+ "r" (exec_addr));
+ } else {
+ __put_user_asm((u64)ppc_inst_suffix(instr) << 32 | ppc_inst_val(instr), patch_addr, err, "std");
+ if (err)
+ return err;
+ asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr),
+ "r" (exec_addr));
+ }
return 0;
}
diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c
index f00dd13b1c3c..5519cec83cc8 100644
--- a/arch/powerpc/lib/feature-fixups.c
+++ b/arch/powerpc/lib/feature-fixups.c
@@ -84,12 +84,13 @@ static int patch_feature_section(unsigned long value, struct fixup_entry *fcur)
src = alt_start;
dest = start;
- for (; src < alt_end; src++, dest++) {
+ for (; src < alt_end; src = (void *)src + ppc_inst_len(ppc_inst_read(src)),
+ (dest = (void *)dest + ppc_inst_len(ppc_inst_read(dest)))) {
if (patch_alt_instruction(src, dest, alt_start, alt_end))
return 1;
}
- for (; dest < end; dest++)
+ for (; dest < end; dest = (void *)dest + ppc_inst_len(ppc_inst(PPC_INST_NOP)))
raw_patch_instruction(dest, ppc_inst(PPC_INST_NOP));
return 0;
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 52ddd3122dc8..8b285bf11218 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1169,10 +1169,12 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
unsigned long int imm;
unsigned long int val, val2;
unsigned int mb, me, sh;
- unsigned int word;
+ unsigned int word, suffix;
long ival;
word = ppc_inst_val(instr);
+ suffix = ppc_inst_suffix(instr);
+
op->type = COMPUTE;
opcode = word >> 26;
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 6f3bcdcfc9c7..b704aebb099a 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -761,8 +761,8 @@ static int xmon_bpt(struct pt_regs *regs)
/* Are we at the trap at bp->instr[1] for some bp? */
bp = in_breakpoint_table(regs->nip, &offset);
- if (bp != NULL && offset == 4) {
- regs->nip = bp->address + 4;
+ if (bp != NULL && (offset == 4 || offset == 8)) {
+ regs->nip = bp->address + offset;
atomic_dec(&bp->ref_count);
return 1;
}
@@ -863,7 +863,7 @@ static struct bpt *in_breakpoint_table(unsigned long nip, unsigned long *offp)
if (off >= sizeof(bpt_table))
return NULL;
*offp = off % BPT_SIZE;
- if (*offp != 0 && *offp != 4)
+ if (*offp != 0 && *offp != 4 && *offp != 8)
return NULL;
return bpts + (off / BPT_SIZE);
}
diff --git a/arch/powerpc/xmon/xmon_bpts.S b/arch/powerpc/xmon/xmon_bpts.S
index ebb2dbc70ca8..09058eb6abbd 100644
--- a/arch/powerpc/xmon/xmon_bpts.S
+++ b/arch/powerpc/xmon/xmon_bpts.S
@@ -3,6 +3,8 @@
#include <asm/asm-compat.h>
#include "xmon_bpts.h"
+/* Prefixed instructions can not cross 64 byte boundaries */
+.align 6
.global bpt_table
bpt_table:
- .space NBPTS * 8
+ .space NBPTS * 16
--
2.17.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox