Re: [PATCH RFC] make error codes a formal part of the ABI

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/01/15 16:21, Jan Beulich wrote:
> Now that we have two cases where patches against hvmloader got
> submitted needing to include the hypervisor's errno.h (for the host's
> system header not necessarily reflecting the correct numbers), take
> this as a strong sign that we need to make the error return values part
> of the hypervisor ABI (which de-fact they've always been).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> There's on small block commented with TBD left in the public header.
> This is the main reason for the submission being RFC. While we don't
> currently use these error codes, I'm not sure if we should leave all
> or some of them out for the time being.

I would suggest that we drop any error codes not in use.  We can always
add them back in in the future if they are needed.

I would also suggest that we note the heritage, being taken from Linux
2.$mumble, which will act as a guide for introducing new codes in the
future.

Finally, I would suggest that we assign the Xen meaning (e.g. EACCES =
bad tool interface) rather than Linux meanings.

>
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -2,7 +2,6 @@
>  #define __ARM_PAGE_H__
>  
>  #include <xen/config.h>
> -#include <xen/errno.h>
>  #include <public/xen.h>
>  #include <asm/processor.h>
>  
> @@ -83,6 +82,7 @@
>  
>  #ifndef __ASSEMBLY__
>  
> +#include <xen/errno.h>
>  #include <xen/types.h>
>  #include <xen/lib.h>
>  
> --- a/xen/include/asm-x86/multicall.h
> +++ b/xen/include/asm-x86/multicall.h
> @@ -24,7 +24,7 @@
>              "    callq *%%rax; "                             \
>              "1:  movq  %%rax,%c4(%0)\n"                      \
>              ".section .fixup,\"ax\"\n"                       \
> -            "2:  movq  $-"STR(ENOSYS)",%%rax\n"              \
> +            "2:  movq  %5,%%rax\n"                           \
>              "    jmp   1b\n"                                 \
>              ".previous\n"                                    \
>              :                                                \
> @@ -32,7 +32,8 @@
>                "i" (offsetof(__typeof__(*_call), op)),        \
>                "i" (offsetof(__typeof__(*_call), args)),      \
>                "i" (sizeof(*(_call)->args)),                  \
> -              "i" (offsetof(__typeof__(*_call), result))     \
> +              "i" (offsetof(__typeof__(*_call), result)),    \
> +              "i" (-ENOSYS)                                  \
>                /* all the caller-saves registers */           \
>              : "rax", "rcx", "rdx", "rsi", "rdi",             \
>                "r8",  "r9",  "r10", "r11" );                  \
> @@ -54,7 +55,7 @@
>              "    callq *%%rax; "                             \
>              "1:  movl  %%eax,%c4(%0)\n"                      \
>              ".section .fixup,\"ax\"\n"                       \
> -            "2:  movl  $-"STR(ENOSYS)",%%eax\n"              \
> +            "2:  movl  %5,%%eax\n"                           \
>              "    jmp   1b\n"                                 \
>              ".previous\n"                                    \
>              :                                                \
> @@ -62,7 +63,8 @@
>                "i" (offsetof(__typeof__(*_call), op)),        \
>                "i" (offsetof(__typeof__(*_call), args)),      \
>                "i" (sizeof(*(_call)->args)),                  \
> -              "i" (offsetof(__typeof__(*_call), result))     \
> +              "i" (offsetof(__typeof__(*_call), result)),    \
> +              "i" (-ENOSYS)                                  \
>                /* all the caller-saves registers */           \
>              : "rax", "rcx", "rdx", "rsi", "rdi",             \
>                "r8",  "r9",  "r10", "r11" )                   \
> --- /dev/null
> +++ b/xen/include/public/errno.h
> @@ -0,0 +1,94 @@
> +#ifndef __XEN_PUBLIC_ERRNO_H__
> +
> +#ifndef __ASSEMBLY__
> +
> +#define XEN_ERRNO(name, value) XEN_##name = value,
> +enum xen_errno {
> +
> +#else /* !__ASSEMBLY__ */
> +
> +#define XEN_ERRNO(name, value) .equ XEN_##name, value
> +
> +#endif /* __ASSEMBLY__ */
> +
> +/* ` enum neg_errnoval {  [ -Efoo for each Efoo in the list below ]  } */
> +/* ` enum errnoval { */
> +
> +#endif /* __XEN_PUBLIC_ERRNO_H__ */
> +
> +#ifdef XEN_ERRNO
> +
> +XEN_ERRNO(EPERM,	 1)	/* Operation not permitted */
> +XEN_ERRNO(ENOENT,	 2)	/* No such file or directory */
> +XEN_ERRNO(ESRCH,	 3)	/* No such process */
> +#ifdef __XEN__
> +XEN_ERRNO(EINTR,	 4)	/* Interrupted system call */
> +#endif

Why is EINTR (and ERESTART lower) hidden from non-xen consumers but
still in the public api?  Would it not be better to keep these in
xen/errno.h rather than public/errno.h ?

~Andrew