LinuxPPC-Dev Archive on lore.kernel.org
 help / color / Atom feed
From: Nicholas Piggin <npiggin@gmail.com>
To: linuxppc-dev@ozlabs.org, Michael Ellerman <mpe@ellerman.id.au>
Cc: christophe.leroy@c-s.fr, jniethe5@gmail.com
Subject: Re: [PATCH v2] powerpc: Add ppc_inst_next()
Date: Sun, 24 May 2020 10:01:12 +1000
Message-ID: <1590278278.q0bfj10pkn.astroid@bobo.none> (raw)
In-Reply-To: <1590277817.8ax5p94lur.astroid@bobo.none>

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.

Thanks,
Nick

  reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-22 13:33 Michael Ellerman
2020-05-23 23:56 ` Nicholas Piggin
2020-05-24  0:01   ` Nicholas Piggin [this message]
2020-05-25  2:41     ` Michael Ellerman
2020-05-25  3:28 ` Jordan Niethe

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=1590278278.q0bfj10pkn.astroid@bobo.none \
    --to=npiggin@gmail.com \
    --cc=christophe.leroy@c-s.fr \
    --cc=jniethe5@gmail.com \
    --cc=linuxppc-dev@ozlabs.org \
    --cc=mpe@ellerman.id.au \
    /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

LinuxPPC-Dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linuxppc-dev/0 linuxppc-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linuxppc-dev linuxppc-dev/ https://lore.kernel.org/linuxppc-dev \
		linuxppc-dev@lists.ozlabs.org linuxppc-dev@ozlabs.org
	public-inbox-index linuxppc-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linuxppc-dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git