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

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