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 ?
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
