Messages in this thread | | | Date | Wed, 24 Mar 2010 18:11:32 -0700 | From | Matt Helsley <> | Subject | Re: [C/R ARM][PATCH 1/3] ARM: Rudimentary syscall interfaces |
| |
On Wed, Mar 24, 2010 at 08:36:39PM +0100, Christoffer Dall wrote: > On Wed, Mar 24, 2010 at 4:53 PM, Oren Laadan <orenl@cs.columbia.edu> wrote: > > > > > > Matt Helsley wrote: > >> > >> On Wed, Mar 24, 2010 at 12:57:46AM -0400, Oren Laadan wrote: > >>> > >>> On Tue, 23 Mar 2010, Matt Helsley wrote: > >>> > >>>> On Tue, Mar 23, 2010 at 08:53:42PM +0000, Russell King - ARM Linux > >>>> wrote: > >>>>> > >>>>> On Sun, Mar 21, 2010 at 09:06:03PM -0400, Christoffer Dall wrote: > >>>>>> > >>>>>> This small commit introduces a global state of system calls for ARM > >>>>>> making it possible for a debugger or checkpointing to gain information > >>>>>> about another process' state with respect to system calls. > >>>>> > >>>>> I don't particularly like the idea that we always store the syscall > >>>>> number to memory for every system call, whether the stored version is > >>>>> used or not. > >>>>> > >>>>> Since ARM caches are generally not write allocate, this means mostly > >>>>> write-only variables can have a higher than expected expense. > >>>>> > >>>>> Is there not some thread flag which can be checked to see if we need to > >>>>> store the syscall number? > >>>> > >>>> Perhaps before we freeze the task we can save the syscall number on ARM. > >>>> The patches suggest that the signal delivery path -- which the freezer > >>>> utilizes -- has the syscall number already. > > > > Actually, the signal path doesn't have the syscall number, it has > > a binary "in syscall" value. > >
Argh. I read too much into the name :(.
> > Well, this could be changed to pass the syscall number through > registers along to try_to_freeze without any mentionable performance > hit.
Yes, that's possible. I was thinking we could still use your thread info field but only store to it when we know it will be useful for c/r rather than for each syscall. Personally, I'd rather avoid passing the extra parameter into try_to_freeze(). Your idea below seems better to me.
> Re-using the assembly code or factoring it out so that it can be used > from multiple places doesn't seem very pleasing to me, as the assembly > code is in the critical path and written specifically for the context > of a process entering the kernel. Please correct me if I'm wrong. > > I imagine simply a function in C, more or less re-implementing the > logic that's already in entry-common.S, might do the trick. I wouldn't > worry much about the performance in this case as it will not be used > often. The following _untested_ snippet illustrates my idea: > > --- > arch/arm/include/asm/syscall.h | 93 +++++++++++++++++++++++++++++++++++++++- > 1 files changed, 92 insertions(+), 1 deletions(-) > > diff --git a/arch/arm/include/asm/syscall.h b/arch/arm/include/asm/syscall.h > index 3b3248f..a7f2615 100644 > --- a/arch/arm/include/asm/syscall.h > +++ b/arch/arm/include/asm/syscall.h > @@ -10,10 +10,101 @@ > #ifndef _ASM_ARM_SYSCALLS_H > #define _ASM_ARM_SYSCALLS_H > > +static inline int get_swi_instruction(struct task_struct *task, > + struct pt_regs *regs, > + unsigned long *instr) > +{ > + struct page *page = NULL; > + unsigned long instr_addr; > + unsigned long *ptr; > + int ret; > + > + instr_addr = regs->ARM_pc - 4; > + > + down_read(&task->mm->mmap_sem); > + ret = get_user_pages(task, task->mm, instr_addr, > + 1, 0, 0, &page, NULL); > + up_read(&task->mm->mmap_sem); > + > + if (ret < 0) > + return ret; > + > + ptr = (unsigned long *)kmap_atomic(page, KM_USER1); > + memcpy(instr, > + ptr + (instr_addr >> PAGE_SHIFT), ^shouldn't this be: instr_addr & PAGE_MASK
> + sizeof(unsigned long)); > + kunmap_atomic(ptr, KM_USER1); > + > + page_cache_release(page); > + > + return 0; > +}
(again, not familiar with ARM so my understanding is:
I guess swi is "syscall word immediate".
The syscall nr is embedded in the instruction as an immediate value and you're getting a copy of that instruction using the value of the pc register just after the syscall instruction was executed.)
Perhaps I am missing or forgetting something. Why isn't this as simple as calling get_user() or even copy_from_user() using instr_addr?
Cheers, -Matt Helsley
| |