Re: [RESEND, V3] powerpc, xmon: Enable HW instruction breakpoint on POWER8

[Anshuman Khandual <khandual@linux.vnet.ibm.com>]

On 11/26/2014 01:55 PM, Michael Ellerman wrote:
> On Tue, 2014-25-11 at 10:08:48 UTC, Anshuman Khandual wrote:
>> This patch enables support for hardware instruction breakpoints
>> on POWER8 with the help of a new register CIABR (Completed
>> Instruction Address Breakpoint Register). With this patch, single
>> hardware instruction breakpoint can be added and cleared during
>> any active xmon debug session. This hardware based instruction
>> breakpoint mechanism works correctly along with the existing TRAP
>> based instruction breakpoints available on xmon.
> 
> 
> Hi Anshuman,
> 
>> diff --git a/arch/powerpc/include/asm/xmon.h b/arch/powerpc/include/asm/xmon.h
>> index 5eb8e59..5d17aec 100644
>> --- a/arch/powerpc/include/asm/xmon.h
>> +++ b/arch/powerpc/include/asm/xmon.h
>> @@ -29,5 +29,11 @@ static inline void xmon_register_spus(struct list_head *list) { };
>>  extern int cpus_are_in_xmon(void);
>>  #endif
> 
> This file is the exported interface *of xmon*, it's not the place to put things
> that xmon needs internally.
> 
> For now just put it in xmon.c

Okay.

> 
>> +#if defined(CONFIG_PPC_BOOK3S_64) && defined(CONFIG_PPC_SPLPAR)
>> +#include <asm/plpar_wrappers.h>
>> +#else
>> +static inline long plapr_set_ciabr(unsigned long ciabr) {return 0; };
>> +#endif
> 
> Also the ifdef is overly verbose, CONFIG_PPC_SPLPAR essentially depends on
> CONFIG_PPC_BOOK3S_64. So you can just use #ifdef CONFIG_PPC_SPLPAR.

Yeah, thats correct.

> 
>> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
>> index b988b5a..c2f601a 100644
>> --- a/arch/powerpc/xmon/xmon.c
>> +++ b/arch/powerpc/xmon/xmon.c
>> @@ -271,6 +273,55 @@ static inline void cinval(void *p)
>>  }
>>  
>>  /*
>> + * write_ciabr
>> + *
>> + * This function writes a value to the
>> + * CIARB register either directly through
>> + * mtspr instruction if the kernel is in HV
>> + * privilege mode or call a hypervisor function
>> + * to achieve the same in case the kernel is in
>> + * supervisor privilege mode.
>> + */
> 
> I'm not really sure a function this small needs a documentation block.
> 
> But if you're going to add one, PLEASE make sure it's an actual kernel-doc
> style comment.
> 
> You can check with:
> 
> $ ./scripts/kernel-doc -text arch/powerpc/xmon/xmon.c
> 
> Which you'll notice prints:
> 
> Warning(arch/powerpc/xmon/xmon.c): no structured comments found
> 
> You need something like:
> 
> /**
>  * write_ciabr() - write the CIABR SPR
>  * @ciabr: The value to write.
>  *
>  * This function writes a value to the CIABR register either directly through
>  * mtspr instruction if the kernel is in HV privilege mode or calls a
>  * hypervisor function to achieve the same in case the kernel is in supervisor
>  * privilege mode.
>  */

Sure.

> 
> 
> 
> The rest of the patch is OK. But I was hoping you'd notice that we no longer
> support any cpus that implement CPU_FTR_IABR. And so you can just repurpose all
> the iabr logic for ciabr.

Okay.

> 
> Something like this, untested:

Yeah it is working on LPAR and also on bare metal platform as well. The new patch
will use some of your suggested code, so can I add your signed-off-by to the patch
as well ?