Re: [PATCH] x86/boot: Remove x86_32 PIC using ebx workaround

[Uros Bizjak]

On Tue, Nov 29, 2022 at 3:46 PM Borislav Petkov <bp@xxxxxxxxx> wrote:
>
> Ok,
>
> I tried to summarize what we talked on the thread and have the whole
> deal explained in greater detail.
>
> Holler if something's missing:
>
> ---
> From: Uros Bizjak <ubizjak@xxxxxxxxx>
> Date: Fri, 4 Nov 2022 13:45:46 +0100
> Subject: [PATCH] x86/boot: Remove x86_32 PIC using %ebx workaround
>
> The currently supported minimum gcc version is 5.1. Before that, the
> PIC register, when generating Position Independent Code, was considered
> "fixed" in the sense that it wasn't in the set of registers available to
> the compiler's register allocator. Which, on x86-32, is already a very
> small set.
>
> What is more, the register allocator was unable to satisfy extended asm
> "=b" constraints. (Yes, PIC code uses %ebx on 32-bit as the base reg.)
>
> With gcc5:

Please say gcc 5.1 here.

Otherwise a really comprehensive and detailed explanation of the issue!

Uros.

>
> "Reuse of the PIC hard register, instead of using a fixed register,
> was implemented on x86/x86-64 targets. This improves generated PIC
> code performance as more hard registers can be used. Shared libraries
> can significantly benefit from this optimization. Currently it is
> switched on only for x86/x86-64 targets. As RA infrastructure is already
> implemented for PIC register reuse, other targets might follow this in
> the future."
>
>   (from: https://gcc.gnu.org/gcc-5/changes.html)
>
> which basically means that the register allocator has a higher degree
> of freedom when handling %ebx, including reloading it with the correct
> value before a PIC access.
>
> Furthermore:
>
>   arch/x86/Makefile:
>           # Never want PIC in a 32-bit kernel, prevent breakage with GCC built
>           # with nonstandard options
>           KBUILD_CFLAGS += -fno-pic
>
>   $ gcc -Wp,-MMD,arch/x86/boot/.cpuflags.o.d ... -fno-pic ... -D__KBUILD_MODNAME=kmod_cpuflags -c -o arch/x86/boot/cpuflags.o arch/x86/boot/cpuflags.c
>
> so the 32-bit workaround in cpuid_count() is fixing exactly nothing
> because 32-bit configs don't even allow PIC builds.
>
> As to 64-bit builds: they're done using -mcmodel=kernel which produces
> RIP-relative addressing for PIC builds and thus does not apply here
> either.
>
> So get rid of the thing and make cpuid_count() nice and simple.
>
> There should be no functional changes resulting from this.
>
>   [ bp: Expand commit message. ]
>
> Signed-off-by: Uros Bizjak <ubizjak@xxxxxxxxx>
> Signed-off-by: Borislav Petkov <bp@xxxxxxx>
> Link: https://lore.kernel.org/r/20221104124546.196077-1-ubizjak@xxxxxxxxx
> ---
>  arch/x86/boot/cpuflags.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/boot/cpuflags.c b/arch/x86/boot/cpuflags.c
> index a83d67ec627d..d75237ba7ce9 100644
> --- a/arch/x86/boot/cpuflags.c
> +++ b/arch/x86/boot/cpuflags.c
> @@ -64,20 +64,11 @@ int has_eflag(unsigned long mask)
>         return !!((f0^f1) & mask);
>  }
>
> -/* Handle x86_32 PIC using ebx. */
> -#if defined(__i386__) && defined(__PIC__)
> -# define EBX_REG "=r"
> -#else
> -# define EBX_REG "=b"
> -#endif
> -
>  void cpuid_count(u32 id, u32 count, u32 *a, u32 *b, u32 *c, u32 *d)
>  {
> -       asm volatile(".ifnc %%ebx,%3 ; movl  %%ebx,%3 ; .endif  \n\t"
> -                    "cpuid                                     \n\t"
> -                    ".ifnc %%ebx,%3 ; xchgl %%ebx,%3 ; .endif  \n\t"
> -                   : "=a" (*a), "=c" (*c), "=d" (*d), EBX_REG (*b)
> -                   : "a" (id), "c" (count)
> +       asm volatile("cpuid"
> +                    : "=a" (*a), "=b" (*b), "=c" (*c), "=d" (*d)
> +                    : "0" (id), "2" (count)
>         );
>  }
>
> --
> 2.35.1
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette