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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
