Re: git: ce5a210997da - main - openzfs: arm64: implement kfpu_begin/kfpu_end

On Wed, Apr 26, 2023 at 11:24 AM Kyle Evans <kevans@freebsd.org> wrote:

> The branch main has been updated by kevans:
>
> URL:
> https://cgit.FreeBSD.org/src/commit/?id=ce5a210997da3c4064cfe162e760379f1fa8b587
>
> commit ce5a210997da3c4064cfe162e760379f1fa8b587
> Author:     Kyle Evans <kevans@FreeBSD.org>
> AuthorDate: 2023-04-26 17:23:48 +0000
> Commit:     Kyle Evans <kevans@FreeBSD.org>
> CommitDate: 2023-04-26 17:24:00 +0000
>
>     openzfs: arm64: implement kfpu_begin/kfpu_end
>
>     This is part one of a fix for booting with ZFS on arm64 using
>     accelerated checksum implementations.  Checksum benchmarking will
>     attempt to use the FPU, so we currently panic quickly on boot.  BLAKE3
>     is still broken, as it clobbers x18 and we promptly discover that fact
>     as soon as we attempt to fetch curthread in kfpu_end().
>
>     Note that _STANDALONE is special-cased here, but ideally we wouldn't be
>     building the code that uses kfpu_begin()/kfpu_end() at all in the
> loader
>     environment.
>

Yes. As noted elsewhere, the upstream is a mess. There's no way to say "I
only want
the generic implementation" for these functions. So we went through some
crazy hoops
to try to disable that... only to run into a buzz-saw of upstream changes
that broke
the crazy hoops so now we're (bogusly in my opinion) forced to include the
acceleration
routines... which we effectively disable here... but since the boot loader
is space constrained
and the link time doesn't know that these routines will never be called,
they bloat
the boot loader... all because there's no effective way to turn off all the
accelerated
versions for arm like this happens to be for x86.

I've not yet had a chance to implement the obvious solution of having an
option just to
turn every acceleration off and not even try to compile it in because the
currount cross
coupling makes that insanely hard (which is how we wind up with needing the
_STANDALONE
stuff here: if we could just disable all smd stuff, this file would never
be included and we'd
not need this silly implementation).

<rant off>

Warner


>     Discussed with: imp (a bit)
>     Differential Revision:  https://reviews.freebsd.org/D39448
> ---
>  .../include/os/freebsd/spl/sys/simd_aarch64.h      | 28
> +++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/sys/contrib/openzfs/include/os/freebsd/spl/sys/simd_aarch64.h
> b/sys/contrib/openzfs/include/os/freebsd/spl/sys/simd_aarch64.h
> index 7d2e2db28017..9edbc5f40455 100644
> --- a/sys/contrib/openzfs/include/os/freebsd/spl/sys/simd_aarch64.h
> +++ b/sys/contrib/openzfs/include/os/freebsd/spl/sys/simd_aarch64.h
> @@ -44,13 +44,39 @@
>  #define        _FREEBSD_SIMD_AARCH64_H
>
>  #include <sys/types.h>
> +#include <sys/ucontext.h>
>  #include <machine/elf.h>
> +#include <machine/fpu.h>
>  #include <machine/md_var.h>
> +#include <machine/pcb.h>
> +
> +#ifdef _STANDALONE
>
>  #define        kfpu_allowed()          0
> -#define        kfpu_initialize(tsk)    do {} while (0)
>  #define        kfpu_begin()            do {} while (0)
>  #define        kfpu_end()              do {} while (0)
> +
> +#else
> +
> +/*
> + * XXX kfpu_allowed() should be 1, but this is pending a fix to the BLAKE3
> + * generated assembly to avoid clobbering x18.  Turn it back on after that
> + * lands.
> + */
> +#define        kfpu_allowed()          0
> +#define        kfpu_begin() do {
>      \
> +       if (__predict_false(!is_fpu_kern_thread(0)))                    \
> +               fpu_kern_enter(curthread, NULL, FPU_KERN_NOCTX);        \
> +} while(0)
> +
> +#define        kfpu_end() do {
>      \
> +       if (__predict_false(curthread->td_pcb->pcb_fpflags &
> PCB_FP_NOSAVE))    \
> +               fpu_kern_leave(curthread, NULL);                        \
> +} while(0)
> +
> +#endif
> +
> +#define        kfpu_initialize(tsk)    do {} while (0)
>  #define        kfpu_init()             (0)
>  #define        kfpu_fini()             do {} while (0)
>
>