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

quoted

quoted

quoted

--
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.

quoted

quoted

quoted

   #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?

quoted

quoted

quoted

+
+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.

quoted

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