From: Michael Ellerman <mpe@ellerman.id.au>
To: Nicholas Piggin <npiggin@gmail.com>, linuxppc-dev@ozlabs.org
Cc: christophe.leroy@c-s.fr, jniethe5@gmail.com
Subject: Re: [PATCH v2] powerpc: Add ppc_inst_next()
Date: Mon, 25 May 2020 12:41:30 +1000 [thread overview]
Message-ID: <87k110g2hx.fsf@mpe.ellerman.id.au> (raw)
In-Reply-To: <1590278278.q0bfj10pkn.astroid@bobo.none>
Nicholas Piggin <npiggin@gmail.com> writes:
> Excerpts from Nicholas Piggin's message of May 24, 2020 9:56 am:
>> Excerpts from Michael Ellerman's message of May 22, 2020 11:33 pm:
>>> In a few places we want to calculate the address of the next
>>> instruction. Previously that was simple, we just added 4 bytes, or if
>>> using a u32 * we incremented that pointer by 1.
>>>
>>> But prefixed instructions make it more complicated, we need to advance
>>> by either 4 or 8 bytes depending on the actual instruction. We also
>>> can't do pointer arithmetic using struct ppc_inst, because it is
>>> always 8 bytes in size on 64-bit, even though we might only need to
>>> advance by 4 bytes.
>>>
>>> So add a ppc_inst_next() helper which calculates the location of the
>>> next instruction, if the given instruction was located at the given
>>> address. Note the instruction doesn't need to actually be at the
>>> address in memory.
>>>
>>> Although it would seem natural for the value to be passed by value,
>>> that makes it too easy to write a loop that will read off the end of a
>>> page, eg:
>>>
>>> for (; src < end; src = ppc_inst_next(src, *src),
>>> dest = ppc_inst_next(dest, *dest))
>>>
>>> As noticed by Christophe and Jordan, if end is the exact end of a
>>> page, and the next page is not mapped, this will fault, because *dest
>>> will read 8 bytes, 4 bytes into the next page.
>>>
>>> So value is passed by reference, so the helper can be careful to use
>>> ppc_inst_read() on it.
>>>
>>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>> ---
>>> arch/powerpc/include/asm/inst.h | 13 +++++++++++++
>>> arch/powerpc/kernel/uprobes.c | 2 +-
>>> arch/powerpc/lib/feature-fixups.c | 15 ++++++++-------
>>> arch/powerpc/xmon/xmon.c | 2 +-
>>> 4 files changed, 23 insertions(+), 9 deletions(-)
>>>
>>> v2: Pass the value as a pointer and use ppc_inst_read() on it.
>>>
>>> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
>>> index d82e0c99cfa1..5b756ba77ed2 100644
>>> --- a/arch/powerpc/include/asm/inst.h
>>> +++ b/arch/powerpc/include/asm/inst.h
>>> @@ -100,6 +100,19 @@ static inline int ppc_inst_len(struct ppc_inst x)
>>> return ppc_inst_prefixed(x) ? 8 : 4;
>>> }
>>>
>>> +/*
>>> + * Return the address of the next instruction, if the instruction @value was
>>> + * located at @location.
>>> + */
>>> +static inline struct ppc_inst *ppc_inst_next(void *location, struct ppc_inst *value)
>>> +{
>>> + struct ppc_inst tmp;
>>> +
>>> + tmp = ppc_inst_read(value);
>>> +
>>> + return location + ppc_inst_len(tmp);
>>> +}
>>> +
>>> int probe_user_read_inst(struct ppc_inst *inst,
>>> struct ppc_inst __user *nip);
>>>
>>> diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
>>> index 83e883e1a42d..d200e7df7167 100644
>>> --- a/arch/powerpc/kernel/uprobes.c
>>> +++ b/arch/powerpc/kernel/uprobes.c
>>> @@ -112,7 +112,7 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>>> * support doesn't exist and have to fix-up the next instruction
>>> * to be executed.
>>> */
>>> - regs->nip = utask->vaddr + ppc_inst_len(ppc_inst_read(&auprobe->insn));
>>> + regs->nip = (unsigned long)ppc_inst_next((void *)utask->vaddr, &auprobe->insn);
>>>
>>> user_disable_single_step(current);
>>> return 0;
>>
>> AFAIKS except for here, there is no need for the void *location arg.
>>
>> I would prefer to drop that and keep this unchanged (it's a slightly
>> special case anyway).
>
> Ooops, I didn't read the last thread. I don't think insert_bpts needs it
> though, only this case. Anyway it's a minor nitpick so if it's already
> been considered, fine.
There's a few places that don't need it, eg:
+ nop = ppc_inst(PPC_INST_NOP);
+ for (; dest < end; dest = ppc_inst_next(dest, &nop))
+ raw_patch_instruction(dest, nop);
But I prefer the way that reads, it's clear we're incrementing by the
length of a nop, even though we could read the nop from dest because we
just patched it.
If we ever did execute-only kernel text, it would help to have the
location and value separate too, because then reading from the text
would require a helper, but reading from data/stack would not.
So I'll go with this version.
cheers
next prev parent reply other threads:[~2020-05-25 2:42 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-22 13:33 [PATCH v2] powerpc: Add ppc_inst_next() Michael Ellerman
2020-05-23 23:56 ` Nicholas Piggin
2020-05-24 0:01 ` Nicholas Piggin
2020-05-25 2:41 ` Michael Ellerman [this message]
2020-05-25 3:28 ` Jordan Niethe
2020-06-09 5:29 ` Michael Ellerman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87k110g2hx.fsf@mpe.ellerman.id.au \
--to=mpe@ellerman.id.au \
--cc=christophe.leroy@c-s.fr \
--cc=jniethe5@gmail.com \
--cc=linuxppc-dev@ozlabs.org \
--cc=npiggin@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).