grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Daniel Kiper
Subject: Re: [PATCH] ieee1275: Include a.out header in assembly of sparc64 boot loader
Date: Thu, 21 Feb 2019 16:40:40 +0100
User-agent: NeoMutt/20170113 (1.7.2)

On Thu, Feb 21, 2019 at 04:28:40PM +0100, John Paul Adrian Glaubitz wrote:
> 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.

If you sprinkle the comments similar to above ones into the code then
I will be happy with the patch.

And sorry for late reply but I am recovering after the travel.

Daniel



reply via email to

[Prev in Thread] Current Thread [Next in Thread]