Re: [PATCH] ieee1275: Include a.out header in assembly of sparc64 boot loader

[Daniel Kiper]

On Sat, Feb 09, 2019 at 02:39:05PM +0100, John Paul Adrian Glaubitz wrote:
> Recent versions of binutils dropped support for the a.out and COFF
> formats on sparc64 targets. Since the boot loader on sparc64 is
> supposed to be an a.out binary and the a.out header entries are
> rather simple to calculate in our case, we just write the header
> ourselves instead of relying on external tools to do that.

I am OK with the approach but:
  - I would like to hear Eric's opinion about it;
    or even see his RB on the final version of the patch,
  - I have a few comments below.

> Signed-off-by: John Paul Adrian Glaubitz <address@hidden>
> ---
> v2:
> - fix grammar in long description
> v3:
> - fix formatting in assembly code
> - update comment in boot loader for consistency
>
>  grub-core/Makefile.core.def            |  6 ++----
>  grub-core/boot/sparc64/ieee1275/boot.S | 18 +++++++++++++++---
>  include/grub/sparc64/ieee1275/boot.h   |  3 +--
>  3 files changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index e16fb06ba..f9f845828 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -402,8 +402,7 @@ image = {
>    i386_qemu_ldflags = 
> '$(TARGET_IMG_BASE_LDOPT),$(GRUB_BOOT_MACHINE_LINK_ADDR)';
>    i386_qemu_ccasflags = 
> '-DGRUB_BOOT_MACHINE_LINK_ADDR=$(GRUB_BOOT_MACHINE_LINK_ADDR)';
>
> -  sparc64_ieee1275_objcopyflags = '-O a.out-sunos-big';
> -  sparc64_ieee1275_ldflags = ' -Wl,-Ttext=0x4000';
> +  sparc64_ieee1275_ldflags = ' -Wl,-Ttext=0x3fe0';

I would like to ask you to add the comment what this number means and
why this number.

>    objcopyflags = '-O binary';
>    enable = i386_pc;
> @@ -432,8 +431,7 @@ image = {
>    i386_pc_ldflags = '$(TARGET_IMG_BASE_LDOPT),0x7C00';
>
>    sparc64_ieee1275 = boot/sparc64/ieee1275/boot.S;
> -  sparc64_ieee1275_objcopyflags = '-O a.out-sunos-big';
> -  sparc64_ieee1275_ldflags = ' -Wl,-Ttext=0x4000';
> +  sparc64_ieee1275_ldflags = ' -Wl,-Ttext=0x3fe0';

Ditto.

>    sparc64_ieee1275_cppflags = '-DCDBOOT=1';
>
>    objcopyflags = '-O binary';
> diff --git a/grub-core/boot/sparc64/ieee1275/boot.S 
> b/grub-core/boot/sparc64/ieee1275/boot.S
> index 9ea9b4e06..a534e1853 100644
> --- a/grub-core/boot/sparc64/ieee1275/boot.S
> +++ b/grub-core/boot/sparc64/ieee1275/boot.S
> @@ -21,6 +21,18 @@
>
>       .text
>       .align  4
> +     /* We're writing the a.out header ourselves as newer
> +      * upstream versions of binutils no longer support
> +      * the a.out format on sparc64.
> +      */
> +     .word   0x1030107                        /* magic number */
> +     .word   512 - GRUB_BOOT_AOUT_HEADER_SIZE /* size of text segment */

Why 512 - GRUB_BOOT_AOUT_HEADER_SIZE? Please add the comment.

> +     .word   0                                /* size of initialized data */
> +     .word   0                                /* size of uninitialized data 
> */
> +     .word   0                                /* size of symbol table || 
> checksum */
> +     .word   _start                           /* entry point */
> +     .word   0                                /* size of text relocation */
> +     .word   0                                /* size of data relocation */

I assume that 0 means that we do not care. Could you say it somewhere in the 
comments?

>       .globl  _start
>  _start:
>       /* OF CIF entry point arrives in %o4 */
> @@ -41,9 +53,9 @@ pic_base:
>        * After loading in that block we will execute it by jumping to the
>        * load address plus the size of the prepended A.OUT header (32 bytes).
>        */
> -     .org GRUB_BOOT_MACHINE_BOOT_DEVPATH
> +     .org GRUB_BOOT_MACHINE_BOOT_DEVPATH + GRUB_BOOT_AOUT_HEADER_SIZE
>  boot_path:
> -     .org GRUB_BOOT_MACHINE_KERNEL_BYTE
> +     .org GRUB_BOOT_MACHINE_KERNEL_BYTE + GRUB_BOOT_AOUT_HEADER_SIZE
>  boot_path_end:
>  kernel_byte:         .xword (2 << 9)
>  kernel_address:              .word  GRUB_BOOT_MACHINE_KERNEL_ADDR
> @@ -52,7 +64,7 @@ kernel_address:             .word  
> GRUB_BOOT_MACHINE_KERNEL_ADDR
>  #define boot_path_end (_start + 1024)
>  #include <grub/offsets.h>
>
> -     .org 8
> +     .org 8 + GRUB_BOOT_AOUT_HEADER_SIZE

If you touch code like that please add comment why 8 + 
GRUB_BOOT_AOUT_HEADER_SIZE.
Or use a constant with meaningful name instead of 8.

Daniel