[PATCH v3 09/14] powerpc/xmon: Add initial support for prefixed instructions

[Nicholas Piggin]

Jordan Niethe's on February 28, 2020 10:37 am:
> On Thu, Feb 27, 2020 at 6:14 PM Christophe Leroy
> <christophe.leroy at c-s.fr> wrote:
>>
>>
>>
>> Le 27/02/2020 à 01:11, Jordan Niethe a écrit :
>> > On Wed, Feb 26, 2020 at 6:10 PM Nicholas Piggin <npiggin at gmail.com> wrote:
>> >>
>> >> Jordan Niethe's on February 26, 2020 2:07 pm:
>> >>> A prefixed instruction is composed of a word prefix and a word suffix.
>> >>> It does not make sense to be able to have a breakpoint on the suffix of
>> >>> a prefixed instruction, so make this impossible.
>> >>>
>> >>> When leaving xmon_core() we check to see if we are currently at a
>> >>> breakpoint. If this is the case, the breakpoint needs to be proceeded
>> >>> from. Initially emulate_step() is tried, but if this fails then we need
>> >>> to execute the saved instruction out of line. The NIP is set to the
>> >>> address of bpt::instr[] for the current breakpoint.  bpt::instr[]
>> >>> contains the instruction replaced by the breakpoint, followed by a trap
>> >>> instruction.  After bpt::instr[0] is executed and we hit the trap we
>> >>> enter back into xmon_bpt(). We know that if we got here and the offset
>> >>> indicates we are at bpt::instr[1] then we have just executed out of line
>> >>> so we can put the NIP back to the instruction after the breakpoint
>> >>> location and continue on.
>> >>>
>> >>> Adding prefixed instructions complicates this as the bpt::instr[1] needs
>> >>> to be used to hold the suffix. To deal with this make bpt::instr[] big
>> >>> enough for three word instructions.  bpt::instr[2] contains the trap,
>> >>> and in the case of word instructions pad bpt::instr[1] with a noop.
>> >>>
>> >>> No support for disassembling prefixed instructions.
>> >>>
>> >>> Signed-off-by: Jordan Niethe <jniethe5 at gmail.com>
>> >>> ---
>> >>> v2: Rename sufx to suffix
>> >>> v3: - Just directly use PPC_INST_NOP
>> >>>      - Typo: plac -> place
>> >>>      - Rename read_inst() to mread_inst(). Do not have it call mread().
>> >>> ---
>> >>>   arch/powerpc/xmon/xmon.c | 90 ++++++++++++++++++++++++++++++++++------
>> >>>   1 file changed, 78 insertions(+), 12 deletions(-)
>> >>>
>> >>> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
>> >>> index a673cf55641c..a73a35aa4a75 100644
>> >>> --- a/arch/powerpc/xmon/xmon.c
>> >>> +++ b/arch/powerpc/xmon/xmon.c
>> >>> @@ -97,7 +97,8 @@ static long *xmon_fault_jmp[NR_CPUS];
>> >>>   /* Breakpoint stuff */
>> >>>   struct bpt {
>> >>>        unsigned long   address;
>> >>> -     unsigned int    instr[2];
>> >>> +     /* Prefixed instructions can not cross 64-byte boundaries */
>> >>> +     unsigned int    instr[3] __aligned(64);
>> >>
>> >> This is pretty wild, I didn't realize xmon executes breakpoints out
>> >> of line like this.
>>
>> Neither did I. That's problematic. Kernel data is mapped NX on some
>> platforms.
>>
>> >>
>> >> IMO the break point entries here should correspond with a range of
>> >> reserved bytes in .text so we patch instructions into normal executable
>> >> pages rather than .data.
>> > Would it make sense to use vmalloc_exec() and use that like we are
>> > going to do in kprobes()?
>>
>> As we are (already) doing in kprobes() you mean ?
> Sorry for the confusion, I was mainly thinking of the patch that you
> pointed out before:
> https://patchwork.ozlabs.org/patch/1232619/
>>
>> In fact kprobes uses module_alloc(), and it works because kprobe depends
>> on module. On some platforms (i.e. book3s/32) vmalloc space is marked NX
>> in segment registers when CONFIG_MODULES is not set, see
>> mmu_mark_initmem_nx().  On other ones the Instruction TLB miss exception
>> does not manage misses at kernel addresses when CONFIG_MODULES is not
>> selected.
>>
>> So if we want XMON to work at all time, we need to use some (linear)
>> text address and use patch_instruction() to change it.
> Thank you for the detailed clarification, I will do it like that.

Yeah I would just make a little array in .text.xmon_bpts or something,
which should do the trick.

That wouldn't depend on this series, but if you want to do it as a fix
up patch before it, that would be good.

Thanks,
Nick