[PATCH] powerpc: Fix compile issue with force DAWR

[PATCH] 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 this recent patch:
   commit c1fe190c06723322f2dfac31d3b982c581e434ef
   Author: Michael Neuling <mikey@neuling.org>
   powerpc: Add force enable of DAWR on P9 option

This builds dawr_force_enable in always via a new file.

Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 arch/powerpc/kernel/Makefile        |  2 +-
 arch/powerpc/kernel/dawr.c          | 11 +++++++++++
 arch/powerpc/kernel/hw_breakpoint.c |  3 ---
 3 files changed, 12 insertions(+), 4 deletions(-)
 create mode 100644 arch/powerpc/kernel/dawr.c

diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 0ea6c4aa3a..48a20ef5be 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -49,7 +49,7 @@ obj-y				:= cputable.o ptrace.o syscalls.o \
 				   signal.o sysfs.o cacheinfo.o time.o \
 				   prom.o traps.o setup-common.o \
 				   udbg.o misc.o io.o misc_$(BITS).o \
-				   of_platform.o prom_parse.o
+				   of_platform.o prom_parse.o dawr.o
 obj-$(CONFIG_PPC64)		+= setup_64.o sys_ppc32.o \
 				   signal_64.o ptrace32.o \
 				   paca.o nvram_64.o firmware.o
diff --git a/arch/powerpc/kernel/dawr.c b/arch/powerpc/kernel/dawr.c
new file mode 100644
index 0000000000..ca343efd23
--- /dev/null
+++ b/arch/powerpc/kernel/dawr.c
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// DAWR global variables
+//
+// Copyright 2019, Michael Neuling, IBM Corporation.
+
+#include <linux/types.h>
+#include <linux/export.h>
+
+bool dawr_force_enable;
+EXPORT_SYMBOL_GPL(dawr_force_enable);
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index da307dd93e..78a17454f4 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -381,9 +381,6 @@ 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)
-- 
2.21.0

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

[Christophe Leroy]

Le 08/05/2019 à 03:30, 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 this recent patch:
>     commit c1fe190c06723322f2dfac31d3b982c581e434ef
>     Author: Michael Neuling <mikey@neuling.org>
>     powerpc: Add force enable of DAWR on P9 option

I think you should use the standard commit format, checkpatch will tell you.

> 
> This builds dawr_force_enable in always via a new file.

Do we really need a new file just for that ?
As far as I understand, it is always compiled in, so can't we use 
another file like traps.o or setup-common.o or somewhere else ?

Or just put an ifdef in arch/powerpc/kvm/book3s_hv_rmhandlers.S ?
Because your fix will put dawr_force_enable on every build even the ones 
who don't need it at all.

Christophe

> 
> Signed-off-by: Michael Neuling <mikey@neuling.org> > ---
>   arch/powerpc/kernel/Makefile        |  2 +-
>   arch/powerpc/kernel/dawr.c          | 11 +++++++++++
>   arch/powerpc/kernel/hw_breakpoint.c |  3 ---
>   3 files changed, 12 insertions(+), 4 deletions(-)
>   create mode 100644 arch/powerpc/kernel/dawr.c
> 
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index 0ea6c4aa3a..48a20ef5be 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -49,7 +49,7 @@ obj-y				:= cputable.o ptrace.o syscalls.o \
>   				   signal.o sysfs.o cacheinfo.o time.o \
>   				   prom.o traps.o setup-common.o \
>   				   udbg.o misc.o io.o misc_$(BITS).o \
> -				   of_platform.o prom_parse.o
> +				   of_platform.o prom_parse.o dawr.o
>   obj-$(CONFIG_PPC64)		+= setup_64.o sys_ppc32.o \
>   				   signal_64.o ptrace32.o \
>   				   paca.o nvram_64.o firmware.o
> diff --git a/arch/powerpc/kernel/dawr.c b/arch/powerpc/kernel/dawr.c
> new file mode 100644
> index 0000000000..ca343efd23
> --- /dev/null
> +++ b/arch/powerpc/kernel/dawr.c
> @@ -0,0 +1,11 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +//
> +// DAWR global variables
> +//
> +// Copyright 2019, Michael Neuling, IBM Corporation.
> +
> +#include <linux/types.h>
> +#include <linux/export.h>
> +
> +bool dawr_force_enable;
> +EXPORT_SYMBOL_GPL(dawr_force_enable);
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index da307dd93e..78a17454f4 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -381,9 +381,6 @@ 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)
> 

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

[Michael Ellerman]

Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Le 08/05/2019 à 03:30, 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 this recent patch:
>>     commit c1fe190c06723322f2dfac31d3b982c581e434ef
>>     Author: Michael Neuling <mikey@neuling.org>
>>     powerpc: Add force enable of DAWR on P9 option
>
> I think you should use the standard commit format, checkpatch will tell you.
>
>> 
>> This builds dawr_force_enable in always via a new file.
>
> Do we really need a new file just for that ?

I told him to make a new file, rather than drop it in some other file
that's already full of unrealted junk :)

I did mean for him to put all the dawr_force_enable code in the file
though, not just the variable.

> As far as I understand, it is always compiled in, so can't we use 
> another file like traps.o or setup-common.o or somewhere else ?
>
> Or just put an ifdef in arch/powerpc/kvm/book3s_hv_rmhandlers.S ?
> Because your fix will put dawr_force_enable on every build even the ones 
> who don't need it at all.

We don't want to use an ifdef in the KVM code, because that would break
the case where you want to enable the DAWR for use by guests but the
host kernel doesn't have PERF support.

It shouldn't be in obj-y, it should at least be 64-bit only.

But it should be pretty trivial to create a config symbol for it, with
something like:

config DAWR_FORCE_ENABLE
	def_bool y
        depends on PERF || KVM

cheers