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

[Francesco Lavra]

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)
> +{
[...]
> +  for (desc = mmap ; desc < mmap_end ;
> +       desc = NEXT_MEMORY_DESCRIPTOR(desc, desc_size))
> +    {
> +      grub_uint64_t start, end;
> +
> +      grub_dprintf("mm", "%s: 0x%08x bytes @ 0x%08x\n",
> +                __FUNCTION__,
> +                (grub_uint32_t) (desc->num_pages << PAGE_SHIFT),
> +                (grub_uint32_t) (desc->physical_start));
> +
> +      if (desc->type != GRUB_EFI_CONVENTIONAL_MEMORY)
> +     continue;
> +
> +      start = desc->physical_start;
> +      end = start + (desc->num_pages << PAGE_SHIFT);
> +      grub_dprintf("mm", "%s: start=0x%016llx, end=0x%016llx\n",
> +               __FUNCTION__, start, end);
> +      start = start < min_start ? min_start : start;
> +      if (start + size > end)
> +     continue;
> +      grub_dprintf("mm", "%s: let's allocate some (0x%x) pages @ 
> 0x%08x...\n",
> +               __FUNCTION__, (size >> PAGE_SHIFT), (grub_addr_t) start);
> +      mem = grub_efi_allocate_pages (start, (size >> PAGE_SHIFT) + 1);
> +      grub_dprintf("mm", "%s: retval=0x%08x\n",
> +                __FUNCTION__, (grub_addr_t) mem);
> +      if (! mem)
> +     {
> +       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.

> +      break;
> +    }
> +
> +  if (! mem)
> +    {
> +      grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot allocate memory");
> +      goto fail;
> +    }
> +
> +  grub_free (mmap);
> +  return mem;
> +
> + fail:
> +  grub_free (mmap);
> +  return NULL;
> +}
[...]
> === 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.

[...]
> --- 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.

> +      .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.

> +      .pe_target = GRUB_PE32_MACHINE_ARMTHUMB_MIXED,
> +      .elf_target = EM_ARM,
> +    },
>    };
>  
>  #define grub_target_to_host32(x) (grub_target_to_host32_real (image_target, 
> (x)))
> 
> === 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
> @@ -58,6 +58,11 @@
>  #error "I'm confused"
>  #endif
>  
> +static Elf_Addr SUFFIX (entry_point);
> +
> +grub_err_t reloc_thm_call (grub_uint16_t *addr, Elf32_Addr sym_addr);
> +grub_err_t reloc_thm_jump19 (grub_uint16_t *addr, Elf32_Addr sym_addr);
> +
>  /* Relocate symbols; note that this function overwrites the symbol table.
>     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?

> +                    sym_addr += 0x400;

Where does this 0x400 value come from?

> +                    *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.

> +                  }
> +                  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.

--
Francesco

grub-mkimage-refactoring.patch
Description: Text Data