Messages in this thread | | | Date | Fri, 6 Apr 2018 14:34:20 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH 0/8] use struct pt_regs based syscall calling for x86-64 |
| |
* Dominik Brodowski <linux@dominikbrodowski.net> wrote:
> > > __sys_mprotect as prefix won't work by the way, as the double-underscore __sys_ > > > variant is already used in net/* for internal syscall helpers. > > > > Ok - then triple underscore - but overall I think it's more confusing. > > > > Btw., what was the problem with calling the x86 ptregs wrapper sys_mprotect? > > > > The only reason I suggested the __sys_x86_ prefix was because you originally > > suggested that there's symbol name overlap, but I don't think that's the case > > within the same kernel build, as the regular non-ptregs prototype: > > Indeed, there's no symbol name overlap within the same kernel build, but > technically different stubs named the same. If that's fine, just drop patch > 8/8 (including the UML fixup) and things should be fine, with the stub and > the entry in the syscall table both named sys_mprotect.
Ok, I've dropped patch #8.
> For IA32_EMULATION, we have __sys_ia32_mprotect as stub for the same > syscall, including this name as entry in syscall_32.tbl. > > More problematic is the naming for the compat stubs for IA32_EMAULATION and > X32, where we have > > __compat_sys_ia32_waitid > __compat_sys_x32_waitid > > for example. We *could* rename one of those to compat_sys_waitid() and levae > the other as-is, but actually I prefer it now how it is.
yeah, this is more symmetric I think.
So right now we have these symbols:
triton:~/tip> grep waitid System.map
ffffffff8105f1e0 t kernel_waitid # common C function (64-bit kargs) ffffffff8105f2b0 t SYSC_waitid # 64-bit uaddr args C function 352 bytes ffffffff8105f410 T sys_waitid # 64-bit-ptregs -> C stub, 32 bytes ffffffff8105f430 T __sys_ia32_waitid # 32-bit-ptregs -> C stub, 32 bytes ffffffff8105f450 t C_SYSC_waitid # 32-bit uaddr args C function, 400 bytes ffffffff8105f5e0 T __compat_sys_ia32_waitid # 32-bit-ptregs -> C stub, 32 bytes ffffffff8105f600 T __compat_sys_x32_waitid # 64-bit-ptregs -> C stub, 32 bytes
BTW., what is the role of generating __sys_ia32_waitid()? It seems unused when a syscall has a compat variant otherwise - like here.
Naming wise the odd thumb out is sys_waitid :-/
I'd argue that we should at minimum name it __sys_waitid:
ffffffff8105f1e0 t kernel_waitid # common C function (64-bit kargs) ffffffff8105f2b0 t SYSC_waitid # 64-bit uaddr args C function ffffffff8105f410 T __sys_waitid # 64-bit-ptregs -> C stub ffffffff8105f430 T __sys_ia32_waitid # 32-bit-ptregs -> C stub ffffffff8105f450 t C_SYSC_waitid # 32-bit uaddr args C function ffffffff8105f5e0 T __compat_sys_ia32_waitid # 32-bit-ptregs -> C stub ffffffff8105f600 T __compat_sys_x32_waitid # 64-bit-ptregs -> C stub
because that makes it all organized based on the same principle:
{__compat|_}_sys{_ia32|_x32|}_waittid
But arguably there are a whole lot more naming weirdnesses we could fix:
- I find it somewhat confusing that that 'C' in C_SYSC stands not for a C callign convention, but for 'COMPAT' - i.e. COMPAT_SYSC would be better.
- Another detail is that why is it called 'SYSC', if the other functions use the 'sys' prefix? Wouldn't 'SYS' be more consistent?
- If we introduced a prefix for the regular 64-bit system call format as well, we could have: x64, x32 and ia32.
- And finally, I think the argument format modifiers should be consistently prefixes - right now they are a mixture of pre- and post-fixes.
I.e. I'd generate the names like this:
__{x64,x32,ia32}[_compat]_sys_waittid()
The fully consistent nomenclature would be someting like this:
ffffffff8105f1e0 t kernel_waitid # common C function (64-bit kargs) ffffffff8105f2b0 t SYS_waitid # 64-bit uaddr args C function ffffffff8105f410 T __x64_sys_waitid # 64-bit-ptregs -> C stub ffffffff8105f430 T __ia32_sys_waitid # 32-bit-ptregs -> C stub ffffffff8105f450 t COMPAT_SYS_waitid # 32-bit uaddr args C function ffffffff8105f5e0 T __ia32_compat_sys_waitid # 32-bit-ptregs -> C stub ffffffff8105f600 T __x32_compat_sys_waitid # 64-bit-ptregs -> C stub
Looks a lot tidier and a lot more logical, doesn't it?
Makes grepping easier as well, because (case-insensitive) patterns like 'sys_waittid' would identify all the variants.
Personally I'd also do a s/ia32/i32 rename:
ffffffff8105f1e0 t kernel_waitid # common C function (64-bit kargs) ffffffff8105f2b0 t SYS_waitid # 64-bit uaddr args C function ffffffff8105f410 T __x64_sys_waitid # 64-bit-ptregs -> C stub ffffffff8105f430 T __i32_sys_waitid # 32-bit-ptregs -> C stub ffffffff8105f450 t COMPAT_SYS_waitid # 32-bit uaddr args C function ffffffff8105f5e0 T __i32_compat_sys_waitid # 32-bit-ptregs -> C stub ffffffff8105f600 T __x32_compat_sys_waitid # 64-bit-ptregs -> C stub
... but maybe that's too much ;-)
Thanks,
Ingo
| |