On Mon, Apr 01, 2013 at 06:24:43PM +0200, Francesco Lavra wrote: > On 03/24/2013 06:01 PM, Leif Lindholm wrote: > > === added file 'grub-core/kern/arm/efi/misc.c' > > --- grub-core/kern/arm/efi/misc.c 1970-01-01 00:00:00 +0000 > > +++ grub-core/kern/arm/efi/misc.c 2013-03-24 15:43:19 +0000 > [...] > > +void * > > +grub_efi_allocate_loader_memory (grub_uint32_t min_offset, grub_uint32_t > > size) > > +{ > [...] > > + { > > + grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot allocate memory"); > > + goto fail; > > + } > > This if block is redundant, since it is repeated just after the for() > loop. The break statement below would do. OK.
> [...] > > === added file 'grub-core/kern/arm/efi/startup.S' > > --- grub-core/kern/arm/efi/startup.S 1970-01-01 00:00:00 +0000 > > +++ grub-core/kern/arm/efi/startup.S 2013-03-24 15:32:46 +0000 > > @@ -0,0 +1,38 @@ > > +/* > > + * (C) Copyright 2013 Free Software Foundation > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License as > > + * published by the Free Software Foundation; either version 2 of > > + * the License, or (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program; if not, write to the Free Software > > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, > > + * MA 02111-1307 USA > > + * > > + */ > > This license header doesn't follow exactly GRUB's template. You're right - I thought I had sanity checked all these... I think this also came from copying ia64 code. > [...] > > --- util/grub-mkimage.c 2013-03-24 14:49:04 +0000 > > +++ util/grub-mkimage.c 2013-03-24 15:32:46 +0000 > > @@ -472,6 +472,27 @@ > > .mod_align = GRUB_KERNEL_ARM_UBOOT_MOD_ALIGN, > > .link_align = 4 > > }, > > + { > > + .dirname = "arm-efi", > > + .names = { "arm-efi", NULL }, > > + .voidp_sizeof = 4, > > + .bigendian = 0, > > + .id = IMAGE_EFI, > > Nitpick: the last two lines above have trailing whitespace. Argh! Fixed. > > + .flags = PLATFORM_FLAGS_NONE, > > + .total_module_size = TARGET_NO_FIELD, > > + .decompressor_compressed_size = TARGET_NO_FIELD, > > + .decompressor_uncompressed_size = TARGET_NO_FIELD, > > + .decompressor_uncompressed_addr = TARGET_NO_FIELD, > > + .section_align = GRUB_PE32_SECTION_ALIGNMENT, > > + .vaddr_offset = ALIGN_UP (GRUB_PE32_MSDOS_STUB_SIZE > > + + GRUB_PE32_SIGNATURE_SIZE > > + + sizeof (struct grub_pe32_coff_header) > > + + sizeof (struct grub_pe32_optional_header) > > + + 4 * sizeof (struct > > grub_pe32_section_table), > > + GRUB_PE32_SECTION_ALIGNMENT), > > This ALIGN_UP() thing is a bit duplicated in this file. How about adding > a #define EFI32_HEADER_SIZE, analogous to EFI64_HEADER_SIZE, and using > the new define? > The attached patch does this simple refactoring and could be applied > right away, independently from the ARM port. Quite happy to use that instead. > > === modified file 'util/grub-mkimagexx.c' > > --- util/grub-mkimagexx.c 2012-02-29 17:57:43 +0000 > > +++ util/grub-mkimagexx.c 2013-03-24 15:32:46 +0000 [...] > > Return the address of a start symbol. */ > > static Elf_Addr > > @@ -528,6 +533,48 @@ > > } > > break; > > #endif > > +#if defined(MKIMAGE_ELF32) > > + case EM_ARM: > > + { > > + sym_addr += addend; > > + sym_addr -= SUFFIX (entry_point); > > + switch (ELF_R_TYPE (info)) > > + { > > + case R_ARM_ABS32: > > + { > > + grub_util_info (" ABS32:\toffset=%d\t(0x%08x)", > > + (int) sym_addr, (int) sym_addr); > > + /* Data will be naturally aligned */ > > + // sym_addr -= offset; > > Maybe this commented line should be removed altogether? Yes. > > + sym_addr += 0x400; > > Where does this 0x400 value come from? *cough* make that "SUFFIX (entry_point)". _start gets bumped down a bit, so all relative relocations need to follow, and then I "un-adjust" for the absolutes. Not too proud of that bit of code. > > + *target = grub_host_to_target32 (grub_target_to_host32 > > (*target) + sym_addr); > > + } > > + break; > > + case R_ARM_THM_CALL: > > + case R_ARM_THM_JUMP24: > > + { > > + grub_util_info (" > > THM_JUMP24:\ttarget=0x%08x\toffset=(0x%08x)", (unsigned int) target, > > sym_addr); > > + sym_addr -= offset; > > + /* Thumb instructions can be 16-bit aligned */ > > + reloc_thm_call ((grub_uint16_t *) target, sym_addr); > > reloc_thm_call() works with native endianness, so it cannot be used as > is here, as explained in the other mail a few hours ago. I don't see grub-mkimage currently being fully cross-platform anyway, so I would (as mentioned in previous email) prefer to postpone any such adjustments until the basic support is in. I have the patches for it, if not entirely up to date. > > + } > > + break; > > + case R_ARM_THM_JUMP19: > > + { > > + grub_util_info (" THM_JUMP19:\toffset=%d\t(0x%08x)", > > + sym_addr, sym_addr); > > + sym_addr -= offset; > > + /* Thumb instructions can be 16-bit aligned */ > > + reloc_thm_jump19 ((grub_uint16_t *) target, sym_addr); > > Ditto for reloc_thm_jump19(), it works with native endianness and cannot > be used as is. Ditto. / Leif _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel