Date: Wed, 26 Apr 2023 12:02:19 -0600 From: Warner Losh <imp@bsdimp.com> To: Kyle Evans <kevans@freebsd.org> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: ce5a210997da - main - openzfs: arm64: implement kfpu_begin/kfpu_end Message-ID: <CANCZdfoY-ZnHp%2BH3J=iB7VS%2B1tO0ViZKWeexr0bicgTeckQCZg@mail.gmail.com> In-Reply-To: <202304261724.33QHOl6x001199@gitrepo.freebsd.org> References: <202304261724.33QHOl6x001199@gitrepo.freebsd.org>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --] 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) > > [-- Attachment #2 --] <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Apr 26, 2023 at 11:24 AM Kyle Evans <<a href="mailto:kevans@freebsd.org">kevans@freebsd.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The branch main has been updated by kevans:<br> <br> URL: <a href="https://cgit.FreeBSD.org/src/commit/?id=ce5a210997da3c4064cfe162e760379f1fa8b587" rel="noreferrer" target="_blank">https://cgit.FreeBSD.org/src/commit/?id=ce5a210997da3c4064cfe162e760379f1fa8b587</a><br>; <br> commit ce5a210997da3c4064cfe162e760379f1fa8b587<br> Author: Kyle Evans <kevans@FreeBSD.org><br> AuthorDate: 2023-04-26 17:23:48 +0000<br> Commit: Kyle Evans <kevans@FreeBSD.org><br> CommitDate: 2023-04-26 17:24:00 +0000<br> <br> openzfs: arm64: implement kfpu_begin/kfpu_end<br> <br> This is part one of a fix for booting with ZFS on arm64 using<br> accelerated checksum implementations. Checksum benchmarking will<br> attempt to use the FPU, so we currently panic quickly on boot. BLAKE3<br> is still broken, as it clobbers x18 and we promptly discover that fact<br> as soon as we attempt to fetch curthread in kfpu_end().<br> <br> Note that _STANDALONE is special-cased here, but ideally we wouldn't be<br> building the code that uses kfpu_begin()/kfpu_end() at all in the loader<br> environment.<br></blockquote><div><br></div><div>Yes. As noted elsewhere, the upstream is a mess. There's no way to say "I only want</div><div>the generic implementation" for these functions. So we went through some crazy hoops</div><div>to try to disable that... only to run into a buzz-saw of upstream changes that broke</div><div>the crazy hoops so now we're (bogusly in my opinion) forced to include the acceleration</div><div>routines... which we effectively disable here... but since the boot loader is space constrained</div><div>and the link time doesn't know that these routines will never be called, they bloat</div><div>the boot loader... all because there's no effective way to turn off all the accelerated</div><div>versions for arm like this happens to be for x86.</div><div><br></div><div>I've not yet had a chance to implement the obvious solution of having an option just to</div><div>turn every acceleration off and not even try to compile it in because the currount cross</div><div>coupling makes that insanely hard (which is how we wind up with needing the _STANDALONE</div><div>stuff here: if we could just disable all smd stuff, this file would never be included and we'd</div><div>not need this silly implementation).</div><div><br></div><div><rant off><br></div><div><br></div><div>Warner<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Discussed with: imp (a bit)<br> Differential Revision: <a href="https://reviews.freebsd.org/D39448" rel="noreferrer" target="_blank">https://reviews.freebsd.org/D39448</a><br>; ---<br> .../include/os/freebsd/spl/sys/simd_aarch64.h | 28 +++++++++++++++++++++-<br> 1 file changed, 27 insertions(+), 1 deletion(-)<br> <br> 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<br> index 7d2e2db28017..9edbc5f40455 100644<br> --- a/sys/contrib/openzfs/include/os/freebsd/spl/sys/simd_aarch64.h<br> +++ b/sys/contrib/openzfs/include/os/freebsd/spl/sys/simd_aarch64.h<br> @@ -44,13 +44,39 @@<br> #define _FREEBSD_SIMD_AARCH64_H<br> <br> #include <sys/types.h><br> +#include <sys/ucontext.h><br> #include <machine/elf.h><br> +#include <machine/fpu.h><br> #include <machine/md_var.h><br> +#include <machine/pcb.h><br> +<br> +#ifdef _STANDALONE<br> <br> #define kfpu_allowed() 0<br> -#define kfpu_initialize(tsk) do {} while (0)<br> #define kfpu_begin() do {} while (0)<br> #define kfpu_end() do {} while (0)<br> +<br> +#else<br> +<br> +/*<br> + * XXX kfpu_allowed() should be 1, but this is pending a fix to the BLAKE3<br> + * generated assembly to avoid clobbering x18. Turn it back on after that<br> + * lands.<br> + */<br> +#define kfpu_allowed() 0<br> +#define kfpu_begin() do { \<br> + if (__predict_false(!is_fpu_kern_thread(0))) \<br> + fpu_kern_enter(curthread, NULL, FPU_KERN_NOCTX); \<br> +} while(0)<br> +<br> +#define kfpu_end() do { \<br> + if (__predict_false(curthread->td_pcb->pcb_fpflags & PCB_FP_NOSAVE)) \<br> + fpu_kern_leave(curthread, NULL); \<br> +} while(0)<br> +<br> +#endif<br> +<br> +#define kfpu_initialize(tsk) do {} while (0)<br> #define kfpu_init() (0)<br> #define kfpu_fini() do {} while (0)<br> <br> </blockquote></div></div>help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfoY-ZnHp%2BH3J=iB7VS%2B1tO0ViZKWeexr0bicgTeckQCZg>