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

[John Paul Adrian Glaubitz]

Hi Daniel!

Thanks for the review!

On 2/20/19 9:59 PM, Daniel Kiper wrote:
> 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,

Ok.

>> -  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.

This is just the offset of the text segment adjusted with the a.out
header added to the beginning of the assembly code. As we're writing
out the header in the assembly source ourselves, we have to adjust
the offset.

The entry point is at 0x4000.

>>    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.

Same here.

>>    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.

The complete sector size is 512 but we have to make space for the header.

>> +    .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?

We just have a text section, nothing else. Hence everything else is zero.

>>      .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.

The 8 is already there. The offset is again just being adjusted to accommodate
the a.out header size which we're now adding ourselves instead of letting
binutils do it for us.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - address@hidden
`. `'   Freie Universitaet Berlin - address@hidden
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913