On Wed, Oct 15, 2025 at 04:42:11PM +0200, Daniel Kiper wrote:
> On Tue, Sep 09, 2025 at 04:09:37PM +0800, Gary Lin via Grub-devel wrote:
> > Implement the necessary functions to dynamically enable SSE and AVX
> > on x86_64 EFI systems when the hardware is capable.
> >
> > Signed-off-by: Gary Lin <[email protected]>
> > ---
> > grub-core/Makefile.core.def | 1 +
> > grub-core/lib/hwfeatures-gcry.c | 9 +
> > grub-core/lib/x86_64/efi/hwfeatures-gcry.c | 244 +++++++++++++++++++++
> > include/grub/x86_64/cpuid.h | 1 +
> > include/grub/x86_64/efi/hwfeatures-gcry.h | 25 +++
> > 5 files changed, 280 insertions(+)
> > create mode 100644 grub-core/lib/x86_64/efi/hwfeatures-gcry.c
> > create mode 100644 include/grub/x86_64/cpuid.h
> > create mode 100644 include/grub/x86_64/efi/hwfeatures-gcry.h
> >
> > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> > index 9b8360c98..df7fc5326 100644
> > --- a/grub-core/Makefile.core.def
> > +++ b/grub-core/Makefile.core.def
> > @@ -1686,6 +1686,7 @@ module = {
> > name = crypto;
> > common = lib/crypto.c;
> > common = lib/hwfeatures-gcry.c;
> > + x86_64_efi = lib/x86_64/efi/hwfeatures-gcry.c;
> >
> > extra_dist = lib/libgcrypt-grub/cipher/crypto.lst;
> > };
> > diff --git a/grub-core/lib/hwfeatures-gcry.c
> > b/grub-core/lib/hwfeatures-gcry.c
> > index 652e67c43..acfa7be33 100644
> > --- a/grub-core/lib/hwfeatures-gcry.c
> > +++ b/grub-core/lib/hwfeatures-gcry.c
> > @@ -19,6 +19,9 @@
> > #include <grub/dl.h>
> >
> > #include <grub/hwfeatures-gcry.h>
> > +#if defined (__x86_64__) && defined (GRUB_MACHINE_EFI)
> > +#include <grub/x86_64/efi/hwfeatures-gcry.h>
> > +#endif
> >
> > GRUB_MOD_LICENSE ("GPLv3+");
> >
> > @@ -33,11 +36,17 @@ grub_gcry_hwf_enabled (void)
> > void
> > grub_enable_gcry_hwf (void)
> > {
> > +#if defined (__x86_64__) && defined (GRUB_MACHINE_EFI)
> > + grub_enable_gcry_hwf_x86_64_efi ();
> > +#endif
> > __gcry_use_hwf = true;
> > }
> >
> > void
> > grub_reset_gcry_hwf (void)
> > {
> > +#if defined (__x86_64__) && defined (GRUB_MACHINE_EFI)
> > + grub_reset_gcry_hwf_x86_64_efi ();
> > +#endif
> > __gcry_use_hwf = false;
> > }
> > diff --git a/grub-core/lib/x86_64/efi/hwfeatures-gcry.c
> > b/grub-core/lib/x86_64/efi/hwfeatures-gcry.c
> > new file mode 100644
> > index 000000000..8e66c1d13
> > --- /dev/null
> > +++ b/grub-core/lib/x86_64/efi/hwfeatures-gcry.c
> > @@ -0,0 +1,244 @@
> > +/*
> > + * GRUB -- GRand Unified Bootloader
> > + * Copyright (C) 2025 Free Software Foundation, Inc.
> > + *
> > + * GRUB is free software: you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation, either version 3 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * GRUB is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with GRUB. If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <grub/types.h>
> > +#include <grub/x86_64/efi/hwfeatures-gcry.h>
> > +#include <grub/x86_64/cpuid.h>
> > +#include <grub/misc.h>
> > +
> > +/*
> > + * Older versions of GCC may reorder the inline asm, which can lead to
> > + * unexpected behavior when reading the Control Registers. The
> > __FORCE_ORDER
> > + * macro is used to prevent this.
> > + *
> > + * Ref:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=aa5cacdc29d76a005cbbee018a47faa6e724dd2d
> > + */
> > +#define __FORCE_ORDER "m"(*(unsigned int *)0x1000UL)
>
> Missing space before 0x1000UL...
>
Will fix it in v2.
> > +#define HW_FEATURE_X86_64_SSE (1 << 0)
> > +#define HW_FEATURE_X86_64_AVX (1 << 1)
> > +
> > +static grub_uint32_t hw_features = 0;
> > +static grub_uint64_t old_cr0, old_cr4, old_xcr0;
> > +
> > +static inline grub_uint64_t
>
> I am not entirely sure why you force inlining here and there. I think
> mostly it is not needed and compiler should be free to do (almost) any
> optimization which is needed...
>
Setting the control registers wrongly may cause CPU execption and crash
the system immediately. For saftey, I followed Linux kernel to use
inline and memory barrier to ensure the order of the asm insn.
On the other hand, it looks okay to remove inline from get_cpuid_*().
I'll update get_cpuid_*() in v2.
> > +read_cr0 (void)
> > +{
> > + grub_uint64_t val;
>
> Missing empty line here...
>
Will fix all the missing empty lines in v2.
> > + asm volatile ("mov %%cr0, %0" : "=r" (val) : __FORCE_ORDER);
> > + return val;
> > +}
> > +
> > +static inline grub_uint64_t
> > +read_cr4 (void)
> > +{
> > + grub_uint64_t val;
>
> Ditto...
>
> > + asm volatile ("mov %%cr4,%0" : "=r" (val) : __FORCE_ORDER);
> > + return val;
> > +}
>
> Anyway, in general I am OK with the patch set except these minor issues
> mentioned above. If you fix them you can add my RB to all patches.
>
Thanks!
BTW, is there any chance to get my PBKDF2 patches reviewed?
https://lore.kernel.org/grub-devel/[email protected]/
It's a small patch set and reduces the decryption time hugely.
Combining all optimization patches (PBKDF2, HW accel, and the memcpy()
patch in the Argon2 patch set), the decryption time of my testing PBKDF2
key slot reduces from 15 second to 1 second. It will be a huge
performance gain for 2.14.
Gary Lin
_______________________________________________
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
