On 11/07, Ingo Molnar wrote:
>
> * Oleg Nesterov <o...@redhat.com> wrote:
>
> > --- a/arch/powerpc/include/asm/uprobes.h
> > +++ b/arch/powerpc/include/asm/uprobes.h
> > @@ -37,6 +37,7 @@ typedef ppc_opcode_t uprobe_opcode_t;
> > struct arch_uprobe {
> > union {
> > u8 insn[MAX_UINSN_BYTES];
> > + u8 ixol[MAX_UINSN_BYTES];
> > u32 ainsn;
> > };
> > };
>
> > --- a/arch/x86/include/asm/uprobes.h
> > +++ b/arch/x86/include/asm/uprobes.h
> > @@ -35,7 +35,10 @@ typedef u8 uprobe_opcode_t;
> >
> > struct arch_uprobe {
> > u16 fixups;
> > - u8 insn[MAX_UINSN_BYTES];
> > + union {
> > + u8 insn[MAX_UINSN_BYTES];
> > + u8 ixol[MAX_UINSN_BYTES];
> > + };
> > #ifdef CONFIG_X86_64
> > unsigned long rip_rela_target_address;
> > #endif
>
> Btw., at least on the surface, the powerpc and x86 definitions seem rather
> similar, barring senseless variations. Would it make sense to generalize
> the data structure a bit more?
Heh. You know, I have another patch, see below. It was not tested yet, it
should be splitted into 3 changes, and we need to cleanup copy_insn() first.
I didn't sent it now because I wanted to merge the minimal changes which
allow us to avoid the new arm arch_upobe_* hooks. And of course it needs
the review.
But in short, I do not think we should try to unify/generalize insn/ixol.
For the moment, please ignore the patch which adds the new ->ixol member.
The generic code knows nothing about uprobe->arch, except: arch_uprobe
must have "u8 insn[MAX_UINSN_BYTES]". And this is why powerpc also has
arch_uprobe->ainsn, it defines ->insn to make the generic code happy.
Fortunately, array == &array, so we can remove this dependency and just
require arch_uprobe->insn of any type, this is what the patch below tries
to do.
> Also, we all hate data structures that are not self-documenting. What does
> 'ixol' mean and what is its role? Is it obvious to the reader of that
> file?
Probably it makes sense a comment near "struct uprobe" anyway, will try
to do this. But I think that with the patch below the role of insn/ixol
is obvious:
arch_uprobe->insn: this is where we copy the original instruction,
arch/ can do whatever it wants with this data.
arch_uprobe->ixol: this is what we copy into xol slot, arch/ should
initialize this data.
x86 and powerpc can use the same member, arm needs to create ixol != insn.
Note: we also have set_swbp() (which doesn't use ->insn) and
set_orig_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
unsigned long vaddr)
{
return uprobe_write_opcode(mm, vaddr, *(uprobe_opcode_t
*)auprobe->insn);
}
I _think_ this should be cleanuped too, but I am not sure and this will
conflict with the pending arm changes. So I am going to try to do this
later, after we merge arm port. And this is really minor.
Oleg.
arch/powerpc/include/asm/uprobes.h | 5 ++---
arch/powerpc/kernel/uprobes.c | 2 +-
kernel/events/uprobes.c | 24 +++++++++++-------------
3 files changed, 14 insertions(+), 17 deletions(-)
diff --git a/arch/powerpc/include/asm/uprobes.h
b/arch/powerpc/include/asm/uprobes.h
index 75c6ecd..7422a99 100644
--- a/arch/powerpc/include/asm/uprobes.h
+++ b/arch/powerpc/include/asm/uprobes.h
@@ -36,9 +36,8 @@ typedef ppc_opcode_t uprobe_opcode_t;
struct arch_uprobe {
union {
- u8 insn[MAX_UINSN_BYTES];
- u8 ixol[MAX_UINSN_BYTES];
- u32 ainsn;
+ u32 insn;
+ u32 ixol;
};
};
diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
index 59f419b..003b209 100644
--- a/arch/powerpc/kernel/uprobes.c
+++ b/arch/powerpc/kernel/uprobes.c
@@ -186,7 +186,7 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe,
struct pt_regs *regs)
* emulate_step() returns 1 if the insn was successfully emulated.
* For all other cases, we need to single-step in hardware.
*/
- ret = emulate_step(regs, auprobe->ainsn);
+ ret = emulate_step(regs, auprobe->insn);
if (ret > 0)
return true;
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index e615b78..aba4ce9 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -329,7 +329,7 @@ int __weak set_swbp(struct arch_uprobe *auprobe, struct
mm_struct *mm, unsigned
int __weak
set_orig_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long
vaddr)
{
- return uprobe_write_opcode(mm, vaddr, *(uprobe_opcode_t
*)auprobe->insn);
+ return uprobe_write_opcode(mm, vaddr, *(uprobe_opcode_t
*)&auprobe->insn);
}
static int match_uprobe(struct uprobe *l, struct uprobe *r)
@@ -504,7 +504,7 @@ static bool consumer_del(struct uprobe *uprobe, struct
uprobe_consumer *uc)
}
static int
-__copy_insn(struct address_space *mapping, struct file *filp, char *insn,
+__copy_insn(struct address_space *mapping, struct file *filp, void *insn,
unsigned long nbytes, loff_t offset)
{
struct page *page;
@@ -527,28 +527,26 @@ __copy_insn(struct address_space *mapping, struct file
*filp, char *insn,
static int copy_insn(struct uprobe *uprobe, struct file *filp)
{
- struct address_space *mapping;
- unsigned long nbytes;
- int bytes;
+ struct address_space *mapping = uprobe->inode->i_mapping;
+ int bytes = sizeof(uprobe->arch.insn);
+ void *insn = &uprobe->arch.insn;
+ int nbytes;
nbytes = PAGE_SIZE - (uprobe->offset & ~PAGE_MASK);
- mapping = uprobe->inode->i_mapping;
/* Instruction at end of binary; copy only available bytes */
- if (uprobe->offset + MAX_UINSN_BYTES > uprobe->inode->i_size)
+ if (uprobe->offset + bytes > uprobe->inode->i_size)
bytes = uprobe->inode->i_size - uprobe->offset;
- else
- bytes = MAX_UINSN_BYTES;
/* Instruction at the page-boundary; copy bytes in second page */
if (nbytes < bytes) {
- int err = __copy_insn(mapping, filp, uprobe->arch.insn + nbytes,
+ int err = __copy_insn(mapping, filp, insn + nbytes,
bytes - nbytes, uprobe->offset + nbytes);
if (err)
return err;
bytes = nbytes;
}
- return __copy_insn(mapping, filp, uprobe->arch.insn, bytes,
uprobe->offset);
+ return __copy_insn(mapping, filp, insn, bytes, uprobe->offset);
}
static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
@@ -569,7 +567,7 @@ static int prepare_uprobe(struct uprobe *uprobe, struct
file *file,
goto out;
ret = -ENOTSUPP;
- if (is_trap_insn((uprobe_opcode_t *)uprobe->arch.insn))
+ if (is_trap_insn((uprobe_opcode_t *)&uprobe->arch.insn))
goto out;
ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr);
@@ -1257,7 +1255,7 @@ static unsigned long xol_get_insn_slot(struct uprobe
*uprobe)
/* Initialize the slot */
copy_to_page(area->page, xol_vaddr,
- uprobe->arch.ixol, sizeof(uprobe->arch.ixol));
+ &uprobe->arch.ixol, sizeof(uprobe->arch.ixol));
/*
* We probably need flush_icache_user_range() but it needs vma.
* This should work on supported architectures too.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
