Hi, Willy
Thanks very much for your comments on the new syscall.h proposal, just replied more, it is a very long email, hope it explains more clearly ;-)
Also CCed i386 and s390 committers, welcome your dicussion.
On Tue, Jul 25, 2023 at 01:44:14PM +0800, Zhangjin Wu wrote:
This discussion does inspire me a lot to shrink the whole architecture specific nolibc my_syscall<N>() macros, like crt.h, a common syscall.h is added to do so. I have finished most of them except the ones passing arguments via stack, still trying to merge these ones.
With this new syscall.h, to support my_syscall<N>, the arch-<ARCH>.h will only require to add ~10 lines to define their own syscall instructions, registers and clobberlist, which looks like this (for powerpc):
#define _NOLIBC_SYSCALL_CALL "sc; bns+ 1f; neg %0, %0; 1:" /* PowerPC doesn't always restore r3-r12 for us */ #define _NOLIBC_SYSCALL_CLOBBERLIST "memory", "cr0", "r12", "r11", "r10", "r9", "r8", "r7", "r6", "r5", "r4" /* PowerPC write GPRS in kernel side but not restore them */ #define _NOLIBC_GPRS_AS_OUTPUT_OPERANDS#define _NOLIBC_REG_NUM "r0" #define _NOLIBC_REG_RET "r3" #define _NOLIBC_REG_arg1 "r3" #define _NOLIBC_REG_arg2 "r4" #define _NOLIBC_REG_arg3 "r5" #define _NOLIBC_REG_arg4 "r6" #define _NOLIBC_REG_arg5 "r7" #define _NOLIBC_REG_arg6 "r8"
Before:
$ ls tools/include/nolibc/arch-*.h | while read f; do git show dfef4fc45d5713eb23d87f0863aff9c33bd4bfaf:$f 2>/dev/null | wc -l | tr -d '\n'; echo " $f"; done 157 tools/include/nolibc/arch-aarch64.h 199 tools/include/nolibc/arch-arm.h 178 tools/include/nolibc/arch-i386.h 164 tools/include/nolibc/arch-loongarch.h 195 tools/include/nolibc/arch-mips.h 0 tools/include/nolibc/arch-powerpc.h 160 tools/include/nolibc/arch-riscv.h 186 tools/include/nolibc/arch-s390.h 176 tools/include/nolibc/arch-x86_64.hAfter:
$ wc -l tools/include/nolibc/arch-*.h 54 tools/include/nolibc/arch-aarch64.h 84 tools/include/nolibc/arch-arm.h 90 tools/include/nolibc/arch-i386.h /* the last one use stack to pass arguments, reserve as-is */ 59 tools/include/nolibc/arch-loongarch.h 120 tools/include/nolibc/arch-mips.h /* the last two use stack to pass arguments, reserve as-is */ 73 tools/include/nolibc/arch-powerpc.h 58 tools/include/nolibc/arch-riscv.h 87 tools/include/nolibc/arch-s390.h 67 tools/include/nolibc/arch-x86_64.hsyscall.h itself:
$ wc -l tools/include/nolibc/syscall.h 112 tools/include/nolibc/syscall.hThe important thing to consider is not the number of lines but the *maintainability*.
The original goal is not really the number of lines (only a 'side-effect'), but is exactly easier porting/maintainability with clearer code architecture, I have wrotten another message to explain more about this background, so, let's directly reply here and discuss more.
You factored the syscall code so much above with all these macros that I don't even understand how they're going to interact with each other, especially "%0".
Yeah, it is my fault, this should be cleaned up with the return register directly:
#define _NOLIBC_SYSCALL_CALL \ "sc; bns+ 1f; neg 3, 3; 1:"
Also I don't know what the macro _NOLIBC_GPRS_AS_OUTPUT_OPERANDS does.
This is the root cause to inspire me to add the new syscall.h, let's explain the background step by step.
All of the other architectures (except PowerPC) restore GPRS for us when return from syscall, so, their clobber list simply not include the GPRS and only need to add the input registers in the 'INPUT Operands' list.
But PowerPC doesn't restore such GPRS for us, I'm not sure if it is a feature (Maybe) or a bug. for PowerPC32, the following line will restore the GPRS for us, but may be not a good idea to do so for it may decrease the syscall performance although save some instructions in user-space and also, the other libcs also follow the current rule, so, this may be a design intention we must follow (welcome your suggestions).
diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S index fe27d41f9a3d..1ed535e9144c 100644 --- a/arch/powerpc/kernel/entry_32.S +++ b/arch/powerpc/kernel/entry_32.S @@ -155,6 +155,7 @@ syscall_exit_finish: bne 3f mtcr r5
+ REST_GPRS(3, 12, r1) 1: REST_GPR(2, r1) REST_GPR(1, r1) rfi
For PowerPC, if with the previous method like the other architectures, the clobber list differs for every my_syscall<N> and all of the input registers must be put in the 'OUTPUT Operands' too to avoid compiler to save and resue a variable in such GPRS across my_syscall<N> calls.
Originally in my local new version of arch-powerpc.h, we got such code for every my_syscall<N>, use my_syscall6() as an example:
#define my_syscall6(num, arg1, arg2, arg3, arg4, arg5, arg6) \ ({ \ register long _ret __asm__ ("r3"); \ register long _num __asm__ ("r0") = (num); \ register long _arg1 __asm__ ("r3") = (long)(arg1); \ register long _arg2 __asm__ ("r4") = (long)(arg2); \ register long _arg3 __asm__ ("r5") = (long)(arg3); \ register long _arg4 __asm__ ("r6") = (long)(arg4); \ register long _arg5 __asm__ ("r7") = (long)(arg5); \ register long _arg6 __asm__ ("r8") = (long)(arg6); \ \ __asm__ volatile ( \ " sc\n" \ " bns+ 1f\n" \ " neg %0, %0\n" \ "1:\n" \ : "=r"(_ret), \ "+r"(_arg2), "+r"(_arg3), "+r"(_arg4), \ "+r"(_arg5), "+r"(_arg6) \ : "0"(_arg1), "r"(_num) \ : _NOLIBC_SYSCALL_CLOBBERLIST \ ); \ _ret; \ })
It almost aligns with the other architectures, but the full clobber list differs for every my_syscall<N>, the basic one is:
/* PowerPC kernel doesn't always restore r4-r12 for us */ #define _NOLIBC_SYSCALL_CLOBBERLIST \ "memory", "cr0", "r12", "r11", "r10", "r9",
Use my_syscall0() as a further example, we need something like this:
#define my_syscall0(num) \ ({ \ register long _ret __asm__ ("r3"); \ register long _num __asm__ ("r0") = (num); \ \ __asm__ volatile ( \ " sc\n" \ " bns+ 1f\n" \ " neg %0, %0\n" \ "1:\n" \ : "=r"(_ret) \ : "r"(_num) \ : _NOLIBC_SYSCALL_CLOBBERLIST, "r8", "r7", "r6", "r5", "r4" \ ); \ _ret; \ })
The additional "r8"..."r4" must be appended to the clobber list for they can not be put together for every my_syscall<N> due to conflicts between they between the clobber list and the "OUTPUT/INPUT operands".
I found a solution to share the same _NOLIBC_SYSCALL_CLOBBERLIST, that is split the "OUTPUT/INPUT Operands" list out of the core syscall assembly:
#define my_syscall6(num, arg1, arg2, arg3, arg4, arg5, arg6) \ ({ \ register long _ret __asm__ ("r3"); \ register long _num __asm__ ("r0") = (num); \ register long _arg1 __asm__ ("r3") = (long)(arg1); \ register long _arg2 __asm__ ("r4") = (long)(arg2); \ register long _arg3 __asm__ ("r5") = (long)(arg3); \ register long _arg4 __asm__ ("r6") = (long)(arg4); \ register long _arg5 __asm__ ("r7") = (long)(arg5); \ register long _arg6 __asm__ ("r8") = (long)(arg6); \ \ __asm__ volatile ("": "+r"(_arg2), "+r"(_arg3), "+r"(_arg4), \ "+r"(_arg5), "+r"(_arg6)::); \ \ __asm__ volatile ( \ " sc\n" \ " bns+ 1f\n" \ " neg %0, %0\n" \ "1:\n" \ : "=r"(_ret) \ : "0"(_arg1), "r"(_num) \ : _NOLIBC_SYSCALL_CLOBBERLIST \ ); \ _ret; \ })
Note, a question here is, the above split still require more discussion to make sure it does work for different toolchains (only test for gcc currently) or even if this method is right from scratch, welcome your suggestion.
As a result, all of the my_syscall<N> are able to share the core syscall calling assembly block, so, here is what we at last have:
#define _my_syscall_tail() \ __asm__ volatile ( \ _NOLIBC_SYSCALL_CALL \ : "=r"(_ret) \ : "r"(_num) \ : _NOLIBC_SYSCALL_CLOBBERLIST \ ); \ _ret
And further, we also found it was possible to share most of them among these not ugly but completely duplicated lines:
register long _ret __asm__ ("r3"); \ register long _num __asm__ ("r0") = (num); \ register long _arg1 __asm__ ("r3") = (long)(arg1); \ register long _arg2 __asm__ ("r4") = (long)(arg2); \ register long _arg3 __asm__ ("r5") = (long)(arg3); \ register long _arg4 __asm__ ("r6") = (long)(arg4); \ register long _arg5 __asm__ ("r7") = (long)(arg5); \ register long _arg6 __asm__ ("r8") = (long)(arg6); \ \ __asm__ volatile ("": "+r"(_arg2), "+r"(_arg3), "+r"(_arg4), \ "+r"(_arg5), "+r"(_arg6)::);
That's why at last we have such blocks (of course, for PowerPC itself it is a big change and not necessary):
#define _my_syscall_head(num) \ register long _ret __asm__ (_NOLIBC_REG_RET); \ register long _num __asm__ (_NOLIBC_REG_NUM) = (num) \
#ifdef _NOLIBC_REG_ERR #define _NOLIBC_REG_EXTRA _NOLIBC_REG_ERR #endif
#ifdef _NOLIBC_REG_EXTRA #define _my_syscall_extra() \ register long reg_extra __asm__ (_NOLIBC_REG_EXTRA); \ __asm__ volatile ("": "=r"(reg_extra)::) #else #define _my_syscall_extra() #endif
/* Architectures like PowerPC write GPRS in kernel side and not restore them */ #ifndef _NOLIBC_GPRS_AS_OUTPUT_OPERANDS #define _my_syscall_argn(n, arg) \ register long _arg##n __asm__ (_NOLIBC_REG_arg##n) = (long)(arg); \ __asm__ volatile ("":: "r"(_arg##n):) #else #define _my_syscall_argn(n, arg) \ register long _arg##n __asm__ (_NOLIBC_REG_arg##n) = (long)(arg); \ __asm__ volatile ("": "+r"(_arg##n)::) #endif
And this further build args for us:
#define _my_syscall_args0()
#define _my_syscall_args1(...) \ _my_syscall_args0(); \ _my_syscall_argn(1, __VA_ARGS__)
#define _my_syscall_args2(arg2, ...) \ _my_syscall_args1(__VA_ARGS__); \ _my_syscall_argn(2, arg2)
#define _my_syscall_args3(arg3, ...) \ _my_syscall_args2(__VA_ARGS__); \ _my_syscall_argn(3, arg3)
#define _my_syscall_args4(arg4, ...) \ _my_syscall_args3(__VA_ARGS__); \ _my_syscall_argn(4, arg4)
#define _my_syscall_args5(arg5, ...) \ _my_syscall_args4(__VA_ARGS__); \ _my_syscall_argn(5, arg5)
#define _my_syscall_args6(arg6, ...) \ _my_syscall_args5(__VA_ARGS__); \ _my_syscall_argn(6, arg6)
And at last:
#define __my_syscall_args(N, ...) _my_syscall_args##N(__VA_ARGS__) #define _my_syscall_args(N, ...) __my_syscall_args(N, ##__VA_ARGS__)
#define __my_syscall_narg(_0, _1, _2, _3, _4, _5, _6, N, ...) N #define _my_syscall_narg(...) __my_syscall_narg(__VA_ARGS__, 6, 5, 4, 3, 2, 1, 0)
#define __my_syscall_argsn(N, argn, ...) \ _my_syscall_args(_my_syscall_narg(NULL, ##__VA_ARGS__), ##__VA_ARGS__); \ _my_syscall_argn(N, argn)
#define _my_syscall_argsn(...) __my_syscall_argsn(_my_syscall_narg(NULL, ##__VA_ARGS__), ##__VA_ARGS__)
/* Note, my_syscall0() has no argument, can not use my_syscalln() */ #define my_syscall0(num) \ ({ \ _my_syscall_head(num); \ _my_syscall_extra(); \ _my_syscall_tail(); \ })
#define my_syscalln(num, ...) \ ({ \ _my_syscall_head(num); \ _my_syscall_extra(); \ _my_syscall_argsn(__VA_ARGS__); \ _my_syscall_tail(); \ })
#define my_syscall1(num, arg1) my_syscalln(num, arg1) #define my_syscall2(num, arg1, arg2) my_syscalln(num, arg2, arg1) #define my_syscall3(num, arg1, arg2, arg3) my_syscalln(num, arg3, arg2, arg1) #define my_syscall4(num, arg1, arg2, arg3, arg4) my_syscalln(num, arg4, arg3, arg2, arg1)
#ifndef my_syscall5 #define my_syscall5(num, arg1, arg2, arg3, arg4, arg5) my_syscalln(num, arg5, arg4, arg3, arg2, arg1) #endif #ifndef my_syscall6 #define my_syscall6(num, arg1, arg2, arg3, arg4, arg5, arg6) my_syscalln(num, arg6, arg5, arg4, arg3, arg2, arg1) #endif
At last, I found this worked on all of the supported architectures, so, the new syscall.h is proposed.
BTW, another question here is, to utilize the feature of __VA_ARGS__ to easier getting the last argument, the order of arguments are reversed during the declarations of the my_syscall<N>, any suggestion on this part? is it possible to not reverse the order? If possible, the my_syscall<N> declarations may be simplified to:
#define my_syscall1(...) my_syscalln(__VA_ARGS__)
And even be possible to define syscall() from unistd.h as a alias of my_syscalln():
#define syscall(...) my_syscalln(__VA_ARGS__)
And further, these three from unistd.h are sharable:
#define __syscall_narg(_0, _1, _2, _3, _4, _5, _6, N, ...) N #define _syscall_narg(...) __syscall_narg(__VA_ARGS__, 6, 5, 4, 3, 2, 1, 0) #define _syscall_n(N, ...) _syscall(N, __VA_ARGS__)
And when someone reports a bug like we had in the past with programs randomly crashing depending on stack alignment and such, it becomes particularly tricky to figure what is expected and how it differs from reality.
Macros are really hard to debug, the above code lines cost me two days ;-)
but after the common syscall.h, the left architecture specific parts are very few and easier to debug and even less copy and paste.
Maybe it's possible to do something in between, like defining a few more slightly larger blocks,
Yeah, here is the method we applied, new blocks added are:
/* for ret and num */ #define _my_syscall_head(num) \
/* for extra err output */ #define _my_syscall_extra() \
/* for every argument, with additional OUTPUT/INPUT operands setting */ #define _my_syscall_argn(n, arg) \
/* for the core syscall calling and return */ #define _my_syscall_tail() \
/* for variable number of args */ #define _my_syscall_args0() #define _my_syscall_args1(...) \ #define _my_syscall_args2(arg2, ...) \ #define _my_syscall_args3(arg3, ...) \ #define _my_syscall_args4(arg4, ...) \ #define _my_syscall_args5(arg5, ...) \ #define _my_syscall_args6(arg6, ...) \
/* unified variable number of args */ #define _my_syscall_argsn(...)
/* my_syscall0 */ #define my_syscall0(num) \
/* my_syscalln */ #define my_syscalln(num, ...) \
Still require more discussion on their names or even more simplification.
I don't know. I still tend to think that this significantly complicates the understanding of the whole thing.
Willy, don't worry, I do think it make things easier, the worse case is using the old code or only use part of our new blocks helpers ;-)
For this new syscall.h, it mainly clear the inline assembly codes, as we know, inline assembly code is a very complicated thing, If we clear the logic carefully (as we target but not yet) in our common code, architecture developers only require to focus on the platform specific definitions, it should be better for portability, review and maintainability.
It is very hard to learn the meanning of the OUTPUT operands, INPUT operands and even the clobber list and even the flags you mentioned below, clearing up them is really required, this new syscall.h also require more comments on the macro functions and variables.
Also, looking at different archs' syscall code, they're not all defined the same way. Some like i386 use "=a" for the return value. Others use "+r" as an input/output value, others "=r",
Agree, this is a very hard part of the inline assembly codes, clearing up the generic versions in syscall.h with additional comments may really help a lot.
For OUTPUT/INPUT operands, they may be enough for the generic versions:
#define _my_syscall_tail() \ __asm__ volatile ( \ _NOLIBC_SYSCALL_CALL \ : "=r"(_ret) \ : "r"(_num) \ : _NOLIBC_SYSCALL_CLOBBERLIST \ ); \ _ret
#ifdef _NOLIBC_REG_ERR #define _NOLIBC_REG_EXTRA _NOLIBC_REG_ERR #endif
#ifdef _NOLIBC_REG_EXTRA #define _my_syscall_extra() \ register long reg_extra __asm__ (_NOLIBC_REG_EXTRA); \ __asm__ volatile ("": "=r"(reg_extra)::) #else #define _my_syscall_extra() #endif
/* Architectures like PowerPC write GPRS in kernel side and not restore them */ #ifndef _NOLIBC_GPRS_AS_OUTPUT_OPERANDS #define _my_syscall_argn(n, arg) \ register long _arg##n __asm__ (_NOLIBC_REG_arg##n) = (long)(arg); \ __asm__ volatile ("":: "r"(_arg##n):) #else #define _my_syscall_argn(n, arg) \ register long _arg##n __asm__ (_NOLIBC_REG_arg##n) = (long)(arg); \ __asm__ volatile ("": "+r"(_arg##n)::) #endif
- "=r" is only required for output registers, it is mainly for "RET" register and "ERR" register.
- "r" is for input, mainly for "NUM" register and the input arguments (sometimes, the input arguments are used as "RET" and "ERR" too).
- "+r" means both input and output, only used by powerpc currently.
For the input registers used as "ERR" or "RET" output, "+r" is used before, but now, we split them to two parts, one is "=r"(_ret) in _my_syscall_tail(), another is in _my_syscall_argn(n, arg), they together work for "+r" = "=r" + "r"
Btw, have checked "=r" instead of "=a" works on i386 too for we already bind the _ret variable with "RET" register, but still need to check if "=a" is necessary?
others "+d" or "=d". And reading the comments, there are some arch-specific choices for these declarations, that are sometimes there to force the toolchain a little bit.
Even for s390, test shows, "r" instead of "d" works too (boot and test passed, not yet checked the size and codes generated), but I didn't find any document about "d" for s390 from the gcc online doc. This part (include all of the supported architectures) should be carefully checked if really adding our new syscall.h. add s390 nolibc committer to CC: list.
If "r" really not works on a target, we can use the logic like this (include syscall.h after architecture specific definitions):
#ifndef my_syscall5 #define my_syscall5(num, arg1, arg2, arg3, arg4, arg5) my_syscalln(num, arg5, arg4, arg3, arg2, arg1) #endif #ifndef my_syscall6 #define my_syscall6(num, arg1, arg2, arg3, arg4, arg5, arg6) my_syscalln(num, arg6, arg5, arg4, arg3, arg2, arg1) #endif
The blocks from syscalls.h are only provided as generic support, the architectures can define their own versions. all of our new blocks can be protected like my_syscall<N> above, for example:
#ifndef _my_syscall_tail #define _my_syscall_tail ... #endif
But for s390 here, if it really requires "d" instead of "r", we are able to allow architectures define their own modifier like this:
#ifndef _NOLIBC_INLINE_ASM_MODIFIER #define _NOLIBC_INLINE_ASM_MODIFIER "r" #endif
Then, we can define _NOLIBC_INLINE_ASM_MODIFIER for s390:
#define _NOLIBC_INLINE_ASM_MODIFIER "d"
But architectures like i386, If "=a", "=b", ... modifiers are necessary, new versions of the blocks should be added for these architectures.
As a short summary for this part, the modifiers used by different architectures should be carefully checked, welcome more suggestions from the toolchains developers or the architecture maintainers, I will compare the code generated too.
Others like MIPS pass some of their args in the stack, so a name only is not sufficient. Thus, trying to factor all of this will not only make it very hard to insert arch-specific adjusments, but it will also make the code much less readable.
Currently, I didn't yet work on merging the 'stack' support, we used '#ifdef's in syscall.h:
#ifndef my_syscall5 #define my_syscall5 ... #endif #ifndef my_syscall6 #define my_syscall6 ... #endif
So, they are the same as before:
$ grep "#define my_syscall" -ur tools/include/nolibc/arch-*.h tools/include/nolibc/arch-i386.h:#define my_syscall6(num, arg1, arg2, arg3, arg4, arg5, arg6) \ tools/include/nolibc/arch-mips.h:#define my_syscall5(num, arg1, arg2, arg3, arg4, arg5) \ tools/include/nolibc/arch-mips.h:#define my_syscall6(num, arg1, arg2, arg3, arg4, arg5, arg6)
Still require more work on how to support these ones, but it is not that urgent ;-)
Also think about this for a moment: you had the opportunity to add two new archs by simply restarting from existing ones. s390 and loongarch were added the same way, leaving enough freedom to the implementers to adjust the definitions to fit their environment's constraints.
I think this reserves as-is if the new arch-<ARCH>.h simply not include our new syscalls.h, so, the new syscalls.h is optional, the worse case is as-is, no regression, enough freedom as before ;-)
And further, some architectures may resue some helpers from our new syscalls.h or at least learn something from what we have done for all of the supported architectures.
And eventually, if our generic versions fits, just defining the syscall instructions, registers and clobber list should be enough.
If you make it very complicated, I suspect we won't get any such contributions anymore just because nobody figures how to find their way through it, or it would require to change again for everyone just to add one specific case. I tend to think that the current situation is already fairly minimal with quite readable and debuggable calls, and that it's what *really* matters.
This may really influence the upstream and review flow:
- If people may send a new architecture support, if just fits the new syscall.h, we may need to review carefully about the test results, especially for the input/output operands, error register.
As tests for powerpc shows, the above issues should be covered by our nolibc-test.
- If people may send a new architecture support as before, If we find it fits our new syscalls.h or it can apply part of the blocks, we can give some suggestions.
- If people send something not just fit our new syscall.h, we may be able to think about if a new 'hook' is required, but it is not necessary, we can delay this change requirement to after a careful design (just like the argument passing via 'stack' case currently) .
When you're trying to reorganize code, it's important to ask yourself whether you would prefer to debug the old one or the new one at 3am. Hint: at 3am, the more abstractions there are, the less understandable it becomes.
Interesting and agree, but this abstraction does clear something to be more undersatndable too ;-)
I do hate hard-debuggable macros, but as we mentioned above, the inline assembly code is another harder parts, the new carefully tuned blocks may really help us to avoid introduce bugs with manually wrotten new codes and also it may help us to avoid copy and paste multiple duplicated lines of the same codes.
Willy, do we need to rename my_syscall<N> to _nolibc_syscall<N> to limit these macros nolibc internally? I plan to rename all of the new adding macros with _nolibc_ (for funcs) or _NOLIBC_ (for variables).
Why not, I'm not opposed to that. Just keep in mind that every single rename complicates backports (or may even silently break them), so it's important to know where you want to go and to try to make changes converge towards something stable and durable.
Yeah, let's keep the changes minimal, we could prefix the new macros with _nolibc_ or _NOLIBC_, the old ones can be kept as-is.
Best regards, Zhangjin
Thanks, Willy