Re: [Minios-devel] [UNIKRAFT PATCH 07/10] lib/syscall_shim: introduce syscalls macro layer

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

On 04.06.19 18:28, Yuri Volchkov wrote:
> To provide a syscall user needs to use macro UK_SYSCALL_PROTO. The
> syntax is the following:
Hum, is UK_SYSCALL_PROTO the right name? I saw that is called 
UK_SYSCALL_DEFINE() in patch number 8.
> UK_SYSCALL_PROTO(syscall_name, argument_1_type, argument_1_name, ...)
> {
>      <syscall_body>;
> }
>
> Signed-off-by: Yuri Volchkov <yuri.volchkov@xxxxxxxxx>
> ---
>   lib/syscall_shim/Makefile.uk          |   5 ++
>   lib/syscall_shim/include/uk/syscall.h | 100 ++++++++++++++++++++++++++
>   2 files changed, 105 insertions(+)
>   create mode 100644 lib/syscall_shim/include/uk/syscall.h
>
> diff --git a/lib/syscall_shim/Makefile.uk b/lib/syscall_shim/Makefile.uk
> index bb76d7b1..67677fc6 100644
> --- a/lib/syscall_shim/Makefile.uk
> +++ b/lib/syscall_shim/Makefile.uk
> @@ -50,4 +50,9 @@ $(__GEN_INCLUDES_PATH)/syscall_stubs.h.new:
>         $(Q) $(AWK) -f $(LIBSYSCALL_SHIM_BASE)/gen_stubs.awk \
>                 $(__SYSCALL_SHIM_TEMPL) > $@
>   
> +CINCLUDES-$(CONFIG_LIBSYSCALL_SHIM)   += -I$(LIBSYSCALL_SHIM_BUILD)/include
> +CXXINCLUDES-$(CONFIG_LIBSYSCALL_SHIM) += -I$(LIBSYSCALL_SHIM_BUILD)/include
> +CINCLUDES-y   += -I$(LIBSYSCALL_SHIM_BASE)/include
> +CXXINCLUDES-y += -I$(LIBSYSCALL_SHIM_BASE)/include
> +
>   LIBSYSCALL_SHIM_CLEAN = $(__PHONY_GEN_SRC) $(__PHONY_GEN_SRC_NEW) $(__GEN_SRC)
> diff --git a/lib/syscall_shim/include/uk/syscall.h 
> b/lib/syscall_shim/include/uk/syscall.h
> new file mode 100644
> index 00000000..19161e5e
> --- /dev/null
> +++ b/lib/syscall_shim/include/uk/syscall.h
> @@ -0,0 +1,100 @@
> +#ifndef __UK_SYSCALL_H__
> +#define __UK_SYSCALL_H__
> +
> +#include <errno.h>
> +#include <uk/print.h>
Doesn't essentials.h need to be included to have the UK_CONCAT macro 
populated?
> +
> +#define __uk_scc(X) ((long) (X))
> +typedef long syscall_arg_t;
> +
> +#define __uk_syscall(syscall_nr, ...) \
> +       UK_CONCAT(uk_syscall_fn_, syscall_nr) (__VA_ARGS__)
> +
> +#define __uk_syscall0(n) __uk_syscall(n)
> +#define __uk_syscall1(n,a) __uk_syscall(n,__uk_scc(a))
> +#define __uk_syscall2(n,a,b) __uk_syscall(n,__uk_scc(a),__uk_scc(b))
> +#define __uk_syscall3(n,a,b,c) 
> __uk_syscall(n,__uk_scc(a),__uk_scc(b),__uk_scc(c))
> +#define __uk_syscall4(n,a,b,c,d) 
> __uk_syscall(n,__uk_scc(a),__uk_scc(b),__uk_scc(c),__uk_scc(d))
> +#define __uk_syscall5(n,a,b,c,d,e) 
> __uk_syscall(n,__uk_scc(a),__uk_scc(b),__uk_scc(c),__uk_scc(d),__uk_scc(e))
> +#define __uk_syscall6(n,a,b,c,d,e,f) 
> __uk_syscall(n,__uk_scc(a),__uk_scc(b),__uk_scc(c),__uk_scc(d),__uk_scc(e),__uk_scc(f))
> +#define __uk_syscall7(n,a,b,c,d,e,f,g) 
> (__uk_syscall)(n,__uk_scc(a),__uk_scc(b),__uk_scc(c),__uk_scc(d),__uk_scc(e),__uk_scc(f),__uk_scc(g))
> +
> +
> +#define __SYSCALL_NARGS_X(a,b,c,d,e,f,g,h,n,...) n
> +#define __SYSCALL_NARGS(...) __SYSCALL_NARGS_X(__VA_ARGS__,7,6,5,4,3,2,1,0,)
> +
> +#define __SYSCALL_DEF_NARGS_X(z, a1,a2, b1,b2, c1,c2, d1,d2, e1,e2, f1,f2, 
> g1,g2, nr, ...) nr
> +#define __SYSCALL_DEF_NARGS(...) __SYSCALL_DEF_NARGS_X(__VA_ARGS__, 7,7, 6,6, 
> 5,5, 4,4, 3,3, 2,2, 1,1,0)
> +
> +#define __UK_NAME2SCALL_FN(name) UK_CONCAT(uk_syscall_, name)
> +
> +#define UK_ARG_MAP1(m, type, arg) m(type, arg)
> +#define UK_ARG_MAP2(m, type, arg, ...) m(type, arg), UK_ARG_MAP1(m, 
> __VA_ARGS__)
> +#define UK_ARG_MAP3(m, type, arg, ...) m(type, arg), UK_ARG_MAP2(m, 
> __VA_ARGS__)
> +#define UK_ARG_MAP4(m, type, arg, ...) m(type, arg), UK_ARG_MAP3(m, 
> __VA_ARGS__)
> +#define UK_ARG_MAP5(m, type, arg, ...) m(type, arg), UK_ARG_MAP4(m, 
> __VA_ARGS__)
> +#define UK_ARG_MAP6(m, type, arg, ...) m(type, arg), UK_ARG_MAP5(m, 
> __VA_ARGS__)
> +#define UK_ARG_MAP7(m, type, arg, ...) m(type, arg), UK_ARG_MAP6(m, 
> __VA_ARGS__)
> +#define UK_ARG_MAPx(nr_args, ...) UK_CONCAT(UK_ARG_MAP, nr_args)(__VA_ARGS__)
> +
> +#define S_ARG_LONG(type, arg) unsigned long arg
> +#define S_ARG_ACTUAL(type, arg) type arg
> +#define S_ARG_CAST(type, arg) (type) arg
> +
> +
> +#ifdef CONFIG_LIBSYSCALL_SHIM
> +#define __UK_SYSCALL_DEFINE(x, name, ...)                              \
> +       static inline long __##name(UK_ARG_MAPx(x, S_ARG_ACTUAL, __VA_ARGS__)); 
> \
> +       int name(UK_ARG_MAPx(x, S_ARG_LONG, __VA_ARGS__))                       
> \
In this type casting wrapper, shouldn't this `int` be a `long` instead?

> +       {                                                               \
> +               long ret = __##name(                                    \
> +                       UK_ARG_MAPx(x, S_ARG_CAST, __VA_ARGS__));               
> \
> +               return ret;                                             \
> +       }                                                               \
> +       static inline long __##name(UK_ARG_MAPx(x, S_ARG_ACTUAL, __VA_ARGS__))
> +#else
> +#define __UK_SYSCALL_DEFINE(x, name, ...)                              \
> +       static inline long name(UK_ARG_MAPx(x, S_ARG_ACTUAL, __VA_ARGS__))
> +#endif
> +
> +#define _UK_SYSCALL_DEFINE(...) __UK_SYSCALL_DEFINE(__VA_ARGS__)
> +#define UK_SYSCALL_DEFINE(name, ...)                           \
> +       _UK_SYSCALL_DEFINE(__SYSCALL_DEF_NARGS(__VA_ARGS__),    \
> +                           __UK_NAME2SCALL_FN(name),           \
> +                           __VA_ARGS__)
> +
> +#define __UK_SPROTO_ARGS_TYPE unsigned long
> +#define __UK_SPROTO_ARGS0()  void
> +#define __UK_SPROTO_ARGS1()  __UK_SPROTO_ARGS_TYPE a
> +#define __UK_SPROTO_ARGS2()  __UK_SPROTO_ARGS1(), __UK_SPROTO_ARGS_TYPE b
> +#define __UK_SPROTO_ARGS3()  __UK_SPROTO_ARGS2(), __UK_SPROTO_ARGS_TYPE c
> +#define __UK_SPROTO_ARGS4()  __UK_SPROTO_ARGS3(), __UK_SPROTO_ARGS_TYPE d
> +#define __UK_SPROTO_ARGS5()  __UK_SPROTO_ARGS4(), __UK_SPROTO_ARGS_TYPE e
> +#define __UK_SPROTO_ARGS6()  __UK_SPROTO_ARGS5(), __UK_SPROTO_ARGS_TYPE f
> +#define __UK_SPROTO_ARGS7()  __UK_SPROTO_ARGS6(), __UK_SPROTO_ARGS_TYPE g
> +#define __UK_SPROTO_ARGSx(args_nr)  \
> +       UK_CONCAT(__UK_SPROTO_ARGS, args_nr)()
> +
> +#define UK_SYSCALL_PROTO(args_nr, syscall_name)                        \
> +       int UK_CONCAT(uk_syscall_, syscall_name)(       \
> +               __UK_SPROTO_ARGSx(args_nr))
> +
> +#define uk_syscall_stub(syscall_name) ({                       \
> +                       uk_pr_debug("syscall \"" syscall_name    \
> +                                   "\" is not implemented");    \
> +                       errno = -ENOSYS;                        \
> +                       -1;                                     \
> +               })
Hum, since your shim layer is mimicking Linux's userspace sys call 
wrapper (<sys/syscall.h>), it is expected that a syscall function 
handles errno and returns -1 in error cases. I suggest that you add a 
comment to UK_SYSCALL_DEFINE() so that everyone that implements such a 
system call is not getting confused with what a syscall in the kernel 
means in terms of return values and arguments.
Could you also write that the return type is `long` on such a defined 
function? This is kind of hidden in the UK_SYSCALL_DEFINE() macro.
> +
> +
> +#ifdef CONFIG_LIBSYSCALL_SHIM
> +#include <uk/bits/syscall_nrs.h>
> +#include <uk/bits/syscall_map.h>
> +#include <uk/bits/provided_syscalls.h>
> +#include <uk/bits/syscall_stubs.h>
> +
> +#define syscall(...)                                                   \
> +       UK_CONCAT(__uk_syscall, __SYSCALL_NARGS(__VA_ARGS__))(__VA_ARGS__)
> +#endif
> +
> +#endif /* __UK_SYSCALL_H__ */
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel