Re: [PATCH] add arm64 UEFI Linux loader

[Leif Lindholm]

On Wed, Dec 18, 2013 at 06:23:06PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko 
wrote:
> >>> +  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.
> > 
> I think that you have to keep orig_fdt as otherwise only first attempt
> to boot will use FDT supplied by system. Second one won't, likely
> resulting in another failure

No, I null/free FDT loaded with "devicetree" command.
Anyway, I have refactored this handling a bit in my updated set,
also improving readability.

> >>> +  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.
>
> They're concatenated in memory if you use common functions and results
> in valid cpio which will be decompressed/parsed by Linux.

Done. Cleaning up patch for submitting v2.

> >>> +  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.
> > 
> if grub_errno is set you shouldn't override it. And please use the same
> message verbatim (unexpected end of file): it decreases work for translators

Well, since I have already checked the file size before this, any
failure will be an i/o error - so I'll just goto fail instead.

/
    Leif