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

[Christophe Leroy <christophe.leroy@c-s.fr>]

Le 27/02/2020 à 01:11, Jordan Niethe a écrit :
> On Wed, Feb 26, 2020 at 6:10 PM Nicholas Piggin <npiggin@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@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 ?

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.

Christophe

>>
>> Anyway that's for patch.
>>
>> Thanks,
>> Nick