[PATCH v5 1/2] powerpc: silence a -Wcast-function-type warning in dawr_write_file_bool

[PATCH v5 1/2] powerpc: silence a -Wcast-function-type warning in dawr_write_file_bool

[Michael Neuling]

From: Mathieu Malaterre <malat@debian.org>

In commit c1fe190c0672 ("powerpc: Add force enable of DAWR on P9
option") the following piece of code was added:

   smp_call_function((smp_call_func_t)set_dawr, &null_brk, 0);

Since GCC 8 this triggers the following warning about incompatible
function types:

  arch/powerpc/kernel/hw_breakpoint.c:408:21: error: cast between incompatible function types from 'int (*)(struct arch_hw_breakpoint *)' to 'void (*)(void *)' [-Werror=cast-function-type]

Since the warning is there for a reason, and should not be hidden behind
a cast, provide an intermediate callback function to avoid the warning.

Fixes: c1fe190c0672 ("powerpc: Add force enable of DAWR on P9 option")
Suggested-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Mathieu Malaterre <malat@debian.org>
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 arch/powerpc/kernel/hw_breakpoint.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index da307dd93e..ca3a2358b7 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -384,6 +384,11 @@ void hw_breakpoint_pmu_read(struct perf_event *bp)
 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)
@@ -403,7 +408,7 @@ static ssize_t dawr_write_file_bool(struct file *file,
 
 	/* 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);
+		smp_call_function(set_dawr_cb, &null_brk, 0);
 
 	return rc;
 }
-- 
2.21.0

[PATCH v5 2/2] powerpc: Fix compile issue with force DAWR

[Michael Neuling]

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.

Fixes: c1fe190c0672 ("powerpc: Add force enable of DAWR on P9 option")
Signed-off-by: Michael Neuling <mikey@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(&current_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
-- 
2.21.0

Re: [PATCH v5 2/2] powerpc: Fix compile issue with force DAWR

[Christophe Leroy]

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@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(&current_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
> 

Re: [PATCH v5 2/2] powerpc: Fix compile issue with force DAWR

[Michael Neuling]

On Tue, 2019-06-18 at 18:28 +0200, Christophe Leroy wrote:
> 
> 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

I agree that their are opportunities to refactor this code and I appreciate your
efforts in making this code better but... 

We have a problem here of not being able to compile an odd ball case that almost
no one ever hits (it was just an odd mpe CI case). We're up to v5 of a simple
fix which is just silly. 

So let's get this fix in and move on to the whole bunch of refactoring we can do
in this code which is already documented in the github issue tracking.

Regards,
Mikey

Re: [PATCH v5 2/2] powerpc: Fix compile issue with force DAWR

[Christophe Leroy]

Le 19/06/2019 à 03:11, Michael Neuling a écrit :
> On Tue, 2019-06-18 at 18:28 +0200, Christophe Leroy wrote:
>>
>> 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
> 
> I agree that their are opportunities to refactor this code and I appreciate your
> efforts in making this code better but...
> 
> We have a problem here of not being able to compile an odd ball case that almost
> no one ever hits (it was just an odd mpe CI case). We're up to v5 of a simple
> fix which is just silly.
> 
> So let's get this fix in and move on to the whole bunch of refactoring we can do
> in this code which is already documented in the github issue tracking.
> 

Agreed.

I've filed the following issue to keep that in mind: 
https://github.com/linuxppc/issues/issues/251

Thanks
Christophe

Re: [PATCH v5 1/2] powerpc: silence a -Wcast-function-type warning in dawr_write_file_bool

[Michael Ellerman]

On Tue, 2019-06-04 at 03:00:36 UTC, Michael Neuling wrote:
> From: Mathieu Malaterre <malat@debian.org>
> 
> In commit c1fe190c0672 ("powerpc: Add force enable of DAWR on P9
> option") the following piece of code was added:
> 
>    smp_call_function((smp_call_func_t)set_dawr, &null_brk, 0);
> 
> Since GCC 8 this triggers the following warning about incompatible
> function types:
> 
>   arch/powerpc/kernel/hw_breakpoint.c:408:21: error: cast between incompatible function types from 'int (*)(struct arch_hw_breakpoint *)' to 'void (*)(void *)' [-Werror=cast-function-type]
> 
> Since the warning is there for a reason, and should not be hidden behind
> a cast, provide an intermediate callback function to avoid the warning.
> 
> Fixes: c1fe190c0672 ("powerpc: Add force enable of DAWR on P9 option")
> Suggested-by: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Mathieu Malaterre <malat@debian.org>
> Signed-off-by: Michael Neuling <mikey@neuling.org>

Series applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/548c54acba5bd1388d50727a9a126a42d0cd4ad0

cheers