Re: [PATCH 7/7] Add support for ARM UEFI ("EFI") platforms

[Leif Lindholm]

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