Re: [PATCH 6/7] add ARM Linux loader

[Leif Lindholm]

On Mon, Apr 01, 2013 at 06:18:17PM +0200, Francesco Lavra wrote:
> On 03/24/2013 06:01 PM, Leif Lindholm wrote:
> > === added directory 'grub-core/loader/arm'
> > === added file 'grub-core/loader/arm/linux.c'
> > --- grub-core/loader/arm/linux.c    1970-01-01 00:00:00 +0000
> > +++ grub-core/loader/arm/linux.c    2013-03-24 13:49:04 +0000
[...]
> > +static grub_addr_t initrd_start;
> > +static grub_size_t initrd_end;
> 
> initrd_end should be the same type as initrd_start.
 
Clearly.

[...]
> > +static grub_err_t
> > +linux_prepare_fdt (void)
> > +{
> > +  int node;
> > +  int retval;
> > +  int tmp_size;
> > +  void *tmp_fdt;
> > +
> > +  tmp_size = fdt_totalsize ((void *) boot_data) + 
> > FDT_ADDITIONAL_ENTRIES_SIZE;
> 
> Having a fixed size limit (FDT_ADDITIONAL_ENTRIES_SIZE) to insert
> variable-sized data (such as the linux command line) seems suboptimal,
> as this may fail when a very long command line is passed. How about
> removing the FDT_ADDITIONAL_ENTRIES_SIZE define from the header file
> include/grub/arm/linux.h (after all, this is an implementation detail
> and IMHO shouldn't go in a public header file) and defining it in this
> function, say:
> 
> #define FDT_ADDITIONAL_ENTRIES_SIZE   (0x100 +        \
>       grub_strlen (linux_args))
 
Mmm. It was only used in one location though, so I'll use your suggestion,
but just inline it.

> [...]
> > +#ifdef GRUB_MACHINE_EFI
> > +  linux_addr = (grub_addr_t) grub_efi_allocate_loader_memory 
> > (LINUX_PHYS_OFFSET, size);
> 
> There should be a check against allocation failure.
 
Yes, this is a global issue with this loader under EFI - dealing with memory
allocation/deallocation. Will be resolved in next version.

> > +static grub_err_t
> > +linux_unload (void)
> > +{
> > +  grub_dl_unref (my_mod);
> > +
> > +  grub_free (linux_args);
> > +  linux_args = NULL;
> 
> linux_args might already be NULL, if memory allocation for it failed in
> grub_cmd_linux().

Yes, but on the other hand grub_free (like libc free) checks for NULL.

> In the EFI case, the memory allocated for the kernel image should be
> freed as well.
 
Yes.

> > +
> > +  initrd_start = initrd_end = 0;
> 
> Why should the initrd data be discarded here?
 
It probably shouldn't.

> > +
> > +  return GRUB_ERR_NONE;
> > +}
> > +static grub_err_t
> > +grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
> > +           int argc, char *argv[])
> > +{
> > +  int size, retval;
> 
> The return type of linux_load() is grub_err_t, so retval should be the
> same type.
> 
> > +  grub_file_t file;
> > +  grub_dl_ref (my_mod);
> > +
> > +  if (argc == 0)
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> > +
> > +  file = grub_file_open (argv[0]);
> > +  if (!file)
> > +    goto fail;
> > +
> > +  retval = linux_load (argv[0]);
> 
> Here the file is already open: why not just pass the file handle to
> linux_load()?
 
Unfortunate leftover from earlier version, thanks.

> > +  grub_file_close (file);
> > +  if (retval != GRUB_ERR_NONE)
> > +    goto fail;
> 
> In order to correctly report all error types, grub_errno should be set
> to retval before entering the error path.
 
OK, adding grub_error() to all failure paths in linux_load.

> > +
> > +  grub_loader_set (linux_boot, linux_unload, 1);
> 
> Since linux_boot() may return control to its caller, the third argument
> of grub_loader_set() should be 0, otherwise linux_boot() returning
> control to its caller results in a dysfunctional state.
 
OK.

> > +
> > +  size = grub_loader_cmdline_size (argc, argv);
> > +  linux_args = grub_malloc (size + sizeof (LINUX_IMAGE));
> > +  if (!linux_args)
> > +    goto fail;
> 
> grub_loader_unset() should be called in this error path, otherwise if
> one tries to boot in this state, linux_prepare_fdt() will access a NULL
> pointer.
 
Yes.

> > +  if (grub_file_read (file, (void *) initrd_start, size) != size)
> > +    goto fail;
> 
> In the EFI case, the memory allocated for the initrd should be freed
> here. And in both EFI and U-Boot cases, initrd_start should be zeroed.
 
Yes.

> > +#else
> > +  boot_data = LINUX_FDT_ADDRESS;
> > +#endif
> > +  grub_dprintf ("loader", "Loading device tree to 0x%08x\n",
> > +           (grub_addr_t) boot_data);
> > +  fdt_move (blob, (void *) boot_data, fdt_totalsize (blob));
> > +  grub_free (blob);
> 
> I don't get the point of allocating memory for the FDT in load_dtb()
> just to move the FDT data to boot_data and then freeing the temporary
> memory. Isn't it easier if load_dtb() operates directly on boot_data?
 
Maybe not the sanest of reasons, but the U-Boot port permits passing
through ATAGs if no FDT is available, so I prefer not corrupting
boot_data until a successful load has occurred.

> > +
> > +  /* 
> > +   * We've successfully loaded an FDT, so any machine type passed
> > +   * from firmware is now obsolete.
> > +   */
> > +  machine_type = ARM_FDT_MACHINE_TYPE;
> > +
> > +  return GRUB_ERR_NONE;
> 
> The file should be closed before returning.

Yes.

/
    Leif