Re: [PATCH] add arm64 UEFI Linux loader

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

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

On 18.12.2013 17:54, Leif Lindholm wrote:
> 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.
> 
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
>>> +  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.
> 

>>> +  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
> /
>     Leif
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]