Re: [PATCH v4 06/10] RISC-V: Add Linux load logic

[Daniel Kiper]

On Wed, Jan 23, 2019 at 11:47:25AM +0000, Leif Lindholm wrote:
> On Tue, Jan 22, 2019 at 05:09:18PM +0100, Alexander Graf wrote:
> > On 17.01.19 13:24, Daniel Kiper wrote:
> > > On Mon, Nov 26, 2018 at 12:38:11AM +0100, Alexander Graf wrote:

[...]

> > >> +  return GRUB_ERR_NONE;
> > >> +
> > >> + failure:
> > >
> > > s/failure/fail/?
> >
> > Why? I mean, sure, I can change it. But why?
>
> $ git grep "fail:" | wc -l
> 180
> $ git grep "failure:" | wc -l
> 5
>
> Err, yeah, fair enough. And the only perpetrators in C code (that
> aren't part of an imported project) were added by me.
>
> Daniel: would you take a single patch for
> loader/arm/linux.c and loader/arm64/linux.c?

If you change label name only then I am OK with that.

> > >> +  grub_fdt_unload();
> > >
> > > s/grub_fdt_unload()/grub_fdt_unload ()/ May I ask you to fix similar
> > > mistakes in the other patches too?
> >
> > Can you please write a checkpatch.pl or similar to catch them? At this
> > point, all those coding style issues just add to frustration on both
> > sides I think. To me, the GNU style comes very close in uglyness to the
> > TianoCore one - and my fingers just simply refuse to naturally adhere to it.
> >
> > That said, same as above. This is a copy from the arm64 linux.c.
> >
> > >
> > >> +  return grub_error(GRUB_ERR_BAD_OS, "failed to install/update FDT");
> > >> +}
> > >> +
> > >> +grub_err_t
> > >> +grub_arch_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, 
> > >> char *args)
> > >> +{
> > >> +  grub_efi_memory_mapped_device_path_t *mempath;
> > >> +  grub_efi_handle_t image_handle;
> > >> +  grub_efi_boot_services_t *b;
> > >> +  grub_efi_status_t status;
> > >> +  grub_efi_loaded_image_t *loaded_image;
> > >> +  int len;
> > >> +
> > >> +  mempath = grub_malloc (2 * sizeof 
> > >> (grub_efi_memory_mapped_device_path_t));
> > >> +  if (!mempath)
> > >> +    return grub_errno;
> > >> +
> > >> +  mempath[0].header.type = GRUB_EFI_HARDWARE_DEVICE_PATH_TYPE;
> > >> +  mempath[0].header.subtype = 
> > >> GRUB_EFI_MEMORY_MAPPED_DEVICE_PATH_SUBTYPE;
> > >> +  mempath[0].header.length = grub_cpu_to_le16_compile_time (sizeof 
> > >> (*mempath));
> > >> +  mempath[0].memory_type = GRUB_EFI_LOADER_DATA;
> > >> +  mempath[0].start_address = addr;
> > >> +  mempath[0].end_address = addr + size;
> > >> +
> > >> +  mempath[1].header.type = GRUB_EFI_END_DEVICE_PATH_TYPE;
> > >> +  mempath[1].header.subtype = GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE;
> > >> +  mempath[1].header.length = sizeof (grub_efi_device_path_t);
> > >> +
> > >> +  b = grub_efi_system_table->boot_services;
> > >> +  status = b->load_image (0, grub_efi_image_handle,
> > >> +                          (grub_efi_device_path_t *) mempath,
> > >> +                          (void *) addr, size, &image_handle);
> > >> +  if (status != GRUB_EFI_SUCCESS)
> > >> +    return grub_error (GRUB_ERR_BAD_OS, "cannot load image");
> > >> +
> > >> +  grub_dprintf ("linux", "linux command line: '%s'\n", args);
> > >> +
> > >> +  /* Convert command line to UCS-2 */
> > >> +  loaded_image = grub_efi_get_loaded_image (image_handle);
> > >> +  loaded_image->load_options_size = len =
> > >> +    (grub_strlen (args) + 1) * sizeof (grub_efi_char16_t);
> > >> +  loaded_image->load_options =
> > >> +    grub_efi_allocate_any_pages (GRUB_EFI_BYTES_TO_PAGES 
> > >> (loaded_image->load_options_size));
> > >> +  if (!loaded_image->load_options)
> > >> +    return grub_errno;
> > >
> > > I am afraid that grub_errno may not be set by
> > > grub_efi_allocate_any_pages() to correct value.
> >
> > True. What is the intended fix? Have the efi helpers set errno or set
> > errno here? Leif?
>
> I mean, that would superficially seem like the right thing to do, but
> I'd really be happy with either. I haven't really managed to find a
> natural pattern to where grub_errno is supposed to be used/set and not.

Could you check what is the rule among several/all EFI GRUB functions?
If they do not set grub_errno then you should set it here. If more EFI
GRUB functions set grub_errno then these ones called here should be
updated accordingly.

[...]

> > >> +  return (grub_arch_efi_linux_boot_image((grub_addr_t)kernel_addr,
> > >> +                                         kernel_size, linux_args));
> > >> +}
> > >> +
> > >> +static grub_err_t
> > >> +grub_linux_unload (void)
> > >> +{
> > >> +  grub_dl_unref (my_mod);
> > >
> > > I think that would be safer if you call grub_dl_unref() just before return
> > > at the end of this function.
> >
> > Same.
>
> Now this bit I know _I_ cargo-culted :)

Well, there is a chance that I do not get it because I am not native speaker.
Could you enlighten me what does it mean?

Daniel