2017-11-22 03:15:05

by Vincent Chen

[permalink] [raw]
Subject: Re: FW: [PATCH 15/31] nds32: System calls handling

2017-11-13 19:42 GMT+08:00 Arnd Bergmann <[email protected]>:
> On Mon, Nov 13, 2017 at 3:51 AM, Vincent Chen <[email protected]> wrote:
>>>>On Wed, Nov 8, 2017 at 6:55 AM, Greentime Hu <[email protected]> wrote:
>>>> From: Greentime Hu <[email protected]>
>
>>
>>>> +#define __ARCH_WANT_SYS_CLONE
>>>
>>>This seems ok, though it would be nice to have the reverse logic and have architectures opt-out of the generic version when they need to provide their own, rather than having most architectures set it.
>>>
>>
>> Thanks
>> I will provide nds32 SYSCALL_DEFINE_5(clone) in the next version patch.
>
> That's not what I meant, my suggestion is to create a new patch to remove the
> __ARCH_WANT_SYS_CLONE symbol from all architectures, and instead use
> something like __ARCH_HAVE_SYS_CLONE on architectures that do *not*
> want the generic implementation.
>
> nds32 clearly wants the generic implementation here, it just shouldn't have to
> select that symbol to get it.
>
>>>> +/*
>>>> + * Special system call wrappers
>>>> + *
>>>> + * $r0 = syscall number
>>>> + * $r8 = syscall table
>>>> + */
>>>> + .type sys_syscall, #function
>>>> +ENTRY(sys_syscall)
>>>> + addi $p1, $r0, #-__NR_syscalls
>>>> + bgtz $p1, 3f
>>>> + move $p1, $r0
>>>> + move $r0, $r1
>>>> + move $r1, $r2
>>>> + move $r2, $r3
>>>> + move $r3, $r4
>>>> + move $r4, $r5
>>>> +! add for syscall 6 args
>>>> + lwi $r5, [$sp + #SP_OFFSET ]
>>>> + lwi $r5, [$r5]
>>>> +! ~add for syscall 6 args
>>>> +
>>>> + lw $p1, [tbl+$p1<<2]
>>>> .+ jr $p1
>>>> +3: b sys_ni_syscall
>>>> +ENDPROC(sys_syscall)
>>>
>>>Can you explain what this is used for?
>>>
>>
>> This is used to handle syscall(int number, ....).
>> Unlike other architectures, the system number shall be determined in
>> compile time when issuing system call in nds32.
>> Therefore, we only can parse the content of syscall(int number, ....)
>> and distribute it to destination handler in kernel space
>> (Other architecture can handle it in user space by glibc's syscall wrapper)
>
> Hmm, I think other architectures that run into this problem use self-modifying
> code for syscall(), but that is also ugly as it requires having a page that
> is both writable and executable.
>
> I think your approach can be tricky for things like seccomp(). It's possible
> that you get all of it right, but if you can come up with a different solution,
> that might be better.
>
> How much would it cost to simply always pass the syscall number
> in a register, and not use the immediate argument at all?
>

After re-evaluating the performance, we decide to use a particular
register to transfer syscall number
instead of immediate argument. So, above sys_syscall will be removed
in the next version patch

Thanks


>>>> --- /dev/null
>>>> +++ b/arch/nds32/kernel/sys_nds32.c
>>>> +
>>>> +long sys_mmap2(unsigned long addr, unsigned long len,
>>>> + unsigned long prot, unsigned long flags,
>>>> + unsigned long fd, unsigned long pgoff) {
>>>> + if (pgoff & (~PAGE_MASK >> 12))
>>>> + return -EINVAL;
>>>> +
>>>> + return sys_mmap_pgoff(addr, len, prot, flags, fd,
>>>> + pgoff >> (PAGE_SHIFT - 12)); }
>>>> +
>>>> +asmlinkage long sys_fadvise64_64_wrapper(int fd, int advice, loff_t offset,
>>>> + loff_t len) {
>>>> + return sys_fadvise64_64(fd, offset, len, advice); }
>>>
>>>You should always use SYSCALL_DEFINE*() macros to define entry points for your own syscalls in C code for consistency. I also wonder if we should just move those two into common code, a lot of architectures need the first one in particular.
>>>
>>
>> The sys_fadvise64_64_wrapper is used to reorder the input parameter.
>>
>> In order to solve register alignment problem, we adjust the input
>> parameter order of fadvise64_64 while issuing this syscall.
>> Therefore, we need this wrapper to reorder the input parameter to fit
>> sys_fadvise64_64's API in kernel.
>
> I understand what it's used for, it's just that I would recommend writing it
> differently, as
>
> SYSCALL_DEFINE4(fadvise64_64_wrapper, int, fd, int, advice, loff_t,
> offset, loff_t, len)
> {
> return sys_fadvise64_64(fd, offset, len, advice);
> }
>
> Arnd

From 1583951033555679020@xxx Mon Nov 13 11:43:39 +0000 2017
X-GM-THRID: 1583483337964688427
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread