Re: [PATCH v5 08/17] common-user: Adjust system call return on FreeBSD

[Warner Losh <imp@bsdimp.com>]

[-- Attachment #1: Type: text/plain, Size: 5512 bytes --]

On Wed, Nov 17, 2021 at 9:04 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> From: Warner Losh <imp@bsdimp.com>
>
> FreeBSD system calls return positive errno.  On the 4 hosts for
> which we have support, error is indicated by the C bit set or clear.
>
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> [rth: Rebase on new safe_syscall_base api; add #error check.]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  common-user/host/aarch64/safe-syscall.inc.S | 12 +++++++++++-
>  common-user/host/arm/safe-syscall.inc.S     | 11 +++++++++++
>  common-user/host/i386/safe-syscall.inc.S    | 10 ++++++++++
>  common-user/host/x86_64/safe-syscall.inc.S  | 10 ++++++++++
>  4 files changed, 42 insertions(+), 1 deletion(-)
>

Reviewed by: Warner Losh <imp@bsdimp.com>


> diff --git a/common-user/host/aarch64/safe-syscall.inc.S
> b/common-user/host/aarch64/safe-syscall.inc.S
> index 95c60d8609..d3f065cdef 100644
> --- a/common-user/host/aarch64/safe-syscall.inc.S
> +++ b/common-user/host/aarch64/safe-syscall.inc.S
> @@ -65,12 +65,22 @@ safe_syscall_start:
>  safe_syscall_end:
>
>         /* code path for having successfully executed the syscall */
> -       cmn     x0, #4095
> +#if defined(__linux__)
> +       /* Linux kernel returns (small) negative errno. */
> +       cmn     x0, #4096
> +       b.hi    0f
> +#elif defined(__FreeBSD__)
> +       /* FreeBSD kernel returns positive errno and C bit set. */
>         b.cs    1f
> +#else
> +#error "unsupported os"
> +#endif
>         ret
>
>         /* code path setting errno */
> +#ifdef __linux__
>  0:     neg     w0, w0                  /* create positive errno */
> +#endif
>  1:     str     w0, [x11]               /* store errno */
>         mov     x0, #-1
>         ret
> diff --git a/common-user/host/arm/safe-syscall.inc.S
> b/common-user/host/arm/safe-syscall.inc.S
> index 17839c6486..328299021d 100644
> --- a/common-user/host/arm/safe-syscall.inc.S
> +++ b/common-user/host/arm/safe-syscall.inc.S
> @@ -82,12 +82,23 @@ safe_syscall_start:
>  safe_syscall_end:
>
>         /* code path for having successfully executed the syscall */
> +#if defined(__linux__)
> +       /* Linux kernel returns (small) negative errno. */
>         cmp     r0, #-4096
>         bhi     0f
> +#elif defined(__FreeBSD__)
> +       /* FreeBSD kernel returns positive errno and C bit set. */
> +       bcs     1f
>

I believe this is OK. On FreeBSD, this will get the proper location for
errno and store
it there based on the current thread's local storage. Since the rest of the
code
extracts the errno directly, and we use that correctly w/o relying on errno
to
evaluate to the error associated with the last system call in this thread
in bsd-user,
I believe this adjustment is fine (also, a prior version seems to work,
this version
isn't yet in the github mirror, so I've not tried it).

This comment applies to all FreeBSD archs, except aarch64 which does a
similar thing,
but in a different way.

Warner

+#else
> +#error "unsupported os"
> +#endif
> +
>  9:     pop     { r4, r5, r6, r7, r8, r9, r10, pc }
>
>         /* code path setting errno */
> +#ifdef __linux__
>  0:     neg     r0, r0          /* create positive errno */
> +#endif
>  1:     str     r0, [r9]        /* store errno */
>         mov     r0, #-1
>         b       9b
> diff --git a/common-user/host/i386/safe-syscall.inc.S
> b/common-user/host/i386/safe-syscall.inc.S
> index ad89521783..c27207492a 100644
> --- a/common-user/host/i386/safe-syscall.inc.S
> +++ b/common-user/host/i386/safe-syscall.inc.S
> @@ -76,8 +76,16 @@ safe_syscall_start:
>  safe_syscall_end:
>
>         /* code path for having successfully executed the syscall */
> +#if defined(__linux__)
> +       /* Linux kernel returns (small) negative errno. */
>         cmp     $-4095, %eax
>         jae     0f
> +#elif defined(__FreeBSD__)
> +       /* FreeBSD kernel returns positive errno and C bit set. */
> +       jc      1f
> +#else
> +#error "unsupported os"
> +#endif
>
>  9:     pop     %ebx
>         .cfi_remember_state
> @@ -97,7 +105,9 @@ safe_syscall_end:
>         .cfi_restore_state
>
>         /* code path setting errno */
> +#ifdef __linux__
>  0:     neg     %eax                    /* create positive errno */
> +#endif
>  1:     mov     8+16(%esp), %ebx        /* load errno pointer */
>         mov     %eax, (%ebx)            /* store errno */
>         mov     $-1, %eax
> diff --git a/common-user/host/x86_64/safe-syscall.inc.S
> b/common-user/host/x86_64/safe-syscall.inc.S
> index 9a0c4c93b4..e3f920fa5c 100644
> --- a/common-user/host/x86_64/safe-syscall.inc.S
> +++ b/common-user/host/x86_64/safe-syscall.inc.S
> @@ -75,8 +75,16 @@ safe_syscall_start:
>  safe_syscall_end:
>
>          /* code path for having successfully executed the syscall */
> +#if defined(__linux__)
> +       /* Linux kernel returns (small) negative errno. */
>          cmp    $-4095, %rax
>          jae    0f
> +#elif defined(__FreeBSD__)
> +       /* FreeBSD kernel returns positive errno and C bit set. */
> +       jc      1f
> +#else
> +#error "unsupported os"
> +#endif
>
>  9:      pop     %rbp
>          .cfi_remember_state
> @@ -86,7 +94,9 @@ safe_syscall_end:
>          .cfi_restore_state
>
>          /* code path setting errno */
> +#ifdef __linux__
>  0:      neg    %eax                    /* create positive errno */
> +#endif
>  1:      mov    %eax, (%rbp)            /* store errno */
>          mov    $-1, %rax
>          jmp    9b
> --
> 2.25.1
>
>

[-- Attachment #2: Type: text/html, Size: 7348 bytes --]