Message ID | 20190513071703.25243-1-mikey@neuling.org (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [v2] powerpc: Fix compile issue with force DAWR | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (8150a153c013aa2dd1ffae43370b89ac1347a7fb) |
snowpatch_ozlabs/build-ppc64le | warning | Build succeeded but added 1 new sparse warnings |
snowpatch_ozlabs/build-ppc64be | warning | Build succeeded but added 1 new sparse warnings |
snowpatch_ozlabs/build-ppc64e | warning | Build succeeded but added 1 new sparse warnings |
snowpatch_ozlabs/build-pmac32 | warning | Build succeeded but added 1 new sparse warnings |
snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 3 checks, 145 lines checked |
Le 13/05/2019 à 09:17, Michael Neuling a écrit : > If you compile with KVM but without CONFIG_HAVE_HW_BREAKPOINT you fail > at linking with: > arch/powerpc/kvm/book3s_hv_rmhandlers.o:(.text+0x708): undefined reference to `dawr_force_enable' > > This was caused by commit c1fe190c0672 ("powerpc: Add force enable of > DAWR on P9 option"). > > This puts more of the dawr infrastructure in a new file. I think you are doing a bit more than that. I think you should explain that you define a new CONFIG_ option, when it is selected, etc ... The commit you are referring to is talking about P9. It looks like your patch covers a lot more, so it should be mentionned her I guess. > > Signed-off-by: Michael Neuling <mikey@neuling.org> You should add a Fixes: tag, ie: Fixes: c1fe190c0672 ("powerpc: Add force enable of DAWR on P9 option") > -- > v2: > Fixes based on Christophe Leroy's comments: > - Fix commit message formatting > - Move more DAWR code into dawr.c > --- > arch/powerpc/Kconfig | 5 ++ > arch/powerpc/include/asm/hw_breakpoint.h | 20 ++++--- > arch/powerpc/kernel/Makefile | 1 + > arch/powerpc/kernel/dawr.c | 75 ++++++++++++++++++++++++ > arch/powerpc/kernel/hw_breakpoint.c | 56 ------------------ > arch/powerpc/kvm/Kconfig | 1 + > 6 files changed, 94 insertions(+), 64 deletions(-) > create mode 100644 arch/powerpc/kernel/dawr.c > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index 2711aac246..f4b19e48cc 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -242,6 +242,7 @@ config PPC > select SYSCTL_EXCEPTION_TRACE > select THREAD_INFO_IN_TASK > select VIRT_TO_BUS if !PPC64 > + select PPC_DAWR_FORCE_ENABLE if PPC64 || PERF What's PERF ? Did you mean PERF_EVENTS ? Then what you mean is: - Selected all the time for PPC64 - Selected for PPC32 when PERF is also selected. Is that what you want ? At first I thought it was linked to P9. And ... did you read below statement ? > # > # Please keep this list sorted alphabetically. > # > @@ -369,6 +370,10 @@ config PPC_ADV_DEBUG_DAC_RANGE > depends on PPC_ADV_DEBUG_REGS && 44x > default y > > +config PPC_DAWR_FORCE_ENABLE > + bool > + default y > + Why defaulting it to y. Then how is it set to n ? > config ZONE_DMA > bool > default y if PPC_BOOK3E_64 > diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h > index 0fe8c1e46b..ffbc8eab41 100644 > --- a/arch/powerpc/include/asm/hw_breakpoint.h > +++ b/arch/powerpc/include/asm/hw_breakpoint.h > @@ -47,6 +47,8 @@ struct arch_hw_breakpoint { > #define HW_BRK_TYPE_PRIV_ALL (HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | \ > HW_BRK_TYPE_HYP) > > +extern int set_dawr(struct arch_hw_breakpoint *brk); > + > #ifdef CONFIG_HAVE_HW_BREAKPOINT > #include <linux/kdebug.h> > #include <asm/reg.h> > @@ -90,18 +92,20 @@ static inline void hw_breakpoint_disable(void) > extern void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs); > int hw_breakpoint_handler(struct die_args *args); > > -extern int set_dawr(struct arch_hw_breakpoint *brk); > -extern bool dawr_force_enable; > -static inline bool dawr_enabled(void) > -{ > - return dawr_force_enable; > -} > - That's a very simple function, why not keep it here (or in another .h) as 'static inline' ? > #else /* CONFIG_HAVE_HW_BREAKPOINT */ > static inline void hw_breakpoint_disable(void) { } > static inline void thread_change_pc(struct task_struct *tsk, > struct pt_regs *regs) { } > -static inline bool dawr_enabled(void) { return false; } > + > #endif /* CONFIG_HAVE_HW_BREAKPOINT */ > + > +extern bool dawr_force_enable; > + > +#ifdef CONFIG_PPC_DAWR_FORCE_ENABLE > +extern bool dawr_enabled(void); Functions should not be 'extern'. I'm sure checkpatch --strict will tell you. > +#else > +#define dawr_enabled() true true by default ? Previously it was false by default. And why a #define ? That's better to keep a static inline. > +#endif > + > #endif /* __KERNEL__ */ > #endif /* _PPC_BOOK3S_64_HW_BREAKPOINT_H */ > diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile > index 0ea6c4aa3a..a9c497c34f 100644 > --- a/arch/powerpc/kernel/Makefile > +++ b/arch/powerpc/kernel/Makefile > @@ -56,6 +56,7 @@ obj-$(CONFIG_PPC64) += setup_64.o sys_ppc32.o \ > obj-$(CONFIG_VDSO32) += vdso32/ > obj-$(CONFIG_PPC_WATCHDOG) += watchdog.o > obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o > +obj-$(CONFIG_PPC_DAWR_FORCE_ENABLE) += dawr.o > obj-$(CONFIG_PPC_BOOK3S_64) += cpu_setup_ppc970.o cpu_setup_pa6t.o > obj-$(CONFIG_PPC_BOOK3S_64) += cpu_setup_power.o > obj-$(CONFIG_PPC_BOOK3S_64) += mce.o mce_power.o > diff --git a/arch/powerpc/kernel/dawr.c b/arch/powerpc/kernel/dawr.c > new file mode 100644 > index 0000000000..cf1d02fe1e > --- /dev/null > +++ b/arch/powerpc/kernel/dawr.c > @@ -0,0 +1,75 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +// > +// DAWR infrastructure > +// > +// Copyright 2019, Michael Neuling, IBM Corporation. > + > +#include <linux/types.h> > +#include <linux/export.h> > +#include <linux/fs.h> > +#include <linux/debugfs.h> > +#include <asm/debugfs.h> > +#include <asm/machdep.h> > +#include <asm/hvcall.h> > + > +bool dawr_force_enable; > +EXPORT_SYMBOL_GPL(dawr_force_enable); > + > +extern bool dawr_enabled(void) extern ???? > +{ > + return dawr_force_enable; > +} > +EXPORT_SYMBOL_GPL(dawr_enabled); Since dawr_force_enable is also exported, I see no point in having such a tiny function as an exported function, was better as a 'static inline'. > + > +static ssize_t dawr_write_file_bool(struct file *file, > + const char __user *user_buf, > + size_t count, loff_t *ppos) > +{ > + struct arch_hw_breakpoint null_brk = {0, 0, 0}; > + size_t rc; > + > + /* Send error to user if they hypervisor won't allow us to write DAWR */ > + if ((!dawr_force_enable) && > + (firmware_has_feature(FW_FEATURE_LPAR)) && > + (set_dawr(&null_brk) != H_SUCCESS)) The above is not real clear. set_dabr() returns 0, H_SUCCESS is not used there. > + return -1; > + > + rc = debugfs_write_file_bool(file, user_buf, count, ppos); > + if (rc) > + return rc; > + > + /* If we are clearing, make sure all CPUs have the DAWR cleared */ > + if (!dawr_force_enable) > + smp_call_function((smp_call_func_t)set_dawr, &null_brk, 0); > + > + return rc; > +} > + > +static const struct file_operations dawr_enable_fops = { > + .read = debugfs_read_file_bool, > + .write = dawr_write_file_bool, > + .open = simple_open, > + .llseek = default_llseek, > +}; > + > +static int __init dawr_force_setup(void) > +{ > + dawr_force_enable = false; The above is not needed, the BSS is zeroised at kernel startup. > + > + if (cpu_has_feature(CPU_FTR_DAWR)) { > + /* Don't setup sysfs file for user control on P8 */ > + dawr_force_enable = true; Strange comment, word "don't" doesn't really fit with a 'true' > + return 0; > + } > + > + if (PVR_VER(mfspr(SPRN_PVR)) == PVR_POWER9) { You could use pvr_version_is(PVR_POWER9) instead of open codiing. > + /* Turn DAWR off by default, but allow admin to turn it on */ > + dawr_force_enable = false; > + debugfs_create_file_unsafe("dawr_enable_dangerous", 0600, > + powerpc_debugfs_root, > + &dawr_force_enable, > + &dawr_enable_fops); > + } > + return 0; > +} > +arch_initcall(dawr_force_setup); Wouldn't it also make sense to move set_dawr() from process.c to here ? > diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c > index da307dd93e..95605a9c9a 100644 > --- a/arch/powerpc/kernel/hw_breakpoint.c > +++ b/arch/powerpc/kernel/hw_breakpoint.c > @@ -380,59 +380,3 @@ void hw_breakpoint_pmu_read(struct perf_event *bp) > { > /* TODO */ > } > - > -bool dawr_force_enable; > -EXPORT_SYMBOL_GPL(dawr_force_enable); > - > -static ssize_t dawr_write_file_bool(struct file *file, > - const char __user *user_buf, > - size_t count, loff_t *ppos) > -{ > - struct arch_hw_breakpoint null_brk = {0, 0, 0}; > - size_t rc; > - > - /* Send error to user if they hypervisor won't allow us to write DAWR */ > - if ((!dawr_force_enable) && > - (firmware_has_feature(FW_FEATURE_LPAR)) && > - (set_dawr(&null_brk) != H_SUCCESS)) > - return -1; > - > - rc = debugfs_write_file_bool(file, user_buf, count, ppos); > - if (rc) > - return rc; > - > - /* If we are clearing, make sure all CPUs have the DAWR cleared */ > - if (!dawr_force_enable) > - smp_call_function((smp_call_func_t)set_dawr, &null_brk, 0); > - > - return rc; > -} > - > -static const struct file_operations dawr_enable_fops = { > - .read = debugfs_read_file_bool, > - .write = dawr_write_file_bool, > - .open = simple_open, > - .llseek = default_llseek, > -}; > - > -static int __init dawr_force_setup(void) > -{ > - dawr_force_enable = false; > - > - if (cpu_has_feature(CPU_FTR_DAWR)) { > - /* Don't setup sysfs file for user control on P8 */ > - dawr_force_enable = true; > - return 0; > - } > - > - if (PVR_VER(mfspr(SPRN_PVR)) == PVR_POWER9) { > - /* Turn DAWR off by default, but allow admin to turn it on */ > - dawr_force_enable = false; > - debugfs_create_file_unsafe("dawr_enable_dangerous", 0600, > - powerpc_debugfs_root, > - &dawr_force_enable, > - &dawr_enable_fops); > - } > - return 0; > -} > -arch_initcall(dawr_force_setup); > diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig > index bfdde04e49..9c0d315108 100644 > --- a/arch/powerpc/kvm/Kconfig > +++ b/arch/powerpc/kvm/Kconfig > @@ -39,6 +39,7 @@ config KVM_BOOK3S_32_HANDLER > config KVM_BOOK3S_64_HANDLER > bool > select KVM_BOOK3S_HANDLER > + select PPC_DAWR_FORCE_ENABLE > > config KVM_BOOK3S_PR_POSSIBLE > bool > Christophe
On Mon, 2019-05-13 at 11:08 +0200, Christophe Leroy wrote: > > Le 13/05/2019 à 09:17, Michael Neuling a écrit : > > If you compile with KVM but without CONFIG_HAVE_HW_BREAKPOINT you fail > > at linking with: > > arch/powerpc/kvm/book3s_hv_rmhandlers.o:(.text+0x708): undefined > > reference to `dawr_force_enable' > > > > This was caused by commit c1fe190c0672 ("powerpc: Add force enable of > > DAWR on P9 option"). > > > > This puts more of the dawr infrastructure in a new file. > > I think you are doing a bit more than that. I think you should explain > that you define a new CONFIG_ option, when it is selected, etc ... > > The commit you are referring to is talking about P9. It looks like your > patch covers a lot more, so it should be mentionned her I guess. Not really. It looks like I'm doing a lot but I'm really just moving code around to deal with the ugliness of a bunch of config options and dependencies. > > Signed-off-by: Michael Neuling <mikey@neuling.org> > > You should add a Fixes: tag, ie: > > Fixes: c1fe190c0672 ("powerpc: Add force enable of DAWR on P9 option") Ok > > > -- > > v2: > > Fixes based on Christophe Leroy's comments: > > - Fix commit message formatting > > - Move more DAWR code into dawr.c > > --- > > arch/powerpc/Kconfig | 5 ++ > > arch/powerpc/include/asm/hw_breakpoint.h | 20 ++++--- > > arch/powerpc/kernel/Makefile | 1 + > > arch/powerpc/kernel/dawr.c | 75 ++++++++++++++++++++++++ > > arch/powerpc/kernel/hw_breakpoint.c | 56 ------------------ > > arch/powerpc/kvm/Kconfig | 1 + > > 6 files changed, 94 insertions(+), 64 deletions(-) > > create mode 100644 arch/powerpc/kernel/dawr.c > > > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > > index 2711aac246..f4b19e48cc 100644 > > --- a/arch/powerpc/Kconfig > > +++ b/arch/powerpc/Kconfig > > @@ -242,6 +242,7 @@ config PPC > > select SYSCTL_EXCEPTION_TRACE > > select THREAD_INFO_IN_TASK > > select VIRT_TO_BUS if !PPC64 > > + select PPC_DAWR_FORCE_ENABLE if PPC64 || PERF > > What's PERF ? Did you mean PERF_EVENTS ? > > Then what you mean is: > - Selected all the time for PPC64 > - Selected for PPC32 when PERF is also selected. > > Is that what you want ? At first I thought it was linked to P9. This is wrong. I think we just want PPC64. PERF is a red herring. > And ... did you read below statement ? Clearly not :-) > > > # > > # Please keep this list sorted alphabetically. > > # > > @@ -369,6 +370,10 @@ config PPC_ADV_DEBUG_DAC_RANGE > > depends on PPC_ADV_DEBUG_REGS && 44x > > default y > > > > +config PPC_DAWR_FORCE_ENABLE > > + bool > > + default y > > + > > Why defaulting it to y. Then how is it set to n ? Good point. > > > config ZONE_DMA > > bool > > default y if PPC_BOOK3E_64 > > diff --git a/arch/powerpc/include/asm/hw_breakpoint.h > > b/arch/powerpc/include/asm/hw_breakpoint.h > > index 0fe8c1e46b..ffbc8eab41 100644 > > --- a/arch/powerpc/include/asm/hw_breakpoint.h > > +++ b/arch/powerpc/include/asm/hw_breakpoint.h > > @@ -47,6 +47,8 @@ struct arch_hw_breakpoint { > > #define HW_BRK_TYPE_PRIV_ALL (HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | > > \ > > HW_BRK_TYPE_HYP) > > > > +extern int set_dawr(struct arch_hw_breakpoint *brk); > > + > > #ifdef CONFIG_HAVE_HW_BREAKPOINT > > #include <linux/kdebug.h> > > #include <asm/reg.h> > > @@ -90,18 +92,20 @@ static inline void hw_breakpoint_disable(void) > > extern void thread_change_pc(struct task_struct *tsk, struct pt_regs > > *regs); > > int hw_breakpoint_handler(struct die_args *args); > > > > -extern int set_dawr(struct arch_hw_breakpoint *brk); > > -extern bool dawr_force_enable; > > -static inline bool dawr_enabled(void) > > -{ > > - return dawr_force_enable; > > -} > > - > > That's a very simple function, why not keep it here (or in another .h) > as 'static inline' ? Sure. > > #else /* CONFIG_HAVE_HW_BREAKPOINT */ > > static inline void hw_breakpoint_disable(void) { } > > static inline void thread_change_pc(struct task_struct *tsk, > > struct pt_regs *regs) { } > > -static inline bool dawr_enabled(void) { return false; } > > + > > #endif /* CONFIG_HAVE_HW_BREAKPOINT */ > > + > > +extern bool dawr_force_enable; > > + > > +#ifdef CONFIG_PPC_DAWR_FORCE_ENABLE > > +extern bool dawr_enabled(void); > > Functions should not be 'extern'. I'm sure checkpatch --strict will tell > you. That's not what's currently being done in this header file. I'm keeping with the style of that file. > > +#else > > +#define dawr_enabled() true > > true by default ? > Previously it was false by default. Thanks, yeah that's wrong but I need to rethink the config option to make it CONFIG_PPC_DAWR. This patch is far more difficult than it should be due to the mess that CONFIG_HAVE_HW_BREAKPOINT and CONFIG_PPC_ADV_DEBUG_REGS creates in ptrace.c and process.c. We really need to fix up https://github.com/linuxppc/issues/issues/128 > And why a #define ? That's better to keep a static inline. Changed. > > > +#endif > > + > > #endif /* __KERNEL__ */ > > #endif /* _PPC_BOOK3S_64_HW_BREAKPOINT_H */ > > diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile > > index 0ea6c4aa3a..a9c497c34f 100644 > > --- a/arch/powerpc/kernel/Makefile > > +++ b/arch/powerpc/kernel/Makefile > > @@ -56,6 +56,7 @@ obj-$(CONFIG_PPC64) += setup_64.o > > sys_ppc32.o \ > > obj-$(CONFIG_VDSO32) += vdso32/ > > obj-$(CONFIG_PPC_WATCHDOG) += watchdog.o > > obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o > > +obj-$(CONFIG_PPC_DAWR_FORCE_ENABLE) += dawr.o > > obj-$(CONFIG_PPC_BOOK3S_64) += cpu_setup_ppc970.o cpu_setup_pa6t.o > > obj-$(CONFIG_PPC_BOOK3S_64) += cpu_setup_power.o > > obj-$(CONFIG_PPC_BOOK3S_64) += mce.o mce_power.o > > diff --git a/arch/powerpc/kernel/dawr.c b/arch/powerpc/kernel/dawr.c > > new file mode 100644 > > index 0000000000..cf1d02fe1e > > --- /dev/null > > +++ b/arch/powerpc/kernel/dawr.c > > @@ -0,0 +1,75 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +// > > +// DAWR infrastructure > > +// > > +// Copyright 2019, Michael Neuling, IBM Corporation. > > + > > +#include <linux/types.h> > > +#include <linux/export.h> > > +#include <linux/fs.h> > > +#include <linux/debugfs.h> > > +#include <asm/debugfs.h> > > +#include <asm/machdep.h> > > +#include <asm/hvcall.h> > > + > > +bool dawr_force_enable; > > +EXPORT_SYMBOL_GPL(dawr_force_enable); > > + > > +extern bool dawr_enabled(void) > > extern ???? oops > > > +{ > > + return dawr_force_enable; > > +} > > +EXPORT_SYMBOL_GPL(dawr_enabled); > > Since dawr_force_enable is also exported, I see no point in having such > a tiny function as an exported function, was better as a 'static inline'. Yep, changed to static inline. > > + > > +static ssize_t dawr_write_file_bool(struct file *file, > > + const char __user *user_buf, > > + size_t count, loff_t *ppos) > > +{ > > + struct arch_hw_breakpoint null_brk = {0, 0, 0}; > > + size_t rc; > > + > > + /* Send error to user if they hypervisor won't allow us to write DAWR */ > > + if ((!dawr_force_enable) && > > + (firmware_has_feature(FW_FEATURE_LPAR)) && > > + (set_dawr(&null_brk) != H_SUCCESS)) > > The above is not real clear. > set_dabr() returns 0, H_SUCCESS is not used there. It pseries_set_dawr() will return a hcall number. This code hasn't changed. I'm just moving it. > > > + return -1; > > + > > + rc = debugfs_write_file_bool(file, user_buf, count, ppos); > > + if (rc) > > + return rc; > > + > > + /* If we are clearing, make sure all CPUs have the DAWR cleared */ > > + if (!dawr_force_enable) > > + smp_call_function((smp_call_func_t)set_dawr, &null_brk, 0); > > + > > + return rc; > > +} > > + > > +static const struct file_operations dawr_enable_fops = { > > + .read = debugfs_read_file_bool, > > + .write = dawr_write_file_bool, > > + .open = simple_open, > > + .llseek = default_llseek, > > +}; > > + > > +static int __init dawr_force_setup(void) > > +{ > > + dawr_force_enable = false; > > The above is not needed, the BSS is zeroised at kernel startup. > > > + > > + if (cpu_has_feature(CPU_FTR_DAWR)) { > > + /* Don't setup sysfs file for user control on P8 */ > > + dawr_force_enable = true; > > Strange comment, word "don't" doesn't really fit with a 'true' > > > + return 0; > > + } > > + > > + if (PVR_VER(mfspr(SPRN_PVR)) == PVR_POWER9) { > > You could use pvr_version_is(PVR_POWER9) instead of open codiing. All this code hasn't changed. I'm just moving it. Feel free to clean it up but lets fix a real problem here. > > > + /* Turn DAWR off by default, but allow admin to turn it on */ > > + dawr_force_enable = false; > > + debugfs_create_file_unsafe("dawr_enable_dangerous", 0600, > > + powerpc_debugfs_root, > > + &dawr_force_enable, > > + &dawr_enable_fops); > > + } > > + return 0; > > +} > > +arch_initcall(dawr_force_setup); > > Wouldn't it also make sense to move set_dawr() from process.c to here ? Yep, done. > > > diff --git a/arch/powerpc/kernel/hw_breakpoint.c > > b/arch/powerpc/kernel/hw_breakpoint.c > > index da307dd93e..95605a9c9a 100644 > > --- a/arch/powerpc/kernel/hw_breakpoint.c > > +++ b/arch/powerpc/kernel/hw_breakpoint.c > > @@ -380,59 +380,3 @@ void hw_breakpoint_pmu_read(struct perf_event *bp) > > { > > /* TODO */ > > } > > - > > -bool dawr_force_enable; > > -EXPORT_SYMBOL_GPL(dawr_force_enable); > > - > > -static ssize_t dawr_write_file_bool(struct file *file, > > - const char __user *user_buf, > > - size_t count, loff_t *ppos) > > -{ > > - struct arch_hw_breakpoint null_brk = {0, 0, 0}; > > - size_t rc; > > - > > - /* Send error to user if they hypervisor won't allow us to write DAWR */ > > - if ((!dawr_force_enable) && > > - (firmware_has_feature(FW_FEATURE_LPAR)) && > > - (set_dawr(&null_brk) != H_SUCCESS)) > > - return -1; > > - > > - rc = debugfs_write_file_bool(file, user_buf, count, ppos); > > - if (rc) > > - return rc; > > - > > - /* If we are clearing, make sure all CPUs have the DAWR cleared */ > > - if (!dawr_force_enable) > > - smp_call_function((smp_call_func_t)set_dawr, &null_brk, 0); > > - > > - return rc; > > -} > > - > > -static const struct file_operations dawr_enable_fops = { > > - .read = debugfs_read_file_bool, > > - .write = dawr_write_file_bool, > > - .open = simple_open, > > - .llseek = default_llseek, > > -}; > > - > > -static int __init dawr_force_setup(void) > > -{ > > - dawr_force_enable = false; > > - > > - if (cpu_has_feature(CPU_FTR_DAWR)) { > > - /* Don't setup sysfs file for user control on P8 */ > > - dawr_force_enable = true; > > - return 0; > > - } > > - > > - if (PVR_VER(mfspr(SPRN_PVR)) == PVR_POWER9) { > > - /* Turn DAWR off by default, but allow admin to turn it on */ > > - dawr_force_enable = false; > > - debugfs_create_file_unsafe("dawr_enable_dangerous", 0600, > > - powerpc_debugfs_root, > > - &dawr_force_enable, > > - &dawr_enable_fops); > > - } > > - return 0; > > -} > > -arch_initcall(dawr_force_setup); > > diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig > > index bfdde04e49..9c0d315108 100644 > > --- a/arch/powerpc/kvm/Kconfig > > +++ b/arch/powerpc/kvm/Kconfig > > @@ -39,6 +39,7 @@ config KVM_BOOK3S_32_HANDLER > > config KVM_BOOK3S_64_HANDLER > > bool > > select KVM_BOOK3S_HANDLER > > + select PPC_DAWR_FORCE_ENABLE > > > > config KVM_BOOK3S_PR_POSSIBLE > > bool > > > > Christophe >
Le 14/05/2019 à 06:47, Michael Neuling a écrit : > On Mon, 2019-05-13 at 11:08 +0200, Christophe Leroy wrote: >> >> Le 13/05/2019 à 09:17, Michael Neuling a écrit : >>> If you compile with KVM but without CONFIG_HAVE_HW_BREAKPOINT you fail >>> at linking with: >>> arch/powerpc/kvm/book3s_hv_rmhandlers.o:(.text+0x708): undefined >>> reference to `dawr_force_enable' >>> >>> This was caused by commit c1fe190c0672 ("powerpc: Add force enable of >>> DAWR on P9 option"). >>> >>> This puts more of the dawr infrastructure in a new file. >> >> I think you are doing a bit more than that. I think you should explain >> that you define a new CONFIG_ option, when it is selected, etc ... >> >> The commit you are referring to is talking about P9. It looks like your >> patch covers a lot more, so it should be mentionned her I guess. > > Not really. It looks like I'm doing a lot but I'm really just moving code around > to deal with the ugliness of a bunch of config options and dependencies. > >>> Signed-off-by: Michael Neuling <mikey@neuling.org> >> >> You should add a Fixes: tag, ie: >> >> Fixes: c1fe190c0672 ("powerpc: Add force enable of DAWR on P9 option") > > Ok > >> >>> -- >>> v2: >>> Fixes based on Christophe Leroy's comments: >>> - Fix commit message formatting >>> - Move more DAWR code into dawr.c >>> --- >>> arch/powerpc/Kconfig | 5 ++ >>> arch/powerpc/include/asm/hw_breakpoint.h | 20 ++++--- >>> arch/powerpc/kernel/Makefile | 1 + >>> arch/powerpc/kernel/dawr.c | 75 ++++++++++++++++++++++++ >>> arch/powerpc/kernel/hw_breakpoint.c | 56 ------------------ >>> arch/powerpc/kvm/Kconfig | 1 + >>> 6 files changed, 94 insertions(+), 64 deletions(-) >>> create mode 100644 arch/powerpc/kernel/dawr.c >>> >>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig >>> index 2711aac246..f4b19e48cc 100644 >>> --- a/arch/powerpc/Kconfig >>> +++ b/arch/powerpc/Kconfig >>> @@ -242,6 +242,7 @@ config PPC >>> select SYSCTL_EXCEPTION_TRACE >>> select THREAD_INFO_IN_TASK >>> select VIRT_TO_BUS if !PPC64 >>> + select PPC_DAWR_FORCE_ENABLE if PPC64 || PERF >> >> What's PERF ? Did you mean PERF_EVENTS ? >> >> Then what you mean is: >> - Selected all the time for PPC64 >> - Selected for PPC32 when PERF is also selected. >> >> Is that what you want ? At first I thought it was linked to P9. > > This is wrong. I think we just want PPC64. PERF is a red herring. Are you sure ? Michael suggested PERF || KVM as far as I remember. > >> And ... did you read below statement ? > > Clearly not :-) > >> >>> # >>> # Please keep this list sorted alphabetically. >>> # >>> @@ -369,6 +370,10 @@ config PPC_ADV_DEBUG_DAC_RANGE >>> depends on PPC_ADV_DEBUG_REGS && 44x >>> default y >>> >>> +config PPC_DAWR_FORCE_ENABLE >>> + bool >>> + default y >>> + >> >> Why defaulting it to y. Then how is it set to n ? > > Good point. > >> >>> config ZONE_DMA >>> bool >>> default y if PPC_BOOK3E_64 >>> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h >>> b/arch/powerpc/include/asm/hw_breakpoint.h >>> index 0fe8c1e46b..ffbc8eab41 100644 >>> --- a/arch/powerpc/include/asm/hw_breakpoint.h >>> +++ b/arch/powerpc/include/asm/hw_breakpoint.h >>> @@ -47,6 +47,8 @@ struct arch_hw_breakpoint { >>> #define HW_BRK_TYPE_PRIV_ALL (HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | >>> \ >>> HW_BRK_TYPE_HYP) >>> >>> +extern int set_dawr(struct arch_hw_breakpoint *brk); >>> + >>> #ifdef CONFIG_HAVE_HW_BREAKPOINT >>> #include <linux/kdebug.h> >>> #include <asm/reg.h> >>> @@ -90,18 +92,20 @@ static inline void hw_breakpoint_disable(void) >>> extern void thread_change_pc(struct task_struct *tsk, struct pt_regs >>> *regs); >>> int hw_breakpoint_handler(struct die_args *args); >>> >>> -extern int set_dawr(struct arch_hw_breakpoint *brk); >>> -extern bool dawr_force_enable; >>> -static inline bool dawr_enabled(void) >>> -{ >>> - return dawr_force_enable; >>> -} >>> - >> >> That's a very simple function, why not keep it here (or in another .h) >> as 'static inline' ? > > Sure. > >>> #else /* CONFIG_HAVE_HW_BREAKPOINT */ >>> static inline void hw_breakpoint_disable(void) { } >>> static inline void thread_change_pc(struct task_struct *tsk, >>> struct pt_regs *regs) { } >>> -static inline bool dawr_enabled(void) { return false; } >>> + >>> #endif /* CONFIG_HAVE_HW_BREAKPOINT */ >>> + >>> +extern bool dawr_force_enable; >>> + >>> +#ifdef CONFIG_PPC_DAWR_FORCE_ENABLE >>> +extern bool dawr_enabled(void); >> >> Functions should not be 'extern'. I'm sure checkpatch --strict will tell >> you. > > That's not what's currently being done in this header file. I'm keeping with > the style of that file. style is not defined on a per file basis. There is the style from the past and the nowadays style. If you keep old style just because the file includes old style statements, then the code will never improve. All new patches should come with clean 'checkpatch' report and follow new style. Having mixed styles in a file is not a problem, that's the way to improvement. See arch/powerpc/mm/mmu_decl.h as an exemple. > >>> +#else >>> +#define dawr_enabled() true >> >> true by default ? >> Previously it was false by default. > > Thanks, yeah that's wrong but I need to rethink the config option to make it > CONFIG_PPC_DAWR. > > This patch is far more difficult than it should be due to the mess that > CONFIG_HAVE_HW_BREAKPOINT and CONFIG_PPC_ADV_DEBUG_REGS creates in ptrace.c and > process.c. We really need to fix up > https://github.com/linuxppc/issues/issues/128 > >> And why a #define ? That's better to keep a static inline. > > Changed. > >> >>> +#endif >>> + >>> #endif /* __KERNEL__ */ >>> #endif /* _PPC_BOOK3S_64_HW_BREAKPOINT_H */ >>> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile >>> index 0ea6c4aa3a..a9c497c34f 100644 >>> --- a/arch/powerpc/kernel/Makefile >>> +++ b/arch/powerpc/kernel/Makefile >>> @@ -56,6 +56,7 @@ obj-$(CONFIG_PPC64) += setup_64.o >>> sys_ppc32.o \ >>> obj-$(CONFIG_VDSO32) += vdso32/ >>> obj-$(CONFIG_PPC_WATCHDOG) += watchdog.o >>> obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o >>> +obj-$(CONFIG_PPC_DAWR_FORCE_ENABLE) += dawr.o >>> obj-$(CONFIG_PPC_BOOK3S_64) += cpu_setup_ppc970.o cpu_setup_pa6t.o >>> obj-$(CONFIG_PPC_BOOK3S_64) += cpu_setup_power.o >>> obj-$(CONFIG_PPC_BOOK3S_64) += mce.o mce_power.o >>> diff --git a/arch/powerpc/kernel/dawr.c b/arch/powerpc/kernel/dawr.c >>> new file mode 100644 >>> index 0000000000..cf1d02fe1e >>> --- /dev/null >>> +++ b/arch/powerpc/kernel/dawr.c >>> @@ -0,0 +1,75 @@ >>> +// SPDX-License-Identifier: GPL-2.0+ >>> +// >>> +// DAWR infrastructure >>> +// >>> +// Copyright 2019, Michael Neuling, IBM Corporation. >>> + >>> +#include <linux/types.h> >>> +#include <linux/export.h> >>> +#include <linux/fs.h> >>> +#include <linux/debugfs.h> >>> +#include <asm/debugfs.h> >>> +#include <asm/machdep.h> >>> +#include <asm/hvcall.h> >>> + >>> +bool dawr_force_enable; >>> +EXPORT_SYMBOL_GPL(dawr_force_enable); >>> + >>> +extern bool dawr_enabled(void) >> >> extern ???? > > oops >> >>> +{ >>> + return dawr_force_enable; >>> +} >>> +EXPORT_SYMBOL_GPL(dawr_enabled); >> >> Since dawr_force_enable is also exported, I see no point in having such >> a tiny function as an exported function, was better as a 'static inline'. > > Yep, changed to static inline. > >>> + >>> +static ssize_t dawr_write_file_bool(struct file *file, >>> + const char __user *user_buf, >>> + size_t count, loff_t *ppos) >>> +{ >>> + struct arch_hw_breakpoint null_brk = {0, 0, 0}; >>> + size_t rc; >>> + >>> + /* Send error to user if they hypervisor won't allow us to write DAWR */ >>> + if ((!dawr_force_enable) && >>> + (firmware_has_feature(FW_FEATURE_LPAR)) && >>> + (set_dawr(&null_brk) != H_SUCCESS)) >> >> The above is not real clear. >> set_dabr() returns 0, H_SUCCESS is not used there. > > It pseries_set_dawr() will return a hcall number. Right, then it maybe means set_dawr() should be fixes ? > > This code hasn't changed. I'm just moving it. Right, but could be an improvment for another patch. As far as I remember you are the one who wrote that code at first place, arent't you ? > >> >>> + return -1; >>> + >>> + rc = debugfs_write_file_bool(file, user_buf, count, ppos); >>> + if (rc) >>> + return rc; >>> + >>> + /* If we are clearing, make sure all CPUs have the DAWR cleared */ >>> + if (!dawr_force_enable) >>> + smp_call_function((smp_call_func_t)set_dawr, &null_brk, 0); >>> + >>> + return rc; >>> +} >>> + >>> +static const struct file_operations dawr_enable_fops = { >>> + .read = debugfs_read_file_bool, >>> + .write = dawr_write_file_bool, >>> + .open = simple_open, >>> + .llseek = default_llseek, >>> +}; >>> + >>> +static int __init dawr_force_setup(void) >>> +{ >>> + dawr_force_enable = false; >> >> The above is not needed, the BSS is zeroised at kernel startup. >> >>> + >>> + if (cpu_has_feature(CPU_FTR_DAWR)) { >>> + /* Don't setup sysfs file for user control on P8 */ >>> + dawr_force_enable = true; >> >> Strange comment, word "don't" doesn't really fit with a 'true' >> >>> + return 0; >>> + } >>> + >>> + if (PVR_VER(mfspr(SPRN_PVR)) == PVR_POWER9) { >> >> You could use pvr_version_is(PVR_POWER9) instead of open codiing. > > All this code hasn't changed. I'm just moving it. Right, but I think the comments are worth it, allthough that would be for another patch. > > Feel free to clean it up but lets fix a real problem here. Yes I can but it will conflict with your patch. Christophe > >> >>> + /* Turn DAWR off by default, but allow admin to turn it on */ >>> + dawr_force_enable = false; >>> + debugfs_create_file_unsafe("dawr_enable_dangerous", 0600, >>> + powerpc_debugfs_root, >>> + &dawr_force_enable, >>> + &dawr_enable_fops); >>> + } >>> + return 0; >>> +} >>> +arch_initcall(dawr_force_setup); >> >> Wouldn't it also make sense to move set_dawr() from process.c to here ? > > Yep, done. > >> >>> diff --git a/arch/powerpc/kernel/hw_breakpoint.c >>> b/arch/powerpc/kernel/hw_breakpoint.c >>> index da307dd93e..95605a9c9a 100644 >>> --- a/arch/powerpc/kernel/hw_breakpoint.c >>> +++ b/arch/powerpc/kernel/hw_breakpoint.c >>> @@ -380,59 +380,3 @@ void hw_breakpoint_pmu_read(struct perf_event *bp) >>> { >>> /* TODO */ >>> } >>> - >>> -bool dawr_force_enable; >>> -EXPORT_SYMBOL_GPL(dawr_force_enable); >>> - >>> -static ssize_t dawr_write_file_bool(struct file *file, >>> - const char __user *user_buf, >>> - size_t count, loff_t *ppos) >>> -{ >>> - struct arch_hw_breakpoint null_brk = {0, 0, 0}; >>> - size_t rc; >>> - >>> - /* Send error to user if they hypervisor won't allow us to write DAWR */ >>> - if ((!dawr_force_enable) && >>> - (firmware_has_feature(FW_FEATURE_LPAR)) && >>> - (set_dawr(&null_brk) != H_SUCCESS)) >>> - return -1; >>> - >>> - rc = debugfs_write_file_bool(file, user_buf, count, ppos); >>> - if (rc) >>> - return rc; >>> - >>> - /* If we are clearing, make sure all CPUs have the DAWR cleared */ >>> - if (!dawr_force_enable) >>> - smp_call_function((smp_call_func_t)set_dawr, &null_brk, 0); >>> - >>> - return rc; >>> -} >>> - >>> -static const struct file_operations dawr_enable_fops = { >>> - .read = debugfs_read_file_bool, >>> - .write = dawr_write_file_bool, >>> - .open = simple_open, >>> - .llseek = default_llseek, >>> -}; >>> - >>> -static int __init dawr_force_setup(void) >>> -{ >>> - dawr_force_enable = false; >>> - >>> - if (cpu_has_feature(CPU_FTR_DAWR)) { >>> - /* Don't setup sysfs file for user control on P8 */ >>> - dawr_force_enable = true; >>> - return 0; >>> - } >>> - >>> - if (PVR_VER(mfspr(SPRN_PVR)) == PVR_POWER9) { >>> - /* Turn DAWR off by default, but allow admin to turn it on */ >>> - dawr_force_enable = false; >>> - debugfs_create_file_unsafe("dawr_enable_dangerous", 0600, >>> - powerpc_debugfs_root, >>> - &dawr_force_enable, >>> - &dawr_enable_fops); >>> - } >>> - return 0; >>> -} >>> -arch_initcall(dawr_force_setup); >>> diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig >>> index bfdde04e49..9c0d315108 100644 >>> --- a/arch/powerpc/kvm/Kconfig >>> +++ b/arch/powerpc/kvm/Kconfig >>> @@ -39,6 +39,7 @@ config KVM_BOOK3S_32_HANDLER >>> config KVM_BOOK3S_64_HANDLER >>> bool >>> select KVM_BOOK3S_HANDLER >>> + select PPC_DAWR_FORCE_ENABLE >>> >>> config KVM_BOOK3S_PR_POSSIBLE >>> bool >>> >> >> Christophe >>
> > > > -- > > > > v2: > > > > Fixes based on Christophe Leroy's comments: > > > > - Fix commit message formatting > > > > - Move more DAWR code into dawr.c > > > > --- > > > > arch/powerpc/Kconfig | 5 ++ > > > > arch/powerpc/include/asm/hw_breakpoint.h | 20 ++++--- > > > > arch/powerpc/kernel/Makefile | 1 + > > > > arch/powerpc/kernel/dawr.c | 75 > > > > ++++++++++++++++++++++++ > > > > arch/powerpc/kernel/hw_breakpoint.c | 56 ------------------ > > > > arch/powerpc/kvm/Kconfig | 1 + > > > > 6 files changed, 94 insertions(+), 64 deletions(-) > > > > create mode 100644 arch/powerpc/kernel/dawr.c > > > > > > > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > > > > index 2711aac246..f4b19e48cc 100644 > > > > --- a/arch/powerpc/Kconfig > > > > +++ b/arch/powerpc/Kconfig > > > > @@ -242,6 +242,7 @@ config PPC > > > > select SYSCTL_EXCEPTION_TRACE > > > > select THREAD_INFO_IN_TASK > > > > select VIRT_TO_BUS if !PPC64 > > > > + select PPC_DAWR_FORCE_ENABLE if PPC64 || PERF > > > > > > What's PERF ? Did you mean PERF_EVENTS ? > > > > > > Then what you mean is: > > > - Selected all the time for PPC64 > > > - Selected for PPC32 when PERF is also selected. > > > > > > Is that what you want ? At first I thought it was linked to P9. > > > > This is wrong. I think we just want PPC64. PERF is a red herring. > > Are you sure ? Michael suggested PERF || KVM as far as I remember. It was mpe but I think it was wrong. We could make it more selective with something like: PPC64 && (HW_BREAK || PPC_ADV_DEBUG_REGS || PERF) but I think that will end up back in the larger mess of https://github.com/linuxppc/issues/issues/128 I don't think the minor size gain is is worth delving in that mess, hence I made it simply PPC64 which is hopefully an improvement on what we have since it eliminates 32bit. > > > > #else /* CONFIG_HAVE_HW_BREAKPOINT */ > > > > static inline void hw_breakpoint_disable(void) { } > > > > static inline void thread_change_pc(struct task_struct *tsk, > > > > struct pt_regs *regs) { } > > > > -static inline bool dawr_enabled(void) { return false; } > > > > + > > > > #endif /* CONFIG_HAVE_HW_BREAKPOINT */ > > > > + > > > > +extern bool dawr_force_enable; > > > > + > > > > +#ifdef CONFIG_PPC_DAWR_FORCE_ENABLE > > > > +extern bool dawr_enabled(void); > > > > > > Functions should not be 'extern'. I'm sure checkpatch --strict will tell > > > you. > > > > That's not what's currently being done in this header file. I'm keeping > > with > > the style of that file. > > style is not defined on a per file basis. There is the style from the > past and the nowadays style. If you keep old style just because the file > includes old style statements, then the code will never improve. > > All new patches should come with clean 'checkpatch' report and follow > new style. Having mixed styles in a file is not a problem, that's the > way to improvement. See arch/powerpc/mm/mmu_decl.h as an exemple. I'm not sure that's mpe's policy. mpe? > > > > + > > > > +static ssize_t dawr_write_file_bool(struct file *file, > > > > + const char __user *user_buf, > > > > + size_t count, loff_t *ppos) > > > > +{ > > > > + struct arch_hw_breakpoint null_brk = {0, 0, 0}; > > > > + size_t rc; > > > > + > > > > + /* Send error to user if they hypervisor won't allow us to write > > > > DAWR */ > > > > + if ((!dawr_force_enable) && > > > > + (firmware_has_feature(FW_FEATURE_LPAR)) && > > > > + (set_dawr(&null_brk) != H_SUCCESS)) > > > > > > The above is not real clear. > > > set_dabr() returns 0, H_SUCCESS is not used there. > > > > It pseries_set_dawr() will return a hcall number. > > Right, then it maybe means set_dawr() should be fixes ? Sorry, I don't understand this. > > This code hasn't changed. I'm just moving it. > > Right, but could be an improvment for another patch. > As far as I remember you are the one who wrote that code at first place, > arent't you ? Yep, classic crap Mikey code :-) Mikey
Le 14/05/2019 à 08:55, Michael Neuling a écrit : > [...] > >>>>> + >>>>> +static ssize_t dawr_write_file_bool(struct file *file, >>>>> + const char __user *user_buf, >>>>> + size_t count, loff_t *ppos) >>>>> +{ >>>>> + struct arch_hw_breakpoint null_brk = {0, 0, 0}; >>>>> + size_t rc; >>>>> + >>>>> + /* Send error to user if they hypervisor won't allow us to write >>>>> DAWR */ >>>>> + if ((!dawr_force_enable) && >>>>> + (firmware_has_feature(FW_FEATURE_LPAR)) && >>>>> + (set_dawr(&null_brk) != H_SUCCESS)) >>>> >>>> The above is not real clear. >>>> set_dabr() returns 0, H_SUCCESS is not used there. >>> >>> It pseries_set_dawr() will return a hcall number. >> >> Right, then it maybe means set_dawr() should be fixes ? > > Sorry, I don't understand this. I meant set_dawr() should be fixed: As the above test hide value 0 by using H_SUCCESS for the test, in order to ease understanding, set_dawr() should return H_SUCCESS instead of return 0; Christophe > >>> This code hasn't changed. I'm just moving it. >> >> Right, but could be an improvment for another patch. >> As far as I remember you are the one who wrote that code at first place, >> arent't you ? > > Yep, classic crap Mikey code :-) > > Mikey >
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 2711aac246..f4b19e48cc 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -242,6 +242,7 @@ config PPC select SYSCTL_EXCEPTION_TRACE select THREAD_INFO_IN_TASK select VIRT_TO_BUS if !PPC64 + select PPC_DAWR_FORCE_ENABLE if PPC64 || PERF # # Please keep this list sorted alphabetically. # @@ -369,6 +370,10 @@ config PPC_ADV_DEBUG_DAC_RANGE depends on PPC_ADV_DEBUG_REGS && 44x default y +config PPC_DAWR_FORCE_ENABLE + bool + default y + config ZONE_DMA bool default y if PPC_BOOK3E_64 diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h index 0fe8c1e46b..ffbc8eab41 100644 --- a/arch/powerpc/include/asm/hw_breakpoint.h +++ b/arch/powerpc/include/asm/hw_breakpoint.h @@ -47,6 +47,8 @@ struct arch_hw_breakpoint { #define HW_BRK_TYPE_PRIV_ALL (HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | \ HW_BRK_TYPE_HYP) +extern int set_dawr(struct arch_hw_breakpoint *brk); + #ifdef CONFIG_HAVE_HW_BREAKPOINT #include <linux/kdebug.h> #include <asm/reg.h> @@ -90,18 +92,20 @@ static inline void hw_breakpoint_disable(void) extern void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs); int hw_breakpoint_handler(struct die_args *args); -extern int set_dawr(struct arch_hw_breakpoint *brk); -extern bool dawr_force_enable; -static inline bool dawr_enabled(void) -{ - return dawr_force_enable; -} - #else /* CONFIG_HAVE_HW_BREAKPOINT */ static inline void hw_breakpoint_disable(void) { } static inline void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs) { } -static inline bool dawr_enabled(void) { return false; } + #endif /* CONFIG_HAVE_HW_BREAKPOINT */ + +extern bool dawr_force_enable; + +#ifdef CONFIG_PPC_DAWR_FORCE_ENABLE +extern bool dawr_enabled(void); +#else +#define dawr_enabled() true +#endif + #endif /* __KERNEL__ */ #endif /* _PPC_BOOK3S_64_HW_BREAKPOINT_H */ diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile index 0ea6c4aa3a..a9c497c34f 100644 --- a/arch/powerpc/kernel/Makefile +++ b/arch/powerpc/kernel/Makefile @@ -56,6 +56,7 @@ obj-$(CONFIG_PPC64) += setup_64.o sys_ppc32.o \ obj-$(CONFIG_VDSO32) += vdso32/ obj-$(CONFIG_PPC_WATCHDOG) += watchdog.o obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o +obj-$(CONFIG_PPC_DAWR_FORCE_ENABLE) += dawr.o obj-$(CONFIG_PPC_BOOK3S_64) += cpu_setup_ppc970.o cpu_setup_pa6t.o obj-$(CONFIG_PPC_BOOK3S_64) += cpu_setup_power.o obj-$(CONFIG_PPC_BOOK3S_64) += mce.o mce_power.o diff --git a/arch/powerpc/kernel/dawr.c b/arch/powerpc/kernel/dawr.c new file mode 100644 index 0000000000..cf1d02fe1e --- /dev/null +++ b/arch/powerpc/kernel/dawr.c @@ -0,0 +1,75 @@ +// SPDX-License-Identifier: GPL-2.0+ +// +// DAWR infrastructure +// +// Copyright 2019, Michael Neuling, IBM Corporation. + +#include <linux/types.h> +#include <linux/export.h> +#include <linux/fs.h> +#include <linux/debugfs.h> +#include <asm/debugfs.h> +#include <asm/machdep.h> +#include <asm/hvcall.h> + +bool dawr_force_enable; +EXPORT_SYMBOL_GPL(dawr_force_enable); + +extern bool dawr_enabled(void) +{ + return dawr_force_enable; +} +EXPORT_SYMBOL_GPL(dawr_enabled); + +static ssize_t dawr_write_file_bool(struct file *file, + const char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct arch_hw_breakpoint null_brk = {0, 0, 0}; + size_t rc; + + /* Send error to user if they hypervisor won't allow us to write DAWR */ + if ((!dawr_force_enable) && + (firmware_has_feature(FW_FEATURE_LPAR)) && + (set_dawr(&null_brk) != H_SUCCESS)) + return -1; + + rc = debugfs_write_file_bool(file, user_buf, count, ppos); + if (rc) + return rc; + + /* If we are clearing, make sure all CPUs have the DAWR cleared */ + if (!dawr_force_enable) + smp_call_function((smp_call_func_t)set_dawr, &null_brk, 0); + + return rc; +} + +static const struct file_operations dawr_enable_fops = { + .read = debugfs_read_file_bool, + .write = dawr_write_file_bool, + .open = simple_open, + .llseek = default_llseek, +}; + +static int __init dawr_force_setup(void) +{ + dawr_force_enable = false; + + if (cpu_has_feature(CPU_FTR_DAWR)) { + /* Don't setup sysfs file for user control on P8 */ + dawr_force_enable = true; + return 0; + } + + if (PVR_VER(mfspr(SPRN_PVR)) == PVR_POWER9) { + /* Turn DAWR off by default, but allow admin to turn it on */ + dawr_force_enable = false; + debugfs_create_file_unsafe("dawr_enable_dangerous", 0600, + powerpc_debugfs_root, + &dawr_force_enable, + &dawr_enable_fops); + } + return 0; +} +arch_initcall(dawr_force_setup); diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c index da307dd93e..95605a9c9a 100644 --- a/arch/powerpc/kernel/hw_breakpoint.c +++ b/arch/powerpc/kernel/hw_breakpoint.c @@ -380,59 +380,3 @@ void hw_breakpoint_pmu_read(struct perf_event *bp) { /* TODO */ } - -bool dawr_force_enable; -EXPORT_SYMBOL_GPL(dawr_force_enable); - -static ssize_t dawr_write_file_bool(struct file *file, - const char __user *user_buf, - size_t count, loff_t *ppos) -{ - struct arch_hw_breakpoint null_brk = {0, 0, 0}; - size_t rc; - - /* Send error to user if they hypervisor won't allow us to write DAWR */ - if ((!dawr_force_enable) && - (firmware_has_feature(FW_FEATURE_LPAR)) && - (set_dawr(&null_brk) != H_SUCCESS)) - return -1; - - rc = debugfs_write_file_bool(file, user_buf, count, ppos); - if (rc) - return rc; - - /* If we are clearing, make sure all CPUs have the DAWR cleared */ - if (!dawr_force_enable) - smp_call_function((smp_call_func_t)set_dawr, &null_brk, 0); - - return rc; -} - -static const struct file_operations dawr_enable_fops = { - .read = debugfs_read_file_bool, - .write = dawr_write_file_bool, - .open = simple_open, - .llseek = default_llseek, -}; - -static int __init dawr_force_setup(void) -{ - dawr_force_enable = false; - - if (cpu_has_feature(CPU_FTR_DAWR)) { - /* Don't setup sysfs file for user control on P8 */ - dawr_force_enable = true; - return 0; - } - - if (PVR_VER(mfspr(SPRN_PVR)) == PVR_POWER9) { - /* Turn DAWR off by default, but allow admin to turn it on */ - dawr_force_enable = false; - debugfs_create_file_unsafe("dawr_enable_dangerous", 0600, - powerpc_debugfs_root, - &dawr_force_enable, - &dawr_enable_fops); - } - return 0; -} -arch_initcall(dawr_force_setup); diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig index bfdde04e49..9c0d315108 100644 --- a/arch/powerpc/kvm/Kconfig +++ b/arch/powerpc/kvm/Kconfig @@ -39,6 +39,7 @@ config KVM_BOOK3S_32_HANDLER config KVM_BOOK3S_64_HANDLER bool select KVM_BOOK3S_HANDLER + select PPC_DAWR_FORCE_ENABLE config KVM_BOOK3S_PR_POSSIBLE bool
If you compile with KVM but without CONFIG_HAVE_HW_BREAKPOINT you fail at linking with: arch/powerpc/kvm/book3s_hv_rmhandlers.o:(.text+0x708): undefined reference to `dawr_force_enable' This was caused by commit c1fe190c0672 ("powerpc: Add force enable of DAWR on P9 option"). This puts more of the dawr infrastructure in a new file. Signed-off-by: Michael Neuling <mikey@neuling.org> -- v2: Fixes based on Christophe Leroy's comments: - Fix commit message formatting - Move more DAWR code into dawr.c --- arch/powerpc/Kconfig | 5 ++ arch/powerpc/include/asm/hw_breakpoint.h | 20 ++++--- arch/powerpc/kernel/Makefile | 1 + arch/powerpc/kernel/dawr.c | 75 ++++++++++++++++++++++++ arch/powerpc/kernel/hw_breakpoint.c | 56 ------------------ arch/powerpc/kvm/Kconfig | 1 + 6 files changed, 94 insertions(+), 64 deletions(-) create mode 100644 arch/powerpc/kernel/dawr.c