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

[Jordan Niethe]

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.
>
> Christophe
>
> >>
> >> Anyway that's for patch.
> >>
> >> Thanks,
> >> Nick