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

[Francesco Lavra <francescolavra.fl@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6778 bytes --]

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

[-- Attachment #2: grub-mkimage-refactoring.patch --]
[-- Type: text/x-patch, Size: 1686 bytes --]

=== modified file 'util/grub-mkimage.c'
--- util/grub-mkimage.c	2013-03-07 07:17:24 +0000
+++ util/grub-mkimage.c	2013-03-31 13:50:50 +0000
@@ -91,6 +91,13 @@
   grub_uint16_t pe_target;
 };
 
+#define EFI32_HEADER_SIZE 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)
+
 #define EFI64_HEADER_SIZE ALIGN_UP (GRUB_PE32_MSDOS_STUB_SIZE		\
 				    + GRUB_PE32_SIGNATURE_SIZE		\
 				    + sizeof (struct grub_pe32_coff_header) \
@@ -182,12 +189,7 @@
       .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),
+      .vaddr_offset = EFI32_HEADER_SIZE,
       .pe_target = GRUB_PE32_MACHINE_I386,
       .elf_target = EM_386,
     },
@@ -1101,12 +1103,7 @@
 	int reloc_addr;
 
 	if (image_target->voidp_sizeof == 4)
-	  header_size = 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);
+	  header_size = EFI32_HEADER_SIZE;
 	else
 	  header_size = EFI64_HEADER_SIZE;