Re: [PATCH] add arm64 UEFI Linux loader

[Leif Lindholm]

On Mon, Dec 16, 2013 at 10:34:51PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko 
wrote:
> > +static void
> > +get_fdt (void)
> > +{
> > +  grub_efi_configuration_table_t *tables;
> > +  unsigned int i;
> > +  int fdt_loaded;
> > +  int size;
> > +
> > +  if (!orig_fdt)
> > +    {
> > +      fdt_loaded = 0;
> > +      /* Look for FDT in UEFI config tables. */
> > +      tables = grub_efi_system_table->configuration_table;
> > +
> > +      for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> > +   if (grub_memcmp (&tables[i].vendor_guid, &fdt_guid, sizeof (fdt_guid))
> > +       == 0)
> > +     {
> > +       orig_fdt = tables[i].vendor_table;
> > +       grub_dprintf ("linux", "found registered FDT @ 0x%p\n", orig_fdt);
> > +       break;
> > +     }
> > +    }
> > +  else
> > +    fdt_loaded = 1;
> > +
> > +  size =
> > +    orig_fdt ? grub_fdt_get_totalsize (orig_fdt) : GRUB_FDT_EMPTY_TREE_SZ;
> > +  size += grub_strlen (linux_args) + 0x400;
> > +
> > +  grub_dprintf ("linux", "allocating %d bytes for fdt\n", size);
> > +  fdt = grub_efi_allocate_pages (0, BYTES_TO_PAGES (size));
> > +  if (!fdt)
> > +    return;
> > +
> > +  if (orig_fdt)
> > +    {
> > +      grub_memmove (fdt, orig_fdt, size);
> > +      grub_fdt_set_totalsize (fdt, size);
> > +      if (fdt_loaded)
> > +   grub_free (orig_fdt);
>
> There is a problem with this: in case of failure orig_fdt isn't
> available for next try anymore.

Right. I need to also NULL orig_fdt, will do.

> > +static grub_err_t
> > +check_kernel (struct linux_kernel_header *lh)
> > +{
> > +  if (lh->magic != GRUB_LINUX_MAGIC)
> > +    return GRUB_ERR_BAD_OS;
>
> You need grub_error here

Yes.

> > +  if ((lh->code0 & 0xffff) != 0x5A4D)
> > +    {
>
> Need a comment that it's MZ/exe header

Yes.

> > +      grub_error (GRUB_ERR_BAD_OS,
>
> Sounds like NOT_IMPLEMENTED_YET

Yes.

> > +             N_
> > +             ("Plain Image kernel not supported - rebuild with 
> > CONFIG_UEFI_STUB enabled.\n"));
> 
> > +      return GRUB_ERR_BAD_OS;
> You can return grub_error (....);

Yes.

> > +  dtb = grub_file_open (filename);
> > +  if (!dtb)
> > +    {
> > +      grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("failed to open file"));
>
> grub_file_open already does that.

OK.

> > +  size = grub_file_size (dtb);
> > +  if (size == 0)
> > +    goto out;
> > +
>
> You propbably need some error

Actually, I'll delete that test - it's a bit arbitrary anyway.

> > +  tmp_fdt = grub_malloc (size);
> > +  if (!tmp_fdt)
> > +    goto out;
> > +
> > +  if (grub_file_read (dtb, tmp_fdt, size) != size)
> > +    {
> > +      grub_free (tmp_fdt);
>
> Where is file closed?

Added.

> > +      return NULL;
> > +    }
> > +
> > +  if (grub_fdt_check_header (tmp_fdt, size) != 0)
> > +    {
> > +      grub_free (tmp_fdt);
> ditto

Ditto.

> > +static grub_err_t
> > +grub_linux_boot (void)
> > +{
> > +  grub_efi_loaded_image_t *image;
> > +  struct linux_kernel_header *lh = kernel_mem;
> > +  struct grub_pe32_optional_header *oh;
> pe32? Not pe64?

Mmm, that's a bit unpretty.
The fields that matter for the loader do not differ, but clearly
should be pe64. Changed.

> > +  kernel_stub entry;
> > +  grub_err_t retval;
> > +
> > +  retval = finalize_params ();
> > +  if (retval != GRUB_ERR_NONE)
> > +    return retval;
> > +
> > +  /*
> > +   * Terminate command line of usurped image, to inform
> > +   * stub loader to get command line out of DT.
> > +   */
> > +  image = grub_efi_get_loaded_image (grub_efi_image_handle);
> > +  image->load_options = NULL;
> > +  image->load_options_size = 0;
> > +
> > +  oh = (void *) ((grub_addr_t) kernel_mem + sizeof ("PE\0") + 
> > lh->hdr_offset
> > +            + sizeof (struct grub_pe32_coff_header));
> > +
> > +  entry = (void *) ((grub_addr_t) kernel_mem + oh->entry_addr);
> > +  grub_dprintf ("linux", "Entry point: %p\n", (void *) entry);
> > +  entry (grub_efi_image_handle, grub_efi_system_table);
> > +
> > +  /* When successful, not reached */
> > +  return GRUB_ERR_NONE;
> Sounds like return grub_errno; then

OK.

> > +}
> > +
> > +static grub_err_t
> > +grub_linux_unload (void)
> > +{
> > +  grub_dl_unref (my_mod);
> > +  loaded = 0;
> > +  if (initrd_mem)
> > +    grub_efi_free_pages ((grub_efi_physical_address_t) initrd_mem,
> > +                    BYTES_TO_PAGES (initrd_size));
> > +  if (kernel_mem)
> > +    grub_efi_free_pages ((grub_efi_physical_address_t) kernel_mem,
> > +                    BYTES_TO_PAGES (kernel_size));
> > +  if (fdt)
> > +    grub_efi_free_pages ((grub_efi_physical_address_t) fdt,
> > +                    BYTES_TO_PAGES (grub_fdt_get_totalsize (fdt)));
> is fdt allocate with malloc or allocate_pages?

grub_efi_allocate_pages().
Fixed in finalize_params(), thanks.

> > +  if (!loaded)
> > +    {
> > +      grub_error (GRUB_ERR_BAD_ARGUMENT,
> > +             N_("you need to load the kernel first"));
> > +      goto fail;
> > +    }
> > +
> > +  files = grub_zalloc (argc * sizeof (files[0]));
> > +  if (!files)
> > +    goto fail;
> > +
> > +  for (i = 0; i < argc; i++)
> > +    {
> > +      grub_file_filter_disable_compression ();
> > +      files[i] = grub_file_open (argv[i]);
> > +      if (!files[i])
> > +   goto fail;
> > +      nfiles++;
> > +      size += ALIGN_UP (grub_file_size (files[i]), 4);
> > +    }
> > +
> Why don't you use methods from loader/linux.c ?

Umm, this entire function is quite embarassing - it is left around from
when I included Matthew Garrett's "linuxefi" code for understanding the
process better..
Updated set contains the much simpler one which I meant to include.
ARM* do not even support multiple initrds.

> > +  if (grub_file_read (file, kernel_mem, kernel_size)
> > +      != (grub_int64_t) kernel_size)
> > +    {
> > +      grub_error (GRUB_ERR_FILE_READ_ERROR, N_("Can't read kernel %s"),
> > +             argv[0]);
> Please look at similar place in other architectures.

i386 looks near-identical, apart from the fact that their bzImage has
a size field which the ARM64 image does not. If you want me to change
something here, I'm afraid you will have to rub my nose in it.

/
    Leif