[PATCH v5 2/2] powerpc: Fix compile issue with force DAWR
Christophe Leroy
christophe.leroy at c-s.fr
Wed Jun 19 02:28:34 AEST 2019
Le 04/06/2019 à 05:00, 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 moves a bunch of code around to fix this. It moves a lot of the
> DAWR code in a new file and creates a new CONFIG_PPC_DAWR to enable
> compiling it.
After looking at all this once more, I'm just wondering: why are we
creating stuff specific to DAWR ?
In the old days, we only add DABR, and everything was named on DABR.
When DAWR was introduced some years ago we renamed stuff like do_dabr()
to do_break() so that we could regroup things together. And now we are
taking dawr() out of the rest. Why not keep dabr() stuff and dawr()
stuff all together in something dedicated to breakpoints, and try to
regroup all breakpoint stuff in a single place ? I see some
breakpointing stuff done in kernel/process.c and other things done in
hw_breakpoint.c, to common functions call from one file to the other,
preventing GCC to fully optimise, etc ...
Also, behing this thinking, I have the idea that we could easily
implement 512 bytes breakpoints on the 8xx too. The 8xx have neither
DABR nor DAWR, but is using a set of comparators. And as you can see in
the 8xx version of __set_dabr() in kernel/process.c, we emulate the DABR
behaviour by setting two comparators. By using the same comparators with
a different setup, we should be able to implement breakpoints on larger
ranges of address.
Christophe
>
> Fixes: c1fe190c0672 ("powerpc: Add force enable of DAWR on P9 option")
> Signed-off-by: Michael Neuling <mikey at neuling.org>
> --
> v5:
> - Changes based on comments by hch
> - Change // to /* comments
> - Change return code from -1 to -ENODEV
> - Remove unneeded default n in new Kconfig option
> - Remove setting to default value
> - Remove unnecessary braces
>
> v4:
> - Fix merge conflict with patch from Mathieu Malaterre:
> powerpc: silence a -Wcast-function-type warning in dawr_write_file_bool
> - Fixed checkpatch issues noticed by Christophe Leroy.
>
> v3:
> Fixes based on Christophe Leroy's comments:
> - Fix Kconfig options to better reflect reality
> - Reorder alphabetically
> - Inline vs #define
> - Fixed default return for dawr_enabled() when CONFIG_PPC_DAWR=N
>
> V2:
> Fixes based on Christophe Leroy's comments:
> - Fix commit message formatting
> - Move more DAWR code into dawr.c
> ---
> arch/powerpc/Kconfig | 4 +
> arch/powerpc/include/asm/hw_breakpoint.h | 21 +++--
> arch/powerpc/kernel/Makefile | 1 +
> arch/powerpc/kernel/dawr.c | 98 ++++++++++++++++++++++++
> arch/powerpc/kernel/hw_breakpoint.c | 61 ---------------
> arch/powerpc/kernel/process.c | 28 -------
> arch/powerpc/kvm/Kconfig | 1 +
> 7 files changed, 118 insertions(+), 96 deletions(-)
> create mode 100644 arch/powerpc/kernel/dawr.c
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 8c1c636308..9d61b36df4 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -234,6 +234,7 @@ config PPC
> select OLD_SIGSUSPEND
> select PCI_DOMAINS if PCI
> select PCI_SYSCALL if PCI
> + select PPC_DAWR if PPC64
> select RTC_LIB
> select SPARSE_IRQ
> select SYSCTL_EXCEPTION_TRACE
> @@ -370,6 +371,9 @@ config PPC_ADV_DEBUG_DAC_RANGE
> depends on PPC_ADV_DEBUG_REGS && 44x
> default y
>
> +config PPC_DAWR
> + bool
> +
> 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..41abdae6d0 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -90,18 +90,25 @@ 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);
> +#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) { }
> +
> +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
> +
> +
> +#ifdef CONFIG_PPC_DAWR
> 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) { }
> +int set_dawr(struct arch_hw_breakpoint *brk);
> +#else
> static inline bool dawr_enabled(void) { return false; }
> -#endif /* CONFIG_HAVE_HW_BREAKPOINT */
> +static inline int set_dawr(struct arch_hw_breakpoint *brk) { return -1; }
> +#endif
> +
> #endif /* __KERNEL__ */
> #endif /* _PPC_BOOK3S_64_HW_BREAKPOINT_H */
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index 0ea6c4aa3a..56dfa7a2a6 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) += 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..ae3bd6abac
> --- /dev/null
> +++ b/arch/powerpc/kernel/dawr.c
> @@ -0,0 +1,98 @@
> +// 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);
> +
> +int set_dawr(struct arch_hw_breakpoint *brk)
> +{
> + unsigned long dawr, dawrx, mrd;
> +
> + dawr = brk->address;
> +
> + dawrx = (brk->type & (HW_BRK_TYPE_READ | HW_BRK_TYPE_WRITE))
> + << (63 - 58);
> + dawrx |= ((brk->type & (HW_BRK_TYPE_TRANSLATE)) >> 2) << (63 - 59);
> + dawrx |= (brk->type & (HW_BRK_TYPE_PRIV_ALL)) >> 3;
> + /* dawr length is stored in field MDR bits 48:53. Matches range in
> + * doublewords (64 bits) baised by -1 eg. 0b000000=1DW and
> + * 0b111111=64DW.
> + * brk->len is in bytes.
> + * This aligns up to double word size, shifts and does the bias.
> + */
> + mrd = ((brk->len + 7) >> 3) - 1;
> + dawrx |= (mrd & 0x3f) << (63 - 53);
> +
> + if (ppc_md.set_dawr)
> + return ppc_md.set_dawr(dawr, dawrx);
> + mtspr(SPRN_DAWR, dawr);
> + mtspr(SPRN_DAWRX, dawrx);
> + return 0;
> +}
> +
> +static void set_dawr_cb(void *info)
> +{
> + set_dawr(info);
> +}
> +
> +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 -ENODEV;
> +
> + 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(set_dawr_cb, &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)
> +{
> + 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 */
> + 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 ca3a2358b7..95605a9c9a 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -380,64 +380,3 @@ void hw_breakpoint_pmu_read(struct perf_event *bp)
> {
> /* TODO */
> }
> -
> -bool dawr_force_enable;
> -EXPORT_SYMBOL_GPL(dawr_force_enable);
> -
> -static void set_dawr_cb(void *info)
> -{
> - set_dawr(info);
> -}
> -
> -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(set_dawr_cb, &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/process.c b/arch/powerpc/kernel/process.c
> index 87da401299..03a2da35ce 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -797,34 +797,6 @@ static inline int set_dabr(struct arch_hw_breakpoint *brk)
> return __set_dabr(dabr, dabrx);
> }
>
> -int set_dawr(struct arch_hw_breakpoint *brk)
> -{
> - unsigned long dawr, dawrx, mrd;
> -
> - dawr = brk->address;
> -
> - dawrx = (brk->type & (HW_BRK_TYPE_READ | HW_BRK_TYPE_WRITE)) \
> - << (63 - 58); //* read/write bits */
> - dawrx |= ((brk->type & (HW_BRK_TYPE_TRANSLATE)) >> 2) \
> - << (63 - 59); //* translate */
> - dawrx |= (brk->type & (HW_BRK_TYPE_PRIV_ALL)) \
> - >> 3; //* PRIM bits */
> - /* dawr length is stored in field MDR bits 48:53. Matches range in
> - doublewords (64 bits) baised by -1 eg. 0b000000=1DW and
> - 0b111111=64DW.
> - brk->len is in bytes.
> - This aligns up to double word size, shifts and does the bias.
> - */
> - mrd = ((brk->len + 7) >> 3) - 1;
> - dawrx |= (mrd & 0x3f) << (63 - 53);
> -
> - if (ppc_md.set_dawr)
> - return ppc_md.set_dawr(dawr, dawrx);
> - mtspr(SPRN_DAWR, dawr);
> - mtspr(SPRN_DAWRX, dawrx);
> - return 0;
> -}
> -
> void __set_breakpoint(struct arch_hw_breakpoint *brk)
> {
> memcpy(this_cpu_ptr(¤t_brk), brk, sizeof(*brk));
> diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig
> index f53997a8ca..b8e13d5a4a 100644
> --- a/arch/powerpc/kvm/Kconfig
> +++ b/arch/powerpc/kvm/Kconfig
> @@ -38,6 +38,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
>
More information about the Linuxppc-dev
mailing list