[PATCH v2] grub-core: Build fixes for i386

[PATCH v2] grub-core: Build fixes for i386

[Jan (janneke) Nieuwenhuizen]

This fixes cross-compiling to x86 (e.g., the Hurd) from x86-linux.

To reproduce, update the Grub source description in your local Guix
archive and run

   ./pre-inst-env guix build --system=i686-linux --target=i586-pc-gnu grub

or install an x86 cross-build environment on x86-linux (32bit!) and
configure to cross build and make, e.g., do something like

    ./configure \
       CC_FOR_BUILD=gcc \
       --build=i686-unknown-linux-gnu
       --host=i586-pc-gnu
    make

* grub-core/lib/i386/relocator64.S: Avoid x86_64 instructions on i386.
---
 grub-core/lib/i386/relocator64.S | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/grub-core/lib/i386/relocator64.S b/grub-core/lib/i386/relocator64.S
index 148f38adb..b4675dd16 100644
--- a/grub-core/lib/i386/relocator64.S
+++ b/grub-core/lib/i386/relocator64.S
@@ -63,8 +63,10 @@ VARIABLE(grub_relocator64_cr3)
 	movq	%rax, %cr3
 #endif
 

+#ifdef __x86_64__
 	.code64
+#endif
 
 	/* mov imm64, %rax */
 	.byte 	0x48
@@ -71,7 +73,14 @@ VARIABLE(grub_relocator64_cr3)
 VARIABLE(grub_relocator64_rsp)
 	.quad	0
 
+#ifdef __x86_64__
 	movq	%rax, %rsp
+#else
+	/* movq	%rax, %rsp */
+	.byte 	0x48
+	.byte	0x89
+	.byte	0xc4
+#endif
 
 #ifdef GRUB_MACHINE_EFI
 	jmp	LOCAL(skip_efi_stack_align)
@@ -85,7 +94,14 @@ VARIABLE(grub_relocator64_rsp)
 	 */
 VARIABLE(grub_relocator64_efi_start)
 	/* Align the stack as UEFI spec requires. */
+#ifdef __x86_64__
 	andq	$~15, %rsp
+#else
+	.byte 0x48
+        .byte 0x83
+        .byte 0xe4
+        .byte 0xf0
+#endif
 
 LOCAL(skip_efi_stack_align):
 #endif
@@ -95,8 +111,15 @@ LOCAL(skip_efi_stack_align):
 VARIABLE(grub_relocator64_rsi)
 	.quad	0
 
+#ifdef	__x86_64__
 	movq	%rax, %rsi
-	
+#else
+	/* movq	%rax, %rsi */
+	.byte 	0x48
+	.byte	0x89
+	.byte 	0xc6
+#endif
+
 	/* mov imm64, %rax */
 	.byte 	0x48
 	.byte	0xb8
@@ -125,7 +148,7 @@ VARIABLE(grub_relocator64_rdx)
 	   payload and makes this implementation easier.  */
 	cld
 
-#ifdef __APPLE__
+#if defined (__APPLE__) || !defined (__x86_64__)
 	.byte 0xff, 0x25
 	.quad 0
 #else
-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.com

Re: [PATCH v2] grub-core: Build fixes for i386

[John Paul Adrian Glaubitz]

Hello Jan!

On 5/18/21 12:47 PM, Jan (janneke) Nieuwenhuizen wrote:
> or install an x86 cross-build environment on x86-linux (32bit!) and
> configure to cross build and make, e.g., do something like

Are we talking about a toolchain which exclusively supports 32-bit builds
only? If yes, wouldn't it make sense to exclude the whole file relocator64.S?

I think hacking the opcodes into the assembly sources directly to allow assembling
the assembly source file with a pure 32-bit assembler seems like a crude workaround
that I wouldn't use in an upstream source tree.

It seems the issue is more that you are trying to build 64-bit code with a toolchain
which exclusively supports 32-bit code which I'm not sure is something that can be
officially supported.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH v2] grub-core: Build fixes for i386

[Daniel Kiper]

Hey Adrian,

On Tue, May 18, 2021 at 12:58:07PM +0200, John Paul Adrian Glaubitz wrote:
> Hello Jan!
>
> On 5/18/21 12:47 PM, Jan (janneke) Nieuwenhuizen wrote:
> > or install an x86 cross-build environment on x86-linux (32bit!) and
> > configure to cross build and make, e.g., do something like
>
> Are we talking about a toolchain which exclusively supports 32-bit builds
> only? If yes, wouldn't it make sense to exclude the whole file relocator64.S?
>
> I think hacking the opcodes into the assembly sources directly to allow assembling
> the assembly source file with a pure 32-bit assembler seems like a crude workaround
> that I wouldn't use in an upstream source tree.
>
> It seems the issue is more that you are trying to build 64-bit code with a toolchain
> which exclusively supports 32-bit code which I'm not sure is something that can be
> officially supported.

When I was looking at the issue first time I thought in the same way.
Though after some thinking I realized that it is perfectly valid to
start 64-bit kernel from 32-bit bootloader which was build in 32-bit
environment. So, I think it makes sense to take that patch, which is not
very complicated and does not introduce anything which did not exist
earlier in the relocator64.S, even a such scenario is not very common
today.

Daniel

Re: [PATCH v2] grub-core: Build fixes for i386

[John Paul Adrian Glaubitz]

Hi Daniel!

On 5/18/21 4:26 PM, Daniel Kiper wrote:
> When I was looking at the issue first time I thought in the same way.
> Though after some thinking I realized that it is perfectly valid to
> start 64-bit kernel from 32-bit bootloader which was build in 32-bit
> environment. So, I think it makes sense to take that patch, which is not
> very complicated and does not introduce anything which did not exist
> earlier in the relocator64.S, even a such scenario is not very common
> today.

But this is about the toolchain, isn't it? Shouldn't even a 32-bit x86
toolchain be able to build 64-bit code?

I just checked standard binutils on a 32-bit Debian system and both the
assembler and the linker support 64-bit targets:

(sid_i386-dchroot)glaubitz@barriere:~$ as --help|grep 64
  --32/--64/--x32         generate 32bit/64bit/x32 code
                           generic32, generic64, i386, i486, i586, i686,
                           pconfig, waitpkg, cldemote, movdiri, movdir64b,
                           nomovdiri, nomovdir64b, noavx512_bf16,
                           generic32, generic64, i8086, i186, i286, i386, i486,
  -mamd64                 accept only AMD64 ISA [default]
  -mintel64               accept only Intel64 ISA
(sid_i386-dchroot)glaubitz@barriere:~$

(sid_i386-dchroot)glaubitz@barriere:~$ ld --help|grep supported
ld: supported targets: elf32-i386 elf32-iamcu pei-i386 elf32-little elf32-big elf64-x86-64 elf32-x86-64 pei-x86-64 elf64-l1om elf64-k1om elf64-little elf64-big pe-x86-64 pe-bigobj-x86-64 pe-i386 srec symbolsrec verilog tekhex binary ihex plugin trad-core
ld: supported emulations: elf_i386 elf_iamcu elf_x86_64 elf32_x86_64 elf_l1om elf_k1om i386pep i386pe
(sid_i386-dchroot)glaubitz@barriere:~$

So I'm not sure what kind of host system we are talking about here? This looks
more like a configuration issue of the distribution in question which deliberately
builds its 32-bit toolchain without 64-bit support.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH v2] grub-core: Build fixes for i386

[John Paul Adrian Glaubitz]

On 5/18/21 4:35 PM, John Paul Adrian Glaubitz wrote:
> So I'm not sure what kind of host system we are talking about here? This looks
> more like a configuration issue of the distribution in question which deliberately
> builds its 32-bit toolchain without 64-bit support.

Hmm, I just noticed that the existing code already does use these crude hacks to be
able to build 64-bit code with a 32-bit assembler:

> http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/lib/i386/relocator64.S#n57

But still, I find that very odd that the host system's toolchain isn't just built with
64-bit support enabled but maybe I'm missing something obvious.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH v2] grub-core: Build fixes for i386

[Matt Turner]

On Tue, May 18, 2021 at 3:48 AM Jan (janneke) Nieuwenhuizen
<janneke@gnu.org> wrote:
>
> This fixes cross-compiling to x86 (e.g., the Hurd) from x86-linux.
>
> To reproduce, update the Grub source description in your local Guix
> archive and run
>
>    ./pre-inst-env guix build --system=i686-linux --target=i586-pc-gnu grub
>
> or install an x86 cross-build environment on x86-linux (32bit!) and
> configure to cross build and make, e.g., do something like
>
>     ./configure \
>        CC_FOR_BUILD=gcc \
>        --build=i686-unknown-linux-gnu
>        --host=i586-pc-gnu
>     make
>
> * grub-core/lib/i386/relocator64.S: Avoid x86_64 instructions on i386.
> ---
>  grub-core/lib/i386/relocator64.S | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/lib/i386/relocator64.S b/grub-core/lib/i386/relocator64.S
> index 148f38adb..b4675dd16 100644
> --- a/grub-core/lib/i386/relocator64.S
> +++ b/grub-core/lib/i386/relocator64.S
> @@ -63,8 +63,10 @@ VARIABLE(grub_relocator64_cr3)
>         movq    %rax, %cr3
>  #endif
>
>
> +#ifdef __x86_64__
>         .code64
> +#endif
>
>         /* mov imm64, %rax */
>         .byte   0x48
> @@ -71,7 +73,14 @@ VARIABLE(grub_relocator64_cr3)
>  VARIABLE(grub_relocator64_rsp)
>         .quad   0
>
> +#ifdef __x86_64__
>         movq    %rax, %rsp
> +#else
> +       /* movq %rax, %rsp */
> +       .byte   0x48
> +       .byte   0x89
> +       .byte   0xc4
> +#endif
>
>  #ifdef GRUB_MACHINE_EFI
>         jmp     LOCAL(skip_efi_stack_align)
> @@ -85,7 +94,14 @@ VARIABLE(grub_relocator64_rsp)
>          */
>  VARIABLE(grub_relocator64_efi_start)
>         /* Align the stack as UEFI spec requires. */
> +#ifdef __x86_64__
>         andq    $~15, %rsp
> +#else
> +       .byte 0x48
> +        .byte 0x83
> +        .byte 0xe4
> +        .byte 0xf0

Indentation on these lines is inconsistent.

Re: [PATCH v2] grub-core: Build fixes for i386

[Daniel Kiper]

Mostly nits... Please take a look below...

On Tue, May 18, 2021 at 12:47:33PM +0200, Jan (janneke) Nieuwenhuizen wrote:
> This fixes cross-compiling to x86 (e.g., the Hurd) from x86-linux.
>
> To reproduce, update the Grub source description in your local Guix

s/Grub/GRUB/

> archive and run
>
>    ./pre-inst-env guix build --system=i686-linux --target=i586-pc-gnu grub
>
> or install an x86 cross-build environment on x86-linux (32bit!) and

s/32bit/32-bit/

> configure to cross build and make, e.g., do something like
>
>     ./configure \
>        CC_FOR_BUILD=gcc \
>        --build=i686-unknown-linux-gnu

Missing "\" at the end of the line...

>        --host=i586-pc-gnu
>     make
>
> * grub-core/lib/i386/relocator64.S: Avoid x86_64 instructions on i386.

Hmmm... What is this?

And you should add your SOB at the end of commit message:

Signed-off-by: Jan (janneke) Nieuwenhuizen <janneke@gnu.org>

> ---
>  grub-core/lib/i386/relocator64.S | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/lib/i386/relocator64.S b/grub-core/lib/i386/relocator64.S
> index 148f38adb..b4675dd16 100644
> --- a/grub-core/lib/i386/relocator64.S
> +++ b/grub-core/lib/i386/relocator64.S
> @@ -63,8 +63,10 @@ VARIABLE(grub_relocator64_cr3)
>  	movq	%rax, %cr3
>  #endif
>
>
> +#ifdef __x86_64__
>  	.code64
> +#endif
>
>  	/* mov imm64, %rax */
>  	.byte 	0x48
> @@ -71,7 +73,14 @@ VARIABLE(grub_relocator64_cr3)
>  VARIABLE(grub_relocator64_rsp)
>  	.quad	0
>
> +#ifdef __x86_64__
>  	movq	%rax, %rsp
> +#else
> +	/* movq	%rax, %rsp */
> +	.byte 	0x48
> +	.byte	0x89
> +	.byte	0xc4
> +#endif
>
>  #ifdef GRUB_MACHINE_EFI
>  	jmp	LOCAL(skip_efi_stack_align)
> @@ -85,7 +94,14 @@ VARIABLE(grub_relocator64_rsp)
>  	 */
>  VARIABLE(grub_relocator64_efi_start)
>  	/* Align the stack as UEFI spec requires. */
> +#ifdef __x86_64__
>  	andq	$~15, %rsp
> +#else
> +	.byte 0x48
> +        .byte 0x83
> +        .byte 0xe4
> +        .byte 0xf0

Formatting is broken here...

> +#endif
>
>  LOCAL(skip_efi_stack_align):
>  #endif
> @@ -95,8 +111,15 @@ LOCAL(skip_efi_stack_align):
>  VARIABLE(grub_relocator64_rsi)
>  	.quad	0
>
> +#ifdef	__x86_64__
>  	movq	%rax, %rsi
> -

This and...

> +#else
> +	/* movq	%rax, %rsi */
> +	.byte 	0x48
> +	.byte	0x89
> +	.byte 	0xc6
> +#endif
> +

... this change look strange. Could you fix it? Or explain in the
commit message you are doing a cleanup by the way...

Daniel

Re: [PATCH v2] grub-core: Build fixes for i386

[Jan Nieuwenhuizen]

Daniel Kiper writes:

Hello,

> Mostly nits... Please take a look below...

Great!

> On Tue, May 18, 2021 at 12:47:33PM +0200, Jan (janneke) Nieuwenhuizen wrote:
>> To reproduce, update the Grub source description in your local Guix
>
> s/Grub/GRUB/

Ok.

>> or install an x86 cross-build environment on x86-linux (32bit!) and
>
> s/32bit/32-bit/

Ok.

>> configure to cross build and make, e.g., do something like
>>
>>     ./configure \
>>        CC_FOR_BUILD=gcc \
>>        --build=i686-unknown-linux-gnu
>
> Missing "\" at the end of the line...

Fixed.

>> * grub-core/lib/i386/relocator64.S: Avoid x86_64 instructions on i386.
>
> Hmmm... What is this?

The "gitlog-to-changelog" scripts needs entries like this in order to
generate a GNU-compliant ChangeLog, see Emacs, LilyPond, Guile, Guix,
etc.  GRUB is the first GNU project that I encounter that has a
different take on this; sorry for missing that!

I have changed it to

--8<---------------cut here---------------start------------->8---
This fixes cross-compiling to x86 (e.g., the Hurd) from x86-linux of

    grub-core/lib/i386/relocator64.S

This file has six sections that only build with a 64-bit assembler,
yet only the first two sections had support for a 32-bit assembler;
this patch completes this for the remaining sections.
--8<---------------cut here---------------end--------------->8---

> And you should add your SOB at the end of commit message:
>
> Signed-off-by: Jan (janneke) Nieuwenhuizen <janneke@gnu.org>

Done.

And sorry; you asked that before.  I failed to do so because I only ever
encountered this use (e.g., quoting the Guix manual)

       When pushing a commit on behalf of somebody else, please add a
    ‘Signed-off-by’ line at the end of the commit log message—e.g., with
    ‘git am --signoff’.  This improves tracking of who did what.

and as I am the author, that would be redundant.

>> ---
>>  grub-core/lib/i386/relocator64.S | 27 +++++++++++++++++++++++++--
>>  1 file changed, 25 insertions(+), 2 deletions(-)
[..]
>> +	.byte 0x48
>> +        .byte 0x83
>> +        .byte 0xe4
>> +        .byte 0xf0
>
> Formatting is broken here...

Oops!  Apparently GRUB uses TAB characters.  Fixed!

(A .dir-locals.el file at the project root is often to avoid such
mistakes.)

>> +#ifdef	__x86_64__
>>  	movq	%rax, %rsi
>> -
>
> This and...
>
>> +#else
>> +	/* movq	%rax, %rsi */
>> +	.byte 	0x48
>> +	.byte	0x89
>> +	.byte 	0xc6
>> +#endif
>> +
>
> ... this change look strange. Could you fix it? Or explain in the
> commit message you are doing a cleanup by the way...

Hmm...what is it that looks strange here?  Obviously the MOV %RAX..
statement must be guarded and get a 32-bit alternative, and the empty
line after the move instruction now moves to the end of the block?

Greetings,
Janneke

-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.com

Re: [PATCH v2] grub-core: Build fixes for i386

[Daniel Kiper]

On Wed, May 26, 2021 at 08:18:11PM +0200, Jan Nieuwenhuizen wrote:
> Daniel Kiper writes:

[...]

> >> * grub-core/lib/i386/relocator64.S: Avoid x86_64 instructions on i386.
> >
> > Hmmm... What is this?
>
> The "gitlog-to-changelog" scripts needs entries like this in order to
> generate a GNU-compliant ChangeLog, see Emacs, LilyPond, Guile, Guix,
> etc.  GRUB is the first GNU project that I encounter that has a
> different take on this; sorry for missing that!

Oh, I did not know about that. I have to take a look at it.

> I have changed it to
>
> --8<---------------cut here---------------start------------->8---
> This fixes cross-compiling to x86 (e.g., the Hurd) from x86-linux of
>
>     grub-core/lib/i386/relocator64.S
>
> This file has six sections that only build with a 64-bit assembler,
> yet only the first two sections had support for a 32-bit assembler;
> this patch completes this for the remaining sections.
> --8<---------------cut here---------------end--------------->8---
>
> > And you should add your SOB at the end of commit message:
> >
> > Signed-off-by: Jan (janneke) Nieuwenhuizen <janneke@gnu.org>
>
> Done.
>
> And sorry; you asked that before.  I failed to do so because I only ever
> encountered this use (e.g., quoting the Guix manual)
>
>        When pushing a commit on behalf of somebody else, please add a
>     ‘Signed-off-by’ line at the end of the commit log message—e.g., with
>     ‘git am --signoff’.  This improves tracking of who did what.
>
> and as I am the author, that would be redundant.

No worries...

> >> +#ifdef	__x86_64__
> >>  	movq	%rax, %rsi
> >> -
> >
> > This and...
> >
> >> +#else
> >> +	/* movq	%rax, %rsi */
> >> +	.byte 	0x48
> >> +	.byte	0x89
> >> +	.byte 	0xc6
> >> +#endif
> >> +
> >
> > ... this change look strange. Could you fix it? Or explain in the
> > commit message you are doing a cleanup by the way...
>
> Hmm...what is it that looks strange here?  Obviously the MOV %RAX..
> statement must be guarded and get a 32-bit alternative, and the empty
> line after the move instruction now moves to the end of the block?

Err... Sorry, I was imprecise. I was thinking about removal and addition
of empty lines. No entirely empty but... Anyway, I will fix it...

Daniel