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

[Leif Lindholm]

On Wed, Jan 23, 2019 at 05:53:00PM +0100, Daniel Kiper wrote:
> 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.

Yeah, that's what I meant. Coming up.

> > > >> +  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.

I'm afraid it doesn't look terribly consistent - some very core
functions, like grub_efi_set_virtual_address_map...

"£$%$^!

Why?
Why???
Why do we have a grub_efi_set_virtual_address_map()?

Requesting permission to nuke.


Anyway, back on topic.
grub_efi_set_variable() sets it.
grub_efi_finish_boot_services() sets it (not that we make use of that
for arm*). To be honest, most calls to grub_error() are in this
function.

> [...]
> 
> > > >> +  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?

I copied existing code without particularly paying any attention to
what it was doing.
Based on https://en.wikipedia.org/wiki/Cargo_cult.

I probably copied i386/pc/linux.c.

/
    Leif