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