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