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